Adjust #63648 PR code to new pure Onyx functions rule by JakubKorytko · Pull Request #63731 · Expensify/App

JakubKorytko · 2025-06-09T06:32:04Z

Explanation of Change

According to slack convo and #63648 @mountiny comments in order to unblock deploy blocker handling non-pure functions added in the PR should be corrected in this PR.

However, to make it work correctly we need to also make functions that call getOneTransactionThreadReportID pure as much as we can without changing the whole codebase in one PR.

Following functions were changed to accept reports from Onyx as a parameters instead of ID's:

getReportPrimaryAction
getSecondaryReportActions
getOneTransactionThreadReportID
isPayAtEndExpenseReport
getReasonAndReportActionThatHasRedBrickRoad
shouldReportBeInOptionList
getSendMoneyFlowOneTransactionThreadID
isOneTransactionReport
hasReportErrorsOtherThanFailedReceipt
isOneTransactionReport
generateReportAttributes
getReasonAndReportActionForRBRInLHNRow
getReasonForShowingRowInLHN

isOneTransactionThread is semi-pure as it is still getting chatReport from allReports.

getWorkspacesUnreadStatuses was removed as it was used nowhere.

Following components now use useOnyx for chatReport and pass it to changed functions to make it more reactive:

MoneyRequestReportActionsList
MoneyRequestReportView
DebugReportPage
ReportDetailsPage
ReportScreen
BaseReportActionContextMenu

Context why these changes were introduced: https://expensify.slack.com/archives/C05LX9D6E07/p1748457444774569

Fixed Issues

$ #63827
PROPOSAL: N/A

Tests

[Issue fixed in the original PR should still be fixed, steps copy-pasted]

Open Expensify app.
Open 1:1 DM.
Click + > Pay user.
Enter amount > Next.
Click Pay elsewhere.
Click on the expense preview.
Verify that Send Money expense preview is displayed.

Because this is non-UI change and nothing was added but it is a huge refractor that cannot really be split into smaller parts as getOneTransactionThreadReportID is called about 40 times in the code, it is extremely hard to create test steps that are specific enough. However, what should be tested is all places where one-transaction report view can be displayed (e.g. Report view, RHP) and places where it shouldn't (e.g. LHN).

Offline tests

Same as tests.

QA Steps

Same as tests.

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Web.mov

MacOS: Desktop

JakubKorytko · 2025-06-09T13:08:04Z

Note: ESLint check is failing but can be safely ignored as this PR is taking care of the beta flag.

melvin-bot · 2025-06-09T13:09:29Z

@aldo-expensify 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]

aldo-expensify

This looks good to me, my only concern is this comment: #63731 (comment)

Do we have to do something about these lint errors?

My guess is "no".

aldo-expensify · 2025-06-09T17:09:00Z

src/libs/OptionsListUtils.ts

+            const report = getReportOrDraftReport(reportID);
+            const chatReport = getReportOrDraftReport(report?.chatReportID);


@JakubKorytko I'm not sure if this matters, but I see you started using getReportOrDraftReport which falls back to the ONYXKEYS.COLLECTION.REPORT_DRAFT collection if it doesn't find the report in ONYXKEYS.COLLECTION.REPORT. In the past, the code was just using ONYXKEYS.COLLECTION.REPORT, is this change safe?

Good point. I'm pretty sure it is, but just to be safe, I'll replace these calls with getReport or something similar tomorrow.

Eventually we will need to replace those too but definitely not in scope of this pr

Eventually we will need to replace those too but definitely not in scope of this pr

agree

mountiny · 2025-06-09T17:31:41Z

Yeah no need to do anything about those

github-actions · 2025-06-10T09:09:23Z

🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here.

allgandalf · 2025-06-10T09:14:56Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
I verified tests pass on all platforms & I tested again on:
- Android: HybridApp
- Android: mWeb Chrome
- iOS: HybridApp
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified there are no new alerts related to the canBeMissing param for useOnyx
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp

Screen.Recording.2025-06-11.at.4.34.50.PM.mov

Android: mWeb Chrome

Screen.Recording.2025-06-11.at.4.45.00.PM.mov

iOS: HybridApp

Screen.Recording.2025-06-11.at.3.12.53.PM.mov

iOS: mWeb Safari

Screen.Recording.2025-06-11.at.3.18.31.PM.mov

MacOS: Chrome / Safari

Screen.Recording.2025-06-11.at.12.26.09.PM.mov

Screen.Recording.2025-06-11.at.4.46.37.PM.mov

MacOS: Desktop

Screen.Recording.2025-06-11.at.12.34.10.PM.mov

allgandalf · 2025-06-10T09:23:26Z

isOneTransactionThread was changed to accept both reports and IDs as there wasn't a clear way to change it's type. So it is semi-pure.

