Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.54k stars 2.88k forks source link

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

Open IuliiaHerets opened 2 months ago

IuliiaHerets commented 2 months ago

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.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): gocemate+workspaceapprover157@gmail.com 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:

Screenshots/Videos

https://github.com/user-attachments/assets/3bd3f688-4859-46e5-88fe-00251895e615

View all open jobs on GitHub

melvin-bot[bot] commented 2 months ago

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[bot] commented 2 months ago

Triggered auto assignment to @francoisl (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

IuliiaHerets commented 2 months ago

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

github-actions[bot] commented 2 months ago

:wave: 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 commented 2 months ago

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 commented 2 months ago

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

francoisl commented 2 months ago

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

trjExpensify commented 2 months ago

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

robertjchen commented 2 months ago

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

parasharrajat commented 2 months ago

I will be happy to review this PR given that I have a lot of context for this feature. Please assign me as C+ if possible. @jliexpensify @robertjchen

jliexpensify commented 2 months ago

Assigned you @parasharrajat

parasharrajat commented 2 months ago

PR is still in progress. Waiting updates.

cdOut commented 2 months ago

Currently waiting on the backend and steps to implement on the frontend suggested by @robertjchen

trjExpensify commented 2 months ago

Currently waiting on the backend and steps to implement on the frontend suggested by @robertjchen

@robertjchen do we need to assign you to this issue for that backend PR, and is it being worked on somewhere to link here?

robertjchen commented 2 months ago

Hm, I'm not sure what backend changes are needed here? My last suggestion here was a frontend approach: https://github.com/Expensify/App/issues/47241#issuecomment-2293798315

Was there a discussion elsewhere that led us to a backend conclusion?

robertjchen commented 2 months ago

I'm okay taking this on, but just wanted to make sure we distributed the work and avoided bottlenecking

cdOut commented 2 months ago

@robertjchen Well I tried the frontend approach but that meant calling the openReport API command which we've settled on discarding, as discussed in this slack thread. In the end we've decided on adding the childCanHold / childCanUnhold boolean flags to the report and last week Vit asked for some steps to add support for this change on the frontend side.

robertjchen commented 2 months ago

Ah got it- thanks for the context! I'll re-assign to myself and revisit 👀

cdOut commented 1 month ago

I've closed the previous PR, @robertjchen please let me know if this moves further and I can help out with the frontend implementation.

robertjchen commented 1 month ago

Diving into this task this week now that other issues are out of the way. More to come as I look into the BE

robertjchen commented 1 month ago

Just getting to this! It looks like the Hold option on the context menu is determined by the report action preview? https://github.com/Expensify/App/blob/main/src/libs/ReportUtils.ts#L3140

@cdOut Just wanted to check what the frontend will be expecting- does the backend need to push up a new childCanHold/childCanUbHold flag, or can it just update the existing properties on the report action (or report object?).

From what I see, once a particular expense is held, we just need to make an Onyx update to all participants of the report with the new/updated propertie(s).

cdOut commented 1 month ago

Let me read up on the context of this issue and I'll get back to you later with what exactly we need to pass here and to what to be able to hold the expense without opening it first.

As far as I remember we settled on adding the childCanHold / childCanUbHold flags to the parent report.

robertjchen commented 1 month ago

Thanks @cdOut . Let me know when you have the details 🙇

cdOut commented 3 weeks ago

@robertjchen What we need to do here is basically add parameters into report which are similar to hasOutstandingChildRequest or hasOutstandingChildTask that are already implemented, but instead we'll pass childCanHold / childCanUnhold into them.

We also cannot really modify the existing params, we have to add the two new ones for sure.

As far as I remember there is already functionality on the backend to get those values based on the child report action so it shouldn't be too difficult to implement and would require very little changes on the frontend as well.

robertjchen commented 3 weeks ago

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 commented 3 weeks ago

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

robertjchen commented 2 weeks ago

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

cdOut commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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

cdOut commented 2 weeks ago

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.

https://github.com/user-attachments/assets/52a71675-2ac6-4f19-b2ed-9e4c9da30ee6

cdOut commented 2 weeks ago

@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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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

robertjchen commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

👍

robertjchen commented 2 weeks ago

That makes sense, I'll look into that now

robertjchen commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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

robertjchen commented 1 week ago

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

robertjchen commented 6 days ago

Draft PR for BE changes up shortly

blazejkustra commented 5 days ago

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

sumo-slonik commented 4 days ago

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[bot] commented 4 days ago

📣 @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. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>