Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hold-On user A tries to unheld expense holded by user B, system message for unheld shown #46946

Closed
2 of 6 tasks
IuliiaHerets opened this issue Aug 7, 2024 · 11 comments
Closed
2 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Needs Reproduction Reproducible steps needed retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Aug 7, 2024

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.17
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4827344
Issue reported by: Applause Internal Team

Action Performed:

  1. User A: Go to https://staging.new.expensify.com/home
  2. User A: Open a chat with user B
  3. User A: create a expense and send it to user B
  4. User B : Login the site
  5. User B: Open the chat with user A
  6. User B : Hold the expense
  7. User A : Try to unhold the expense which was unhold by user B
  8. User A : Note as expected unexpected error is shown but system message for unhold is displayed. Close the error.
  9. User A : Tap header and note "hold" option displayed

Expected Result:

When user A tries to unheld expense holded by user B, only unexpected error must be shown and no system message of unheld must be displayed.

Actual Result:

When user A tries to unheld expense holded by user B, unexpected error is shown correctly but system message for unheld is shown incorrectly.

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Bug6563977_1723000627691.Screenrecorder-2024-08-07-08-23-03-437_compress_1.mp4

View all open jobs on GitHub

@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 7, 2024
Copy link

melvin-bot bot commented Aug 7, 2024

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

@IuliiaHerets
Copy link
Author

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

@bernhardoj
Copy link
Contributor

This issue is the same as #46551

Proposal

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

The unhold system message shows when unhold expense but fails.

What is the root cause of that problem?

It's not a problem that the unhold expense system message is shown even when fails, but the issue here is that the system message looks like the request is a success. When the unhold request fails, we set the error to the transaction data instead of the system message action.

App/src/libs/actions/IOU.ts

Lines 7565 to 7573 in eced170

const failureData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
value: {
pendingAction: null,
errors: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('iou.error.genericUnholdExpenseFailureMessage'),
},
},

And that will show the error below the receipt title. It's the same with hold too.

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

To make it clearer, we need to set the error pattern for the unhold system message. To do that, we need to set the error to the optimistic unhold action instead.

{
    onyxMethod: Onyx.METHOD.MERGE,
    key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
    value: {
        [createdReportAction.reportActionID]: {
            errors: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('iou.error.genericUnholdExpenseFailureMessage'),
        },
    },
},
image

I just realized the hold status is cleared optimistically but not reverted when fails, so we can fix that too here.

const transaction = allTransactions[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`];
...
comment: {
    hold: transaction?.comment.hold,
}

Do the same for hold too, but we will set it for 2 actions, hold request and hold reason action.

@mallenexpensify
Copy link
Contributor

@robertjchen I was thinking of putting this issue on hold, pending the below . What cha think?

@melvin-bot melvin-bot bot added the Overdue label Aug 12, 2024
Copy link

melvin-bot bot commented Aug 13, 2024

@mallenexpensify Huh... This is 4 days overdue. Who can take care of this?

@robertjchen
Copy link
Contributor

Oh yep, this should be addressed by the above issue ^

@robertjchen
Copy link
Contributor

Let's test again now that the changes for that are out? 🙏

@mallenexpensify mallenexpensify added Needs Reproduction Reproducible steps needed retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause labels Aug 13, 2024
@MelvinBot
Copy link

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

@mallenexpensify
Copy link
Contributor

Through retest weekly on here. @bernhardoj are you able to reproduce?

@melvin-bot melvin-bot bot removed the Overdue label Aug 13, 2024
@bernhardoj
Copy link
Contributor

I can't reproduce this anymore since the unhold option is not shown to the other user anymore.

web.mp4

@mallenexpensify
Copy link
Contributor

Thanks @bernhardoj , going to close this then as it's not an issue/bug any more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Needs Reproduction Reproducible steps needed retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause
Projects
None yet
Development

No branches or pull requests

5 participants