@JakubKorytko can you explain more with this statement, what's the problem exactly ?

allgandalf · 2025-06-10T09:29:12Z

@mountiny lets cancel the adhoc workflow, there are some updates still being pushed by @JakubKorytko

JakubKorytko · 2025-06-10T10:33:19Z

@aldo-expensify I've changed added getReportOrDraftReport calls to directly accessing allReports because as @mountiny said, it will be removed sooner or later and I think this is the best way of getting rid of Onyx.connect in components - moving it up and closer to components in different PR's because doing it all at once would result in PR having thousands of lines changed.

Do we have to do something about these lint errors?

Can be safely ignored as #63533 is taking care of the beta flag.

can you explain more with this statement, what's the problem exactly ?

No longer a thing, it was done that way to avoid using allReports but it is safer than getReportOrDraftReport which will be deleted later anyway.

github-actions · 2025-06-10T11:38:01Z

🚧 @trjExpensify has triggered a test Expensify/App build. You can view the workflow run here.

mountiny · 2025-06-10T11:38:57Z

@JakubKorytko could you please get someone from SWM assigned to take over this and handle any blockers/ comments instead of you? thanks!

JakubKorytko · 2025-06-10T12:02:18Z

@JakubKorytko could you please get someone from SWM assigned to take over this and handle any blockers/ comments instead of you? thanks!

About the comments, I will address them and get this merged, but asked SWM about someone to take care of potential blockers.

github-actions · 2025-06-10T12:16:21Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
Built from App PR #63731.

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/63731/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/63731/index.html

Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/63731/NewExpensify.dmg	https://63731.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

blazejkustra · 2025-06-10T12:25:28Z

but asked SWM about someone to take care of potential blockers.

