App icon indicating copy to clipboard operation
App copied to clipboard

[HOLD 53738][$250] Hold doesn't show in three dot menu until report is opened

Open m-natarajan opened this issue 1 year ago • 33 comments

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.60-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @garrettmknight Slack conversation (hyperlinked to channel name): ts_external_expensify_expense

Action Performed:

  1. Create a workspace
  2. Invite Submitter + Approver A
  3. Enable Workflows > Approval + Delay Submission
  4. Set Approver A as approver
  5. As Submitter, create expense in WS chat + submit
  6. As Approver A, navigate to Submitter's WS chat + click the three dot menu on the expense

Expected Result:

Since this is a one expense report, you should see Hold in the three dot menu for the report

Actual Result:

Describe what actually happened

Workaround:

You don't see Hold until you access the expense/report and back out into the WS chat again.

Platforms:

Which of our officially supported platforms is this issue occurring on?

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/0c884c5e-83c8-4125-8087-45f44aca66eb

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021857521697236720139
  • Upwork Job ID: 1857521697236720139
  • Last Price Increase: 2024-11-22
Issue OwnerCurrent Issue Owner: @neil-marcellini

m-natarajan avatar Nov 12 '24 14:11 m-natarajan

Triggered auto assignment to @CortneyOfstad (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] avatar Nov 12 '24 14:11 melvin-bot[bot]

I was able to recreate this under my expensifail.com account, so getting some eyes on this!

CortneyOfstad avatar Nov 15 '24 20:11 CortneyOfstad

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

melvin-bot[bot] avatar Nov 15 '24 20:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 15 '24 20:11 melvin-bot[bot]

Edited by proposal-police: This proposal was edited at 2024-11-16 23:02:19 UTC.

Proposal

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

The three dot menu doesn't show Hold menu item when a new report is created

What is the root cause of that problem?

The issue is in the file BaseReportActionContextMenu file. This is the piece of code where the problem is.

  const requestParentReportAction = useMemo(() => {
        if (isMoneyRequestReport || isInvoiceReport) {
            if (!paginatedReportActions || !transactionThreadReport?.parentReportActionID) {
                return undefined;
            }
            return paginatedReportActions.find((action) => action.reportActionID === transactionThreadReport.parentReportActionID);
        }
        return parentReportAction;
    }, [parentReportAction, isMoneyRequestReport, isInvoiceReport, paginatedReportActions, transactionThreadReport?.parentReportActionID]);

    const moneyRequestAction = transactionThreadReportID ? requestParentReportAction : parentReportAction;

The moneyRequestAction is required to be true to show Hold menu item. We have transactionThreadReportID and so the first operation of the ternary operator gets evaluated. requestParentReportAction returns undefined since this condition gets evaluated to true due to absence of parentReportActionID in transactionThreadReport.

if (!paginatedReportActions || !transactionThreadReport?.parentReportActionID) {
                return undefined;
}

like mentioned in the workaround, once we open the report and get back to the chat, we start seeing Hold coz transactionThreadReport get populated with all it's fields including parentReportActionID and

return paginatedReportActions.find((action) => action.reportActionID === transactionThreadReport.parentReportActionID);

gets evaluated to true.

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

The problem is with report loading since our condition relies on transactionThreadReport and when the report is first created, transactionThreadReport object is not loaded fully (missing major fields including parentReportActionID). So have got to make sure it gets loaded properly in Onyx collection.

What alternative solutions did you explore? (Optional)

Alternate would be to change the condition required to show Hold but I wouldn't go that route as it might break some other stuff that we are unaware of.

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

me-ZaidAli avatar Nov 16 '24 12:11 me-ZaidAli

📣 @me-ZaidAli! 📣 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>

melvin-bot[bot] avatar Nov 16 '24 12:11 melvin-bot[bot]

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~01dc2d382489d84c0b

me-ZaidAli avatar Nov 16 '24 12:11 me-ZaidAli

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

melvin-bot[bot] avatar Nov 16 '24 12:11 melvin-bot[bot]

@ahmedGaber93 any thoughts on the proposal above? Thanks!

CortneyOfstad avatar Nov 18 '24 18:11 CortneyOfstad

Sorry for the delay, Reviewing today

ahmedGaber93 avatar Nov 19 '24 06:11 ahmedGaber93

@CortneyOfstad can clarify whether individual expenses inside multiple expense report holdable or no?

me-ZaidAli avatar Nov 19 '24 08:11 me-ZaidAli

I think this is a BE issue.

