[Held requests] Hold button on expense preview appears only if open the expense first

[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: 9.0.19-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4845380&group_by=cases:section_id&group_order=asc&group_id=309128
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

Precondition:

1. Login as the owner of the workspace> Create a workspace
2. Invite the approver and employee
3. Enable "workflows"> On the "Workflow" editor - enable "Add Approvals"> Set the Approver account as the Approver

Steps:

1. Log in as the employee and submit 2 expenses to the workspace
2. Log in as the Approver> Navigate to the employee workspace chat with expenses
3. Right click on expenses to open the menu

Expected Result:

There should be Hold option when approver open the right click menu from expense

Actual Result:

Hold button on expense preview appears only if open the transaction thread page first

Workaround:

Unknown

Platforms:

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6569910_1723470417784.Recording__3689.mp4

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @jliexpensify (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 @francoisl (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[IuliiaHerets]

We think that this bug might be related to #wave-collect - Release 1

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[cdOut]

This is definitely related to the changes I've made in the hold cleanup PR, please assign me so I can look into fixing this issue.

[parasharrajat]

I don't this this is a regression but a new case to be handled.

[francoisl]

Sounds good, assigned to @cdOut and going to demote to non-blocker.

[trjExpensify]

I have a feeling this is related to some other issues in this list and OpenReport. CC: @robertjchen

[robertjchen]

It looks like this is still a strictly frontend issue? The hold option should be available in the menu without having to open the expense first

[robertjchen]

The BE change has now been deployed! The child reportAction summary will now return the childCanHold and childCanUnhold state in addition to the existing fields below. Please let me know if you're able to see and use the new fields 👍

childOldestFourAccountIDs
childCommenterCount
childVisibleActionCount
childLastVisibleActionCreated
childType
childReportNotificationPreference
childMoneyRequestCount
childLastMoneyRequestComment
childLastActorAccountID
childType
childLastReceiptTransactionIDs
childRecentReceiptTransactionIDs
childRecentReceiptTransactionIDs

[cdOut]

Thank you Robert, I'll raise a PR with the newly added params for this one later today.

[robertjchen]

Thanks @cdOut , let me know if you need anything else!

[cdOut]

@robertjchen I've tried implementing it with the newly added params, and while it does solve the issue of displaying properly when we can and cannot hold/unhold, we are actually missing data required to make an API call required to change the state of the hold.

That is being done through the IOU methods unholdRequest and putOnHold (This one isn't called directly but it is ran after going through the reason page so we still need data for it). They both require transactionID and reportID - I tried finding them in the data we have, but the required actions to retrieve them aren't yet loaded if we don't open the request. We'd have to add both of them to the params for me to fix the ability to change hold status.

[cdOut]

We could send over one additional paramchildHoldData instead of the two new ones with the following structure:

{
        isOnHold: boolean,
        transactionID: string,
        reportID: string,
},

Which we'd set to null/undefined if it wasn't holdable, isOnHold would help us disconcern whether it's currently held or unheld and what method to call on it, and the rest is the data required for me to make the API call through the IOU methods.

[robertjchen]

@cdOut Would we need to also include multiple transactionIDs and reportIDs if multiple children are on hold? 🤔

[cdOut]

Would we need to also include multiple transactionIDs and reportIDs if multiple children are on hold? 🤔

@robertjchen No, I don't think so. what we're implementing here into the context menu is supposed to be an alternative to the promoted action which we have in the report header menu, and in there we can only hold singular requests. Hold isn't currently available for the whole thread, I've attached a video as a reminder (Holding multiple requests at once is available through search, but then we have all the data loaded into onyx so we don't have to include it in this case).

So I'm fairly sure we'd only have to put in a singular requestID and a transactionID for that specific report preview action.

Screen.Recording.2024-10-22.at.06.59.51.mov

[cdOut]

@robertjchen Do you think the data structure I suggested for it is okay? I already have it implemented on the frontend so I could have it ready before we merge the backend changes.

[robertjchen]

@cdOut From my understanding, instead of returning childCanHold and childCanUnhold, we are to return a new childHoldData object instead, is that correct?

The problem is that at the point of the logic where we generate those flags on the backend, we get a list of held transactions (which implies there are multiple transactionIDs that need to be returned).

This means that the childHoldData object must be returned elsewhere and not at the current location where the childCanHold and childCanUnhold flags are.

Let me know if that makes sense- could you please link your draft PR so I can see where this is happening? Thanks!

[cdOut]

Hm the preview report action seems like the most suitable place to include this sort of data as it's very easy to read in the context menu code on the frontend, other places might prove to be problematic.

I'll raise the draft PR for it in a moment and cc you so you can take a look.

[cdOut]

@robertjchen Here's the draft for it, I've prepared it fully to support the suggested Hold data structure.

[robertjchen]

@cdOut I see, but the problem remains that for the report preview of the parent, we can have child reports with multiple transactions which are either on hold or not on hold.

Maybe the best approach is to just return the first held transaction in childHoldData (even if there are multiple held transactions) and run with that for now?

[cdOut]

Well if the preview is a summary of multiple expenses, we should pass null / undefined as childHoldData. Only if it's a reportAction of a preview for a singular expense we should pass the required data.

[robertjchen]

👍

[robertjchen]

That makes sense, I'll look into that now

[robertjchen]

This also means that in the case where a child transaction is on hold, but cannot be unheld (due to permissions), we will also return null / undefined as childHoldData.

[cdOut]

Yup, that's exactly it.

If the preview contains multiple expenses or is unholdable, we return null / undefined. For all other cases we fill out the data so I can properly display the option to hold / unhold them in the context menu.

[cdOut]

@robertjchen any updates on the backend for this issue? Thanks!

[robertjchen]

Testing out some local changes at the moment, aiming on having the changes reviewed and deployed tomorrow

[robertjchen]

Draft PR for BE changes up shortly

[blazejkustra]

@robertjchen Please ping me once that's deployed, thanks! 🙌

[sumo-slonik]

Hi everyone, unfortunately @cdOut has ended his cooperation with us and now I will take care of the completion of this issue. I will be very grateful for updates as soon as the backend is ready.

[melvin-bot]

📣 @sumo-slonik! 📣
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>

[robertjchen]

Circling back to this just now, will update shortly