Hey, that's me! I'll watch out for potential blockers (🤞 there won't be any)

allgandalf

codewise this LGTM!, I still need to perform some tests to make sure everything is correct, will update the checklist tomorrow

mountiny · 2025-06-10T22:55:42Z

@blazejkustra @JakubKorytko could you please merge main once you get a chance?

JakubKorytko · 2025-06-11T02:55:40Z

Can do a little later.

JakubKorytko · 2025-06-11T05:36:25Z

ESLint check failing - unrelated to this PR, not something I've added and I see that it will be fixed somewhere else.

'getPolicy' is deprecated. Get the data straight from Onyx - This will be fixed as part of https://github.com/Expensify/Expensify/issues/507850

allgandalf

Code LGTM and tests well on all platforms. Hopefully no bugs should pass 🙏 ,

Thanks for taking care of this despite being ill @JakubKorytko 🙇 , get better soon!

mountiny · 2025-06-11T11:40:06Z

All yours @aldo-expensify

allgandalf · 2025-06-11T20:17:22Z

@JakubKorytko can you fix conflicts please

JakubKorytko · 2025-06-11T21:15:46Z

@allgandalf Yeah, done, had to change getSecondaryReportActions signature though.

Pros: for the first time in this PR history - no checks failing 💅
Cons: We are close to hit 800 lines changed in total 🚀

aldo-expensify

LGTM

aldo-expensify · 2025-06-11T22:38:31Z

Leaving unmerged in case @mountiny wants to take a final look

mountiny

Ah sorry, I did not expect to wait for me, changes look good to me, thanks @JakubKorytko @blazejkustra

@allgandalf @blazejkustra lets be ready for some regressions that slipped through testing, @allgandalf could you please leave a comment listing out the tests you have done during your testing, at least some of them. Thanks!

OSBotify · 2025-06-12T22:44:19Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2025-06-16T11:10:31Z

🚀 Deployed to staging by https://github.com/aldo-expensify in version: 9.1.67-0 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	cancelled 🔪
🍎 iOS 🍎	cancelled 🔪

github-actions · 2025-06-17T17:16:52Z

🚀 Deployed to production by https://github.com/lakchote in version: 9.1.67-2 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

JakubKorytko mentioned this pull request Jun 9, 2025

[CP Staging][Better Expense Report View] Fix Send Money flow issue caused by excluding pay from one transaction thread logic #63648

Merged

48 tasks

JakubKorytko force-pushed the korytko/adjust-63648-code-to-new-onyx-rule branch 2 times, most recently from 5c08c43 to 230e138 Compare June 9, 2025 08:18

Replace Onyx.connect calls to useOnyx calls

240df21

JakubKorytko force-pushed the korytko/adjust-63648-code-to-new-onyx-rule branch from 230e138 to 240df21 Compare June 9, 2025 11:21

JakubKorytko added 2 commits June 9, 2025 13:23

Merge branch 'main' into korytko/adjust-63648-code-to-new-onyx-rule

d22e447

Fix failing checks

9509710

JakubKorytko force-pushed the korytko/adjust-63648-code-to-new-onyx-rule branch from 358f664 to 9509710 Compare June 9, 2025 12:58

JakubKorytko changed the title ~~[WIP] Adjust #63648 PR code to new pure Onyx functions rule~~ Adjust #63648 PR code to new pure Onyx functions rule Jun 9, 2025

JakubKorytko marked this pull request as ready for review June 9, 2025 13:09

JakubKorytko requested a review from a team as a code owner June 9, 2025 13:09

melvin-bot bot requested a review from aldo-expensify June 9, 2025 13:09

melvin-bot bot removed the request for review from a team June 9, 2025 13:09

aldo-expensify reviewed Jun 9, 2025

View reviewed changes

mountiny assigned JakubKorytko Jun 10, 2025

mountiny requested a review from allgandalf June 10, 2025 09:08

This comment has been minimized.

Sign in to view

JakubKorytko added 2 commits June 10, 2025 11:53

Merge branch 'main' into korytko/adjust-63648-code-to-new-onyx-rule

53f32c6

Change getReportOrDraftReport to direct allReports calls

d6112d4

Fix isOneTransactionThread

28f8c81

JakubKorytko force-pushed the korytko/adjust-63648-code-to-new-onyx-rule branch from 4ad3e9a to 28f8c81 Compare June 10, 2025 11:04

allgandalf reviewed Jun 10, 2025

View reviewed changes

JakubKorytko added 2 commits June 11, 2025 07:08

Merge branch 'main' into korytko/adjust-63648-code-to-new-onyx-rule

0d76b8b

Fix spell check

1a88d7c

allgandalf approved these changes Jun 11, 2025

View reviewed changes

melvin-bot bot requested a review from aldo-expensify June 11, 2025 11:19

JakubKorytko added 2 commits June 11, 2025 22:43

Merge branch 'main' into korytko/adjust-63648-code-to-new-onyx-rule

624802b

Make getSecondaryReportActions accept object instead of separate params

e3d651a

aldo-expensify previously approved these changes Jun 11, 2025

View reviewed changes

Merge branch 'main' into korytko/adjust-63648-code-to-new-onyx-rule

2edaf2c

JakubKorytko dismissed aldo-expensify’s stale review via 2edaf2c June 12, 2025 19:08

mountiny approved these changes Jun 12, 2025

View reviewed changes

aldo-expensify approved these changes Jun 12, 2025

View reviewed changes

aldo-expensify merged commit cab551a into Expensify:main Jun 12, 2025
19 checks passed

QichenZhu mentioned this pull request Jun 16, 2025

LHN-LHN doesn't show the message sent or the system message #64161

Merged

51 tasks

github-actions bot mentioned this pull request Jun 16, 2025

Deploy Checklist: New Expensify 2025-06-16 #64227

Closed

48 tasks

melvin-bot bot mentioned this pull request Jun 17, 2025

[Due for payment 2025-06-24] [$500] Update getOneTransactionThreadReportID to pure function #63827

Closed

		const report = getReportOrDraftReport(reportID);
		const chatReport = getReportOrDraftReport(report?.chatReportID);

Conversation

JakubKorytko commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

JakubKorytko commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melvin-bot bot commented Jun 9, 2025

Uh oh!

aldo-expensify left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aldo-expensify Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

JakubKorytko Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

mountiny Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

aldo-expensify Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

mountiny commented Jun 9, 2025

Uh oh!

github-actions bot commented Jun 10, 2025

Uh oh!

allgandalf commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

allgandalf commented Jun 10, 2025

Uh oh!

allgandalf commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

JakubKorytko commented Jun 10, 2025

Uh oh!

github-actions bot commented Jun 10, 2025

Uh oh!

mountiny commented Jun 10, 2025

Uh oh!

JakubKorytko commented Jun 10, 2025

Uh oh!

github-actions bot commented Jun 10, 2025

Uh oh!

blazejkustra commented Jun 10, 2025

Uh oh!

allgandalf left a comment

Choose a reason for hiding this comment

Uh oh!

mountiny commented Jun 10, 2025

Uh oh!

JakubKorytko commented Jun 11, 2025

Uh oh!

JakubKorytko commented Jun 11, 2025

Uh oh!

allgandalf left a comment

Choose a reason for hiding this comment

Uh oh!

mountiny commented Jun 11, 2025

Uh oh!

allgandalf commented Jun 11, 2025

Uh oh!

JakubKorytko commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aldo-expensify left a comment

Choose a reason for hiding this comment

JakubKorytko commented Jun 9, 2025 •

edited

Loading

JakubKorytko commented Jun 9, 2025 •

edited

Loading

aldo-expensify left a comment •

edited

Loading

allgandalf commented Jun 10, 2025 •

edited

Loading

allgandalf commented Jun 10, 2025 •

edited

Loading

JakubKorytko commented Jun 11, 2025 •

edited

Loading