-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Make SearchMoneyRequestReportPage show single transaction view correctly #59735
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
Make SearchMoneyRequestReportPage show single transaction view correctly #59735
Conversation
The change to show single transaction view instead of a list with one expense is pretty smooth, but we got one problem left: Recently we made some changes on BE, so that when we use This view is implemented with the assumption that To solve this problem, we need BE to:
|
Yeah that makes sense, will look into updating the BE but i dont think e have to hold as its super rare for the report to have more than 50 report actions if there is a single expense on it. It works fine in your testing right? |
Yeah I was about to say it doesn't need any hold, because of that. It tests well on my side. |
} | ||
return acc; | ||
}, 0) === 1, | ||
[reportActions], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const shouldUseSingleTransactionView = useMemo(() => { const iouActionsCount = reportActions.filter( action => action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU ).length; return iouActionsCount === 1; }, [reportActions]);
Don’t you think that filter
would work the same way and be easier to understand? I generally like reduce
, but I feel like it might be harder for some people to read.
@@ -118,6 +132,26 @@ function MoneyRequestReportView({report, policy, reportMetadata, shouldDisplayRe | |||
return; | |||
} | |||
|
|||
if (isLoadingApp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's great that you used an early return here instead of a ternary — it really improves readability.
I agree. @mountiny are you taking this one? |
@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] |
🚧 @mountiny has triggered a test app build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-04-09.at.10.30.38.PM.movAndroid: mWeb ChromeScreen.Recording.2025-04-09.at.10.26.39.PM.moviOS: NativeScreenRecording_04-09-2025.22-35-39_1.MP4iOS: mWeb SafariScreen.Recording.2025-04-09.at.10.34.12.PM.movMacOS: Chrome / SafariScreen.Recording.2025-04-09.at.10.22.53.PM.movMacOS: DesktopScreen.Recording.2025-04-09.at.10.34.45.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works in offline mode as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, changes look good to me
Yes |
@martasudol is going to review this one too just a sec |
Looks good to me also 🚀 |
Let's get this merged then! It's blocking some of our other work. |
✋ 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/mountiny in version: 9.1.27-0 🚀
|
report={report} | ||
policy={policy} | ||
reportActions={reportActions} | ||
transactionThreadReportID={undefined} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SzymczakJ @allgandalf Is there any reason why we pass undefined here? Or this is a mistake
Coming from: #60079
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a mistake. In fact, this is probably something that we forgot to add, because this transactionThreadReportID has been undefined, since the creation of this file.
Adding transactionThreadReportID might solve the issue.
🚀 Deployed to production by https://github.com/marcaaron in version: 9.1.28-15 🚀
|
Explanation of Change
This PR changes the way we show single expense reports on
SearchMoneyRequestReportPage
. When report has only one expense on it we show it like we did on chat page, instead of showing transaction list with only one item.To accomplish that we use the
ReportActionsView
component which originally lives as a part ofReportScreen
and we render it conditionally in case when there's only oneIOU
type reportAction in the report.Fixed Issues
$ #59459 (One-expense report view)
$ #59295 (No option to add expense if report is created offline)
PROPOSAL:
Tests
Note: to test all of these fixes you have to have tableReportView permission
Additionally test if "No option to add expense if report is created offline" is not longer reproducible.
Offline tests
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
https://github.com/user-attachments/assets/7ea6e8f5-2e29-448e-bdce-7d3d4d6f4fe3Android: mWeb Chrome
androidWeb.mov
iOS: Native
ios.mov
iOS: mWeb Safari
iosweb.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov