[Awaiting Payment 20th August] [Held requests] Clean up the hold/unhold logic

[trjExpensify]

Coming from here, we're seeing some inconsistencies in the intended design for hold/unhold logic. For example, a member on a workspace can unhold their expense even if the approver put it on hold.

@robertjchen @BartoszGrajdek let's go through this and make sure we clean up any inconsistencies. Any questions or further clarifiers necessary, let's align in the Slack thread.

Hold

Expenses on expenseReports

• Members can hold their own expenses.
• Approvers can hold expenses submitted to them.
• Admins can hold any expense on a workspace they’re an admin of.

Expenses on iouReports

• Members can hold each others expenses.

Unhold

Expenses on expenseReports

• Members can unhold their own expenses, but only if they themselves put the expense on hold.
• Approvers can unhold expenses submitted to them.
• Admins can unhold any held expense on a workspace they’re an admin of.

Expenses on iouReports

• Members can only unhold expenses they put on hold themselves.

[melvin-bot]

Current assignee @trjExpensify is eligible for the Bug assigner, not assigning anyone new.

[robertjchen]

Thanks for going through this and laying out all the cases! 🙇

[trjExpensify]

Anytime!

[cdOut]

Hi, I'm Tymoteusz Kałuzieński from Software Mansion, the expert agency, and I'm going to be working on this issue.

[BartoszGrajdek]

Hi! I'm currently quite busy with maintainment of live markdown lib, but @cdOut from our SWM team has the capacity to take this over. He has already worked with hold requests feature, so he knows everything necessary to work on this issue. 🙏🏻

[trjExpensify]

Great!

[robertjchen]

Started on the backend, draft PR linked!

Having reviewed the existing logic, it looks like the only key thing we would have to change, at least for the backend, is just the unhold case (for both iouReports and expenseReports): Members can only unhold expenses they put on hold themselves. (with the exception of approvers and admins). All other cases appear to be working as intended.

I should have the backend PR wrapped up with automated tests tomorrow.

@cdOut Let me know if you have any questions on the frontend! In the Hold reportAction, we store the accountID of the person who performed the initial hold action, so you should be able to check that and display the Unhold button if the viewer is authorized to perform the action 👍

[robertjchen]

Backend PR completed and now under review 👍

[robertjchen]

@cdOut Just to clarify that the frontend changes can be done independently without having to wait for the backend, let me know if you have any questions on the flows! 🙇

[cdOut]

I'll start looking into this one today, I'll let you know if I have any questions.

[parasharrajat]

Happy to review this PR.

[trjExpensify]

Assigning @parasharrajat for the review. @cdOut let us know when the PR is up. Thanks!

[cdOut]

I'll have a draft PR ready by the EOD.

[cdOut]

My work on this got delayed due to having to implement some changes after merging the Details Revamp flow for hold/unhold logic, I should have a PR for this today.

[melvin-bot]

@robertjchen, @trjExpensify, @parasharrajat, @cdOut Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[cdOut]

It is in draft, will move through to review by tomorrow.

[robertjchen]

Thanks @cdOut ! Let me know if you need anything else, the backend fixes were deployed 👍

[robertjchen]

In progress, some dependencies to be sorted out. Latest update here: #45151 (comment)

[robertjchen]

Finishing touches on the PR, looks on track to be merged later today.

[robertjchen]

Under review/testing, looks like a few final bugs to hammer out, but we're close

[robertjchen]

PR was updated and now under review! 🏃

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

Closing this out as this was internal, please re-open if we missed anything.

[parasharrajat]

@robertjchen C+ payment is pending on this.

[dylanexpensify]

Payment summary:

• Contributor+: @parasharrajat $250

Please request on NewDot!

[trjExpensify]

Thanks for that, Dylan! With the payment summary done, we can close it now. Thanks!

[parasharrajat]

payment requested as per #44573 (comment)

[JmillsExpensify]

$250 approved for @parasharrajat