[$250] Dupe detection - "Keep this one" button appears when there is a paid expense #48366

IuliiaHerets · 2024-08-30T20:07:47Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: v9.0.27-0
Reproducible in staging?: Y
Reproducible in production?: N
Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com
**Issue reported by:**Applause Internal Team

Action Performed:

Go to staging.new.expensify.com
Go to workspace chat.
Submit an expense and pay it.
Submit another two same expenses as Step 3.
Go to transaction thread (any expense).
Click Review duplicates.
Note that Keep this one button does not appear when there is a paid expense.
Click Keep all.
Submit another expense that is same as the previous expenses.
Go to transaction thread of the new expense.
Click Review duplicates.
Note that now "Keep this one" button appears for the other two expenses in Step 4 which did not have the button previously and only the newest expense does not have the button.

Expected Result:

"Keep this one" button should not appear for all expenses in Review duplicates flow when there is a paid expense.

Actual Result:

"Keep this one" button appears for all expenses in Review duplicates flow when there is a paid expense.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Bug6588098_1725045313385.20240831_025446.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01647e38725804bb80
Upwork Job ID: 1829614254279842129
Last Price Increase: 2024-08-30
Automatic offers:

c3024 | Reviewer | 103803663
Krishna2323 | Contributor | 103803665

Issue Owner

Current Issue Owner: @VictoriaExpensify

melvin-bot · 2024-08-30T20:07:56Z

Triggered auto assignment to @marcochavezf (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

melvin-bot · 2024-08-30T20:07:57Z

Triggered auto assignment to @VictoriaExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

github-actions · 2024-08-30T20:08:00Z

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

marcochavezf · 2024-08-30T20:16:37Z

Dupe detection is under beta

melvin-bot · 2024-08-30T20:16:47Z

Job added to Upwork: https://www.upwork.com/jobs/~01647e38725804bb80

melvin-bot · 2024-08-30T20:16:52Z

Triggered auto assignment to Contributor-plus team member for initial proposal review - @c3024 (External)

Nodebrute · 2024-08-30T20:20:21Z

@marcochavezf Could you please update the actual result and expected result? They are same.

Nodebrute · 2024-08-30T20:33:12Z

~~Regression from this PR.~~,Based on this comment it seems we decided to keep this option hidden when there is a submitted expense.

Krishna2323 · 2024-08-30T21:43:45Z

Edited by proposal-police: This proposal was edited at 2024-08-30 22:12:14 UTC.

Proposal 

Please re-state the problem that we are trying to solve in this issue.

Dupe detection - "Keep this one" button appears when there is a paid expense

What is the root cause of that problem?

When we select Keep all first time the duplicate transaction violation is removed and when we review the duplicates second time the allDuplicates & duplicates length will be 0 and shouldShowKeepButton will become true.

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Lines 127 to 139 in 6659109

    
           const allDuplicates = useMemo( 
        
               () => 
        
                   transactionViolations?.[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction?.transactionID}`]?.find( 
        
                       (violation) => violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION, 
        
                   )?.data?.duplicates ?? [], 
        
               [transaction?.transactionID, transactionViolations], 
        
           ); 
        
           // Remove settled transactions from duplicates 
        
           const duplicates = useMemo(() => TransactionUtils.removeSettledAndApprovedTransactions(allDuplicates), [allDuplicates]); 
        
           // When there are no settled transactions in duplicates, show the "Keep this one" button 
        
           const shouldShowKeepButton = allDuplicates.length === duplicates.length;

What changes do you think we should make in order to solve the problem? 

If the length of allDuplicates & duplicates is zero, shouldShowKeepButton should become false.

const shouldShowKeepButton = !!(allDuplicates.length && allDuplicates.length === duplicates.length);
// OR
const shouldShowKeepButton = !!(allDuplicates.length && duplicates.length && allDuplicates.length === duplicates.length);

What alternative solutions did you explore? (Optional)

As the resolved transactions will not any duplicates because the violatiion is already dismissed, we can use the threadReportID from params const transactionID = TransactionUtils.getTransactionID(route.params.threadReportID ?? '');. We already have reviewingTransactionID so we can use that instead.

Then use the new transactionID to get the duplicates.

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Lines 127 to 139 in 6659109

    
           const allDuplicates = useMemo( 
        
               () => 
        
                   transactionViolations?.[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction?.transactionID}`]?.find( 
        
                       (violation) => violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION, 
        
                   )?.data?.duplicates ?? [], 
        
               [transaction?.transactionID, transactionViolations], 
        
           ); 
        
           // Remove settled transactions from duplicates 
        
           const duplicates = useMemo(() => TransactionUtils.removeSettledAndApprovedTransactions(allDuplicates), [allDuplicates]); 
        
           // When there are no settled transactions in duplicates, show the "Keep this one" button 
        
           const shouldShowKeepButton = allDuplicates.length === duplicates.length;

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Line 150 in 6659109

    
           const reviewingTransactionID = ReportActionsUtils.isMoneyRequestAction(parentReportAction) ? ReportActionsUtils.getOriginalMessage(parentReportAction)?.IOUTransactionID ?? '-1' : '-1';

