Show ConfirmationPage on duplicate review page when duplicates are resolved#83661
Show ConfirmationPage on duplicate review page when duplicates are resolved#83661
Conversation
When duplicates have already been resolved via "Keep All", navigating forward to the review page now shows a ConfirmationPage with a descriptive message instead of a broken review UI with no duplicates. Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: View the translation diffdiff --git a/src/languages/it.ts b/src/languages/it.ts
index 4838f1f5..ea9dfaf2 100644
--- a/src/languages/it.ts
+++ b/src/languages/it.ts
@@ -1416,7 +1416,7 @@ const translations: TranslationDeepObject<typeof en> = {
expensesOnHold: 'Tutte le spese sono state messe in sospeso. Controlla i commenti per conoscere i prossimi passaggi.',
expenseDuplicate: 'Questa spesa ha dettagli simili a un’altra. Controlla i duplicati per continuare.',
someDuplicatesArePaid: 'Alcuni di questi duplicati sono già stati approvati o pagati.',
- reviewDuplicates: 'Controlla i duplicati',
+ reviewDuplicates: 'Controlla duplicati',
keepAll: 'Mantieni tutto',
confirmApprove: 'Conferma l’importo approvato',
confirmApprovalAmount: 'Approva solo le spese conformi oppure approva l’intero rapporto.',
diff --git a/src/languages/ja.ts b/src/languages/ja.ts
index 3d5380b2..4c38e392 100644
--- a/src/languages/ja.ts
+++ b/src/languages/ja.ts
@@ -1405,7 +1405,7 @@ const translations: TranslationDeepObject<typeof en> = {
expenseOnHold: 'この経費は保留されています。今後の手順についてコメントを確認してください。',
expensesOnHold: 'すべての経費が保留になっています。次のステップについてはコメントを確認してください。',
expenseDuplicate: 'この経費は別の経費と詳細がよく似ています。続行するには重複しているものを確認してください。',
- someDuplicatesArePaid: 'これらの重複のうち、いくつかはすでに承認または支払い済みです。',
+ someDuplicatesArePaid: 'これらの重複の一部は、すでに承認または支払い済みです。',
reviewDuplicates: '重複を確認',
keepAll: 'すべて保持',
confirmApprove: '承認金額を確認',
diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts
index 32ca2927..ddce2901 100644
--- a/src/languages/zh-hans.ts
+++ b/src/languages/zh-hans.ts
@@ -1385,8 +1385,8 @@ const translations: TranslationDeepObject<typeof en> = {
expenseOnHold: '此报销已被搁置。请查看评论以了解下一步操作。',
expensesOnHold: '所有报销都已被搁置。请查看评论以了解下一步操作。',
expenseDuplicate: '此报销与另一笔报销的明细相似。请查看可能重复的报销后再继续。',
- someDuplicatesArePaid: '其中有些重复项已经被批准或支付。',
- reviewDuplicates: '查看重复项',
+ someDuplicatesArePaid: '其中一些重复项已被批准或支付。',
+ reviewDuplicates: '审核重复项',
keepAll: '全部保留',
confirmApprove: '确认批准金额',
confirmApprovalAmount: '仅批准合规报销，或批准整份报销报告。',
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
…s to all language files Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
Fixed the failing |
|
@MelvinBot Please apply the patch from #83661 (comment) so that translations are added correctly for all other languages |
…ates keys Applied the translation quality improvements from the Polyglot Parrot bot for it.ts, ja.ts, and zh-hans.ts. Also translated the noDuplicatesTitle and noDuplicatesDescription keys from English to the proper language for all non-English language files (de, fr, it, ja, nl, pl, pt-BR, zh-hans). Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
Applied the Polyglot Parrot translation patch and translated the Changes made:
Commit: a60dd6b |
Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
Fixed the failing Prettier check: |
|
@MelvinBot Mark the PR as "Ready for Review" |
melvin-bot
bot
requested review from
joekaufmanexpensify and
rojiphil
and removed request for
a team
|
PR has been marked as Ready for Review. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp83661-android-hybrid-001.mp4Android: mWeb Chrome83661-mweb-chrome-001.mp4iOS: HybridApp83661-ios-hybrid-001.mp4iOS: mWeb Safari83661-mweb-safari-002.mp4MacOS: Chrome / Safari83661-web-chrome-001.mp4 |
rojiphil
left a comment
rojiphil
left a comment
There was a problem hiding this comment.
@JS00001 @joekaufmanexpensify Changes LGTM and works well too.
All yours. Thanks.
![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: 3d7153bc3a
ℹ️ 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".
| ); | ||
| } | ||
|
|
||
| if (transactionID && duplicateTransactionIDs.length === 0) { |
There was a problem hiding this comment.
This new branch can run even when the transaction has been deleted, because it only checks transactionID and duplicateTransactionIDs.length === 0 and does not guard on wasTransactionDeleted/shouldShowNotFound. In the deleted-parent-action case (pendingAction: DELETE), transactionID can still be present, so reopening this route can now show an “All set” confirmation instead of the existing not-found handling, which misrepresents a deleted expense as resolved duplicates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid catch. Added a !shouldShowNotFound guard to the condition so that deleted transactions correctly fall through to the FullPageNotFoundView instead of showing the "All set" confirmation page.
When a transaction is deleted, transactionID can still be present while duplicateTransactionIDs is empty. Without checking shouldShowNotFound, the ConfirmationPage would render instead of the FullPageNotFoundView, misrepresenting a deleted expense as resolved duplicates. Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
rojiphil
left a comment
rojiphil
left a comment
There was a problem hiding this comment.
LGTM
83661-web-chrome-002.mp4
|
🚧 @JS00001 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/JS00001 in version: 9.3.28-0 🚀
|
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.3.30-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.30-3 🚀
|
4 participants
Explanation of Change
When a user resolves duplicate expenses by clicking "Keep All" on the duplicates review page, pressing the browser forward button would re-open the review page in a broken state (showing a single transaction with no actual duplicates to review). This happened because the
shouldShowNotFoundguard inReview.tsxonly checked if the transaction was deleted, not whether duplicates still existed.This PR adds a check for
duplicateTransactionIDs.length === 0after the page has loaded. When duplicates have already been resolved and the user navigates forward to the review page, aConfirmationPagecomponent is rendered with a descriptive message ("All set! There are no duplicate transactions for review here.") and a "Got it" button to navigate back. This provides better UX than showing "Not Found" since the transaction itself still exists — only the duplicates have been resolved.Fixed Issues
$ #83532
PROPOSAL: #83532 (comment)
Tests
Offline tests
QA Steps
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