[No QA] Revert: Merging tax rates#83233
Conversation
dominictb
changed the title
melvin-bot
bot
requested review from
heyjennahay and
suneox
and removed request for
a team
dominictb
requested review from
hoangzinh,
puneetlath and
youssef-lr
and removed request for
heyjennahay,
suneox and
youssef-lr
![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: 1e1dedfc73
ℹ️ 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".
| ] | ||
| : []; | ||
| const optimisticSourceReportData: Array<OnyxUpdate<typeof ONYXKEYS.COLLECTION.REPORT>> = | ||
| transactionsOfSourceReport.length === 1 |
There was a problem hiding this comment.
When merging two single-expense reports, users can choose the source report as the final reportID; in that case the merged expense should remain in sourceTransaction.reportID, but this condition now deletes the source report whenever transactionsOfSourceReport.length === 1. Because the success path does not recreate that report key, the optimistic update can remove an active report from Onyx and leave navigation/state inconsistent until a later full refresh.
Useful? React with 👍 / 👎.
| const formattedTaxAmount = updatedTransaction?.taxAmount | ||
| ? convertToDisplayString(Math.abs(updatedTransaction?.taxAmount), transactionCurrency) | ||
| : convertToDisplayString(Math.abs(transactionTaxAmount ?? 0), transactionCurrency); |
There was a problem hiding this comment.
The new truthy check for updatedTransaction?.taxAmount treats 0 as “no updated value”, so if a user changes tax to zero (for example via a 0% rate), the UI falls back to transactionTaxAmount and can show an outdated non-zero tax amount in the merge/preview display. The previous !== undefined behavior handled zero correctly, so this introduces incorrect totals in that scenario.
Useful? React with 👍 / 👎.
|
Merging since this is a straight revert. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @puneetlath 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! 🧪🧪
|
[No QA] Revert: Merging tax rates (cherry picked from commit 15f27bb)
[No QA] Revert: Merging tax rates (cherry picked from commit 15f27bb) (cherry-picked to staging by puneetlath)
[No QA] Revert: Merging tax rates (cherry picked from commit 15f27bb) (cherry-picked to staging by puneetlath)
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.
|
…320187993-1 🍒 Cherry pick PR #83233 to staging 🍒
|
🚀 Cherry-picked to staging by https://github.com/puneetlath in version: 9.3.24-3 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.24-3 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/puneetlath in version: 9.3.25-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.25-13 🚀
|
3 participants
Explanation of Change
Reverts #71244
Fixed Issues
$ #70058
$ #83082
$ #83083
$ #83086
$ #83089
$ #83093
$ #83157
$ #83098
$ #83153
$ #83157
$ #83165
$ #83180
PROPOSAL:
Tests
NA. Straight revert
Offline tests
QA Steps
NA. Straight revert
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari