Skip to content
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

Remove ReportActionsUtils.getParentReportAction(), ReportUtils.getParentReport(), TransactionUtils.getLinkedTransaction(), ReportUtils.getPolicy(), and TransactionUtils.getTransaction() #27262

Closed
24 of 25 tasks
tgolen opened this issue Sep 12, 2023 · 25 comments
Assignees
Labels
Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Task

Comments

@tgolen
Copy link
Contributor

tgolen commented Sep 12, 2023

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

Libs

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0156f7187832f56740
  • Upwork Job ID: 1762552955483959296
  • Last Price Increase: 2024-02-27
@tgolen tgolen self-assigned this Sep 12, 2023
@tgolen tgolen changed the title Remove ReportActionsUtils.getParentReportAction() and TransactionUtils.getLinkedTransaction() Remove ReportActionsUtils.getParentReportAction(), TransactionUtils.getLinkedTransaction(), and TransactionUtils.getTransaction() Sep 12, 2023
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Sep 12, 2023
@techievivek
Copy link
Contributor

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

@tgolen
Copy link
Contributor Author

tgolen commented Oct 20, 2023

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

@tgolen
Copy link
Contributor Author

tgolen commented Nov 3, 2023

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

@tgolen
Copy link
Contributor Author

tgolen commented Nov 17, 2023

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

@tgolen tgolen changed the title Remove ReportActionsUtils.getParentReportAction(), ReportUtils.getParentReport(), TransactionUtils.getLinkedTransaction(), and TransactionUtils.getTransaction() Remove ReportActionsUtils.getParentReportAction(), ReportUtils.getParentReport(), TransactionUtils.getLinkedTransaction(), ReportUtils.getPolicy(), and TransactionUtils.getTransaction() Mar 14, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Mar 14, 2024
@tgolen
Copy link
Contributor Author

tgolen commented Mar 15, 2024

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
Copy link
Contributor Author

tgolen commented Mar 29, 2024

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
Copy link
Contributor Author

tgolen commented Apr 12, 2024

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
Copy link
Contributor Author

tgolen commented May 3, 2024

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
Copy link
Contributor Author

tgolen commented May 10, 2024

Weekly Update

  • Same as last week

@tgolen
Copy link
Contributor Author

tgolen commented May 17, 2024

Weekly Update

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

Copy link

melvin-bot bot commented Jun 10, 2024

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
Copy link
Contributor Author

tgolen commented Jul 26, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Task
Projects
None yet
Development

No branches or pull requests

5 participants