Add One Transaction Report View #36934
Conversation
combine report actions for report view
NikkiWines
changed the title
NikkiWines
changed the title
src/libs/ReportUtils.ts
Outdated
| /** | ||
| * Checks if a report has only one transaction associated with it | ||
| */ | ||
| function isOneTransactionReport(reportOrID: OnyxEntry<Report> | string): boolean { | ||
| const report = typeof reportOrID === 'object' ? reportOrID : allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportOrID}`] ?? null; | ||
|
|
||
| // Check the parent report (which would be the IOU or expense report if the passed report is an IOU or expense request) | ||
| // to see how many IOU report actions it contains | ||
| const iouReportActions = ReportActionsUtils.getIOUReportActions(report?.reportID ?? ''); | ||
| return (iouReportActions?.length ?? 0) === 1; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the reportID of the first transaction thread associated with a report | ||
| */ | ||
| function getOneTransactionThreadReportID(reportOrID: OnyxEntry<Report> | string): string | undefined { | ||
| const report = typeof reportOrID === 'object' ? reportOrID : allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportOrID}`] ?? null; | ||
|
|
||
| // Get all IOU report actions for the report. | ||
| const iouReportAction = ReportActionsUtils.getIOUReportActions(report?.reportID ?? '')?.find(reportAction => reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && reportAction.childReportID); | ||
| return iouReportAction ? String(iouReportAction.childReportID) : '0' | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a report is a transaction thread associated with a report that has only one transaction | ||
| */ | ||
| function isOneTransactionThread(reportOrID: OnyxEntry<Report> | string): boolean { | ||
| const report = typeof reportOrID === 'object' ? reportOrID : allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportOrID}`] ?? null; | ||
| const parentReport = getParentReport(report); | ||
| return isOneTransactionReport(parentReport?.reportID ?? ''); | ||
| } | ||
|
|
There was a problem hiding this comment.
These methods are anti-patterns, like the ones being removed in #27262.
The problem with them is in how they are used. If a view component uses these methods, then the view is accessing data that is not connected via withOnyx(). Meaning that if the onyx data changes, the view won't re-render and it will have stale data.
What you'll have to do instead is make sure the component is connected to the REPORT collection, and then using they key function and maybe a selector, you'll have to look up all this stuff from the connected data.
I hope that makes sense.
There was a problem hiding this comment.
I was not awaer of that tracking/ clean up issue, thanks for raising this. It makes sense, annoyingly this is easy to miss, these methods are convenient so that you might not realize the consequence
|
Apologies if you are still making progress on the screenshots but for this screen here: I don't think we need the extra "Total" row above the total that is found in the expense details body. So we can remove the one on top. |
|
I asked about that here a few weeks ago and Tom brought up that we might need to keep it because there may be distinctions between the report currency and transaction currency - definitely open to revisiting that decision though! |
|
Ah okay - is it possible to just not show it if it's the same value? cc @trjExpensify |
|
Personally, I think ideally we'd avoid conditional logic where we only occasionally show certain elements. This was brought up in an earlier discussion for part of this view, as that sort of approach can lead to a less clean implementation. The |
|
Yeah, I agree it looks strange when the currencies are the same. It's the multi-currency case we need it for really. |
… report from auth
…ame currency minor style
|
What's the next step for this PR? |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.59-0 🚀
|
|
cc @NikkiWines - looks like this one generated a few bugs. Can you take a look please? Thanks! |
|
Lots of edge cases coming up! Luckily most of these are pretty easily addressed. The main one that I think will involve more discussion is how to handle the status of an expense in the one transaction view given the original design involved just showing the report header, not the request header. Going to raise that in slack once I've got the majority of these blockers under control Edit: raised here |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.60-13 🚀
|
|
We missed an edge case to hide the IOUpreview for send money one-transaction reports. #39490 |
| // Get a sorted array of reportActions for both the current report and the transaction thread report associated with this report (if there is one) | ||
| // so that we display transaction-level and report-level report actions in order in the one-transaction view | ||
| const combinedReportActions = useMemo(() => { | ||
| if (isEmptyObject(transactionThreadReportActions)) { |
There was a problem hiding this comment.
In case the thread report actions aren't fetched yet, it causes an issue #43034. So instead of actions we can validate only transactionThreadReportID.
| {transactionThreadReport && !isEmptyObject(transactionThreadReport) ? ( | ||
| <> | ||
| {transactionCurrency !== report.currency && ( | ||
| <MoneyReportView | ||
| report={report} | ||
| policy={policy} | ||
| shouldShowHorizontalRule={!shouldHideThreadDividerLine} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
For BZ checklist in #44162:
MoneyReportView is briefly displayed even when there are no transactions (optimistic).
This caused flickering effect. More details in this PR description
Details
Held on the following PRs
Auth PR: https://github.com/Expensify/Auth/pull/9940 ✅
Web PR: https://github.com/Expensify/Web-Expensify/pull/40900 ✅
Fixed Issues
Part 3 of 3
$ https://github.com/Expensify/Expensify/issues/342922
$ #38655
Tests
(Before checking out this branch)
(After checking out this branch)
Screen.Recording.2024-03-08.at.17.02.18.mov
Screen.Recording.2024-03-08.at.17.12.11.mov
Offline tests
Offline view won't shows the one-transaction view and will be the same as it is currently
QA Steps
Same as test steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.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 so 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
Screen.Recording.2024-03-15.at.19.00.08.mov
Android: mWeb Chrome
Screen.Recording.2024-03-15.at.19.22.16.mov
iOS: Native
Simulator.Screen.Recording.-.iPhone.15.-.2024-03-15.at.16.30.38.mp4
iOS: mWeb Safari
workspace.chat.test.mp4
iou.test.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-03-08.at.17.02.18.mov
MacOS: Desktop
Screen.Recording.2024-03-15.at.15.18.35.mov