Result

Krishna2323 · 2024-08-30T22:12:23Z

Proposal Updated

Added alternative

c3024 · 2024-08-31T06:03:07Z

Each transaction seems to have its own MoneyRequestPreviewContent, so it looks more appropriate to check the lengths of allDuplicates and duplicates for that transaction, as suggested in the main solution of @Krishna2323's proposal, rather than getting violations from the transaction that led us to the review duplicates modal, as suggested in the alternative solution.

IMO we should proceed with the primary solution suggested by @Krishna2323.

🎀 👀 🎀 C+ Reviewed

melvin-bot · 2024-08-31T06:03:15Z

Current assignee @marcochavezf is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

dominictb · 2024-08-31T09:13:38Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

"Keep this one" button appears for all expenses in Review duplicates flow when there is a paid expense.

What is the root cause of that problem?

If there was a paid expense, we want to:

Hide the "Keep this one" button.
Display text "Some of these duplicates have been approved or paid already." below the "Keep all" button.

But we are using different conditions to display the above.

With (1), we are using:

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Line 136 in 6659109

    
           const duplicates = useMemo(() => TransactionUtils.removeSettledAndApprovedTransactions(allDuplicates), [allDuplicates]);

With (2), we are using:

App/src/pages/TransactionDuplicate/Review.tsx

Line 47 in 6659109

    
           const hasSettledOrApprovedTransaction = transactions.some((transaction) => ReportUtils.isSettled(transaction?.reportID) || ReportUtils.isReportApproved(transaction?.reportID));

In case of this bug, assume in step 8, we the "Keep all" will apply to 3 transactions A (paid expense), B, C. Then in step 9, we create an additional transaction D. It will clear all transactionVolitions data in A, B, C.
Then in step 12, the allDuplicates in B and C is [] because of:

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Line 127 in 6659109

