[HOLD for payment 2024-10-30] [$250] [Dupe detection] Report header has "Hold" action when the expense is already in Hold status

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.41-1
Reproducible in staging?: Y
Reproducible in production?: N/A - new feature, doesn't exist in prod
Issue was found when executing this PR: #48522
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. [Employee] Submit two same expenses in the workspace chat.
3. [Admin] Go to any of the submitted expense in the workspace chat.
4. [Admin] Go offline.
5. [Admin] Click Review duplicates.
6. [Admin] Click Keep this one (any).
7. [Admin] Click Confirm on the confirmation page.
8. [Admin] Go to the other transaction thread that has "Hold" action in the expense preview.
9. [Admin] Click on the report header.

Expected Result:

Since the other expense has "Hold" action, the report header should have the "Unhold" action.

Actual Result:

The report header has "Hold" action when the expense is already in Hold status.

Workaround:

Unknown

Platforms:

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6617758_1727464896179.20240928_031804.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021840780088904015602
• Upwork Job ID: 1840780088904015602
• Last Price Increase: 2024-09-30

Issue Owner

Current Issue Owner: @stephanieelliott

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @stephanieelliott (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]

👋 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:

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

[IuliiaHerets]

On prod, the other expense vanishes instantly

bandicam.2024-09-28.00-52-50-976.mp4

[IuliiaHerets]

We think that this bug might be related to #wave-collect - Release 1

[roryabraham]

Dupe detection is behind a beta w/ only internal users and developers, this is NAB

[daledah]

Edited by proposal-police: This proposal was edited at 2024-09-29 10:06:58 UTC.

Proposal

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

The report header has "Hold" action when the expense is already in Hold status.

What is the root cause of that problem?

In

App/src/libs/TransactionUtils/index.ts

Lines 703 to 709 in 3047c1b

	function isOnHold(transaction: OnyxEntry<Transaction>): boolean {
	if (!transaction) {
	return false;
	}
	return !!transaction.comment?.hold || isDuplicate(transaction.transactionID, true);
	}

We check if a transaction is on hold if either: transaction has a hold comment, or transaction is duplicate

The second condition returns false when we resolve duplicates, because of:

App/src/libs/actions/IOU.ts

Lines 8261 to 8270 in 3047c1b

	const optimisticTransactionViolations: OnyxUpdate[] = [...params.transactionIDList, params.transactionID].map((id) => {
	const violations = allTransactionViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${id}`] ?? [];
	const newViolation = {name: CONST.VIOLATIONS.HOLD, type: CONST.VIOLATION_TYPES.VIOLATION};
	const updatedViolations = id === params.transactionID ? violations : [...violations, newViolation];
	return {
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${id}`,
	value: updatedViolations.filter((violation) => violation.name !== CONST.VIOLATIONS.DUPLICATED_TRANSACTION),
	};
	});

and

App/src/libs/TransactionUtils/index.ts

Lines 689 to 697 in 3047c1b

	const hasDuplicatedViolation = !!allTransactionViolations?.[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`]?.some(
	(violation: TransactionViolation) => violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION,
	);
	if (!checkDismissed) {
	return hasDuplicatedViolation;
	}
	const didDismissedViolation =
	allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`]?.comment?.dismissedViolations?.duplicatedTransaction?.[currentUserEmail] === `${currentUserAccountID}`;
	return hasDuplicatedViolation && !didDismissedViolation;

so we only need to focus on the first condition.

When an admin resolves an employee's duplicate, we call resolveDuplicates:

App/src/libs/actions/IOU.ts

Line 8233 in 3047c1b

