feat: Add 'Approver' column to Category and Tag list views#84112
feat: Add 'Approver' column to Category and Tag list views#84112lakchote merged 6 commits intoExpensify:mainfrom
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
melvin-bot
bot
requested review from
heyjennahay and
situchan
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13606dbe2f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| approver, | ||
| id: '-1', | ||
| }; |
There was a problem hiding this comment.
Creating a new tag approver rule without any id here makes later updates ambiguous, because renamePolicyTag() in this same file still finds the rule to mutate via findIndex((rule) => rule.id === policyTagRule.id). If multiple approval rules in the policy have missing IDs (which can happen after optimistic edits), renaming a tag can update the wrong rule and leave the intended tag approver rule unchanged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
If I remember correctly, I think we should not default string IDs to any value. Therefore, I've removed the id: '-1' assignment from the new optimistic rule here. Surely, the policyTagRule.id would return undefined in this case after adding a new approver, but it will populated with the correct id after the page refresh, so the issue wouldn't happen in that case
However, this issue exists on main too. Before this PR, setPolicyTagApprover assigned id: '-1' to every optimistic rule. So with multiple tag approvers set without refreshing the page, findIndex((rule) => rule.id === policyTagRule.id) always returns index 0 (the first rule), which is the same issue, but different placeholder value (-1 vs undefined)
To ensure indexToUpdate return the correct index, we could change the findIndex in renamePolicyTag to match by tag name instead of id:
const indexToUpdate = updatedApprovalRules.findIndex((rule) =>
rule.applyWhen.find(
({condition, field, value}) => condition === CONST.POLICY.RULE_CONDITIONS.MATCHES && field === CONST.POLICY.FIELDS.TAG && value === policyTag.oldName,
),
);Let me know what do you think about this @situchan
Or alternatively, we can remove successData in setPolicyTagApprover which would allow the backend response to populate the real id, rather than overwriting with optimistic data again
There was a problem hiding this comment.
Any reason for not leaving as is? What issue you faced with id: '-1',?
There was a problem hiding this comment.
However, this issue exists on main too.
yes, so it's out of scope.
There was a problem hiding this comment.
Any reason for not leaving as is? What issue you faced with
id: '-1',?
Actually there is no issue with id: '-1' here. I initially thought that we shouldn't default a string ID to any value. The real issue is in this condition, which filter the rule by id. Since the id can be the same optimistically, it caused the same issue as on the category page. Since now we filter it with reference equality instead, I don't think there is an issue with id: '-1' assignment. Updated the PR!
|
Went ahead and queued up an adhoc |
|
🚧 @JmillsExpensify has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Overall this looks good in testing. @Expensify/product-pr do we want to show the |
|
Yeah, I thought we weren't going to show it unless they have one assigned. I'm kinda' cool with that for now, tbh. |
|
Coming from #83385 (comment) Should we show avatar or not in this PR? |
|
Yes here in the Members table, Workflows is out of scope. 👍 |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6feee3f216
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| </View> | ||
| {shouldShowApproverColumn && ( | ||
| <View style={[glCodeContainerStyle, styles.flexRow, styles.alignItemsCenter]}> | ||
| {approverPersonalDetail ? ( |
There was a problem hiding this comment.
The approver cell is rendered only when approverPersonalDetail exists, so any rule whose approver email is not yet in PERSONAL_DETAILS_LIST will show an empty column even though an approver is set. This is user-visible in control workspaces with rules enabled (e.g., newly loaded members, offline/cache-miss cases) and makes configured approvers appear unset; unlike CategorySettingsPage/TagSettingsPage, there is no email fallback here. Please render a fallback avatar/text from approverEmail instead of returning null when lookup misses.
Useful? React with 👍 / 👎.
|
Agree with Tom. Let's show the avatar in the Members table. Then let's keep the Approver column even if no approver is selected. It helps advertise the feature for now. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-03-05.at.5.20.13.PM.moviOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-03-05.at.5.13.43.PM.mov |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6feee3f216
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| {approverPersonalDetail ? ( | ||
| <> |
There was a problem hiding this comment.
This branch renders the Approver cell only when approverPersonalDetail exists, so a valid approver rule can appear blank whenever the approver email is not yet in the personal-details cache (for example on a fresh session or offline cache miss). In that state the workspace still has an approver configured, but the new list column hides it entirely. Please fall back to showing the approver email (and a default avatar) when personal details are unavailable.
Useful? React with 👍 / 👎.
| {approverPersonalDetail ? ( | ||
| <> |
There was a problem hiding this comment.
The Tag list uses the same approverPersonalDetail ? ... : null guard, so the new Approver column becomes empty whenever the approver login is present in policy rules but absent from getPersonalDetailByEmail cache. That makes configured approvers invisible in the list for affected users and environments. Add a fallback path that renders at least the approver email (with a default avatar) when personal details are missing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@ryntgh please address Melvin's concerns above.
If personal details doesn't exist, fallback to email. Just copy the logic from here:
App/src/components/WorkspaceMembersSelectionList.tsx
Lines 56 to 65 in 6feee3f
|
@situchan updated! Could you please check it again? |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
🚧 @lakchote has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.3.32-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.32-3 🚀
|
6 participants
Explanation of Change
This PR adds an "Approver" column to the Category and Tag list views, which only appears if the workspace has the rules feature enabled
Fixed Issues
$ #83385
PROPOSAL: #83385 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Precondition: Create a control workspace with Categories, Tags, and Rules enabled
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android-App.mp4
Android: mWeb Chrome
Android-mWeb.mp4
iOS: Native
iOS-App.mp4
iOS: mWeb Safari
iOS-mWeb.mp4
MacOS: Chrome / Safari
MacOS-Chrome.mp4