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

[HOLD for payment 2024-06-28] [HOLD for payment 2024-06-24] [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [POLISH][Simplified Collect] Create utils files for each page #38477

Closed
luacmartins opened this issue Mar 18, 2024 · 28 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Weekly KSv2

Comments

@luacmartins
Copy link
Contributor

Policy.ts is a large file and it'll only get larger and larger. We can keep the code more organized by separating each individual Members, Categories, Tags and Distance Rates into its own utils file, like we did for taxes here, i.e. /actions/Policy/TaxRates.ts, etc

@luacmartins luacmartins added the Daily KSv2 label Mar 18, 2024
@trjExpensify trjExpensify added the Hot Pick Ready for an engineer to pick up and run with label Mar 20, 2024
@melvin-bot melvin-bot bot added the Overdue label Mar 20, 2024
@amyevans amyevans self-assigned this Mar 21, 2024
@melvin-bot melvin-bot bot removed the Overdue label Mar 21, 2024
@amyevans amyevans added Overdue and removed Hot Pick Ready for an engineer to pick up and run with Overdue labels Mar 21, 2024
@melvin-bot melvin-bot bot added the Overdue label Mar 25, 2024
@amyevans
Copy link
Contributor

Started on this today, I'll split it up into a few PRs to make review a little easier.

Looking at Category to start, 1 clarifying question: Policy.ts contains category-related functions that are used by the Simplified Collect Category page (like openPolicyCategoriesPage), as well as others used when requesting money (like buildOptimisticPolicyRecentlyUsedCategories). I was going to move them all, but wanted to check that matches what you were thinking @luacmartins?

@melvin-bot melvin-bot bot removed the Overdue label Mar 26, 2024
@luacmartins
Copy link
Contributor Author

Yea, that makes sense.

@melvin-bot melvin-bot bot added the Overdue label Apr 1, 2024
@amyevans
Copy link
Contributor

amyevans commented Apr 3, 2024

No update - haven't had much time to swing back around to this between some OOO and other commitments the past few days but will prioritize shortly

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Apr 3, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

@amyevans Eep! 4 days overdue now. Issues have feelings too...

Copy link

melvin-bot bot commented Apr 10, 2024

@amyevans Still overdue 6 days?! Let's take care of this!

Copy link

melvin-bot bot commented Apr 12, 2024

@amyevans 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

Copy link

melvin-bot bot commented Apr 16, 2024

@amyevans 12 days overdue now... This issue's end is nigh!

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Apr 19, 2024
Copy link

melvin-bot bot commented Apr 19, 2024

This issue has not been updated in over 14 days. @amyevans eroding to Weekly issue.

@melvin-bot melvin-bot bot removed the Overdue label Apr 19, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 29, 2024
@amyevans
Copy link
Contributor

amyevans commented May 7, 2024

Have been prioritizing a fast-apis issue and detailed design for the Billing card project. If anyone's interested in knocking this out feel free to take it from me, otherwise I'll work on it once I hit a lull in other work

@melvin-bot melvin-bot bot removed the Overdue label May 7, 2024
@melvin-bot melvin-bot bot changed the title [POLISH][Simplified Collect] Create utils files for each page [HOLD for payment 2024-06-18] [POLISH][Simplified Collect] Create utils files for each page Jun 11, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jun 11, 2024
Copy link

melvin-bot bot commented Jun 11, 2024

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

Copy link

melvin-bot bot commented Jun 11, 2024

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

If no regressions arise, payment will be issued on 2024-06-18. 🎊

@dangrous
Copy link
Contributor

Did the migration solve the issue?

Unfortunately not. It's three functions that are causing the issue:

  • removePendingFieldsFromCustomUnit defined in Policy.ts, used in Category.ts and DistanceRate.ts
  • navigateWhenEnableFeature defined in Policy.ts, used in Policy.ts, Tag.ts, Category.ts, and DistanceRate.ts
  • buildOptimisticPolicyCategories currently defined in BOTH Policy.ts and Category.ts, used in Policy.ts and Category.ts

Ideally we'd keep the first two where they are (in Policy.ts) and move the last one to ONLY Category.ts but that's posing the problem.

The last one makes the most sense to move or duplicate, since the top two are shared in multiple files (though the top two are also much shorter).

So maybe we leave it? Unless there's an additional file we could create with those and perhaps other utility methods or something like that.

@luacmartins
Copy link
Contributor Author

Could we move the first 2 to PolicyUtils? For the buildOptimisticPolicyCategories, how does it create a circular dependency? I don't see Policy.ts importing Category.ts and vice-versa, so could we define it in Category.ts and then import Category in Policy.ts?

@dangrous
Copy link
Contributor

Can't import category into policy (which it's not doing now) because category is already importing policy.

But moving those two into PolicyUtils should work, I can spin that up.

@luacmartins
Copy link
Contributor Author

Can't import category into policy (which it's not doing now) because category is already importing policy.

Ah I missed the Policy import on Category the first time since it's importing specific functions. But the only imports from Policy are the 2 functions we're moving to PolicyUtils, no? So that means we can remove that Policy import in Category

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Jun 12, 2024
@dangrous
Copy link
Contributor

yep! that PR above should solve our problems

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jun 13, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-06-18] [POLISH][Simplified Collect] Create utils files for each page [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [POLISH][Simplified Collect] Create utils files for each page Jun 13, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jun 13, 2024
Copy link

melvin-bot bot commented Jun 13, 2024

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

Copy link

melvin-bot bot commented Jun 13, 2024

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

If no regressions arise, payment will be issued on 2024-06-20. 🎊

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jun 17, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [POLISH][Simplified Collect] Create utils files for each page [HOLD for payment 2024-06-24] [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [POLISH][Simplified Collect] Create utils files for each page Jun 17, 2024
Copy link

melvin-bot bot commented Jun 17, 2024

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

If no regressions arise, payment will be issued on 2024-06-24. 🎊

@dangrous
Copy link
Contributor

No payment needed here but I'll close this on the 24th. Not expecting any regressions (though when ARE we expecting them?)

@luacmartins
Copy link
Contributor Author

@dangrous I think we can close this issue since it's deployed to prod and was internal. We'll know if there's a regression haha

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jun 21, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-06-24] [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [POLISH][Simplified Collect] Create utils files for each page [HOLD for payment 2024-06-28] [HOLD for payment 2024-06-24] [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [POLISH][Simplified Collect] Create utils files for each page Jun 21, 2024
Copy link

melvin-bot bot commented Jun 21, 2024

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

If no regressions arise, payment will be issued on 2024-06-28. 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Weekly KSv2
Projects
Archived in project
Development

No branches or pull requests

4 participants