[HOLD for payment 2024-07-10] [HOLD for payment 2024-07-02] [$500] Remove exported ReportUtils.getReport()

[tgolen]

This is coming from #27262. You can read the issue description there to get the context behind the problem being solved and the mess being cleaned up.

Problem

ReportActionUtils.getReport() is called from many view components and other action files which is an anti-pattern.

Why this is important to fix

It maintains a more pure and exact flow of data through the react application. If the view is using report action data, then it needs to subscribe to the data in Onyx so that it's guaranteed that the data will never be stale or out-of-date.

Solution

• Use useOnyx() to load the data in view components
• Use Onyx.connect() to load the data in other action files
• Remove the usage of ReportUtils.getReport() and prevent it from being exported by adding a test to https://github.com/Expensify/App/blob/main/tests/actions/EnforceActionExportRestrictions.ts

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014f8c2e6e8d2c1390
• Upwork Job ID: 1780353316902400000
• Last Price Increase: 2024-06-12
• Automatic offers:
• s77rt | Reviewer | 0
• gijoe0295 | Contributor | 0
• nkdengineer | Contributor | 102706170

Issue Owner

Current Issue Owner: @MitchExpensify

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~014f8c2e6e8d2c1390

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External)

[melvin-bot]

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[gijoe0295]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Remove exported ReportUtils.getReport()

What is the root cause of that problem?

This is a polish issue.

What changes do you think we should make in order to solve the problem?

There are currently 28 places using getReport(), we should either use withOnyx (12 places) or Onyx.connect instead. Note that we should keep it inside ReportUtils file, just no longer export it. Then add a test to EnforceActionExportRestrictions to make sure we won't use it in the future.

let allReports: OnyxCollection<Report>;
Onyx.connect({
    key: ONYXKEYS.COLLECTION.REPORT,
    waitForCollectionCallback: true,
    callback: (value) => (allReports = value),
});

withOnyx(
  report: {
      key: ({chatReportID}) => `${ONYXKEYS.COLLECTION.REPORT}${chatReportID}`,
  },
)

What alternative solutions did you explore? (Optional)

NA

[nkdengineer]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Remove deprecated ReportUtils.getReport() method

What is the root cause of that problem?

This is refactor issue

What changes do you think we should make in order to solve the problem?

1. For component we should get report via withOnyx of if the component already subscribe the report collection, we will get the report from this

report: {
    key: ({reportID}) =>  `${ONYXKEYS.COLLECTION.REPORT}${reportID}`
}

2. For the libs action, we should use Onyx.connect to get allReports and then use this to get the report

let allReports: OnyxCollection<Report>;
Onyx.connect({
    key: ONYXKEYS.COLLECTION.REPORT,
    waitForCollectionCallback: true,
    callback: (value) => (allReports = value),
});

OPTION: We can create a local function in action file to get the report with reportID and use this in other place of this file that we want.

3. Add a test here to verify that we do not export getReport function in ReportUtils

it('does not export getReport', () => {
    // @ts-expect-error the test is asserting that it's undefined, so the TS error is normal
    expect(ReportUtils.getReport).toBeUndefined();
});

App/tests/actions/EnforceActionExportRestrictions.ts

Line 8 in d8361ee

