[HOLD for payment 2024-08-13] [$250] [Held requests] Partially approved expenses are not GBR'ed in LHN

[lanitochka17]

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.10-2
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/4747965
Issue reported by: Applause - Internal Team

Action Performed:

Pre-requisite:

1. Create a workspace and add user A (as a member) and user B (as an admin).
2. More features > Enable workflows > Enable Add Approvals > Set user B as an approver
   Steps
3. As user A submit two expenses to the workspace
4. As user B (the approver) navigate to the expense report and hold one of the expenses
5. As user B (the approver) click on the approve option
6. A modal is displayed to approve the full report amount or just the pending expense
7. Approve the pending expense (Submitted expense that is not on hold)
8. When the action is completed observe the LHN and workspace chat report.

Expected Result:

A new report is created with the expense on hold and is GBR’ed in the LHN (green dot)

Actual Result:

A new report is created with the expense on hold but is not GBR’ed in the LHN (green dot)

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

Add any screenshot/video evidence

Bug6549292_1721646141815.2024-07-22_13_36_59.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0130a7781bd6d708e1
• Upwork Job ID: 1816612436734076568
• Last Price Increase: 2024-07-25
• Automatic offers:
• ikevin127 | Reviewer | 103312301
• nkdengineer | Contributor | 103312306

Issue Owner

Current Issue Owner: @VictoriaExpensify

[melvin-bot]

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

[melvin-bot]

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

[lanitochka17]

@VictoriaExpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[melvin-bot]

@VictoriaExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[VictoriaExpensify]

Ok I've recreated the issue and agree it should be fixed:

I think this is part of Wave Collect.

[melvin-bot]

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

[melvin-bot]

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

[nkdengineer]

Proposal

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

A new report is created with the expense on hold but is not GBR’ed in the LHN (green dot)

What is the root cause of that problem?

The Actual behavior in the OP is actually the correct behavior, as all outstanding requests are on HOLD, there's no action from the approver. The ball is on the request submitter to add more information/clarification so the request can be taken off HOLD.

That behavior is consistent with the back-end too.

The problem here is that it doesn't work now in offline mode because we're not setting optimistic data correctly. In

App/src/libs/actions/IOU.ts

Line 6753 in 750c34e

hasOutstandingChildRequest: hasIOUToApproveOrPay(chatReport, full ? expenseReport?.reportID ?? '-1' : '-1'),

, we don't set the IOU report ID to exclude in case of partial approval, this causes the hasOutstandingChildRequest to be true and GBR shows after approving partially, although there's no action for the approver, after approving that and held expenses moved to a new report.

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

Update

App/src/libs/actions/IOU.ts

Line 6753 in 750c34e

hasOutstandingChildRequest: hasIOUToApproveOrPay(chatReport, full ? expenseReport?.reportID ?? '-1' : '-1'),

to

hasOutstandingChildRequest: hasIOUToApproveOrPay(chatReport, expenseReport?.reportID ?? '-1'),

What alternative solutions did you explore? (Optional)

This hasIOUToApproveOrPay could be missing in other optimistic cases too, such as when holding a request, paying, ... we need to check those places and add if needed (some adjustments could also be needed, eg. we need to pass the updated IOU report data/transaction data in hasIOUToApproveOrPay and merge it with the iouReport here, and do not use excludedIOUReportID). This will make the hasOutstandingChildRequest (GBR) behavior consistent with the back-end.

[ikevin127]

I confirm that the issue is reproducible as per OP steps - but I'm not sure whether we want to fix this as per the OP Expected result.

What happens currently:

1. User A (Member - Submitter) submits 2 expenses.
2. User B (Admin - Approver) sets one of User A's expenses on Hold, adding a reason for Hold.
3. User B (Admin - Approver) will be shown a GBR because they can still Approve one of the expenses (the one which wasn't put on Hold).
4. User B (Admin - Approver) approves the other expense which which wasn't put on Hold -> no other indicator (RBR nor GBR) is shown for the (Admin - Approver) anymore.
5. User A (Member - Submitter) will see a RBR because one of the expenses was put on Hold by User B, giving a specific reason for the Hold.

Notes:

• User B should not see any indicator (RBR nor GBR) since on their side there's no pending action.
• User A will see a RBR because one of their expenses was put on Hold for a specific reason given by User B and they have to be notified of that - so RBR makes sense for User A in this case.

@nkdengineer's proposal makes sense to me as far as explaining the way in which the Actual result might be the correct behaviour in this case, this being backed by and consistent with the BE data as well. As far as the solution goes, I tested the offline behaviour and there's indeed an inconsistency compared to the online behaviour -> which can be fixed by the proposed main solution.

As for the proposed alternative solution: I think that proposition is out of scope for this issue.

This being said, I'll defer to the CME as to whether we should actually fix this issue as per the OP Expected result and change the current behaviour, or only fix the offline behaviour to have consistency with the current online behaviour (OP Actual result).

🎀👀🎀 C+ reviewed

cc @danieldoglas

[melvin-bot]

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

[nkdengineer]

Friendly bump @danieldoglas for assignment, thank you

[melvin-bot]

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

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

[ikevin127]

@danieldoglas Not sure if you've read through #45902 (comment) before assignment - because assignment here without further discussion means we're not fixing the issue as the OP expects, but instead just aligning the offline behaviour with what we have online.

Just want to confirm that we're good with ^ that as to not have any misunderstandings when moving on with the described plan.

[danieldoglas]

I agree that the actual solution is the correct one - there's nothing for the approver to work, so it doesn't make sense to create a GBR.

@VictoriaExpensify do you disagree?

[ikevin127]

Thanks for the feedback! In this case I think @nkdengineer can move forward with opening the PR.

If Victoria disagrees, we can change course at any time.

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

• fix: Partially approved expenses are not GBR'ed in LHN #46467

If no regressions arise, payment will be issued on 2024-08-13. 🎊

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

• @ikevin127 requires payment automatic offer (Reviewer)
• @nkdengineer requires payment automatic offer (Contributor)

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

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

[ikevin127]

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: Create optimistic report when approving/paying for report with hold expenses #42573.
• [@ikevin127] 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: Create optimistic report when approving/paying for report with hold expenses #42573 (comment).
• [@ikevin127] 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: N/A.
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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.

Regression Test Proposal

1. Create a workspace and add user A (as a member) and user B (as an admin).
2. More features > Enable workflows > Enable Add Approvals > Set user B as an approver.
3. As user A submit two expenses to the workspace.
4. As user B (the approver) navigate to the expense report and hold one of the expenses.
5. 4. As user B, go offline.
6. As user B (the approver) click on the Approve option.
7. A modal is displayed to approve the full report amount or just the pending expense.
8. Approve the pending expense only (submitted expense that is not on hold).
9. When the action is completed observe the LHN and workspace chat report.
10. Verify that user B does not see any indicator (RBR nor GBR) since on their side there's no pending action.

Do we agree 👍 or 👎.

[VictoriaExpensify]

lgtm. @danieldoglas - what do you think?

[mallenexpensify]

Contributor: @nkdengineer paid $250 via Upwork
Contributor+: @ikevin127 paid $250 via Upwork.

Test case

• https://github.com/Expensify/Expensify/issues/420497

Thx y'all 🎉