Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
6 tasks
luacmartins opened this issue May 3, 2024 · 52 comments
Open
6 tasks

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

luacmartins opened this issue May 3, 2024 · 52 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2

Comments

@luacmartins
Copy link
Contributor

luacmartins commented May 3, 2024

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?

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

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-06-07
Issue OwnerCurrent Issue Owner: @fedirjh
@luacmartins luacmartins added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 3, 2024
@luacmartins luacmartins self-assigned this May 3, 2024
@luacmartins luacmartins added the External Added to denote the issue can be worked on by a contributor label May 3, 2024
Copy link

melvin-bot bot commented May 3, 2024

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

@melvin-bot melvin-bot bot changed the title [Search v1] RHP reports show incorrect headers [$250] [Search v1] RHP reports show incorrect headers May 3, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 3, 2024
Copy link

melvin-bot bot commented May 3, 2024

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.

Copy link

melvin-bot bot commented May 3, 2024

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

@Niki106
Copy link

Niki106 commented May 3, 2024

Hi,
I'd like to work on this.

Copy link

melvin-bot bot commented May 3, 2024

📣 @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
Copy link

Niki106 commented May 3, 2024

@cretadn22
Copy link
Contributor

cretadn22 commented May 3, 2024

Action Performed:
Coming from #41347 (comment), 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
Copy link

Niki106 commented May 3, 2024

OK, I will do it quickly.

@rakhaxor
Copy link

rakhaxor commented May 3, 2024

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

Copy link

melvin-bot bot commented May 3, 2024

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

@nkdengineer
Copy link
Contributor

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

