Feat/dupe detection confirmation#42571
Conversation
…tkiewicz/expensify-app into feat/dupe-detection-confirmation
…tkiewicz/expensify-app into feat/dupe-detection-confirmation
…tkiewicz/expensify-app into feat/dupe-detection-confirmation
…tkiewicz/expensify-app into feat/dupe-detection-confirmation
…tkiewicz/expensify-app into feat/dupe-detection-confirmation
…tkiewicz/expensify-app into feat/dupe-detection-confirmation
…duplicates navigator
…dd api call to merge duplicates, fixes for displaying and aplaying taxes
…tkiewicz/expensify-app into feat/dupe-detection-confirmation
…tkiewicz/expensify-app into feat/dupe-detection-confirmation
…tkiewicz/expensify-app into feat/dupe-detection-confirmation
…tkiewicz/expensify-app into feat/dupe-detection-confirmation
…tkiewicz/expensify-app into feat/dupe-detection-confirmation
…tkiewicz/expensify-app into feat/dupe-detection-confirmation
|
@kubabutkiewicz when you're back on Monday, please make this PR your top focus. We're aiming to get this final PR for dupe detection merged by early next week! |
|
@parasharrajat It would have been great to have had that the checklist completed either way even though you found that small bug so it was ready to be merged this morning |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@pecanoro Thanks. That was the only bug I saw so we are good here. I was just going to add remaining screenshots now. |
|
🚀 Deployed to staging by https://github.com/pecanoro in version: 9.0.10-2 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.0.10-3 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.10-7 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.11-5 🚀
|
|
Coming from this issue #45814, It seems we missed disabling the ReceiptEmptyState on confirmation screen App/src/components/ReportActionItem/MoneyRequestView.tsx Lines 477 to 478 in 864c69f |
| shouldShowOfflineIndicator | ||
| > | ||
| <HeaderWithBackButton title={translate('iou.reviewDuplicates')} /> | ||
| <ScrollView style={styles.mb3}> |
There was a problem hiding this comment.
We should hide this page when user directly opens it via URL #45832
|
Another edge case where we should prevent direct access to the page if user tries to open it with invalid params. #45835 |
| @@ -257,7 +268,6 @@ function MoneyRequestView({ | |||
| if (hasReceipt) { | |||
| receiptURIs = ReceiptUtils.getThumbnailAndImageURIs(transaction); | |||
There was a problem hiding this comment.
You forgot to change this line to take updatedTransaction. This caused #47774
| </View> | ||
| {/* We need that provider here becuase MoneyRequestView component require that */} | ||
| <ShowContextMenuContext.Provider value={contextValue}> | ||
| <MoneyRequestView |
There was a problem hiding this comment.
MoneyRequestView should have known that the view is displayed for dupe detection as the display of transaction receipt image depends on money request report id but the transaction itself may not belong to the same report. This caused the transaction receipt image to shown a not found image in #50107
| ...restReviewDuplicateTransaction, | ||
| modifiedMerchant: reviewDuplicateTransaction?.merchant, | ||
| merchant: reviewDuplicateTransaction?.merchant, | ||
| comment: {comment: reviewDuplicateTransaction?.description}, |
There was a problem hiding this comment.
Omitting other reviewDuplicateTransaction.comment properties caused an issue where if default tax is selected, Default label is not shown next to tax name. More info: #47975
|
|
||
| function buildTransactionsMergeParams(reviewDuplicates: OnyxEntry<ReviewDuplicates>, originalTransaction: Partial<Transaction>): TransactionMergeParams { | ||
| return { | ||
| amount: -getAmount(originalTransaction as OnyxEntry<Transaction>, false), |
There was a problem hiding this comment.
{BZ CHECKLIST https://github.com/Expensify/App/issues/55404} There was regression that -ve amount was not correctly resolved in expense reports, we fixed it by passing true in getAmount second param
| const transactionsMergeParams = useMemo(() => TransactionUtils.buildTransactionsMergeParams(reviewDuplicates, transaction), [reviewDuplicates, transaction]); | ||
| const mergeDuplicates = useCallback(() => { | ||
| IOU.mergeDuplicates(transactionsMergeParams); | ||
| Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(reportAction?.childReportID ?? '-1')); |
There was a problem hiding this comment.
Coming from #58497. The resolve duplicates screen was still in the stack when tapping the back button after confirming duplicates. We should've used dismissModal before navigating here to resolve this.
Details
Fixed Issues
$ #39810
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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
MacOS: Desktop