Remove the getTripTransactions method #55264
Conversation
src/libs/ReportUtils.ts
Outdated
| .filter((report) => report && report?.parentReportID === tripRoomReportID) | ||
| .map((report) => report?.reportID); | ||
|
|
||
| return tripTransactionReportIDs.flatMap((reportID) => getReportTransactions(reportID)); |
There was a problem hiding this comment.
It looks like getReportTransactions() is also being called, which is another part that is breaking the reactivity. The transactions should be connected to the component.
There was a problem hiding this comment.
I saw someone is submitting a PR with something kinda like what we need: #55297
Maybe it can give you some ideas?
There was a problem hiding this comment.
Yesterday, I spent time investigating this issue, aiming to find a more optimal way to connect a trip room with its "trip transactions."
The current structure for each trip consists of a report of type tripRoom, which may have one or more child reports (expenses). Some of these child reports include transactions containing reservation data, which we ultimately want to display in the trip summary.
I explored the possibility of identifying common fields that could connect the parent trip room to these transactions directly. However, at this point, the only viable option is to fetch all reports and transactions from Onyx and then filter them—a process that is potentially expensive for bigger accounts.
Based on my investigation, I think the solution implemented in my last commit is currently the best (and only) approach to calculate this data.
There was a problem hiding this comment.
@rlinoz @tgolen Do you think it would be reasonable to introduce an easier way to link the parent trip chat with its corresponding trip transactions?
One idea I had is to add a tripID field to transactions that include reservationList. This would allow us to skip fetching reports from Onyx and instead directly filter the transactions. Here's some pseudocode to illustrate:
function getAllTripsTransactions(tripID: string, transactions: OnyxCollection<Transaction>): Transaction[] {
const tripTransactions: Transaction[] = Object.values(transactions ?? {}).filter(
(transaction) => transaction?.receipt?.reservationList.tripID === tripID,
);
return tripTransactions;
}
const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION);
const tripTransactions = getAllTripsTransactions(tripID, transactions)The only drawback I see with this approach is that it would require a migration to ensure backward compatibility. We would need to update existing transactions to include the tripID so older data remains functional, which could add some complexity to the implementation.
Do you think the trade-off is worth it for the potential performance and simplicity gains?
There was a problem hiding this comment.
I like the suggestion, but that does add some complexity here that I feel probably isn't worth it. I will also CC @stitesExpensify on this one since he knows a lot about how this travel data is stored.
I agree that I don't think we can avoid having this component connected to both the report_ and transaction_ collections, but if each one uses a selector, there shouldn't be any problem with it, right?
I think the selector for transaction_ would be something like (t) => !tripReportID ? false : t.reportID === tripReportID and it would return false until the tripReportID is found by the report_ selector.
I hope that makes sense. I can work up a more complete demo if you'd like.
There was a problem hiding this comment.
I think this creates a problem where if the user moves a transaction to other report we will always have to check if the report is for a trip, otherwise we delete the tripID from the transaction. That could work.
I am wondering if we should calculate the expenses of a tripRoom on OpenReport, but I guess this creates a similar problem that we would have to send an onyxupdate in the same place we would delete the tripID.
cc: @stitesExpensify for thoutghts
There was a problem hiding this comment.
@tgolen Another iteration is up! This time I moved it to a hook with selectors as suggested 😄
There was a problem hiding this comment.
Couldn't get around to this today, I'll take a look on Monday
There was a problem hiding this comment.
To clarify @rlinoz, your concern is that if they move the only trip expense out of a tripRoom, then we should make it not a tripRoom anymore and just have it be a normal report?
There was a problem hiding this comment.
I mean, if the expense is moved to some other place it should't be considered in the tripRoom preview, but from @tgolen's suggestion we might not have to update the backend at all?
melvin-bot
bot
requested review from
shubham1206agra
and removed request for
a team
|
@shubham1206agra Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Back to you @tgolen! 🙇 |
|
@shubham1206agra Can you complete the checklist and testing, please? |
|
bump @shubham1206agra |
|
I merged the newest main for you @shubham1206agra |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-01-23.at.8.00.21.PM.movAndroid: mWeb ChromeScreen.Recording.2025-01-23.at.7.22.49.PM.moviOS: NativeScreen.Recording.2025-01-23.at.7.49.52.PM.moviOS: mWeb SafariScreen.Recording.2025-01-23.at.7.18.57.PM.movMacOS: Chrome / SafariScreen.Recording.2025-01-23.at.7.12.04.PM.movMacOS: DesktopScreen.Recording.2025-01-23.at.7.28.05.PM.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.0.89-0 🚀
|
Explanation of Change
Fixed Issues
$ #55077
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.webm
Android: mWeb Chrome
iOS: Native
ios.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
desktop.mov