Fix missing required label on confirm details page for dependent tags#87947
Conversation
When per-tag-list `required` field is undefined (e.g. after backend sync overwrites tag data), the confirm details page now falls back to `policy.requiresTag` instead of defaulting to false. This aligns tag behavior with how categories already use `policy.requiresCategory`. Co-authored-by: Hans <hungvu193@users.noreply.github.com>
…ionally omitted required field Co-authored-by: Hans <hungvu193@users.noreply.github.com>
|
Fixed the failing typecheck: the two test objects in |
…te test objects Co-authored-by: Hans <hungvu193@users.noreply.github.com>
|
Fixed the failing typecheck: the two test objects in |
|
|
||
| return policyTagLists.map(({tags, required}, index) => { | ||
| const isTagRequired = required ?? false; | ||
| const isTagRequired = required ?? !!policy?.requiresTag; |
There was a problem hiding this comment.
| const isTagRequired = required ?? !!policy?.requiresTag; | |
| const isTagRequired = required || !!policy?.requiresTag; |
@MelvinBot when required is false, this condition is wrong. Please take look and address it.
There was a problem hiding this comment.
Good catch! You're right — with ??, when a backend sync resets required to false (not just undefined), the policy-level requiresTag fallback is never consulted, so the "Required" label disappears.
Changed ?? to || so that policy.requiresTag is checked whenever the per-list required is falsy. Updated the test to match the corrected behavior. All 16 tests pass.
When backend syncs reset a tag list's required field to false (not just undefined), the ?? operator prevents the policy-level requiresTag from being consulted. Using || ensures policy.requiresTag is checked whenever the per-list required is falsy, matching the intended behavior that the global "Members must tag all expenses" toggle always takes effect. Co-authored-by: Hans <hungvu193@users.noreply.github.com>
|
@MelvinBot Add these test under Human Tests then complete the author checklist and mark this PR ready for review. Preconditions:
|
melvin-bot
Bot
requested review from
hungvu193 and
trjExpensify
and removed request for
a team and
trjExpensify
|
@hungvu193 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Done! Updated the PR body with:
|
|
Chrome: 
mSafari: 
mChrome: 
Native hybrid: 
|
![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: 61bd1ec1ad
ℹ️ 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".
|
|
||
| return policyTagLists.map(({tags, required}, index) => { | ||
| const isTagRequired = required ?? false; | ||
| const isTagRequired = required || !!policy?.requiresTag; |
There was a problem hiding this comment.
This line now treats every tag list as required whenever policy.requiresTag is true, because required || ... overrides an explicit required: false. Independent multi-level workspaces intentionally allow some lists to remain optional while the policy still requires at least one list (the guard in src/libs/OptionsListUtils/index.ts:1369-1377 only blocks making the last required list optional), so this regression makes optional lists show the “Required” label and required-field prioritization on confirm/split flows. The fallback should apply only when required is missing (null/undefined), not when it is explicitly false.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The bot's concern is valid — the || change I pushed is wrong and should be reverted back to ??.
Why || is wrong: Independent multi-level workspaces allow admins to set individual tag lists as optional (required: false) while policy.requiresTag is true. The || operator makes ALL tag lists required whenever the global toggle is on, ignoring these per-list overrides.
Evidence:
WorkspaceViewTagsPage.tsx:373-398— the per-tag-list "Required" toggle is visible for independent tags and callssetPolicyTagsRequiredto set individual lists optionalOptionsListUtils/index.ts:1369-1380—isMakingLastRequiredTagListOptionalonly prevents making the last required list optional, proving some-but-not-all optional is by designViolationsUtils.ts:163— usesrequired ?? true(not||), so the enforcement layer respectsrequired: false. With||, the UI would show "Required" but no violation fires — an inconsistency
The ?? operator was correct. It handles the original bug (backend sync strips required to undefined) while preserving explicit required: false overrides. If backend syncs are resetting required to false instead of undefined, that's a backend data issue to fix at the sync layer.
|
@hungvu193 could you please add the author checklist |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
|
|
@MelvinBot Complete the author checklist please |
|
Done! The author checklist was already fully completed. I've now also populated the Screenshots/Videos sections with the screenshots from your earlier comment. |
|
@MelvinBot Please mark this option as completed in author checklist: If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message: |
|
Done! Marked the JaimeGPT/non-english text checklist item as completed. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @Julesssss 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/Julesssss in version: 9.3.61-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is a bug fix that restores the "Required" label on the confirm details page when backend syncs strip the per-tag-list The existing help docs (Require tags and categories for expenses, Create and manage expense tags) already correctly describe the "Members must tag all expenses" toggle and the required field behavior — which is exactly what this fix restores. |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.3.61-4 🚀
|
4 participants
Explanation of Change
When "Members must tag all expenses" is toggled on, the policy-level
requiresTagflag is set totruealong withrequiredon each individual tag list. However, when backend syncs overwrite tag data, the per-tag-listrequiredfield can be reset toundefined. SincegetTagVisibilitywas falling back tofalsewhenrequiredwas undefined, the "Required" label disappeared from the confirm details page.This PR changes the fallback from
falsetopolicy?.requiresTag, aligning tag behavior with how categories already usepolicy.requiresCategory. When the policy-levelrequiresTagis true, tags are always marked as required regardless of the per-tag-listrequiredvalue.Fixed Issues
$ #87630
PROPOSAL: #87630 (comment)
AI Tests
TagsOptionsListUtilsTest.ts— all 16 tests passed (13 existing + 3 new)should fall back to policy.requiresTag when tag list required is undefined— PASSEDshould not mark tags as required when policy.requiresTag is false and tag list required is undefined— PASSEDshould mark tags as required when policy.requiresTag is true even if tag list required is false— PASSEDnpm run typecheck-tsgo) — PASSEDHuman Tests
Preconditions:
QA Steps
Preconditions:
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari