[HOLD for payment 2024-03-19] [$500] [Held Requests] Hold request modal does not dismiss for Employee after admin pays the request

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Found when validating PR : #33897

Version Number: v1.4.43-6
Reproducible in staging?: Y
Reproducible in production?: N
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal team
Slack conversation:

Action Performed:

Precondition:
Admin and employee are in the same workspace.

1. [Employee] Create a manual request from workspace chat.
2. [Employee] Submit the request.
3. [Employee] Go to request details page.
4. [Employee] Click 3-dot menu > Hold request.
5. [Admin] Pay the request.
6. [Employee] After the background shows "paid", add a reason and submit it.

Expected Result:

In Step 5, Hold Request modal should dismiss for Employee after Admin pays it.

Actual Result:

In Step 5, Hold Request modal does not dismiss for Employee after Admin pays it. As a result, employee can hold the paid request.

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

Bug6386751_1708514994509.bandicam_2024-02-21_04-11-41-368.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01dad4e5f90367adf7
• Upwork Job ID: 1760292458905010176
• Last Price Increase: 2024-02-28
• Automatic offers:
• alitoshmatov | Reviewer | 0
• WikusKriek | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

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

[melvin-bot]

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

[izarutskaya]

We think that this bug might be related to #vip-bills
CC @davidcardoza

[neonbhai]

Proposal

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

Hold request modal does not dismiss for Employee after admin pays the request

What is the root cause of that problem?

We don't check if the request is Paid when Hold Request is pressed on HoldReasonPage.tsx

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

We should fail with error or disable the button instead of dismissing without user action like we do in other areas of the App. We should disable the Hold Request button or show error when pressed if the report is Settled and could not be held.

We should check for isSettled using:

const isSettled = ReportUtils.isSettled(reportID);

[dukenv0307]

Proposal

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

In Step 5, Hold Request modal does not dismiss for Employee after Admin pays it. As a result, employee can hold the paid request.

What is the root cause of that problem?

We don't dismiss the hold request page if the request is paid

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

In HoldReasonPage, we should use the same check as we do when we display the hold request option in header. If the check is false we will dismiss the hold request page since we cannot hold request. This also can help us to prevent openning the hold request page by deeplink if we cannot do this action.

App/src/components/MoneyRequestHeader.tsx

Line 82 in 20ca041

const isRequestModifiable = !isSettled && !isApproved && !ReportActionsUtils.isDeletedAction(parentReportAction);

App/src/components/MoneyRequestHeader.tsx

Line 120 in 20ca041

if (!isOnHold && (isRequestIOU || canModifyStatus)) {

What alternative solutions did you explore? (Optional)

NA

[puneetlath]

I don't think this needs to be a blocker. Going to assign to @robertjchen as CME to decide what the ideal behavior is, since he's leading the "hold" project.

[robertjchen]

Yep, not a deploy blocker but great catch here- I think the big UX difference between the two above proposals are:

We should disable the Hold Request button or show error when pressed if the report is Settled and could not be held.

and

we will dismiss the hold request page since we cannot hold request. This also can help us to prevent openning the hold request page by deeplink if we cannot do this action.

I'm partial to the latter one (@dukenv0307 's approach), but is there room to combine both approaches? 🤔 What do you think @alitoshmatov ?

cc: @Expensify/design for guidance on what would be the most consistent experience.

[dukenv0307]

we will dismiss the hold request page since we cannot hold request. This also can help us to prevent openning the hold request page by deeplink if we cannot do this action.

My approach is the same way as we do for edit flow

App/src/pages/EditRequestPage.js

Lines 106 to 108 in 3e77445

	// Dismiss the modal when a current user cannot edit a money request.
	Navigation.isNavigationReady().then(() => {
	Navigation.dismissModal();

[shawnborton]

Hmm I think I would just block form submission on the hold modal in this case, letting them know that the expense status changed and they can no longer hold it as the form error message. Otherwise it feels a bit strange to automatically close the modal for them if they don't know why that's happening.

[melvin-bot]

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

[robertjchen]

Agreed with @WikusKriek proposal 👍 Let's move forward with the PR, and we can finalize the messaging when reviewing it

[melvin-bot]

📣 @alitoshmatov 🎉 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]

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

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

• 30714 Error and block submit when you can't hold request #37477

If no regressions arise, payment will be issued on 2024-03-19. 🎊

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

• @WikusKriek requires payment automatic offer (Contributor)
• @alitoshmatov requires payment automatic offer (Reviewer)

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

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

[JmillsExpensify]

@alitoshmatov Do you mind helping with the BZ checklist? In the meantime, payment summary as follows:

• Contributor: @WikusKriek $500
• Contributor+: @alitoshmatov $500

[alitoshmatov]

• The PR that introduced the bug has been identified. Link to the PR: [WAVE 7] Hold Requests: Individual Money Request Page, Reason Interstitial, Banner #33897
• 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: https://github.com/Expensify/App/pull/33897/files#r1532582648
• 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: No need, quite an edge case
• Determine if we should create a regression test for this bug. No need, quite an edge case

[alitoshmatov]

@JmillsExpensify Completed the checklist

[JmillsExpensify]

Many thanks! All contracts have been paid out.