Fixed money request optimistic violations#94105
Conversation
- Expensify#93932: re-enabling categories now writes the missingCategory violation for existing "Uncategorized" expenses, so the error shows on the preview immediately instead of only after opening the expense. The sentinel is normalized to empty in pushTransactionViolationsOnyxData, scoped to category-requirement updates so unrelated toggles are unaffected. - Expensify#93929: a disabled-but-not-deleted tax rate is now treated as out of policy, so deleting a tag offline no longer drops the "tax no longer valid" violation. getIsViolationFixed is aligned to the same check. - Expensify#93945: multi-level (independent/dependent) tag fields now show the delete-confirmation modal after Tags is disabled, matching single-level tags, instead of opening the Tag RHP.
![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: 294e800be0
ℹ️ 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".
| pushTransactionViolationsOnyxData(onyxData, policyData, {...policyOptimisticData, requiresTag: false}, {}, policyTagsOptimisticData, autoSelections); | ||
| } else { | ||
| pushTransactionViolationsOnyxData(onyxData, policyData, policyOptimisticData); | ||
| const policyTag = PolicyUtils.getTagLists(policyData.tags).at(0); |
There was a problem hiding this comment.
In multi-level tag policies where the Tags feature was disabled, Onyx can contain every tag list with enabled: false. This branch only builds optimistic data for getTagLists(...).at(0), so after toggling the feature back on, especially offline or on a slow connection, only the first level becomes selectable while later levels remain disabled; the recompute below can still leave tagOutOfPolicy violations for those selected levels and dependent tag pickers have no valid child choices until the server response arrives. Build the optimistic and failure updates for every tag list, not just the first one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@lakchote this comment expects to show violation in every tag of dependant tags. And I think it makes sense. But BE sends only first:
"transactionViolations_3030744692880219138": [
{ "name": "tagOutOfPolicy", "data": { "tagName": "State" }, "showInReview": true, "type": "violation" }
]State is orderWeight: 0 — the first level. One violation for the whole multi-level tag, labeled with the first level.
So which is expected? cc @trjExpensify
REC-20260620055314.mp4
There was a problem hiding this comment.
@lakchote this comment expects to show violation in every tag of dependant tags. And I think it makes sense. But BE sends only first:
"transactionViolations_3030744692880219138": [ { "name": "tagOutOfPolicy", "data": { "tagName": "State" }, "showInReview": true, "type": "violation" } ]
Stateis orderWeight: 0 — the first level. One violation for the whole multi-level tag, labeled with the first level.So which is expected? cc @trjExpensify
BE should be the source of truth here.
There was a problem hiding this comment.
Hi @lakchote @mukhrr, regarding this issue #93945, does it make sense if the 2nd-level tag doesn't show "no longer valid" error, but we still show "Delete modal" for them?
I'd say it still makes sense since it's a 2nd level tag and the 1st level tag is out of policy.
The backend emits a single tagOutOfPolicy for the first invalid tag level it hits. @hoangzinh @mukhrr
There was a problem hiding this comment.
@hoangzinh based on this, I think we are good to move forward
|
@lakchote I think we need another help in BE:
When a tax rate is disabled, the response sends only the tax violation for the affected transaction. Since Onyx merges arrays by replacement, this overwrites the transaction's full violation set and drops every other still-valid violation. Repro
Response payload (the bug) { "key": "transactionViolations_<id>", "onyxMethod": "merge",
"value": [{ "name": "taxOutOfPolicy", "type": "violation", "showInReview": true }] }The transaction still has no category, so Expected: return the transaction's full current violation set (e.g. REC-20260621112707.mp4 |
|
@mukhrr are you handling #93932 (comment) as regression? |
|
@Krishna2323 no it's separate bug |
|
If it's a separate bug, then I'm not sure why we're fixing it here. It should have gone through the regular issue review and PR process. |
|
These are bugs we found during fixing original bug |
I have a PR up for review to fix this. |
|
I think BE is deployed. I tested. @hoangzinh could you pls review changes? thanks |
Yes it is deployed now. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-07-06.at.21.55.07.movAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-07-06.at.21.36.04.mp4Screen.Recording.2026-07-06.at.21.39.59.mov |
| const autoSelections = pushTransactionAutoSelectionsOnyxData(onyxData, policyData, {...policyOptimisticData, requiresTag: false}, {}, policyTagsOptimisticData); | ||
|
|
||
| pushTransactionViolationsOnyxData(onyxData, policyData, {...policyOptimisticData, requiresTag: false}, {}, policyTagsOptimisticData, autoSelections); | ||
| } else if (Object.values(policyData.tags).some((tagList) => Object.keys(tagList.tags).length > 0)) { |
There was a problem hiding this comment.
It's a new behavior; we optimistically enable all the tags in each tag level.
There was a problem hiding this comment.
Sorry. I am not sure I follow your feedback. Can you elaborate a bit, pls?! thanks
There was a problem hiding this comment.
Oh got it now. Scoped re-enable symmetrically to the first list only.
| } as Partial<PolicyTagList>, | ||
| }; | ||
| // Disable every level's tags (not just the first list) so multi-level policies optimistically flag their now | ||
| // stale tags, and restore them on failure. |
There was a problem hiding this comment.
Given this #94105 (comment), do we realy need to disable all tag levels?
There was a problem hiding this comment.
Hmm, yes. you are right
|
@mukhrr It removed all dependent tags when I deleted the first tag. This behavior differs from that in Staging. Screen.Recording.2026-07-02.at.17.59.18.mov |
@hoangzinh do you think ours is expected? since it took you three actions to remove invalid tags in Staging cc @lakchote |
|
I don't know but I think we should keep current behavior |
We should keep the same behavior as on staging @mukhrr |
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.
|
@hoangzinh This is now fixed |
|
@mukhrr the 2nd tag level (and 3rd) is not:
Screen.Recording.2026-07-03.at.20.56.40.mov |
@hoangzinh it is the exact same as Staging. I see pending/loading is not finished and no error is visible in the 2nd repro (of this branch). And it's same with staging (1st repro). DIfference your staging repro kinda fast loaded. Pls, try once more Here is current state in this branch: REC-20260703201217.mp4 |
|
@mukhrr let me retest it today |
|
@mukhrr it seems we can reproduce that issue when offline Screen.Recording.2026-07-06.at.20.06.58.mov |
@hoangzinh yes, but it is what we have on Staging. Do we want to fix this as well? |
Oh, hold on. I couldn't reproduce it in staging before, but now I can. So, just leave this behavior as it is. |
|
@lakchote thanks. now it should be okay |
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
At least one of these bugs was cited to have come from a performance refactor PR here. Did anyone loop in @BartekObudzinski and @mountiny for visibility to ensure how we fix it doesn't cause a perf regression?
|
Thanks for the ping @BartekObudzinski can you please review this pr in relation to our performance change? thanks |
|
Thanks, yeah I quick perf check and this doesn't cause any regression. The violation logic here only runs on mutations, not on render, and the Search views don't touch any of it. rows still read violations from the snapshot like before, all good from me and perf perspective @mountiny @trjExpensify @mukhrr |
|
🚧 lakchote has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
8 participants
Explanation of Change
Fixed Issues
$ #93945
#93929
#93932
#93770
PROPOSAL:
Tests
#93945:
Precondition:
• Workspace has independant or dependant tags.
#93929:
• Prerequisite: Account has at least one workspace.
• Prerequisite 2: "Tags" and "Taxes" enabled.
• Prerequisite 3: One tag created and one tax available apart from default.
#93932:
#93770:
Preconditions:
Steps:
Verify without opening the expense row, the expense row's indicator should updates live after changing a workspace rule
Verify that no errors appear in the JS console
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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_app.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
IOS_app.mp4
iOS: mWeb Safari
IOS_web.mp4
MacOS: Chrome / Safari
web.mp4