[HOLD for payment 2024-10-14] [$250] [Dupe detection] Use new ResolveDuplicates when approver is resolving duplicates

[pecanoro]

Part of the Duplicate Detection project

Main issue: https://github.com/Expensify/Expensify/issues/411008
Project: Dupe detection

Feature Description

Right now, we always call MergeTransactions when resolving duplicates and click Confirm at the last page of resolving duplicates. However, we need to call a new command called ResolveDuplicates when an approver or admin (basically anyone but the submitter) is the person resolving the duplicates. The behaviour (so we can create the proper optimistic actions) should be as follows:

• ResolveDuplicates API endpoint needs the following parameters:

            'authToken' => $authToken,
            'transactionIDToKeep' => $transactionIDToKeep,
            'transactionIDList' => $transactionIDList,
            'created' => $created,
            'merchant' => $merchant,
            'amount' => $amount,
            'currency' => $currency,
            'category' => $category,
            'comment' => $comment,
            'billable' => $billable,
            'reimbursable' => $reimbursable,
            'tag' => $tag,
            'optimisticReportActionID' => $optimisticReportActionID,
            'reportActionIDList' => $reportActionIDList,

This command will update the transaction to match the fields we chose during the flow. It should already be doing this.

• It won't delete the rest of the transactions in transactionIDList but it will HOLD them by adding the hold NVP to it and will add the corresponding HOLD actions to each of them.
• It will add a DISMISSEDVIOLATION action to the transactionIDToKeep
• It will dismiss the violation by adding the corresponding transaction NVP for all transactions.

Main issue: https://github.com/Expensify/Expensify/issues/411008

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011781060ec2a6c428
• Upwork Job ID: 1830626997874011636
• Last Price Increase: 2024-09-02
• Automatic offers:
• nkdengineer | Contributor | 103792699

Issue Owner

Current Issue Owner: @

Issue Owner

Current Issue Owner: @parasharrajat

[pecanoro]

@parasharrajat I am assigning you as the C+ in this issue since you reviewed the main ones.

[parasharrajat]

Sure

[melvin-bot]

Triggered auto assignment to @sonialiap (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.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~011781060ec2a6c428

[melvin-bot]

Current assignee @parasharrajat is eligible for the External assigner, not assigning anyone new.

[nkdengineer]

Proposal

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

Add a new command for admin/approver when resolving the duplicate transaction from submitter

What is the root cause of that problem?

We're always calling mergeDuplicates function without checking the current user is admin/approver or not

App/src/pages/TransactionDuplicate/Confirmation.tsx

Lines 45 to 48 in e60b821

	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'));

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

1. To check the user is admin/approver we can use the same way we do here.

App/src/libs/ReportUtils.ts

Lines 3074 to 3079 in e60b821

	const isActionOwner =
	typeof parentReportAction?.actorAccountID === 'number' &&
	typeof currentUserPersonalDetails?.accountID === 'number' &&
	parentReportAction.actorAccountID === currentUserPersonalDetails?.accountID;
	const isApprover = isMoneyRequestReport(moneyRequestReport) && moneyRequestReport?.managerID !== null && currentUserPersonalDetails?.accountID === moneyRequestReport?.managerID;
	const isAdmin = isPolicyAdmin(moneyRequestReport.policyID ?? '-1', allPolicies);

2. Create a new function resolveDuplicates that will call ResolveDuplicates comman with the param as described here. It will be almost the same with mergeDuplicates, the only difference with mergeDuplicates is

It won't delete the rest of the transactions in transactionIDList but it will HOLD them by adding the hold NVP to it and will add the corresponding HOLD actions to each of them.

• We will not delete the transaction in transactionIDList. Instead, we will add a hold action for these transaction thread reports in optimistic data and reset in failure data and also add hold violation for these transaction

It will add a DISMISSEDVIOLATION action to the transactionIDToKeep

• Add a DISMISSEDVIOLATION action with reason in originalMessage is duplicatedTransaction for the transaction thread report of transactionIDToKeep list

It will dismiss the violation by adding the corresponding transaction NVP for all transactions.

• Remove violation of all transactions in transactionIDToKeep

What alternative solutions did you explore? (Optional)

[parasharrajat]

The proposal sounds good @nkdengineer sounds good to me.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[pecanoro]

Assigning @nkdengineer!

[melvin-bot]

📣 @nkdengineer 🎉 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 📖

[nkdengineer]

Will raise the PR tomorrow.

[nkdengineer]

@pecanoro In the ResolveDuplicates API, what is the structure of reportActionIDList params?

[pecanoro]

In the ResolveDuplicates API, what is the structure of reportActionIDList params?

A comma-separated list of report actions

[nkdengineer]

A comma-separated list of report actions

@pecanoro Can you give an example? Do we need to add reportID in data?

[pecanoro]

@pecanoro Can you give an example?

Imagine we have 3 duplicates with transactionIDs 111, 222 and 333. We want to keep 111 and hold the other two. Then:

'transactionIDToKeep' => '111',
'transactionIDList' => '222,333',

Do we need to add reportID in data?

Nope, no reportID since they are not changing reports

[nkdengineer]

@pecanoro I mean I need an example for reportActionIDList param.

[nkdengineer]

The payload here.

[pecanoro]

There is something off, also because both hold actions are returned for the same report

[pecanoro]

Ah nvm, forget my previous comment, you were resolving duplicates with only two transactions. I am going to check the database

[pecanoro]

Ah I found the problem, another parameter name that got cleared because of sanitization. I am going to rename it to something better either way. I will create a PR in the back-end

[pecanoro]

@nkdengineer BE fix is on staging, you can test it in the PR now! New parameter is called dismissedViolationReportActionID instead of optimisticReportActionID

[parasharrajat]

@pecanoro Does this change affect keep All functionality as well? Do we need to change that too?

[pecanoro]

@parasharrajat No, it should not change it

[parasharrajat]

Got it. Thanks.

[parasharrajat]

Note for future: I noticed that when admin resolves a duplicate, the transaction is no more duplicate for admin but same transaction still shown duplicate for the original user who requested. I confirmed that this is expected #48522 (comment)

[nkdengineer]

@parasharrajat The original PR was reverted so I created a new one here to fix another bug #49900

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.45-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• implement resolveDuplicates API and fix regression #50035

If no regressions arise, payment will be issued on 2024-10-14. 🎊

For reference, here are some details about the assignees on this issue:

• @parasharrajat requires payment through NewDot Manual Requests
• @nkdengineer requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:
• [@parasharrajat] 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:
• [@parasharrajat] 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:
• [@parasharrajat] Determine if we should create a regression test for this bug.
• [@parasharrajat] 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.
• [@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[sonialiap]

Payment summary:

• @parasharrajat $250 please request in ND and complete the checklist
• @nkdengineer $250 paid in upwork ✔️

[parasharrajat]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: New Feature
• [@parasharrajat] 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: New feature
• [@parasharrajat] 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: Not needed
• [@parasharrajat] Determine if we should create a regression test for this bug. Yes
• [@parasharrajat] 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 Steps

1. [Employee] Submit two expenses that are duplicated
2. [Admin] Go to the transaction thread of a transaction
3. [Admin] Review the duplicate and chose any transaction to keep
4. [Admin] Confirm to resolve the duplicate
5. Verify that the transaction thread of the kept transaction has a dismiss violation action resolved the duplicate
6. The preview of other duplicate transactions has hold message and the transaction thread has a hold action
7. Click on back button browser
8. Verify that the confirmation page doesn't appear

Do you agree 👍 or 👎 ?