[HOLD for payment 2024-04-09] [Held Requests] Approving/Paying expense reports with held requests

[robertjchen]

Suggested Implementation Guide

Paying expense reports with held requests (admin)

When paying out an expense report, we need to total up and present held and non-held amounts.

Under src/components create a new <ProcessMoneyRequestHoldMenu. This component will solely contain a <PopoverMenu with iou.confirmPay (or iou.confirmApproval) as the header text, iou.confirmPayAmount (or iou.confirmApprovalAmount) as the description body depending on props.type, followed by two <Button>'s one for the held amount and payable amount. The two props the menu takes are:

• props.fullAmount
• props.heldAmount
• props.type - either 'pay' or 'approve'

This component is only displayed if the payer can unhold the requests, which we'll determine with the function below.

In src/components/ReportActionItem/ReportPreview.js, update <SettlementButton to first call a new local method confirmSettle(), we will move the existing IOU.payMoneyRequest into this new local method.

The new local method would do the following:

1. Total up all reimbursable requests on the report and determine if there are any held amounts within that total
2. If not, directly call IOU.payMoneyRequest
3. Otherwise, check if the user has permissions to unhold the requests
4. If not, directly call IOU.payMoneyRequest
5. If they can, make the new <PayMoneyRequestHoldMenu visible, passing the full and held amounts to it so that it may be displayed on the buttons.

In src/libs/actions/IOU.js update payMoneyRequest() to take a new additional flag full which will determine whether or not we pay the full amount, or just the held amounts. This flag will also be passed to the backend API as part of the call.

In the case where we reveal the <PayMoneyRequestHoldMenu, the user has the option to pay the full amount or just the non-held amount. Depending on what they select, we'll either call IOU.payMoneyRequest with the full parameter set to true or false.

Approving expense reports with held requests (approvers)

Similar to the above, we'll make the same changes but to different places:

In src/components/ReportActionItem/ReportPreview.js, update <Button under shouldShowApproveButton to first call a new local method confirmApproval(), we will move the existing IOU.approveMoneyRequest into this new local method.

In the new local method, check for held amounts, permissions and call IOU.approveMoneyRequest() as appropriate (same logic as above).

Pass 'approve' as the type parameter to the <ProcessMoneyRequestHoldMenu component.

The rest should be the same, particularly with adding a new full parameter to IOU.payMoneyRequest and passing that onwards into the backend.

[BartoszGrajdek]

Hello, I'm Bartosz Grajdek from Software Mansion, the expert agency, and I would like to work on this task.

[BartoszGrajdek]

There's a mistake in the task description I think

followed by two Button's one for the held amount and payable amount

Shouldn't it say followed by two <Button>'s one for the non-held amount and one for payable amount?
Same thing here:

If they can, make the new <PayMoneyRequestHoldMenu visible, passing the full and held amounts to it so that it may be displayed on the buttons.

[BartoszGrajdek]

Also in:

Approving expense reports with held requests (approvers)
In src/components/ReportActionItem/ReportPreview.js, update <Button under shouldShowApproveButton

I think that the file you meant was actually src/components/MoneyReportHeader

[robertjchen]

You are correct, apologies for the confusion! 🙇

[robertjchen]

In progress!

[robertjchen]

In progress!

[BartoszGrajdek]

After a discussion with Robert, it turns out that some things weren't mentioned in the issue description, but they are necessary for it to work properly. Working on them right now and discussing everything with Robert accordingly 🙏🏻 

[robertjchen]

We're working on getting hold functionality out first, and will raise discussion around offline flows/final items around this shortly.

[robertjchen]

Please see main issue for a full-picture update: https://github.com/Expensify/Expensify/issues/274076#issuecomment-1878519877

[robertjchen]

In progress under WIP PR: #33124

[robertjchen]

Please see https://github.com/Expensify/Expensify/issues/274076#issuecomment-1899437428 for the latest consolidated update!

[robertjchen]

Backend changes for this was merged and on the way out by Wednesday 🙌

[trjExpensify]

Yo yo! @robertjchen @JmillsExpensify @BartoszGrajdek... just a heads up, I'm going to go through and prefix these project issues with [Held requests] because I can filter on that for the #wave-collect project board, but equally, a lot of people using in:title: [Hold] to filter out issues on hold.

[robertjchen]

Make sense, thanks! I've added it to all the other related GH issues from this list as well 👍

[robertjchen]

Latest update: https://expensify.slack.com/archives/C036QM0SLJK/p1710490163098179?thread_ts=1710203436.990009&cid=C036QM0SLJK

• Marco was able to reproduce the issue with the partial pay/approve flow and has a draft PR up.

[robertjchen]

Latest update with more context/discusion on Slack. We're trying to get past a backend bug with report previews that'll allow us to continue with the FE PR.

[robertjchen]

• Backend bugs were resolved and the changes were merged and deployed 🙌
• FE PR now unblocked, final Onyx-related changes in progress. Hopefully, we can get these merged by EOW? 🤞

[robertjchen]

• https://github.com/Expensify/Web-Expensify/pull/41362 were reviewed/merged and should be deployed to production today.
• The FE PR was updated/re-tested and Offline behavior worked great, we encountered one final error during testing, see below.
• Marco is actively investigating the final The requested amount has changed error on the backend. Hopefully it's not too bad and we can get everything on staging for testing by midweek? 🙏

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[bondydaa]

I had this blocker #39335 created which I think is related to this PR #33124.

The "bug" is that when you click approve on the request preview in the "main chat" it doesn't trigger the hold confirmation model like it does when you're directly viewing the manual request. I don't actually see that mentioned in the OP though so it might just not have been considered initially.

Is this something we'd want to block the roll out of the feature for? I'm leaning "no" but just want to double check that with ya'll.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[robertjchen]

@bondydaa Ah, great catch- definitely not a blocker, but definitely a polish item we should address!

[bondydaa]

okay coolio I'll remove the label then, want me to just assign the issue to @BartoszGrajdek and/or you? or let it go through the normal external bug flow?

[robertjchen]

I'd say external would be good!

[robertjchen]

Update

The FE changes are on staging and we've had a few bug reports come through already:

• No confirmation prompt when approving held request in the main chat - proposals by contributor under review
• Pay with Expensify button does not include amount after approving held request - proposals by contributors already lined up
• Confirmation to pay is still shown after approving all the requests - Appears to be frontend issue, should be able to be handled by contributors

Keep those reports coming! 🙌

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

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

• [WAVE7] Hold Requests: Approving/Paying expense reports with held requests #33124

If no regressions arise, payment will be issued on 2024-04-09. 🎊

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

• @BartoszGrajdek does not require payment (Contractor)