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

IuliiaHerets commented 2 weeks ago

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?

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/b6fcb1c2-51d5-4ea2-a787-be5bf7db6752

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: @Krishna2323

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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[bot] commented 2 weeks ago

:wave: 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 commented 2 weeks ago

Dupe detection is under beta

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

Nodebrute commented 2 weeks ago

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

Nodebrute commented 2 weeks ago

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

Krishna2323 commented 2 weeks ago

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. https://github.com/Expensify/App/blob/6659109a023db44d4021ef1dfdaf8cbc21191c76/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L127-L139

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. https://github.com/Expensify/App/blob/6659109a023db44d4021ef1dfdaf8cbc21191c76/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L127-L139 https://github.com/Expensify/App/blob/6659109a023db44d4021ef1dfdaf8cbc21191c76/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L150
Result

Krishna2323 commented 2 weeks ago

Proposal Updated

Added alternative

c3024 commented 2 weeks ago

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[bot] commented 2 weeks ago

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

dominictb commented 2 weeks ago

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: https://github.com/Expensify/App/blob/6659109a023db44d4021ef1dfdaf8cbc21191c76/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L136

With (2), we are using:

https://github.com/Expensify/App/blob/6659109a023db44d4021ef1dfdaf8cbc21191c76/src/pages/TransactionDuplicate/Review.tsx#L47

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: https://github.com/Expensify/App/blob/6659109a023db44d4021ef1dfdaf8cbc21191c76/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L127 hence shouldShowKeepButton will be true because of: https://github.com/Expensify/App/blob/6659109a023db44d4021ef1dfdaf8cbc21191c76/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L139

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:

https://github.com/Expensify/App/blob/6659109a023db44d4021ef1dfdaf8cbc21191c76/src/pages/TransactionDuplicate/Review.tsx#L47

In details, we should pass down the hasSettledOrApprovedTransaction in Review page to its childs MoneyRequestPreviewContent, so: https://github.com/Expensify/App/blob/6659109a023db44d4021ef1dfdaf8cbc21191c76/src/pages/TransactionDuplicate/Review.tsx#L60 will be:
```
            <DuplicateTransactionsList hasSettledOrApprovedTransaction={hasSettledOrApprovedTransaction} transactions={transactions} />
```
Then in MoneyRequestPreviewContent, condition:

https://github.com/Expensify/App/blob/6659109a023db44d4021ef1dfdaf8cbc21191c76/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L447

will be:

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

What alternative solutions did you explore? (Optional)

dominictb commented 2 weeks ago

@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 commented 1 week ago

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

dominictb commented 1 week ago

@c3024 Can you check my comment? cc @marcochavezf

melvin-bot[bot] commented 1 week ago

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

marcochavezf commented 1 week ago

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[bot] commented 1 week ago

📣 @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[bot] commented 1 week ago

📣 @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 commented 1 week ago

@c3024, PR ready for review ^

VictoriaExpensify commented 4 days ago

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

c3024 commented 4 days ago

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

Krishna2323 commented 4 days ago

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

c3024 commented 1 day ago

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

Expensify / App