[HOLD for payment 2024-11-18] [$250] Hybrid iOS app –IOU –Approval button is missing after payment of the report with hold expense

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: v9.0.49-0
Reproducible in staging?: N
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y
Issue was repro when executing this PR: #49910
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

1. Go to Hybrid app
2. Log in with your Approver account
3. As employee Go to the room with the two expenses
4. As employee: hold one expense
5. As Approver: Approve just the pending amount
6. As Approver: back to room by click to the header

Expected Result:

The approval button should load after payment of the report with hold expense

Actual Result:

The approval button is missing after payment of the report with hold expense

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6636810_1729099904332.Ap.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021848434780732463293
• Upwork Job ID: 1848434780732463293
• Last Price Increase: 2024-10-21
• Automatic offers:
• jjcoffee | Reviewer | 104595665
• ikevin127 | Contributor | 104595666

Issue Owner

Current Issue Owner: @sonialiap

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

[IuliiaHerets]

@sonialiap 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

[NJ-2020]

Proposal

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

Hybrid iOS app –IOU –Approval button is missing after payment of the report with hold expense

What is the root cause of that problem?

Right here we check if the unheld total amount is not equal to 0 then we will show the approve button

App/src/libs/actions/IOU.ts

Lines 6990 to 6992 in 66cf824

	const unheldTotalIsZero = iouReport && iouReport.unheldTotal === 0;
	return isCurrentUserManager && !isOpenExpenseReport && !isApproved && !iouSettled && !isArchivedReport && !unheldTotalIsZero;

Since we hold the expense the unheld total amount become 0 which will not show the approve button

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

We should remove the unheldTotalIsZero variable and remove the return check condition

App/src/libs/actions/IOU.ts

Lines 6990 to 6992 in 66cf824

	const unheldTotalIsZero = iouReport && iouReport.unheldTotal === 0;
	return isCurrentUserManager && !isOpenExpenseReport && !isApproved && !iouSettled && !isArchivedReport && !unheldTotalIsZero;

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[ikevin127]

Proposal

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

Approval button is missing after payment of a report with two expenses, one on hold and the other not, once Approving the expense that's not on hold -> the Approve button is missing for the held expense once Submitted.

What is the root cause of that problem?

• The logic which causes this behaviour was added by PR Fix Scanning expense can be approved as 0.00 amount and can not be edited #50817 on purpose in order to solve issue [$250] Hold - Scanning expense can be approved as 0.00 amount and can not be edited #48590 where the Approve button was showing scan expense while still scanning.

This is what is causing our issue because the logic added by the above mentioned PR is deliberately hiding Approve in our case too, even though the steps in our issue don't mention scan expenses but rather manual expenses.

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

Given the RCA, we cannot simply remove the logic that the above mentioned PR added because it would revert the fix for which the logic was added as fix for this issue:

Approver should not be able to approve scanning expense

Here's what only removing the mentioned logic would look like for the other issue vs how what I'm proposing would look:

Only remove logic	Replace logic (below proposal)
proposal-1.mov	proposal-2.mov

Therefore here's my proposal:

1. Remove the unheldTotalIsZero logic implementation from here:

App/src/libs/actions/IOU.ts

Lines 6991 to 6993 in 179d0f0

	const unheldTotalIsZero = iouReport && iouReport.unheldTotal === 0;
	return isCurrentUserManager && !isOpenExpenseReport && !isApproved && !iouSettled && !isArchivedReport && !unheldTotalIsZero;

2. Replace the above implementation with:

    let isTransactionBeingScanned = false;
    const reportTransactions = TransactionUtils.getAllReportTransactions(iouReport?.reportID);
    for (const transaction of reportTransactions) {
        const hasReceipt = TransactionUtils.hasReceipt(transaction);
        const isReceiptBeingScanned = TransactionUtils.isReceiptBeingScanned(transaction);

        // If a transaction has receipt (scan) and its receipt is being scanned, flag it
        if (hasReceipt && isReceiptBeingScanned) {
            isTransactionBeingScanned = true;
        }
    }

    return isCurrentUserManager && !isOpenExpenseReport && !isApproved && !iouSettled && !isArchivedReport && !isTransactionBeingScanned;

