Remove ReportActionsUtils.getParentReportAction(), TransactionUtils.getLinkedTransaction(), and TransactionUtils.getTransaction()

[tgolen]

Problem

These methods are anti-patterns because they are most always used for loading data into a component without using withOnyx(). This breaks the data flow of a react application. (data is coming from somewhere that is not props or state and cannot be debugged in react dev tools).

Solution

Switch all references to properly use withOnyx() for components and connect() for libs.

Components

• [x] MoneyRequestHeader @tgolen https://github.com/Expensify/App/pull/28645
• [ ] OptionRowLHNData
• [x] EditRequestPage @tgolen
• [x] ~SplitBillDetailsPage~
• [x] extractAttachmentsFromReport @tgolen https://github.com/Expensify/App/pull/30866
• [x] #32309
• [x] #32443
• [x] FlagCommentPage @tgolen https://github.com/Expensify/App/pull/33676
• [x] ReportScreen @tgolen https://github.com/Expensify/App/pull/33970
• [x] ReportActionItem @tgolen https://github.com/Expensify/App/pull/34057
• [x] ~ReportActionList~ No longer references deprecated methods
• [ ] withReportAndReportActionOrNotFound @tgolen https://github.com/Expensify/App/pull/34113
• [x] ~ComposerWithSuggestions @tgolen~ already removed

Libs

• [ ] IOU
• [ ] Task
• [ ] ReportUtils
• [ ] TransactiionUtils

[techievivek]

Adding @aimane-chnaif to the GH as he helped review the PR: https://expensify.slack.com/archives/C02NK2DQWUX/p1694749699352029

[tgolen]

I'm slowly working my way through these, but I wasn't able to get to any of them this week.

[tgolen]

I wrote a new PR for the next one on the list today.

[tgolen]

I haven't had time to do any additional work on this.

[bernhardoj]

Coming from https://expensify.slack.com/archives/C01GTK53T8Q/p1688630003862989, here are some util functions that currently depend on Onyx.connect which I think can be replaced with withOnyx.

CardUtils	isExpensifyCard, isCorporateCard, getCardDescription
CurrencyUtils	getCurrenyDecimals. getCurrencySymbol, isValidCurrencyCode
GroupChatUtils	getGroupChatName
LocalePhoneNumber	formatPhoneNumber
LoginUtils	appendCountryCode
PersonalDetailsUtils	getAccountIDsByLogins, getLoginsByAccountIDs
ReportActionsUtils	getLastVisibleAction, getReportAction, getReportPreviewAction, getAllReportActions, findPreviousAction
TransactionUtils	getAllReportTransactions
PersonalDetails	getDisplayName, getDisplayNameForTypingIndicator
ReimbursementAccount/store.js	hasCreditBankAccount

[tgolen]

Thanks @bernhardoj. What do you think of this approach?

1. Create a PR that marks all those methods as deprecated (same as I did for getParentReportAction() and the others)
2. Start tackling these one-by-one to replace each one

[bernhardoj]

@tgolen oh, my bad, I didn't mean to completely drop the whole function.

What I mean is more like replacing the source of the onyx data from Onyx.connect to withOnyx, for example, getGroupChatName.

getGroupChatName depends on ONYXKEYS.PERSONAL_DETAILS_LIST. Currently, we get the onyx data with Onyx.connect which could break the reactive behavior.

https://github.com/Expensify/App/blob/d7366c6cb1c1989858006f3c9a0cb80d8f5ee6f7/src/libs/GroupChatUtils.ts#L7-L19

To fix it, we should pass the personal details onyx from the component that uses it.

function getGroupChatName(report, personalDetails) {}

getGroupChatName(report, personalDetails)
withOnyx({
    personalDetails: {key: ONYXKEYS.PERSONAL_DETAILS_LIST}
})

For the plan, I think we can immediately start tackling each function. But there are some function that has a lot of usage which I think can be split into a few parts.

[tgolen]

OK, yeah, I know what you mean now. @marcaaron and I have gone back and forth on this topic, and generally we've settled in a place where if there is no problem, then we have left it alone. However, I think it's a safer environment in general if data is usually fetched from withOnyx and then passed to action files. There are some performance optimizations that can be gained by having less things in withOnyx though, so there will always be some kind of balance we have to determine. For now, let's try to wrap up the current methods that I've marked as deprecated, and then we can tackle these one-by-one and figure out what to do with them (if anything).

[bernhardoj]

Sounds good 👍

[melvin-bot[bot]]

This issue has not been updated in over 15 days. @tgolen eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[tgolen]

I've got PRs for the few remaining components 🎉

[melvin-bot[bot]]

This issue has not been updated in over 15 days. @tgolen eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0156f7187832f56740

[melvin-bot[bot]]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @jjcoffee (Internal)

[tgolen]

@jjcoffee could you please do a C+ review on https://github.com/Expensify/App/pull/37340?

[jjcoffee]

@tgolen Sure, will get to that today!

[tgolen]

Weekly Update

• I had one PR merged this week
• I submitted a new PR yesterday for another item on the list

Next Steps

• Work on FlagCommentPage next

ETA

• All done by April 1 (accounts for some OOO I have coming up)

[tgolen]

Weekly Update

• I've fixed a couple more this week
• I've found even more bad methods ReportUtils.getPolicy() which I've added to the list
• I'm going to try a slightly different strategy of getting contributors to fix these

Next Steps

• Start creating contributor issues to clean up the rest of this

ETA

• No change

[tgolen]

Weekly Update

• I have been created contributor issues to handle the remaining items and they are moving forward

Next Steps

• Do a final cleanup of ReportUtils once all the current issues are done

ETA

• All done by April 9

[tgolen]

Weekly Update

• There were three PRs created last week for most of the remaining items

Next Steps

• Once all PRs are merged, @tgolen take one last look at ReportUtils and make sure it is not exporting anything "bad", then this issue can be closed

ETA

• This is bleeding into next week due to slow PR reviews, so updated ETA is Wednesday, April 17.

[tgolen]

Weekly Update

• There is one final PR that is waiting to close out, then this can be closed.
• I haven't found any more bad instances

Next Steps

• Wait for this PR to be merged and then close this out

ETA

• It looks like it might be merged next week sometime.

[tgolen]

Weekly Update

• Same as last week

[tgolen]

Weekly Update

• I bumped the PR that is holding this up. The contributor hasn't updated it in a couple of weeks now.

[melvin-bot[bot]]

This issue has not been updated in over 15 days. @tgolen eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[tgolen]

ReportUtils is the only remaining item, but I took a look at it and I think it's mostly fine for now so I am going to close this out.