The reportAction data that comes from pusher doesn't have childReportID that used here which break showing the hold item. And if you open any other report, then go back to the expense report to load the API data, you will see the issue is fixed.

https://github.com/user-attachments/assets/f6851eb4-b66e-4cbf-8806-fca1fa4fc0a0


Comparison of reportAction between pusher and API data.

Screenshot 2024-11-19 at 10 01 23 PM

  1. add console.log for reportAction
  2. reproduce the issue
  3. hover on the reportAction view to log the reportAction data that come from the pusher (doesn't have childReportID)
  4. open any other report, then back to the expense report
  5. hover on the reportAction view to log the reportAction data that come from the API (have the childReportID)

the missed reportAction.childReportID is not the only problem, in the data above I see the pusher data have reportAction?.originalMessage?.linkedReportID which is equal to the missedreportAction?.childReportID so we can use linkedReportID in FE here (just for testing) . but after trying to use it the code starts to work fine then break again in canHoldUnholdReportAction, so I think we have another missed data.

ahmedGaber93 avatar Nov 20 '24 06:11 ahmedGaber93

@CortneyOfstad I think this is a BE issue and should be internally.

ahmedGaber93 avatar Nov 20 '24 07:11 ahmedGaber93

@ahmedGaber93 which environment are you testing it on? for me on staging the childReportId is present in reportAction . The issue that you mentioned with canHoldUnholdReportAction happens here since we are unable to fetch childReport for the IOU report action. So when we open the report, FE fetches the report data from BE and it's cached locally for us to access in the chat. transactionThreadReport becomes available and so we start seeing the Hold option. In addition to this issue there is a lot of refactoring needed in the component since it's filled with stuff and conditions that are not even needed.

me-ZaidAli avatar Nov 20 '24 11:11 me-ZaidAli

@me-ZaidAli Testing on DEV with latest main branch.

ahmedGaber93 avatar Nov 20 '24 13:11 ahmedGaber93

Testing on staging childReportID is not present with pusher data and present only on API data.

https://github.com/user-attachments/assets/059233e1-d97b-4295-b38a-1c54bfc9fd94

ahmedGaber93 avatar Nov 20 '24 13:11 ahmedGaber93

The issue that you mentioned with canHoldUnholdReportAction happens here since we are unable to fetch childReport for the IOU report action.

I agree with this part, the parentReportActionID also is not present on transactionThreadReport that come from pusher, so we also need to fix it from BE side

Screenshot 2024-11-20 at 6 15 39 AM

I think the missed data from pusher that need to fix this is:

  1. reportAction.childReportID
  2. transactionThreadReport.parentReportActionID

ahmedGaber93 avatar Nov 20 '24 14:11 ahmedGaber93

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Nov 22 '24 16:11 melvin-bot[bot]

@CortneyOfstad bump https://github.com/Expensify/App/issues/52401#issuecomment-2487676060

ahmedGaber93 avatar Nov 24 '24 08:11 ahmedGaber93

Moving to Hot Picks

muttmuure avatar Nov 25 '24 17:11 muttmuure

Thanks Matt! Sorry for the delay here!

CortneyOfstad avatar Nov 26 '24 17:11 CortneyOfstad

@CortneyOfstad, @ahmedGaber93 Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Nov 27 '24 09:11 melvin-bot[bot]

Not overdue, waiting for an internal developer.

ahmedGaber93 avatar Nov 27 '24 11:11 ahmedGaber93

@CortneyOfstad, @ahmedGaber93 Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] avatar Dec 03 '24 09:12 melvin-bot[bot]

Not overdue, waiting for an internal developer.

ahmedGaber93 avatar Dec 03 '24 09:12 ahmedGaber93

Can someone please open a separate GH issue for an internal engineer using this template? It will ensure that an engineer is assigned and they have the full context of what they need to do without reading through the entire comment history and try to piece together what the problem is.

tgolen avatar Dec 03 '24 16:12 tgolen

I'll grab it and sort it out

neil-marcellini avatar Dec 03 '24 17:12 neil-marcellini

Sounds good @tgolen and thank you @neil-marcellini!

CortneyOfstad avatar Dec 03 '24 20:12 CortneyOfstad

I wasn't able to reproduce this on the latest dev environment. I tried to reproduce on staging, but I'm blocked by a separate bug. I will report it, put this one on HOLD, and try to fix that bug first.

neil-marcellini avatar Dec 04 '24 22:12 neil-marcellini

This was reported in Slack here, we'll discuss from there.

neil-marcellini avatar Dec 04 '24 22:12 neil-marcellini