const allDuplicates = useMemo(

hence shouldShowKeepButton will be true because of:

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Line 139 in 6659109

const shouldShowKeepButton = allDuplicates.length === duplicates.length;

hence the "Keep this one" button appear in case of B and C.

What changes do you think we should make in order to solve the problem?

To display (1), we should use the same condition as we used with (1), that is:

App/src/pages/TransactionDuplicate/Review.tsx

Line 47 in 6659109

    
           const hasSettledOrApprovedTransaction = transactions.some((transaction) => ReportUtils.isSettled(transaction?.reportID) || ReportUtils.isReportApproved(transaction?.reportID));

In details, we should pass down the hasSettledOrApprovedTransaction in Review page to its childs MoneyRequestPreviewContent, so:

App/src/pages/TransactionDuplicate/Review.tsx

Line 60 in 6659109

<DuplicateTransactionsList transactions={transactions} />

will be:

                <DuplicateTransactionsList hasSettledOrApprovedTransaction={hasSettledOrApprovedTransaction} transactions={transactions} />

Then in MoneyRequestPreviewContent, condition:

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Line 447 in 6659109

    
           {isReviewDuplicateTransactionPage && !isSettled && !isApproved && shouldShowKeepButton && (

will be:

            {isReviewDuplicateTransactionPage && !isSettled && !isApproved && !hasSettledOrApprovedTransaction && (

What alternative solutions did you explore? (Optional)

dominictb · 2024-08-31T09:17:05Z

@c3024 Sorry for my lateness, and you have already reviewed the existing proposal. But can you review my proposal above? It will make the logic more consistent.

Krishna2323 · 2024-09-03T02:25:44Z

@marcochavezf, friendly bump for assignment if you agree with the C+ decision here.

dominictb · 2024-09-03T02:47:27Z

@c3024 Can you check my comment? cc @marcochavezf

melvin-bot · 2024-09-03T18:31:38Z

@marcochavezf, @VictoriaExpensify, @c3024 Whoops! This issue is 2 days overdue. Let's get this updated quick!

marcochavezf · 2024-09-03T21:11:55Z

Hey guys, thanks for the patience here. I agree with @c3024's decision, assigning @Krishna2323 🚀

@dominictb thanks for bringing this up, if we find out that we'd need to include part of your proposal in the final PR, we can make a partial payment :)

melvin-bot · 2024-09-03T21:12:11Z

📣 @c3024 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

melvin-bot · 2024-09-03T21:12:15Z

📣 @Krishna2323 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

Krishna2323 · 2024-09-04T15:15:00Z

@c3024, PR ready for review ^

VictoriaExpensify · 2024-09-11T06:14:35Z

Bump @c3024 - can you please review @Krishna2323 PR?

c3024 · 2024-09-11T06:15:54Z

I already reviewed the PR. It was merged and deployed to staging 5 days ago.

Krishna2323 · 2024-09-11T06:15:58Z

@VictoriaExpensify, PR is merged and deployed to staging 5 days ago :)

c3024 · 2024-09-14T09:55:44Z

PR deployed to production on 11-Sep. Automation failed here and on the PR. I think this should be on HOLD for payment till 18-Sep or 19-Sep.

cc: @VictoriaExpensify

c3024 · 2024-09-23T15:22:24Z

@VictoriaExpensify, gentle bump for payment. Automation failed here.

Krishna2323 · 2024-09-26T19:13:54Z

@VictoriaExpensify, friendly bump for payment 🫠

c3024 · 2024-09-30T06:54:29Z

[@c3024] The PR that introduced the bug has been identified. Link to the PR: fix: remove settled transactions from duplicates #47479

[@c3024] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/47479/files#r1780524724

[@c3024] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: No discussion was started because this could not have been identified earlier.

[@c3024] Determine if we should create a regression test for this bug. Yes

[@c3024] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression test proposal

Go to a workspace chat.
Submit an expense and get it paid.
Submit two more expenses with same details (amount, merchant etc.) as Step 2 in the same workspace chat.
Click on the report preview in the workspace chat
Click on any of the transactions to go to transaction thread
Click on "Review duplicates".
Click "Keep all".
Go back to the workspace chat and submit another expense with same details as in Step 2.
Go to the transaction thread of this expense.
Click on "Review duplicates".
Verify "Keep this one" button does not appear for any duplicate expense.

mallenexpensify · 2024-09-30T18:27:06Z

Contributor: @Krishna2323 paid $250 via Upwork
Contributor+: @c3024 paid $250 via Upwork.

Test case created, thx @c3024 and @Krishna2323

https://github.com/Expensify/Expensify/issues/432112

IuliiaHerets added DeployBlockerCash This issue or pull request should block deployment Bug Something is broken. Auto assigns a BugZero manager. labels Aug 30, 2024

melvin-bot bot assigned marcochavezf Aug 30, 2024

melvin-bot bot assigned VictoriaExpensify Aug 30, 2024

melvin-bot bot added the Daily KSv2 label Aug 30, 2024

github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Aug 30, 2024

github-actions bot mentioned this issue Aug 30, 2024

Deploy Checklist: New Expensify 2024-08-30 #48360

Closed

45 tasks

marcochavezf added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Aug 30, 2024

marcochavezf added the External Added to denote the issue can be worked on by a contributor label Aug 30, 2024

melvin-bot bot changed the title ~~Dupe detection - "Keep this one" button appears when there is a paid expense~~ [$250] Dupe detection - "Keep this one" button appears when there is a paid expense Aug 30, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 30, 2024

melvin-bot bot assigned c3024 Aug 30, 2024

melvin-bot bot added the Overdue label Sep 3, 2024

marcochavezf assigned Krishna2323 Sep 3, 2024

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 3, 2024

melvin-bot bot removed the Overdue label Sep 3, 2024

Krishna2323 mentioned this issue Sep 4, 2024

fix: Dupe detection - 'Keep this one' button appears when there is a paid expense. #48557

Merged

50 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 4, 2024

c3024 mentioned this issue Sep 30, 2024

fix: remove settled transactions from duplicates #47479

Merged

50 tasks

mallenexpensify closed this as completed Sep 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[$250] Dupe detection - "Keep this one" button appears when there is a paid expense #48366

[$250] Dupe detection - "Keep this one" button appears when there is a paid expense #48366

IuliiaHerets commented Aug 30, 2024 •

edited by marcochavezf

Loading

melvin-bot bot commented Aug 30, 2024

melvin-bot bot commented Aug 30, 2024

github-actions bot commented Aug 30, 2024

marcochavezf commented Aug 30, 2024

melvin-bot bot commented Aug 30, 2024

melvin-bot bot commented Aug 30, 2024

Nodebrute commented Aug 30, 2024

Nodebrute commented Aug 30, 2024 •

edited

Loading

Krishna2323 commented Aug 30, 2024 •

edited

Loading

Krishna2323 commented Aug 30, 2024

c3024 commented Aug 31, 2024

melvin-bot bot commented Aug 31, 2024

dominictb commented Aug 31, 2024

dominictb commented Aug 31, 2024

Krishna2323 commented Sep 3, 2024

dominictb commented Sep 3, 2024

melvin-bot bot commented Sep 3, 2024

marcochavezf commented Sep 3, 2024

melvin-bot bot commented Sep 3, 2024

melvin-bot bot commented Sep 3, 2024

Krishna2323 commented Sep 4, 2024

VictoriaExpensify commented Sep 11, 2024

c3024 commented Sep 11, 2024

Krishna2323 commented Sep 11, 2024

c3024 commented Sep 14, 2024

c3024 commented Sep 23, 2024

Krishna2323 commented Sep 26, 2024

c3024 commented Sep 30, 2024

mallenexpensify commented Sep 30, 2024

[$250] Dupe detection - "Keep this one" button appears when there is a paid expense #48366

[$250] Dupe detection - "Keep this one" button appears when there is a paid expense #48366

Comments

IuliiaHerets commented Aug 30, 2024 • edited by marcochavezf Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Aug 30, 2024

melvin-bot bot commented Aug 30, 2024

github-actions bot commented Aug 30, 2024

marcochavezf commented Aug 30, 2024

melvin-bot bot commented Aug 30, 2024

melvin-bot bot commented Aug 30, 2024

Nodebrute commented Aug 30, 2024

Nodebrute commented Aug 30, 2024 • edited Loading

Krishna2323 commented Aug 30, 2024 • edited Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

Result

Krishna2323 commented Aug 30, 2024

Proposal Updated

c3024 commented Aug 31, 2024

melvin-bot bot commented Aug 31, 2024

dominictb commented Aug 31, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

dominictb commented Aug 31, 2024

Krishna2323 commented Sep 3, 2024

dominictb commented Sep 3, 2024

melvin-bot bot commented Sep 3, 2024

marcochavezf commented Sep 3, 2024

melvin-bot bot commented Sep 3, 2024

melvin-bot bot commented Sep 3, 2024

Krishna2323 commented Sep 4, 2024

VictoriaExpensify commented Sep 11, 2024

c3024 commented Sep 11, 2024

Krishna2323 commented Sep 11, 2024

c3024 commented Sep 14, 2024

c3024 commented Sep 23, 2024

Krishna2323 commented Sep 26, 2024

c3024 commented Sep 30, 2024

Regression test proposal

mallenexpensify commented Sep 30, 2024

IuliiaHerets commented Aug 30, 2024 •

edited by marcochavezf

Loading

Nodebrute commented Aug 30, 2024 •

edited

Loading

Krishna2323 commented Aug 30, 2024 •

edited

Loading

Proposal 

What changes do you think we should make in order to solve the problem? 