[HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] Expenses - Hold Expense option is displayed on expenses on a closed report

[isagoico]

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: v1.4.74-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
Issue reported by: Applause - Internal team
Found while writing test cases for the Hold feature here https://github.com/Expensify/Expensify/issues/392550

Action Performed:

1. Set a workspace without an approval flow
2. Invite a employee
3. As the employee - Submit 2 expenses to the workspace
4. Submit the report - Since there's no approval workflow - the report should auto-close
5. Navigate to a expense of the closed report
6. Click on the 3-dot menu
7. Click on Hold Expense

Expected Result:

The Hold option should not display in expenses attached to a closed report.

Actual Result:

The Hold expense option is displayed in expenses linked to a closed report. Nothing happens when clicking on hold expense on a closed report.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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

Hold.Expense.mp4

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @sonialiap

[melvin-bot]

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

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

[ShridharGoel]

Proposal

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

Hold Expense option is displayed on expenses on a closed report.

What is the root cause of that problem?

We don't check if the parent report is archived or not below:

App/src/components/MoneyRequestHeader.tsx

Line 107 in b8b4e98

const canHoldOrUnholdRequest = !isSettled && !isApproved && !isDeletedParentAction;

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

Update to include check to ensure that canHoldOrUnholdRequest is true only when parent report is not archived:

const canHoldOrUnholdRequest = !isSettled && !isApproved && !isDeletedParentAction && !ReportUtils.isArchivedRoom(parentReport);

It's also okay to have it like this: const isParentReportArchived = ReportUtils.isArchivedRoom(parentReport) and then use !isParentReportArchived in the check.

We can also include a check for !ReportUtils.isArchivedRoom(getReport(report?.reportID)).

[nkdengineer]

Proposal

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

The Hold expense option is displayed in expenses linked to a closed report. Nothing happens when clicking on hold expense on a closed report.

What is the root cause of that problem?

1. We don't check the parent report is archived or not here

App/src/components/MoneyRequestHeader.tsx

Line 113 in e9cf0c5

const canHoldOrUnholdRequest = !isSettled && !isApproved && !isDeletedParentAction;

2. As the video, when we hold the request and BE returns error, there are no error displayed for hold request action
   The problem is we don't generate the error for hold action in failureData. This also happens for untold action

App/src/libs/actions/IOU.ts

Line 6673 in e9cf0c5

const failureData: OnyxUpdate[] = [

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

1. We should add the check !ReportUtils.isArchivedRoom(parentReport) here so the hold request option will not appear if the expense report is closed

App/src/components/MoneyRequestHeader.tsx

Line 113 in e9cf0c5

const canHoldOrUnholdRequest = !isSettled && !isApproved && !isDeletedParentAction;

2. We should generate error for hold action in failureData so when BE returns error the error will appears and we can clear this action. We should do the same for unHold action. I know with point 1 we've hidden the hold option for this case but we can have another case that hold API will return error like: a device hold request in offline and another device hold request online. For this case BE also returns error so we should generate error for hold and unhold action.

App/src/libs/actions/IOU.ts

Line 6673 in e9cf0c5

const failureData: OnyxUpdate[] = [

What alternative solutions did you explore? (Optional)

NA

[melvin-bot]

@rojiphil, @sonialiap Huh... This is 4 days overdue. Who can take care of this?

[rojiphil]

Thanks for all your proposals.
@ShridharGoel proposal LGTM as that was the first one to address the root cause and the solution.
🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @dangrous, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[nkdengineer]

As the video, when we hold the request and BE returns error, there are no error displayed for hold request action

@rojiphil I think we also need to fix the second bug as well.

[dangrous]

Hey @rojiphil! I am leaning towards taking @nkdengineer's proposal as a more complete solution - not showing the error seems like an oversight in the code and it would be good to get that fixed. What do you think?

[rojiphil]

I am leaning towards taking @nkdengineer's proposal as a more complete solution - not showing the error seems like an oversight in the code and it would be good to get that fixed. What do you think?

Oh! Yes. Sound's good. Let's go with @nkdengineer's proposal as this proposal is a more complete solution.

[nkdengineer]

@rojiphil this PR is ready for preview

[ShridharGoel]

@dangrous @rojiphil Shouldn't the other bug be handled in another issue? That would only happen for other scenarios and not when hold expense is selected on closed report since we are anyways not going to show the option.

[rojiphil]

@ShridharGoel I agree that the other bug is not directly related to the issue at hand here. But at times when the solution is pretty straightforward, we also consider any other closely related issue and give preference to that proposal for a more complete solution. We just did that here although I agree that your proposal has got the correct RCA and solution for the mentioned problem here.

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

• fix: hold Expense option is displayed on expenses on a closed report #42574

If no regressions arise, payment will be issued on 2024-06-18. 🎊

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

• @rojiphil requires payment (Needs manual offer from BZ)
• @nkdengineer 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:

• [@rojiphil] The PR that introduced the bug has been identified. Link to the PR:
• [@rojiphil] 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:
• [@rojiphil] 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:
• [@rojiphil] Determine if we should create a regression test for this bug.
• [@rojiphil] 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:

[melvin-bot]

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

• fix: hold Expense option is displayed on expenses on a closed report #42574

If no regressions arise, payment will be issued on 2024-06-20. 🎊

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

• @rojiphil requires payment (Needs manual offer from BZ)
• @nkdengineer 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:

• [@rojiphil] The PR that introduced the bug has been identified. Link to the PR:
• [@rojiphil] 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:
• [@rojiphil] 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:
• [@rojiphil] Determine if we should create a regression test for this bug.
• [@rojiphil] 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:
@rojiphil reviewer $250 - offer sent - please complete the checklist
@nkdengineer contributor $250 - offer sent - paid ✔️

Upwork post

[nkdengineer]

Thanks @sonialiap, I accepted it

[melvin-bot]

Payment Summary

Upwork Job

• ROLE: @rojiphil paid $(AMOUNT) via Upwork (LINK)
• ROLE: @nkdengineer paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@sonialiap)

• 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//hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[rojiphil]

• [@rojiphil] The PR that introduced the bug has been identified. Link to the PR: Offending PR
• [@rojiphil] 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: Added comment
• [@rojiphil] 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 Required. Existing checklist is good enough to capture such issues.
• [@rojiphil] Determine if we should create a regression test for this bug. : Not needed as this is an edge case and does not come in the critical path.
• [@rojiphil] 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. NA

[rojiphil]

@rojiphil reviewer $250 - offer sent - please complete the #42202 (comment)

@sonialiap Completed BZ checklist and accepted offer. Thanks

[sonialiap]

Thanks Roji! Paid ✔️