Fix required tags state after switching tag levels#89147
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@hoangzinh 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] |
trjExpensify
left a comment
There was a problem hiding this comment.
I think the UX on what's being introduced here with the double modal preventing the user from switching to multiTags is pretty sub-par. Did we explore solving this in a different way? Like disabling the requiredTag toggle when you take this action, allowing the user to enable it on whatever new levels of tags they add?
CC: @Expensify/design
|
The last dialog seems to trigger as a result of changing the tags and the first one is just a fair warning. Wouldn't it make more sense to just delete the last one. Cause that is the auto-dialog that triggers when you delete your last tag/category, no? |
|
If using double modals is not good, should we combine them into a new warning modal? For example:
|
|
I like that idea. Curious what @trjExpensify and @Expensify/design thinks. As long as the automatic modal still applies when you delete your last category or tag outside this flow. |
|
Overall, I think we're revealing a bit too much of a technical nuance here. I think the root cause of that is partly because of how multiTags are added/removed via a CSV, can't be independently managed, and ultimately how they are stored. We're going to look at solving that in the future, as it has many downstream consequences - including this one. The customer just wants to move from single to multi level tags. Anyhow @hoangzinh I'm curious about your take on the feasibility of disabling requiredTags when you're wiping all of them in this flow to switch from single level tags to multi level tags? |
It's feasible, @trjExpensify. I think we need to:
However, if we choose this approach, can we consider doing nothing for this issue? Current behavior still allows customers to clean all tags and lets users switch to multi-level tags while keeping |
|
@trjExpensify, could you please take a look at this comment, when you get a chance. Thanks. |
I think I agree. It's a bummer that the user gets all the way through the switch flow and then lands on a "You can't do that" modal. It might be better if we just warn them that switching tag levels with disable "Members must tag all expenses" and they'll have to re-enable it or something like that. Not totally sure what would be best, but I agree with Tom that this doesn't feel like it. |
What do you mean by "do nothing for this issue"? If the toggle for required tags is enabled and the user has no tags at all, what are the downstream consequences of that? |
Hi @trjExpensify, I apologize for my previous comment regarding the current behavior here. Upon retesting, I found that the backend actually functions as you described. Specifically, the backend will automatically disable The only issue we currently face is in the FE. Users must revisit the workspace to see that Screen.Recording.2026-05-09.at.17.48.10.mov |
|
Ah cool, then we don't need the "double modal" either. Nice! 👍 |
|
Yes, @trjExpensify @nabi-ebrahimi can you work on this FE part?
|
| }, [isMultiLevelTags, policyTagLists]); | ||
|
|
||
| const handlePostConfirmTagSwitch = () => { | ||
| cleanPolicyTags(policyID, !!policy?.requiresTag); |
There was a problem hiding this comment.
I believe it will enable the policy.requiresTag when the user switches between a single tag and a multi-level tag. We should only optimistically mark policy.requiresTag as false when it is enabled.
There was a problem hiding this comment.
@hoangzinh Thank you for pointing this out. I updated the logic so that we only pass this flag when \policy.requiresTagis explicitly enabled, which ensures we only markpolicy.requiresTagasfalse in the intended case.
| } | ||
|
|
||
| function cleanPolicyTags(policyID: string) { | ||
| // We do not have any optimistic data or success data for this command as this action cannot be done offline |
There was a problem hiding this comment.
This action can not be done offline. Should we only mark it on successData?
There was a problem hiding this comment.
@hoangzinh Thank you for the suggestion. I moved the \requiresTagupdate fromoptimisticDatatosuccessData only, since this action is not supported offline. This means the UI will now reflect the change only after the request succeeds. I also updated the related tests accordingly.
@nabi-ebrahimi Should we remove this test step? It seems redundant to me. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-05-15.at.17.20.35.movAndroid: mWeb ChromeScreen.Recording.2026-05-15.at.17.14.04.moviOS: HybridAppScreen.Recording.2026-05-15.at.17.33.22.moviOS: mWeb SafariScreen.Recording.2026-05-15.at.17.24.10.movMacOS: Chrome / SafariScreen.Recording.2026-05-15.at.17.10.08.mov |
@hoangzinh Thank you for catching this. I agree that this test step is redundant, so I removed it from the test steps. |
|
@nabi-ebrahimi can you update the PR description + PR title so it matches what we are trying to fix? |
@hoangzinh Thank you for the clarification. I have updated the PR title and description to better reflect the issue we are addressing in this PR. |
|
@trjExpensify can you review this PR again? TY |
trjExpensify
left a comment
There was a problem hiding this comment.
Much more graceful. Thank you!
|
@robertjchen, could you please take a look at this pr, when you get a chance. thanks. |
|
🚧 @robertjchen 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/robertjchen in version: 9.3.80-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is a frontend bug fix for UI state synchronization — the "Members must tag all expenses" toggle now correctly reflects the backend state immediately after switching tag levels, instead of requiring the user to navigate away and back. The underlying behavior is unchanged, and the relevant help articles (Create and Manage Expense Tags, Require Tags and Categories for Expenses) don't document this internal state management detail, nor should they. |
|
Deploy Blocker ##91483 was identified to be related to this PR. |
|
@nabi-ebrahimi our PR is reverted. Can you take a look at DB and bring our PR back again? TY |
|
🚀 Deployed to staging by https://github.com/robertjchen in version: 9.3.81-0 🚀
|
|
No help site changes are required. This PR is a bug fix for frontend state synchronization — after switching between single-level and multi-level tags, the Members must tag all expenses toggle now immediately reflects the backend state instead of appearing stale. No user-facing workflows, UI labels, navigation paths, or features changed. The relevant help articles ( |
Explanation of Change
When a workspace switches between single-level and multi-level tags, the backend clears the existing tags and disables
Members must tag all expensesif keeping it enabled would leave the workspace in an invalid state.The problem is that the frontend did not reflect that backend change immediately. This created a confusing experience where the switch flow completed successfully, but the UI still showed
Members must tag all expensesas enabled until the user left and revisited the workspace settings. In other words, the workspace state was valid on the backend, but the frontend temporarily showed stale information.This PR fixes that inconsistency by updating the frontend after the tag cleanup request succeeds, so users see the correct required-tags state right away and the UI stays aligned with the backend during the tag-level switching flow.
Fixed Issues
$ #87472
PROPOSAL: #87472 (comment)
Tests
Workspace Settingsfor a Control Workspace →Tags.Add tags, enter any name, and clickSave.More→Settings, then enableMembers must tag all expenses.More→Import Spreadsheet→Multi-level tags.Switch Tag Levelswarning modal appears, then clickSwitch Tag Levels.More→Settings.Members must tag all expensesis automatically disabled after the switch succeeds.Offline tests
Not applicable
QA Steps
Same as Tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Screen.Recording.2026-05-15.at.4.40.58.PM.mov
Android: mWeb Chrome
Screen.Recording.2026-05-15.at.5.51.07.PM.mov
iOS: Native
Screen.Recording.2026-05-15.at.5.12.41.PM.mov
iOS: mWeb Safari
Screen.Recording.2026-05-15.at.5.17.25.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-05-15.at.4.37.25.PM.mov