auto select some values of transaction after deletion one of last 2 elements#86932
Conversation
…mplement auto-selection logic
…Is and implement auto-selection logic"
# Conflicts: # src/libs/actions/TaxRate.ts # src/pages/workspace/distanceRates/PolicyDistanceRateDetailsPage.tsx # src/pages/workspace/distanceRates/PolicyDistanceRatesPage.tsx
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.
|
melvin-bot
Bot
requested review from
hoangzinh and
joekaufmanexpensify
and removed request for
a team
|
@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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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: 84e8ff6d48
ℹ️ 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".
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
|
Is this PR ready for review? @mukhrr |
@hoangzinh yes, it is! |
|
@MelvinBot please review this PR |
PR ReviewOverall the implementation for category and tag auto-selection looks solid — correct gating on 1. Missing scope: tax rates and distance ratesThe approved proposal explicitly covers four fields: categories, tags, distance rates, and tax rates. This PR only implements categories and tags — it looks like tax/distance rate support was added initially then removed. Is that planned for a follow-up PR? If so, please note that in the PR description. If it's expected in this PR, it still needs to be added. 2.
|
|
@hoangzinh does Melvin's comment make sense? And are we okay to proceed or you want me to fix minor bugs it mentioned? |
@mukhrr can you check (2) and (3) in Melvin's review feeback? |
| // Recompute transaction violations so toggling tax tracking immediately clears/adds the tax violation, | ||
| // instead of leaving a stale one until the report reloads. | ||
| if (policyData) { | ||
| ReportUtils.pushTransactionViolationsOnyxData(onyxData, policyData, {tax: {trackingEnabled: enabled}}); |
There was a problem hiding this comment.
I think we need to pass the optimistic defaultTaxRates here if shouldAddDefaultTaxRatesData is true
There was a problem hiding this comment.
Good catch, done. When shouldAddDefaultTaxRatesData is true we optimistically write defaultTaxRates onto the policy, but the recompute was only getting {tax: {trackingEnabled: enabled}} — so getOptimisticPolicyState built a policy with tax tracking on but no rates, making isTaxInPolicy always false and flagging any transaction whose tax code is one of the new default rates as taxOutOfPolicy.
… to Onyx, so isTaxInPolicy is evaluated correctly.
|
@mukhrr lint is failing |
|
@lakchote done |
|
🚧 @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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.4.10-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 I reviewed the changes in this PR against the help site files under Help site changes are required. Most of this PR is internal violation-recomputation logic (bug fixes for when The tax equivalent is already documented in Track Taxes ("What happens if an expense with tax is moved to a workspace where tax tracking is disabled?"), so I added parallel FAQ entries for categories and tags to keep the docs consistent. Draft help site PR: #93865 It adds:
@mukhrr, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.4.12-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Docs review: no help site changes required. I reviewed the changes in this PR against the help site articles under Why: This PR is a set of bug fixes to expense violation display logic plus two new edge-case confirmation modals (
These changes restore the intended/expected product behavior — they do not introduce, remove, or rename any documented feature, setting label, tab, or workflow. The relevant help articles (Create expense categories, Create and manage expense tags, Track taxes, Require tags and categories for expenses) document how to set up and enable/disable these features, but do not document the in-expense violation messaging or confirmation dialogs that this PR touches. Their content remains accurate, so there is nothing to add or correct. @mukhrr, if you'd like the help site to start documenting the violation messages a member sees when a category/tag/tax becomes invalid (which would be a net-new doc rather than a change required by this PR), let me know and I can draft that separately. Otherwise, no draft help site PR was created since none is needed. |
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.4.14-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Correction to my previous comment — help site changes are required, and a draft PR already exists. My earlier comment in this session ( A draft help site PR covering exactly this already exists from a prior review and does not need to be recreated: Draft help site PR: #93865 It adds:
Please disregard my @mukhrr, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
I believe this shouldn't be deploy blocker, since we already fixed original bug in this PR and this one #93932 as bug 6 was decided to fix relatively. What do you think, @lakchote ? cc @hoangzinh |
|
Deploy Blocker #93945 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.4.14-1 🚀
|
- #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. - #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. - #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.
8 participants
Explanation of Change
We have fixed 8 bugs in total in this PR: #86220, #88338 and + 6 bugs found while implementing the PR.
Fixed Issues
$ #86220
#88338
PROPOSAL: #86220 (comment)
Tests
Bug 1:
Bug 2:
Bug 3
Bug 4
Bug 5
Bug 6
#86220:
Preconditions:
Verify both in preview and details page, tag and category are auto-selected to the last left tag/category
Verify that no errors appear in the JS console
Offline tests
#88338:
QA Steps
The same as tests
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.mp4
Android: mWeb Chrome
android.mp4
iOS: Native
ios_app.mp4
iOS: mWeb Safari
ios_web.mp4
MacOS: Chrome / Safari
web.mp4