function resolveDuplicates(params: TransactionMergeParams) {

Let's look at the onyx data returned by server when online:

We see that it sets a hold property which has value of report action's id in the report, effectively make isOnHold's first condition to return true. However in resolveDuplicates, we don't have logic to set optimistic data for this property yet, which makes isOnHold returns false, the button show Hold message when user is offline.

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

Update resolveDuplicates to add optimistic data for hold actions aswell

The logic should be added in this loop:

App/src/libs/actions/IOU.ts

Line 8291 in 3047c1b

transactionThreadReportIDList.forEach((transactionThreadReportID) => {

Pseudo code:

const optimisticHoldTransactionActions = []
const failureHoldTransactionActions = []
transactionThreadReportIDList.forEach((transactionThreadReportID) => {
        const createdReportAction = ReportUtils.buildOptimisticHoldReportAction();
        reportActionIDList.push(createdReportAction.reportActionID);
        const transactionID = TransactionUtils.getTransactionID(transactionThreadReportID ?? '-1');
        optimisticHoldTransactionActions.push({
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
            value: {
                comment: {
                    hold: createdReportAction.reportActionID,
                },
            },
        });
        failureHoldTransactionActions.push({
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
            value: {
                comment: {
                    hold: null,
                },
            },
        });

What alternative solutions did you explore? (Optional)

[jasperhuangg]

Since all of these actions happen offline this issue is in the front-end and can therefore be fixed externally.

[melvin-bot]

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

[melvin-bot]

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

[twilight2294]

Proposal

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

The hold action is not updated optimistically if we resolve duplicates while offline

What is the root cause of that problem?

We should the unhold button only when we have hold comment from the transaction report or when there is a duplicate expense:

App/src/libs/TransactionUtils/index.ts

Line 708 in 3047c1b

return !!transaction.comment?.hold || isDuplicate(transaction.transactionID, true);

Now when we resolve duplicates in the function here, we do not optimistically set the hold status which is required. We only get that when the BE responds to the api call. And hence when offline the button doesn't change the text as well as the functionality.

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

To address this bug we should update the optimisticHoldActions and failureHoldActions to also optimistically set the hold status for the transaction.

Note:

• we shouldn't remove the optimisticHoldActions because that will be used to unhold the expense when offline. if we do not set the REPORT_ACTIONS key then there will be regression.
• Also we should put pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE, for optimistic case and pendingAction: null, for failure case, this is because we are still in offline mode.
• Also in failure data we must set genericHoldExpenseFailureMessage error.

So the correct and updated code should be as follows:

--- a/src/libs/actions/IOU.ts
+++ b/src/libs/actions/IOU.ts
@@ -8291,22 +8291,48 @@ function resolveDuplicates(params: TransactionMergeParams) {
     transactionThreadReportIDList.forEach((transactionThreadReportID) => {
         const createdReportAction = ReportUtils.buildOptimisticHoldReportAction();
         reportActionIDList.push(createdReportAction.reportActionID);
+        const transactionID = TransactionUtils.getTransactionID(transactionThreadReportID ?? '-1');
         optimisticHoldActions.push(
             {
                 onyxMethod: Onyx.METHOD.MERGE,
                 key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${transactionThreadReportID}`,
                 value: {
                     [createdReportAction.reportActionID]: createdReportAction,
                 },
+             },
+            {
+                onyxMethod: Onyx.METHOD.MERGE,
+                key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
+                value: {
+                    pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
+                    comment: {
+                        hold: null,
+                    },
+                },
+            },
+        );
         failureHoldActions.push(
             {
                 onyxMethod: Onyx.METHOD.MERGE,
                 key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${transactionThreadReportID}`,
                 value: {
                     [createdReportAction.reportActionID]: {
                         errors: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('iou.error.genericHoldExpenseFailureMessage'),
                     },
                 },
             },
+            {
+                onyxMethod: Onyx.METHOD.MERGE,
+                key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
+                value: {
+                    pendingAction: null,
+                    comment: {
+                        hold: null,
+                    },
+                     errors: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('iou.error.genericHoldExpenseFailureMessage'),
+                 },
             },

 

What alternative solutions did you explore? (Optional)

[melvin-bot]

@eVoloshchak, @stephanieelliott, @jasperhuangg Whoops! This issue is 2 days overdue. Let's get this updated quick!

[jasperhuangg]

@eVoloshchak Please review proposals!

[daledah]

@eVoloshchak @jasperhuangg PR is ready for review.

[stephanieelliott]

@eVoloshchak the PR is waiting on your review, can you take a look?

[stephanieelliott]

@eVoloshchak the #50383 is waiting on your review, can you take a look?

^^ Bump @eVoloshchak

[stephanieelliott]

Hey @eVoloshchak this PR is blocked on you, can you review please? #50383

[situchan]

I can review PR if needed

[stephanieelliott]

PR is on staging!

[yuwenmemon]

This doesn't appear to be fixed by the PR as the original issue is still reproducible on Mobile Web: #50383 (comment)

[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.52-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: report header has "Hold" action when the expense is already in Hold status #50383

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

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

• @eVoloshchak requires payment through NewDot Manual Requests
• @daledah requires payment (Needs manual offer from BZ)

[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:

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

[melvin-bot]

Payment Summary

Upwork Job

• Reviewer: @eVoloshchak owed $250 via NewDot
• ROLE: @daledah paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@stephanieelliott)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1840780088904015602/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[stephanieelliott]

Summarizing payment on this issue:

• Contributor: @eVoloshchak $250 via NewDot - Please request
• Contributor+: @daledah $250 via Upwork - PAID

Upwork job is here: https://www.upwork.com/jobs/~021851909474139768777

[daledah]

@stephanieelliott accepted thx

[stephanieelliott]

All paid!

[melvin-bot]

@stephanieelliott @jasperhuangg Be sure to fill out the Contact List!