describe('ReportUtils', () => {

What alternative solutions did you explore? (Optional)

NA

[paultsimura]

@tgolen have we already found a way to synchronize all the Onyx reads?

Using Onyx.connect sometimes leads to unexpected bugs:
https://expensify.slack.com/archives/C01GTK53T8Q/p1711017867335759
https://expensify.slack.com/archives/C01GTK53T8Q/p1709843924926719

[tgolen]

If most of the code used withOnyx() to access the report then I believe the problem goes away. Right? This would be the preferred method. I think those threads illustrate the exact problem of why getReport() is not a good pattern.

[paultsimura]

If most of the code used withOnyx() to access the report then I believe the problem goes away

I'm not quite sure how this should help. The first thread was about Onyx.connect inside OptionListUtils not being executed before OptionListUtils.getReportOption is called, therefore allReports were empty.

For the record, OptionListUtils.getReportOption was called from within a component that uses withOnyx to fetch the report.

[tgolen]

For the record, OptionListUtils.getReportOption was called from within a component that uses withOnyx to fetch the report.

Yes, this is what I mean. If the report was already fetched in the component, then the report should be passed directly to OptionListUtils.getReportOption(). It shouldn't be necessary for OptionListUtils.getReportOption() to try to access allReports to get the report and it wouldn't matter if allReports was empty or not.

Keep in mind, I am only speaking to the theory behind it. It's been a long time since I've dug into the OptionsListUtils code, so I don't currently know how possible it is to actually do this.

[paultsimura]

If the report was already fetched in the component, then the report should be passed directly to OptionListUtils.getReportOption()

Passing objects around as parameters just because we cannot reliably fetch them from any place feels like a heavier anti-pattern than the one we are trying to solve by removing ReportUtils.getReport, IMHO.

Also, what do you think about this suggestion by @hurali97?

[tgolen]

what do you think about #35234 (comment) suggestion by @hurali97?

I think it reflects what I would expect for large collections. This is why I think waitForCollectionCallback is preferred and I think a better solution than building a singleton would be to do either or both of:

• Use waitForCollectionCallback everywhere
• Improve getCollectionDataAndSendAsObject to be more performant

I'm not flatly against a singleton, but I think that is a last resort and I have always been concerned that it's the "easy solution" which covers up deeper problems. I'd rather fix the deeper problems, and only when those are completely exhausted, consider using a singleton.

[s77rt]

@tgolen

Use Onyx.connect() to load the data in other action files

Why would that be needed? Or what's the difference between calling ReportUtils.getReport (which already uses Onyx.connect) vs. calling Onyx.connect in each action file. The latter will only increase memory usage

[s77rt]

@gijoe0295 @nkdengineer Thank you for your proposals. Given this is a straightforward feature/refactor request we should go with the first proposal (@gijoe0295's proposal)

🎀 👀 🎀 C+ reviewed
Link to proposal

[melvin-bot]

Current assignee @tgolen is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[tgolen]

Before accepting any proposals... @gijoe0295 did you take a look at the references to see if it's easy (or even possible) to get the report using withOnyx() in the components that need it? This was one of the bigger concerns I had about it. Some references might be easy, but others, it might not be very straightforward how to get the report using withOnyx().

[tgolen]

The latter will only increase memory usage

Regarding memory usage, I don't think there is that big of an impact. They should all be references to the same objects, so memory is negligible. If I'm wrong about that, please profile it and show the memory problem exists and is significant enough to be concerned about.

what's the difference between calling ReportUtils.getReport (which already uses Onyx.connect) vs. calling Onyx.connect in each action file.

You're right that there is no technical difference between the two. The difference for me is a code quality and maintenance issue. As soon as ReportUtils.getReport() is exported, even if you and I agree it should only be used in other action files, there are going to be 10 random engineers that see the exported function and use it in front-end view components. If you know of a different way of solving for this problem, I am open to suggestions. I don't mind if library files share these utility lookup methods, but I feel VERY STRONGLY that view components should not be using them.

[gijoe0295]

@tgolen I investigated and verified that we can use withOnyx for all existing components. Some places might use conditions or computed values to get the report but those conditions/values can be evaluated inside withOnyx's key callback.

We can optionally migrate to useOnyx that was just implemented in #34339 and recommended to use in this thread although it is new and not time-proven. Wdyt?

[tgolen]

Great, thank you for confirming that. Using useOnyx() is fine as well 👍

[nkdengineer]

@s77rt @tgolen Please help to take a look at this PR. @war-in Please add the issue link and ready for review so bot can assign @s77rt as C+ in this PR.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.1-19 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Remove export ReportUtil.getReport function #43632
• Fix displaying room mentions as IDs #44140

If no regressions arise, payment will be issued on 2024-07-02. 🎊

For reference, here are some details about the assignees on this issue:

• @s77rt requires payment automatic offer (Reviewer)
• @nkdengineer requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@s77rt] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@s77rt] Determine if we should create a regression test for this bug.
• [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@MitchExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[MitchExpensify]

Payment summary:

• $500 @s77rt requires payment automatic offer (Reviewer)
• $500 @nkdengineer requires payment automatic offer (Contributor)

[s77rt]

Checklist ^ does not apply here. Not a bug

[MitchExpensify]

Paid and contract ended for @s77rt

Please accept this offer @nkdengineer so I can do the same for you: https://www.upwork.com/nx/wm/offer/102706170, thanks!

[s77rt]

@MitchExpensify Payment due is $250. -50% for regression #43632 (comment). I have refunded $250.

[nkdengineer]

@MitchExpensify I accepted the offer too, thanks

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.3-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Remove export ReportUtil.getReport function #43632
• Fix displaying room mentions as IDs #44140

If no regressions arise, payment will be issued on 2024-07-10. 🎊

For reference, here are some details about the assignees on this issue:

• @s77rt requires payment automatic offer (Reviewer)
• @nkdengineer requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@s77rt] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@s77rt] Determine if we should create a regression test for this bug.
• [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@MitchExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[MitchExpensify]

Paid! Thanks @nkdengineer