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
2.99k stars 2.5k forks source link

[$250] [Search v1] RHP reports show incorrect headers #41585

Open luacmartins opened 2 weeks ago

luacmartins commented 2 weeks 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: Reproducible in staging?: Reproducible in production?: 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: Slack conversation:

Action Performed:

Coming from here, the report headers in the RHP are incorrect. They should show the headers for a transaction thread.

Expected Result:

Reports show incorrect header

Actual Result:

Reports don't show correct header

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~017a5debcfaef3f611
  • Upwork Job ID: 1786404294566879232
  • Last Price Increase: 2024-05-17
Issue OwnerCurrent Issue Owner: @fedirjh
melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @JmillsExpensify (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 weeks ago

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

Niki106 commented 2 weeks ago

Hi, I'd like to work on this.

melvin-bot[bot] commented 2 weeks ago

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

Contributor details nikimijajlovicfl@hotmail.com https://www.upwork.com/freelancers/~012b26235f47ef40e2

cretadn22 commented 2 weeks ago

Action Performed: Coming from https://github.com/Expensify/App/pull/41347#issuecomment-2091956546, the report headers in the RHP are incorrect. They should show the headers for a transaction thread. Expected Result: Reports show incorrect header Actual Result: Reports don't show correct header

it's a bit confusing

@luacmartins @fedirjh Could you detail the description of this bug or attach a video?

Niki106 commented 2 weeks ago

OK, I will do it quickly.

rakhaxor commented 2 weeks ago

Contributor details Your Expensify account email: funsofts@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01f94219e6101472c1

melvin-bot[bot] commented 2 weeks ago

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

nkdengineer commented 2 weeks ago

Proposal

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

Reports don't show correct header

What is the root cause of that problem?

We render the header as MoneyRequestHeader if it's a money request by using isMoneyRequest function

https://github.com/Expensify/App/blob/2f5173c9c0f041967acfbdf9581ce52327623fa6/src/pages/home/ReportScreen.tsx#L338-L339

In this function, we're checking if the parentReport is IOU report and the parentReportAction is transaction thread.

https://github.com/Expensify/App/blob/2f5173c9c0f041967acfbdf9581ce52327623fa6/src/libs/ReportUtils.ts#L1343

But if the IOU report has notification as hidden, OpenApp API doesn't return this report. So after we logout and login again, the IOU report doesn't exist and then when we open the transaction thread report, the header is rendered as HeaderView component that shows the Join button.

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

  1. In ReportActionItem, we only checking the parentReportAction is transaction thread report or not to display MoneyRequestView.

https://github.com/Expensify/App/blob/2f5173c9c0f041967acfbdf9581ce52327623fa6/src/pages/home/report/ReportActionItem.tsx#L768

So we can do the same for isIOURequest and isExpenseRequest function here

!isEmptyObject(parentReportAction) && ReportActionsUtils.isTransactionThread(parentReportAction);

https://github.com/Expensify/App/blob/2f5173c9c0f041967acfbdf9581ce52327623fa6/src/libs/ReportUtils.ts#L1343

  1. To display the parent subtitle in header in MoneyRequestView or ReportScreen we should call OpenReport API with reportID is the parentReportID of transaction thread report if the parent report doesn't exist.

What alternative solutions did you explore? (Optional)

NA

tsa321 commented 1 week ago

Proposal

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

RHP search report view displays incorrect headers (different from report screen view in center)

What is the root cause of that problem?

The search result is generated by API.read(READCOMMANDS.SEARCH and stored in onyx Snapshot

When user select search result item, the app will display RHP report view. Based on:

https://github.com/Expensify/App/blob/ab1e924bf0d4f928a9006f5bdd6b9da60346b1df/src/components/Search.tsx#L67-L69

The parameter used to open report is item.transactionThreadReportID, then the openReport function will open the thread:

https://github.com/Expensify/App/blob/ab1e924bf0d4f928a9006f5bdd6b9da60346b1df/src/components/Search.tsx#L55

But the actual report thread in the search result is placed in the reportID property, so instead of opening thread report the openReport wil open thread of thread of the search result.

For more details:

In Snapshot onyx:

  1. For transaction from + button -> Send invoice or Pay someone , the transaction thread id is stored in reportID and not in transactionThreadReportID. The transactionThreadReportID contains thread id generated by server that we don't even visit that thread yet.
  2. For transaction from + button -> Submit expense, the transaction thread is correctly stored in transactionThreadReportID.

This will cause different report header than the real thread of the search result. In reportscreen:

https://github.com/Expensify/App/blob/ab1e924bf0d4f928a9006f5bdd6b9da60346b1df/src/pages/home/ReportScreen.tsx#L361-L370

In this line because of different reportID the ReportUtils.isMoneyRequestReport(report) || ReportUtils.isInvoiceReport(report) check will fail and other headerview will be used and will display join button.

Additional info: @luacmartins it is in my RCA. Let me confirm with images. This is transaction thread:
transaction thread ![Screenshot 2024-05-07 at 07 36 39](https://github.com/Expensify/App/assets/114975543/42e51cd7-68c9-4544-93a5-54dd61f55ab4)

This is iou/expense report:
iou/expense report: ![Screenshot 2024-05-07 at 07 36 20](https://github.com/Expensify/App/assets/114975543/71fb89be-bebb-4e56-8482-3e493f082ede)

In Snapshot onyx: 1. For transaction from `+ button` -> `Send invoice` or `Pay someone` , the transaction thread id is stored in `reportID` and not in `transactionThreadReportID`. The `transactionThreadReportID` contains thread id generated by server that we don't even visit that thread yet. 2. For transaction from `+ button` -> `Submit expense`, the transaction thread is correctly stored in `transactionThreadReportID`.

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

In:

https://github.com/Expensify/App/blob/ab1e924bf0d4f928a9006f5bdd6b9da60346b1df/src/components/Search.tsx#L68

We could use item.reportID instead of item.transactionThreadReportID.

If the report type is iou submit expense type from + button -> submit expense, the parent report will be displayed instead of transaction thread, in this case we could use item.transactionThreadReportID.

fedirjh commented 1 week ago

@nkdengineer and @tsa321 Can you please share testing steps you used to replicate the issue?


@Niki106 To get started, you should refer to the CONTRIBUTING Document for guidance.

tsa321 commented 1 week ago

@fedirjh make invoice request to someone and select a workspace. + button -> Send invoice -> enter amount -> select a user to send the invoice -> select a workspace. Then in the search page result (Troubleshoot -> navigate to new search page) click on the invoice request item (the top most item you just created). The join button will be displayed in the RHP report view.

fedirjh commented 1 week ago

Thank you @tsa321. I am able to replicate with your steps.

fedirjh commented 1 week ago

@nkdengineer Thank you for the proposal, can you elaborate more on the second point?

fedirjh commented 1 week ago

@tsa321 After testing your solution, It did not appear to be the correct one.

  1. The issue can be reproduced when navigating to the transaction thread from outside the search page.
  2. The issue is not limited to invoice requests, but is also replicable with other transactions.
Screenshot 2024-05-06 at 10 54 52 AM
nkdengineer commented 1 week ago

To display the parent subtitle in header in MoneyRequestView or ReportScreen we should call OpenReport API with reportID is the parentReportID of transaction thread report if the parent report doesn't exist.

@fedirjh After fixing the point 1, the header is rendered correctly with MoneyRequestHeader but the parent subtitle that will bring us to the IOU report still doesn't appear because the IOU report is empty

https://github.com/Expensify/App/blob/942cad969430f17c5477ae519bf6c3508cb761f8/src/components/AvatarWithDisplayName.tsx#L140-L141

So to display this we should call OpenReport API with reportID is parentReportID of the transaction thread report in ReportScreen like this

useEffect(() => {
    if (!report.parentReportID) {
        return;
    }
    Report.openReport(report.parentReportID);
}, [report.parentReportID])
tsa321 commented 1 week ago

@fedirjh for point 1,

  1. The issue can be reproduced when navigating to the transaction thread from outside the search page.

Yes of course, because the RHP is just a report screen so it will reflect what is shown by report screen in center. It is same component. The join button is displayed because it is thread of thread of the transaction report (this thread report id is generated by back end) and we don't join the thread yet so the join button is shown.

You can try this in any other thread, then press three dot menu button and then click leave, the join button will be shown if you go back to that thread.

What I am trying to show here is, The Right panel doesn't use correct report id:

video: https://github.com/Expensify/App/assets/114975543/2b47288c-6937-456b-a175-0087d4a1de25

As you can see in the video, I think the intended report id should be displayed in right panel is report id displayed in center in this video. I have send some messages in the report, but because the app use item.transactionThreadReportID the other report shown instead and there is no reply at all in the report in Right panel. Also, as you can see in the url bar the report id shown in the center and right panel is different.

  1. The issue is not limited to invoice requests, but is also replicable with other transactions.

For second point could you give me steps to reproduce it for other request?

luacmartins commented 1 week ago

But the actual report thread in the search result is placed in the reportID property, so instead of opening thread report the openReport wil open thread of thread of the search result.

@nkdengineer I'm not sure that I understand this part of your RCA. We want to open the transactionThreadReportID so users can view/edit the transaction directly. We do not want to open the iou/expense report, which is what reportID is storing. So it seems like the issue comes from the fact that we're opening the transactionThreadReportID, but don't have the parent report stored locally, is that right?

tsa321 commented 1 week ago

@luacmartins it is in my RCA.

Let me confirm with images.

This is transaction thread:

transaction thread ![Screenshot 2024-05-07 at 07 36 39](https://github.com/Expensify/App/assets/114975543/42e51cd7-68c9-4544-93a5-54dd61f55ab4)


This is iou/expense report:

iou/expense report: ![Screenshot 2024-05-07 at 07 36 20](https://github.com/Expensify/App/assets/114975543/71fb89be-bebb-4e56-8482-3e493f082ede)


In Snapshot onyx:

  1. For transaction from + button -> Send invoice or Pay someone , the transaction thread id is stored in reportID and not in transactionThreadReportID. The transactionThreadReportID contains thread id generated by server that we don't even visit that thread yet.
  2. For transaction from + button -> Submit expense, the transaction thread is correctly stored in transactionThreadReportID.
luacmartins commented 1 week ago

For transaction from + button -> Send invoice or Pay someone , the transaction thread id is stored in reportID and not in transactionThreadReportID. The transactionThreadReportID contains thread id generated by server that we don't even visit that thread yet.

This seems like an issue that needs to be fixed. @fedirjh did you get a chance to go over the proposals yet?

fedirjh commented 1 week ago

The join button is displayed because it is thread of thread of the transaction report (this thread report id is generated by back end) and we don't join the thread yet so the join button is shown.

@luacmartins @tsa321 , As already explained in this proposal , the transaction request is treated as a thread because we have a check on the parent report isIOUReport(parentReport), however, this check fails because we don't have the parent report stored locally.

For second point could you give me steps to reproduce it for other request?

@tsa321 Just any older money request (make sure a skeleton is displayed at first when you open the transaction)

melvin-bot[bot] commented 1 week ago

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

@JmillsExpensify, @luacmartins, @fedirjh Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 4 days ago

@JmillsExpensify, @luacmartins, @fedirjh 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

luacmartins commented 4 days ago

@fedirjh are we still looking for proposals or are we selecting one of the current proposals?

JmillsExpensify commented 3 days ago

Looks like we're still waiting for a successful proposal, though it'd be good to confirm.

melvin-bot[bot] commented 1 day ago

📣 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] commented 1 day ago

@JmillsExpensify @luacmartins @fedirjh this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!