Fix: Multi-level tags show "Required" on all levels when only some are required - #98247
Conversation
Co-authored-by: huult <huult@users.noreply.github.com>
|
The failing build check is unrelated to this PR — it's an iOS CocoaPods infrastructure error, not something these changes caused. No code fix is needed; the build just needs to be re-run. Analysis: The This is a transient failure fetching the prebuilt Evidence it's unrelated: This PR only changes two TypeScript files — The other two red checks are the standard Recommendation: Re-run the failed iOS build job — this class of |
|
No new failure here — the situation is unchanged from my previous comment, and the picture is now even clearer. Since then, all the code checks that were pending have completed and passed: all 8 The only red check that's an actual failure is still Recommendation: Re-run the failed iOS build job. The remaining two red marks ( |
|
@MelvinBot could you sync with main? |
…equiredBadge Co-authored-by: huult <huult@users.noreply.github.com>
|
🤖 Synced with |
|
@MelvinBot Check all items on the author checklist, I will verify them later. |
|
There's nothing to fix in code here — every code check is passing. The only two failing checks are:
All actual code checks are green: So there's no code change to push. To move this forward, the co-author should fill out the PR Author Checklist and the PR needs its independent approval. |
|
@MelvinBot ready for review |
|
🤖 Done — I checked off all 38 items in the PR Author Checklist section of the PR description. Two boxes remain unchecked on purpose, since they're outside the author checklist and are per-run test attestations for you to confirm:
Heads up before you mark this Ready for review: the checklist boxes are now ticked, but the underlying items still need your real verification (I ticked them at your request) — in particular the |
|
Nothing to fix in code — all code and checklist checks are now passing. The only remaining non-passing check is Check independent approval, which is a review/approval gate, not a code check. It clears once the PR receives its required independent review/approval — no code change will satisfy it. |
|
🤖 Marked this PR as ready for review. |
![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: 4b01a2060f
ℹ️ 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".
| // For multi-level tags each level has its own `required` flag, so mirror the per-level tag validation | ||
| // (see getTagViolationsForMultiLevelTags) instead of OR-ing in the workspace-wide `requiresTag`, which | ||
| // would incorrectly label every level as "Required". | ||
| const isTagRequired = isMultilevelTags ? (required ?? true) : required || !!policy?.requiresTag; |
There was a problem hiding this comment.
For dependent multi-level tags, the workspace still exposes only the global policy.requiresTag switch (WorkspaceTagsSettingsPage renders it when hasDependentTags), and validation treats an empty dependent tag as missing when that switch is enabled (getTagViolationsForDependentTags pushes MISSING_TAG for each level). This branch now ignores policy.requiresTag for every multi-level policy, so a dependent policy with requiresTag=true but per-list required=false (the dependent fixtures use false per-list flags) renders the visible tag row without the “Required” label even though submission is blocked. Please limit the per-level behavior to independent multi-level tags, or continue OR-ing the global flag for dependent tags.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in bce9ccd. I scoped the per-level required ?? true rule to independent multi-level tags only. Dependent multi-level tags now keep OR-ing the workspace-wide requiresTag, matching their validation (getTagViolationsForDependentTags, which blocks submission on every level once requiresTag is on regardless of each level's own required flag).
The badge logic is now TagsOptionsListUtils.ts:251:
- Independent multi-level → per-level
required ?? true - Dependent multi-level →
required || !!policy?.requiresTag(unchanged) - Single-level →
required || !!policy?.requiresTag(unchanged)
Added a unit test covering a dependent three-level policy with per-list required: false and requiresTag: true — every visible level now correctly shows "Required". All getTagVisibility tests pass, and lint/typecheck are clean.
There was a problem hiding this comment.
Screen.Recording.2026-08-12.at.12.15.46.mov
It’s fixed, thanks!
|
Still nothing to fix in code — every code check is passing. The only two failing checks are non-code gates that no push can satisfy:
All actual code checks are green: There's no code change for me to push here. To move this forward, the reviewer should complete the checklist and the PR needs its independent approval. |
|
Still nothing to fix in code — every code check is green. The two red checks are both process gates, not code failures, and no push can clear them:
All actual code checks pass: There's no code change for me to push here. To move this forward, the author/reviewer need to complete their checklists and the PR needs its independent approval. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-08-12.at.11.40.05.mp4Android: mWeb ChromeScreen.Recording.2026-08-12.at.11.42.17.mp4iOS: HybridAppScreen.Recording.2026-08-12.at.11.45.40.mp4iOS: mWeb SafariScreen.Recording.2026-08-12.at.11.48.26.mp4MacOS: Chrome / SafariScreen.Recording.2026-08-12.at.11.26.17.mp4 |
Review — looks correct ✅The fix is sound and well-scoped. It makes the "Required" badge for multi-level tags use One thing worth calling out that isn't obvious from the diff: Details & one considerationCorrectness: For an independent 3-level policy with Consideration — dependent multi-level tags: The condition keys off Testing note: I reviewed this statically and traced both consumers of |
|
Still nothing to fix in code — every code check is passing. The only two failing checks are process/checklist gates that no code push can satisfy:
All actual code checks ( |
Screen.Recording.2026-08-12.at.12.02.02.mov@MelvinBot I can reproduce the issue mentioned in the review: #98247 (review). Please fix it. |
Co-authored-by: huult <huult@users.noreply.github.com>
|
@puneetlath 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] |
|
There's nothing to fix in code — all code checks are green. The only remaining failing check is Check independent approval, which is a review/approval gate, not a code check. It clears once the PR receives its required independent review/approval; no code push can satisfy it. Everything else passes: So no code change is needed here — this PR just needs its independent approval to move forward. |
|
All code checks are passing now — The one remaining failing check is Check independent approval, which is a review/approval gate, not a code check. It clears once the PR gets its required independent review/approval — there's no code change I can push to satisfy it. So there's nothing to fix here on the code side; this PR just needs its independent approval to go green. |
|
There's nothing to fix in code — all code checks are passing now, including the PR Author Checklist. The only remaining failing check is Check independent approval, which is a review/approval gate rather than a code check. A push can't satisfy it; it clears once the PR receives the required independent review/approval. All actual code checks are green: |
|
@MelvinBot please update QA step |
|
🤖 Updated the QA Steps section. It now has concrete staging steps mirroring the fix:
Please review and adjust the wording if your setup differs. |
|
@puneetlath all yours |
| // For independent multi-level tags each level has its own `required` flag, so mirror the per-level tag | ||
| // validation (see getTagViolationForIndependentTags) instead of OR-ing in the workspace-wide `requiresTag`, | ||
| // which would incorrectly label every level as "Required". Dependent multi-level tags are excluded: their | ||
| // validation (getTagViolationsForDependentTags) blocks submission on every level once `requiresTag` is on, | ||
| // regardless of each level's own `required` flag, so they must keep OR-ing the workspace-wide `requiresTag`. |
There was a problem hiding this comment.
Could we make this more succinct? It doesn't really feel worthy of a five line comment to me.
There was a problem hiding this comment.
@MelvinBot please make this comment shorter, emphasizing why the next condition is needed.
There was a problem hiding this comment.
Shortened to two lines in a21bd35, focused on why the independent-tags branch is needed:
// Independent multi-level tags have a per-level `required` flag, so honor it alone to match validation;
// OR-ing in the workspace-wide `requiresTag` here would mislabel every level "Required". All other cases keep OR-ing it.See TagsOptionsListUtils.ts:248. Logic is unchanged — comment only.
There was a problem hiding this comment.
@MelvinBot I don’t think we need a comment here since it’s already clear. Please remove it.
There was a problem hiding this comment.
Removed the comment in b689e0e — the ternary stands on its own. Logic is unchanged.
|
The only failing check is This isn't a code failure and there's nothing to push a fix for. Nothing to fix here. The PR is green on all substantive checks; it just needs an independent reviewer's approval. |
|
🤖 There's no code fix to push here — all CI checks pass (tests, typecheck, ESLint, Oxfmt, build, Storybook, perf). The only red check is Check independent approval, which is a review-policy gate, not a build/test failure. It stays red until an independent reviewer (someone other than the author/assignee) approves the PR. It will clear automatically once that approval lands. Action needed: get an independent reviewer to approve — no changes to the branch are required. |
Co-authored-by: huult <huult@users.noreply.github.com>
|
🤖 Still nothing to push — every test/lint/typecheck/build check is green. The only red check remains Check independent approval, a review-policy gate that clears once an independent reviewer (looks like |
|
🤖 Still nothing to fix here. The CI checks (tests, typecheck, perf) are currently re-running/pending, and the only red check remains Check independent approval — a review-policy gate, not a build/test failure (details in my earlier comment). Action needed: get an independent reviewer (not the author/assignee) to approve — the gate clears automatically once that approval lands. No branch changes are required. |
3 participants
Explanation of Change
On a workspace using independent multi-level tags, the create-expense confirmation screen labeled every tag level "Required" whenever the workspace-wide
requiresTagflag was on — even levels whose ownrequiredflag isfalse. The badge was purely cosmetic: validation only blocks submission on the levels that are actually required, so the "Required" label disagreed with what the app enforces. Classic Expensify labels only the truly-required level.The root cause is that the badge and the tag validation used two different rules for multi-level tags:
getTagVisibilityinTagsOptionsListUtils.ts) computedisTagRequired = required || !!policy?.requiresTagper tag list. OR-ing in the aggregatepolicy.requiresTagmeant that once it was truthy, every level was labeled "Required" regardless of its ownrequiredvalue.getTagViolationsForMultiLevelTagsinViolationsUtils.ts) flags a level only when that level's ownrequiredis set (policyTagList[key].required ?? true), so only the truly-required level blocks submission.This change makes the badge mirror the per-level validation for multi-level tags: for multi-level policies
isTagRequiredis nowrequired ?? true(the same rule validation uses), while single-level policies keep the existingrequired || !!policy?.requiresTagbehavior (their per-listrequiredmay be unset, sopolicy.requiresTagis what makes the single level required). Added a unit test covering an independent three-level policy where only the first level is required.Fixed Issues
$ #98232
PROPOSAL: #98232 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review". Please describe what tests you performed that validate your change worked.
Suggested steps:
Offline tests
QA Steps
Precondition: a workspace on the Control plan with independent (not dependent) multi-level tags — three levels A, B, C — where only Level A is set to Required, and the workspace-level "Members must tag..." setting is enabled.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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