chore: Replace account details plan data occurrences #3587
Conversation
Bundle ReportChanges will decrease total bundle size by 731 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3587 +/- ##
==========================================
- Coverage 98.90% 98.90% -0.01%
==========================================
Files 810 810
Lines 14555 14548 -7
Branches 4143 4146 +3
==========================================
- Hits 14395 14388 -7
Misses 153 153
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3587 +/- ##
==========================================
- Coverage 98.90% 98.90% -0.01%
==========================================
Files 810 810
Lines 14555 14548 -7
Branches 4150 4139 -11
==========================================
- Hits 14395 14388 -7
Misses 153 153
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #3587 +/- ##
==========================================
- Coverage 98.90% 98.90% -0.01%
==========================================
Files 810 810
Lines 14555 14548 -7
Branches 4143 4146 +3
==========================================
- Hits 14395 14388 -7
Misses 153 153
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
Bundle ReportChanges will decrease total bundle size by 731 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3587 +/- ##
==========================================
- Coverage 98.90% 98.90% -0.01%
==========================================
Files 810 810
Lines 14555 14548 -7
Branches 4150 4146 -4
==========================================
- Hits 14395 14388 -7
Misses 153 153
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
| const handleActivate = | ||
| (accountDetails, plan, planUserCount, activate, setIsOpen) => (user) => { | ||
| if ( | ||
| accountDetails?.activatedUserCount >= planUserCount && |
There was a problem hiding this comment.
I actually just removed accountDetails altogether in the isFreePlan migration PR, wondering if you'd be open to either making the similar change on your end or letting me merge first and rebase
| @@ -291,7 +291,7 @@ describe('FreePlanCard', () => { | |||
| wrapper, | |||
| }) | |||
|
|
|||
| const link = await screen.findByRole('link', { name: /Manage plan/ }) | |||
| const link = await screen.findByRole('link', { name: /Upgrade/ }) | |||
There was a problem hiding this comment.
do we know what caused this test to change?
There was a problem hiding this comment.
Not sure how it used to work, free plan should never have "manage plan" option
| selectedPlan, | ||
| newPlanName, | ||
| seats, | ||
| nextBillingDate, | ||
| }: { | ||
| currentPlan?: z.infer<typeof PlanSchema> | ||
| currentPlan?: PlanName |
There was a problem hiding this comment.
Keeping this as the Plan object would actually open us up a lot better than "downscaling" to PlanName, I have a similar update in the isFreePlan I believe that can hopefully help here
There was a problem hiding this comment.
It’s not preferred to pass down all props as an object if you only need 1 or 2 properties. I really think this would be a good refactor to implement here — check this out: StackOverflow link.
And fwiw the plan schema is expanding, and I doubt we’ll need all the props here. The same goes for the ones below; it’s just more explicit, expected, and manageable this way. I don't mind if you want to merge your PR first as it's a big one, i can then make the little changes to adjust with your work.
There was a problem hiding this comment.
Yep I understand what you're saying! I think in the future we could go back and pick off the properties that are needed if performance ends up being a concern here. But ultimately, I think this is a micro-optimization since the "Plan" object is a static object
src/shared/utils/upgradeForm.ts
Outdated
| @@ -142,16 +144,16 @@ export function shouldRenderCancelLink({ | |||
| trialStatus, | |||
| }: { | |||
| cancelAtPeriodEnd: boolean | |||
| plan: Plan | |||
| plan: PlanName | |||
There was a problem hiding this comment.
Keeping this as Plan would help a lot in the future as well since we'll be using properties off the plan instead of the planName
RulaKhaled
force-pushed
the
replace-account-details-plan-occurences
branch
from
b4d455b to
733a1eb
Compare
src/shared/utils/upgradeForm.ts
Outdated
| activatedUserCount, | ||
| inactiveUserCount, | ||
| trialStatus, | ||
| isSentryUpgrade, | ||
| isFreePlan, | ||
| isFreePlan: plan?.value === Plans.USERS_BASIC, |
There was a problem hiding this comment.
this should use plan.isFreePlan right?
Description
Replacing
account details>planwithplan data>details.Notable Changes
Screenshots
nothing visual changed.
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.