This ensures we don't show Approve for a scan expense that's still scanning, while fixing our issue of the Approve button not showing at all.

[jjcoffee]

@ikevin127's proposal LGTM! I agree that we can't just remove the unheldTotalIsZero as the other proposal suggests. Also, I'm guessing this isn't limited to the hybrid app.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[M00rish]

Hi guys, I just want to note that this PR was just pushed to production few hours ago and the issue was opened a few days ago, so maybe it has to do with something else I think..., also are not we supposed to unhold the expense before approving or paying?

thanks

[melvin-bot]

📣 @M00rish! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[ikevin127]

@marcochavezf Looks like @M00rish's PR is still within the regression period as it was deployed to staging only 4 days ago - so they might want to fix this one as a follow-up.

Edit: They are also right that their PR got merged on staging 4 days ago whereas this issue was opened 6 days ago so it could not be a regression of their PR - but the solution I proposed and was selected by C+ keeps the other PRs functionality while fixing this issue so I guess we're good to go here ?

More context in #48590 (comment) (the other issue) which introduced the logic which I propose to change.

[jjcoffee]

@ikevin127 I think the RCA in your proposal goes back to #49910 since there they optimistically added unHeldTotal = 0, which would then trigger the unheldTotalIsZero logic added in the other PR.

App/src/libs/actions/IOU.ts

Lines 6527 to 6533 in d82a253

	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT}${optimisticExpenseReport.reportID}`,
	value: {
	...optimisticExpenseReport,
	unheldTotal: 0,
	},
	},

Is it possible to reproduce without the changes in #50817? I'm wondering if the RCA might go a bit deeper.

[ikevin127]

@jjcoffee I tried reproducing this issue with both #49910 & #50817 changes reverted and with that, our issue is not reproducible anymore:

Screen.Recording.2024-10-23.at.17.59.07.mov

You are right about the offending PR, looks like the unHeldTotal = 0 addition is the root cause for our issue.

[jjcoffee]

@marcochavezf I think we're good to proceed with @ikevin127's proposal still.

[marcochavezf]

Ok sounds good, thanks for the resolution. Assigning @ikevin127 🚀

[melvin-bot]

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

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

PR will be ready for review in ~ 1 hour.
Edit: ♻️ PR is ready for review.

[ikevin127]

♻️ Status update: Currently awaiting @marcochavezf's input on the PR to determine whether we're going to merge or not.

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

• Approval button is missing after payment of the report with hold expense #51503

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

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

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

[melvin-bot]

@jjcoffee / @ikevin127 @sonialiap The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[ikevin127]

cc @sonialiap for visibility as this was due on 18th

[jjcoffee]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production
• 2b. Reported on staging (deploy blocker)
• 2c. Reported on both staging and production
• 2d. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] 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/49910/files#r1848000831

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue: https://github.com/Expensify/Expensify/issues/445796

Regression Test Proposal

Precondition:

• A workspace with an Employee and a general Approver (non-admin, can approve of any Employee).

Test:

1. Log in with your Approver account.
2. Log in with your Employee account on a different session (private tab or different browser).
3. As Employee: Go to your WS room and send two manual expenses.
4. As Employee: Hold one of the expenses.
5. As Approver: Approve just the pending amount (the expense which is not on hold).
6. As Approver: Verify that the held expense is now separate (below the approved one) and shows the Approve button.

Do we agree 👍 or 👎

[sonialiap]

Payment summary:

• @jjcoffee $250 - paid in upwork ✔️
• @ikevin127 $250 - paid in upwork ✔️

I was OOO on the 18th

[jjcoffee]

@sonialiap Checklist complete! 🙏

[sonialiap]

Thanks Joel! Payment completed