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

IuliiaHerets commented 1 month 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: 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: https://github.com/Expensify/App/pull/48522 Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Go to staging.new.expensify.com
[Employee] Submit two same expenses in the workspace chat.
[Admin] Go to any of the submitted expense in the workspace chat.
[Admin] Go offline.
[Admin] Click Review duplicates.
[Admin] Click Keep this one (any).
[Admin] Click Confirm on the confirmation page.
[Admin] Go to the other transaction thread that has "Hold" action in the expense preview.
[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:

[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/0055f62e-e602-418a-8393-ccaf2387c219

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

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

melvin-bot[bot] commented 1 month ago

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[bot] commented 1 month 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.

IuliiaHerets commented 1 month ago

On prod, the other expense vanishes instantly

https://github.com/user-attachments/assets/7f756a65-9140-466b-a8c1-2cb2e554584b

IuliiaHerets commented 1 month ago

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

roryabraham commented 1 month ago

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

daledah commented 1 month ago

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

https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/libs/TransactionUtils/index.ts#L703-L709

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:

https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/libs/actions/IOU.ts#L8261-L8270

and

https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/libs/TransactionUtils/index.ts#L689-L697

so we only need to focus on the first condition.

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

https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/libs/actions/IOU.ts#L8233

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:

https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/libs/actions/IOU.ts#L8291

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 commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

twilight2294 commented 1 month ago

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: https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/libs/TransactionUtils/index.ts#L708

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

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

jasperhuangg commented 1 month ago

@eVoloshchak Please review proposals!

melvin-bot[bot] commented 1 month ago

📣 @daledah You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

twilight2294 commented 1 month ago

@jasperhuangg I am confused here, you gave 👍 to my proposal but assigned it to the other contributor, can you assign me here instead please :))

@daledah please hold the PR until this is cleared

melvin-bot[bot] commented 1 month ago

@eVoloshchak, @stephanieelliott, @jasperhuangg, @daledah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

twilight2294 commented 1 month ago

@jasperhuangg can you please assign me here, please check comment

jasperhuangg commented 1 month ago

@twilight294 sorry for the confusion, I gave thumbs to your proposal but didn't intend on assigning you, I was acknowledging that I saw your proposal.

daledah commented 1 month ago

@jasperhuangg I'll open a PR soon

daledah commented 1 month ago

@eVoloshchak @jasperhuangg PR is ready for review.

stephanieelliott commented 1 month ago

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

stephanieelliott commented 1 month ago

@eVoloshchak the https://github.com/Expensify/App/pull/50383 is waiting on your review, can you take a look?

^^ Bump @eVoloshchak

stephanieelliott commented 1 month ago

Hey @eVoloshchak this PR is blocked on you, can you review please? https://github.com/Expensify/App/pull/50383

situchan commented 1 month ago

I can review PR if needed

stephanieelliott commented 3 weeks ago

PR is on staging!

yuwenmemon commented 3 weeks ago

This doesn't appear to be fixed by the PR as the original issue is still reproducible on Mobile Web: https://github.com/Expensify/App/pull/50383#issuecomment-2430541377

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

https://github.com/Expensify/App/pull/50383

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

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

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

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

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

@stephanieelliott accepted thx

stephanieelliott commented 1 week ago

All paid!

melvin-bot[bot] commented 1 week ago

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

Expensify / App