[Better Expense Reports] Add MoneyRequestReportView#58360
[Better Expense Reports] Add MoneyRequestReportView#58360mountiny merged 9 commits intoExpensify:mainfrom
Conversation
6506e89 to
8f630ba
Compare
ceca442 to
5ed0a75
Compare
33c9d0e to
35d7b65
Compare
| const {isOffline} = useNetwork(); | ||
|
|
||
| const {shouldUseNarrowLayout} = useResponsiveLayout(); | ||
| const contentListHeight = useRef(0); |
There was a problem hiding this comment.
Please note this is a cleanup.
I have noticed that this ref was not used anywhere. It was updated in onContentSizeChange and never used after that nor passed down.
There was a problem hiding this comment.
Kinda feel like ideally we would do that in a separate PR in case there is any regression with the hook
35d7b65 to
dd279c5
Compare
|
I have some problems building android hybrid app. Need to do a clean install, so I will upload android videos tomorrow. |
|
@DylanDylann 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] |
|
CC @mountiny @luacmartins ready to review, please take a look at the description. |
|
Do you want the @Expensify/design to do a thorough review of this or not yet? |
|
@DylanDylann let's prioritize reviewing this PR when you're online |
@shawnborton I think we can save design review for later when we have more of the functionality in place. |
luacmartins
left a comment
There was a problem hiding this comment.
Left a few comments. Additionally, found some minor issues:
-
Incorrect cursor shown on hover
Screen.Recording.2025-03-17.at.1.26.44.PM.mov
Screen.Recording.2025-03-17.at.1.27.08.PM.mov
| shouldShow: (data: OnyxTypes.SearchResults['data'], metadata: OnyxTypes.SearchResults['search']) => boolean; | ||
| }; | ||
|
|
||
| const shouldShowColumnConfig: Record<SearchColumnType, ShouldShowSearchColumnFn> = { |
There was a problem hiding this comment.
Why did we move this out of SearchColumnConfig?
There was a problem hiding this comment.
Short answer: code decoupling + TS types
In search table some of shouldShow functions needed extra search data to make the decision whether to display a column. These function had signatures like this:
(data: OnyxTypes.SearchResults['data'], metadata: OnyxTypes.SearchResults['search']) => booleanI needed to create a clean reusable SortableTableHeader component. Inside this component I need a shouldShow function, but I don't want to make its arguments depend on search data/metadata. It would be hard to use it for ReportsView then.
Because of that, inside SearchTableHeader I had to split types that are connected to search data (shouldShow) from columnsConfig. That means columnsConfig is now only names of columns + translation keys - generic types.
I'm quite happy with this solution.
(If you remember @luacmartins we had similar problems with chat name generation, where it was tightly coupled to reports data in Onyx and hard to reuse in Search - I want to avoid that here)
src/libs/ReportActionsUtils.ts
Outdated
| const currentAction = reportActions.at(actionIndex); | ||
| const nextAction = findNextAction(reportActions, actionIndex); | ||
|
|
||
| // Todo first should have avatar - verify that this works with long chats |
There was a problem hiding this comment.
Same here, are we doing this in a follow up?
| // Todo first should have avatar - verify that this works with long chats | |
| // Todo first should have avatar - verify that this works with long chats |
There was a problem hiding this comment.
+1 lets link issues in which we would handle this in each TODO
There was a problem hiding this comment.
let's combine these two, because both touch the same part of code - displaying avatars on grouped comments.
Since the existing solution is working specifically only on InvertedList I had to re-implement it for non-inverted lists.
Both TODO comments are leftovers after my change so they should be fixed together.
mountiny
left a comment
There was a problem hiding this comment.
Thanks! going to merge this so @JakubKorytko and other PRs can base their changes from this more easily today but @luacmartins please feel free to review this PR still
|
@JakubKorytko Could work those notes into his PR |
|
Noted, will take a look later, focusing on sorting functionality rn. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
I will also work on those. I'm adding comments to the original issue so that we don't forget about anything 👍 |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.16-0 🚀
|
|
I believe that this PR is the cause of this deploy blocker. I have reverted this in |
|
It's behind a beta 👍 |
|
Noted. We demoted it from being a blocker. |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.1.16-4 🚀
|
| function findNextAction(reportActions: ReportAction[], actionIndex: number): OnyxEntry<ReportAction> { | ||
| for (let i = actionIndex - 1; i > 0; i--) { |
There was a problem hiding this comment.
This loop stops at 1 (i > 0) and does not check the first report action. The condition should have been i >= 0.
Coming from #59948
| shouldUseThreadDividerLine={shouldUseThreadDividerLine} | ||
| shouldDisplayReplyDivider={visibleReportActions.length > 1} | ||
| isFirstVisibleReportAction={firstVisibleReportActionID === reportAction.reportActionID} | ||
| /> |
There was a problem hiding this comment.
Missed passing linkedReportActionID here for message highlighting when clicking header to go back to a linkedReportAction, leading to this issue:
| lastReportAction={lastReportAction} | ||
| /> | ||
| ) : null} | ||
| </View> |
There was a problem hiding this comment.
Coming from #60747, missing <PortalHost name="suggestions" /> resulted in a broken suggestion list.
|
Missed a part of the unread marker implementation logic from ReportActionsList. This was fixed here. #62597 |
| return ( | ||
| <View style={styles.flex1}> | ||
| {report ? ( | ||
| <FlatList |
There was a problem hiding this comment.
Coming from #66988, we should disable removeClippedSubviews like what we did in reportActionList
| return ( | ||
| <> | ||
| {!displayNarrowVersion && ( | ||
| <MoneyRequestReportTableHeader |
There was a problem hiding this comment.
From #76352 (comment): we missed wrapping the component with OfflineWithFeedback. That wrapper is necessary when creating reports while offline.






This PR adds new view called

MoneyRequestReportViewwhich will display a grouped list of report transactions at top and chat items below.It implements parts of: https://docs.google.com/document/d/12RD9MO-CXFwcehkqLdWvx0__mK8Ou_0cMBYG5BVNULc/edit?tab=t.0#bookmark=id.vbs1mmt4xhw4
Explanation of Change
MoneyRequestReportView, which displays list of transactions and chat itemsreportIDReportScreenbut we filter outCREATEDandIOUactions because CREATED is not relevant to this view and IOU is the transaction list ☝️ReportScreen,ReportActionsViewandReportActionsListSearchLHN)tableReportViewbeta to redirect Search report items to the new view - without this beta the user cannot access new screenSearchTableHeaderinto component calledSortableTableHeaderto use the same header both inSearchtable andMoneyRequestReporttableNOTE: this PR is already quite big so I wanted to divide it into smaller parts to allow for reasonable review.
These don't yet work and will be done in separated PR
Fixed Issues
$ #57508
PROPOSAL:
Tests
Important: in order to test change function
canUseTableReportViewto return true, or have BETAtableReportViewsetExpense ReportsOffline tests
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: mWeb Chrome
iOS: Native
rec-report-ios.mp4
iOS: mWeb Safari
rec-report-ios-mweb.mp4
MacOS: Chrome / Safari
rec-report-web.mp4
MacOS: Desktop