if (isSingleTransactionView) {
headerView = (

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

return isIOUReport(parentReport) && !isEmptyObject(parentReportAction) && ReportActionsUtils.isTransactionThread(parentReportAction);

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.

if (ReportActionsUtils.isTransactionThread(parentReportAction)) {

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

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

return isIOUReport(parentReport) && !isEmptyObject(parentReportAction) && ReportActionsUtils.isTransactionThread(parentReportAction);

  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
Copy link
Contributor

tsa321 commented May 5, 2024

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(READ_COMMANDS.SEARCH and stored in onyx Snapshot_

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

onSelectRow={(item) => {
openReport(item.transactionThreadReportID);
}}

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

Navigation.navigate(ROUTES.SEARCH_REPORT.getRoute(query, reportID));

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:

if (ReportUtils.isMoneyRequestReport(report) || ReportUtils.isInvoiceReport(report)) {
headerView = (
<MoneyReportHeader
report={report}
policy={policy}
transactionThreadReportID={transactionThreadReportID}
reportActions={reportActions}
shouldUseNarrowLayout={shouldUseNarrowLayout}
/>
);

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


This is iou/expense report:
iou/expense report:

Screenshot 2024-05-07 at 07 36 20


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:

openReport(item.transactionThreadReportID);

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.

@melvin-bot melvin-bot bot added the Overdue label May 5, 2024
@fedirjh
Copy link
Contributor

fedirjh commented May 6, 2024

@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.

@melvin-bot melvin-bot bot removed the Overdue label May 6, 2024
@tsa321
Copy link
Contributor

tsa321 commented May 6, 2024

@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
Copy link
Contributor

fedirjh commented May 6, 2024

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

@fedirjh
Copy link
Contributor

fedirjh commented May 6, 2024

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

@fedirjh
Copy link
Contributor

fedirjh commented May 6, 2024

@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
Copy link
Contributor

nkdengineer commented May 6, 2024

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

{Object.keys(parentNavigationSubtitleData).length > 0 && (
<ParentNavigationSubtitle

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
Copy link
Contributor

tsa321 commented May 6, 2024

@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:
macos-web-d.mp4

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?

@JmillsExpensify
Copy link

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

@JmillsExpensify JmillsExpensify added Weekly KSv2 and removed Daily KSv2 labels May 15, 2024
@melvin-bot melvin-bot bot removed the Overdue label May 15, 2024
Copy link

melvin-bot bot commented May 17, 2024

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

Copy link

melvin-bot bot commented May 17, 2024

@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!

@fedirjh
Copy link
Contributor

fedirjh commented May 19, 2024

Sorry I have missed this one.

@luacmartins There are two separate bugs. One bug affects the expense report and causes the incorrect header to be displayed because the parent report is missing in Onyx. We have two options (maybe more?) to fix this bug:

  1. Update the backend to include the parent report when opening the transaction thread.
  2. Update the front end to fetch the parent report as suggested in this proposal [$250] [Search v1] RHP reports show incorrect headers #41585 (comment). However, this solution may not be safe as it will fetch the parent report whenever it exists, leading to an increase in the number of openReport requests. @nkdengineer, can we update the hook to minimize these requests by only fetching the one from the search view?
Screenshot 2024-05-19 at 11 54 48 PM

The second bug is related to invoices. Even if the parent report already exists, the header displays the join thread action. Check the header, we have a link to the parent report which indicates that the parent report exists. I think @rezkiy37 Can help us debug it further since he implemented the invoice flow.

Screenshot 2024-05-19 at 11 54 22 PM

@tsa321
Copy link
Contributor

tsa321 commented May 19, 2024

@fedirjh could you re-review my proposal.

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.

TLDR:

Just change this line into:

openReport(item.transactionThreadReportID);

into openReport(item.reportID);

try it for these two type of transacations (transaction from + button -> Send invoice or Pay someone) and see if the join button disappears.

@nkdengineer
Copy link
Contributor

@nkdengineer, can we update the hook to minimize these requests by only fetching the one from the search view?

@fedirjh I think we can add to call Report.openReport for parent report here when we select a row here

openReport(item.transactionThreadReportID);

@rezkiy37
Copy link
Contributor

...I think @rezkiy37 Can help us debug it further since he implemented the invoice flow.

I see the work in progress here. If you need any help, please ping me 🙂

@fedirjh
Copy link
Contributor

fedirjh commented May 20, 2024

could you re-review my proposal.

@tsa321 I don't find your proposal valid because the root cause analysis doesn't explain the bug's cause.

the transaction thread id is stored in reportID and not in transactionThreadReportID

Can elaborate on that?

@tsa321
Copy link
Contributor

tsa321 commented May 20, 2024

the transaction thread id is stored in reportID and not in transactionThreadReportID

@fedirjh it is from the server, the data is from the server.

@luacmartins
Copy link
Contributor Author

@tsa321 are you saying that the server is returning the transaction thread reportID in the reportID key instead of transactionThreadReportID?

@tsa321
Copy link
Contributor

tsa321 commented May 21, 2024

@luacmartins yes.

Copy link

melvin-bot bot commented May 24, 2024

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

@nkdengineer
Copy link
Contributor

the transaction thread id is stored in reportID and not in transactionThreadReportID

I checked and I think it's incorrect, the reportID here is the ID of the expense report for the case send invoice because we only have one transaction so the expense report is displayed as combine report that isn't the transaction thread report.

@nkdengineer
Copy link
Contributor

For the header of the send invoice transaction thread, the problem is isSingleTransactionView doesn't not cover the case the report is send invoice request.

In the second thought if we remove the check isIOUReport in isIOURequest and isExpenseReport in isExpenseRequest, that makes it's wrong because then we will not be able to distinguish between expense and iou request. My idea here is only update isSingleTransactionView to check the parentReportAction is transaction thread or not like this because as I mentioned in my proposal we also only checking the parentReportAction is transaction thread report or not to display MoneyRequestView. With this, we also fix the case for send invoice.

const isSingleTransactionView = ReportActionsUtils.isTransactionThread(parentReportAction) || ReportUtils.isTrackExpenseReport(report) || ;

const isSingleTransactionView = ReportUtils.isMoneyRequest(report) || ReportUtils.isTrackExpenseReport(report);

@fedirjh What do you think?

@luacmartins
Copy link
Contributor Author

@fedirjh were you able to take a look at the comments above?

@melvin-bot melvin-bot bot added the Overdue label May 30, 2024
Copy link

melvin-bot bot commented May 31, 2024

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

Copy link

melvin-bot bot commented May 31, 2024

@JmillsExpensify @luacmartins @fedirjh this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@luacmartins
Copy link
Contributor Author

@nkdengineer we can't rely on checking the parent report action because that data is not available in search.

@nkdengineer
Copy link
Contributor

@luacmartins When we view a search result, OpenReport API will be called and it will contain parent report action data.

Screen.Recording.2024-06-01.at.04.33.55.mov

@luacmartins
Copy link
Contributor Author

That's correct. That being said, I've seen the incorrect header being shown before the call to OpenReport returns a response. How will we handle that case?

@fedirjh
Copy link
Contributor

fedirjh commented Jun 3, 2024

@luacmartins What do you think about updating the backend so that the parent report action data is returned within the OpenReport of the transaction report? I believe this change would simplify things and decrease the number of API calls made to the server.

@melvin-bot melvin-bot bot removed the Overdue label Jun 3, 2024
@luacmartins
Copy link
Contributor Author

luacmartins commented Jun 4, 2024

@fedirjh don't we already return that in OpenReport? I just tested opening a transaction thread and the parent IOU action was returned as well

Screenshot 2024-06-04 at 4 59 16 PM

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@nkdengineer
Copy link
Contributor

That's correct. That being said, I've seen the incorrect header being shown before the call to OpenReport returns a response. How will we handle that case?

@luacmartins

I just checked and saw that:

  1. If the transaction thread report doesn't return in OpenApp API (notification is hidden), when we open this report skeleton will be displayed before OpenReport API is complete so this case works well with parentReportAction check

  2. If this report is returned in OpenApp, I see that parentReportAction of this is also return as well so this case also work well

Correct me if I missed st.

Copy link

melvin-bot bot commented Jun 7, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2
Projects
Status: Polish
Development

No branches or pull requests

10 participants