Move multi-level tag Required from the Tags table to Rules - #98414
Move multi-level tag Required from the Tags table to Rules#98414Krishna2323 wants to merge 11 commits into
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@MelvinBot review |
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.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62ceef335c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
Reviewed the changes. The overall approach is clean — gating the old Required column/switches/bulk-actions behind 🐛 Per-level toggles render stale (all-off) after a delayed tag loadThis is exactly testing step 10 (fresh login / hard refresh → go straight to Rules → General → Require fields). The sync effect is one-shot per
Suggested fix: sync ❓ "Can't make all tag lists optional" guard is dropped for independent multi-level tagsOn the old Tags table, toggling off the last required list is hard-blocked with the Nothing else blocking from my read. Happy to implement either fix — reply with |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…s section Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
🚧 JmillsExpensify 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! 🧪🧪
|
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@JmillsExpensify could you please share the steps you followed? Did you use the |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@MelvinBot review and also check if there are any behavioral changes. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
On the locking question and behavioral changes: Why the tag Required toggle is locked on a NetSuite workspaceIt's locked purely because an accounting connection is active — not because of anything tag-specific. The page gates both the Category and Tag Required toggles on
So the mental model of "category is locked because we force categorization" isn't quite what the code does — both Category and Tag are locked for the same reason: while a connection is active, accounting "owns" these features (mirroring the Categories/Tags toggles on More features). The lock isn't introduced by this PR — it already existed on this page for the single Category/Tag toggles. Behavioral change this PR does introduceYes — for independent multi-level tags (your exact case: NetSuite classes + locations imported as two tag lists), this is a real change:
Net effect: on a NetSuite-connected workspace, the class/location Required toggles are now locked where they were previously editable in the Tags table. That's why the screen looks off to you. RecommendationWorth a product decision before merging: is "Required" supposed to be owned by the accounting connection at all? Enabling/disabling tags being accounting-owned makes sense (NetSuite controls which tags exist), but "Required" is an Expensify-side coding rule that admins arguably should still control while connected. If the intent is to keep it editable, the per-level toggles (and the existing single toggle) shouldn't include I'd confirm the intended behavior with the issue owner / design before adjusting the gate. |
@Krishna2323 this doesn't sound right. The isConnectedToAccounting lock was added for This is an account not on the beta: 2026-08-12_15-18-52.mp4 |


Explanation of Change
Fixed Issues
$ #95764
PROPOSAL:
Tests
Testing steps
Per-level Required — independent multi-level tags
rulesRevampenabled, create two independent tag lists (e.g. State, Region), each with enabled tags.Single/dependent tags
Disabled tag levels
Tags load without visiting Tags
Required removed from Tags table (beta on)
Beta off
rulesRevampand verify all existing Required controls/actions return and work, including the warning when making the last required tag list optional.Offline tests
QA Steps
Same as tests
Verify that no errors appear in the JS console
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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