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.98k stars 2.98k forks source link

[$250] Remove the `getTripTransactions` method #55077

Open tgolen opened 2 weeks ago

tgolen commented 2 weeks ago

Coming from https://github.com/Expensify/App/pull/54965#discussion_r1910531896

Problem

When getTripTransactions() is called, it accesses data from Onyx that the view is not connected to. This breaks the reactivity of the component because when the transactions are updated in Onyx, the component won't receive the changes. This leads to stale data.

Solution

Remove getTripTransactions() and connect to the data using useOnyx() instead.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021877740338376092500
  • Upwork Job ID: 1877740338376092500
  • Last Price Increase: 2025-01-10
  • Automatic offers:
    • shubham1206agra | Contributor | 105641744
Issue OwnerCurrent Issue Owner: @s77rt
melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @isabelastisser (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

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

melvin-bot[bot] commented 2 weeks ago

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

shubham1206agra commented 2 weeks ago

@tgolen You need to assign me for review as no one has trip rooms set up right now.

mohit6789 commented 2 weeks ago

Proposal

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

Make changes in getTripTransactions() so it will use reports using withOnyx() instead of Onyx.connect

What is the root cause of that problem?

Improvement remove getTripTransactions and use withOnyx()

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

  1. Make changes in ReportsUtlis so it can access reports data using withOnyx().

https://github.com/Expensify/App/blob/3541c893fb338341da376860ce97a4110ef7d9a5/src/libs/ReportUtils.ts#L8175-L8183

function getTripTransactions(reports: OnyxCollection<Report>, tripRoomReportID: string | undefined, reportFieldToCompare: 'parentReportID' | 'reportID' = 'parentReportID'): Transaction[] {
    const tripTransactionReportIDs = Object.values(reports ?? {})
        .filter((report) => report && report?.[reportFieldToCompare] === tripRoomReportID)
        .map((report) => report?.reportID);
    return tripTransactionReportIDs.flatMap((reportID) => getReportTransactions(reportID));
}
  1. pass reports data from TripDetailsView.tsx

https://github.com/Expensify/App/blob/3541c893fb338341da376860ce97a4110ef7d9a5/src/components/ReportActionItem/TripDetailsView.tsx#L149

const [reports] = useOnyx(ONYXKEYS.COLLECTION.REPORT);
const tripTransactions = ReportUtils.getTripTransactions(reports, tripRoomReportID);
  1. pass reports data from TripRoomPreview.tsx https://github.com/Expensify/App/blob/3541c893fb338341da376860ce97a4110ef7d9a5/src/components/ReportActionItem/TripRoomPreview.tsx#L120

changes in both the components

const [reports] = useOnyx(ONYXKEYS.COLLECTION.REPORT);
const tripTransactions = ReportUtils.getTripTransactions(reports, chatReport?.reportID);

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

github-actions[bot] commented 2 weeks ago

github123 Your proposal will be dismissed because you did not follow the proposal template.

Shahidullah-Muffakir commented 2 weeks ago

Proposal

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

When getTripTransactions() is called directly in the component, it accesses Onyx data that the component is not connected to. This breaks reactivity because the component won't re-render when the underlying transactions data changes in Onyx, leading to stale data being displayed to users.

What is the root cause of that problem?

The root cause is that we're accessing Onyx data through a utility function without establishing a proper subscription to that data.

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

Instead of calling getTripTransactions() directly, we should connect to the data using useOnyx with a selector that utilizes the existing utility function. Like this: here: https://github.com/Expensify/App/blob/3541c893fb338341da376860ce97a4110ef7d9a5/src/components/ReportActionItem/TripDetailsView.tsx#L149 and here: https://github.com/Expensify/App/blob/3541c893fb338341da376860ce97a4110ef7d9a5/src/components/ReportActionItem/TripRoomPreview.tsx#L120

write it like this:

    const [tripTransactions] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {
        selector:(reports)=>  ReportUtils.getTripTransactions(reports, chatReport?.reportID),
    });

and accept reports as param in the getTripTransactions

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 2 weeks ago

πŸ“£ @shubham1206agra πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

shubham1206agra commented 2 weeks ago

I am sorry everyone. But this will be handled by @blazejkustra since you don't have access to trip rooms.

shubham1206agra commented 2 weeks ago

@blazejkustra Please comment here so we can get you assigned.

blazejkustra commented 2 weeks ago

Commenting for assigment.

tgolen commented 2 weeks ago

Sorry, I didn't realize this was a locked-down feature!

isabelastisser commented 1 week ago

@shubham1206agra @blazejkustra, can you please provide an update? What are the next steps? Thanks!

blazejkustra commented 1 week ago

Sorry, I had other tasks that required my attention. I'm starting to work on this one now!

Remove getTripTransactions() and connect to the data using withOnyx() instead.

@tgolen I assume you meant to use useOnyx, and not withOnyx (as it is deprecated)

tgolen commented 1 week ago

Ah, haha, yes. useOnyx πŸ˜‚

blazejkustra commented 1 week ago

@tgolen Draft PR is up, please let me know if it’s in line with what you had in mind!

I found another issue though, and I'm not sure what is the best approach to fixing it and I'd be glad to hear your thoughts (also cc @rlinoz @shubham1206agra). Basically, whenever you make a reservation, a report action is added to the workspace chat with a trip preview (TripRoomPreview component). And while it looks well when the nested data is loaded:

Image

It doesn't work right after I sign in, this is because the nested trips aren't fetched from the backend:

Image

In order to see them I have to click on 'View trip', which as a side effect calls OpenReport for the trip:

Image

Any idea how to fix it? Maybe there is a similar flow in the app already.

rlinoz commented 1 week ago

This is the same as report previews right? I think the fix is to return the reports either on OpenApp or on OpenReport for the parent report, not sure which one we do for report previews

tgolen commented 1 week ago

Sounds like a backend fix needs to be done for that, yeah. I'll cc @stitesExpensify about it so he is aware of the issue.

blazejkustra commented 5 days ago

Update: PR is under review