-
Notifications
You must be signed in to change notification settings - Fork 22
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
ref: Update useAvailablePlans to have isSentry/TeamPlan AND remove isSentryPlan helper for GQL field #3610
base: main
Are you sure you want to change the base?
Conversation
…ose isTeamPlan and isSentryPlan
…trailing test fixes
value={value} | ||
baseUnitPrice={baseUnitPrice} | ||
/> | ||
{baseUnitPrice ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically you don't need base UnitPrice OR plan here for this component to render, but I left this as is to not accidentally break something
@@ -20,116 +21,57 @@ describe('PlanPricing', () => { | |||
}) | |||
}) | |||
|
|||
describe('user is on a basic plan', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of the tests in this file are obsolete now that we only care about the "type" of plan, annual vs. monthly and old vs. new plans don't matter in gazebo anymore
Bundle ReportChanges will decrease total bundle size by 958 bytes (-0.01%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
Bundle ReportChanges will decrease total bundle size by 958 bytes (-0.01%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
|
||
import ProPlanDetails from './ProPlanDetails' | ||
import SentryPlanDetails from './SentryPlanDetails' | ||
import TeamPlanDetails from './TeamPlanDetails' | ||
|
||
function UpgradeDetails({ selectedPlan }: { selectedPlan: IndividualPlan }) { | ||
if (isSentryPlan(selectedPlan.value)) { | ||
if (selectedPlan.isSentryPlan) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is why I wanted to lump in the updates to IndividualPlan (and useAvailablePlans) in this PR, so we didn't have to do a similar short term bandage like we did on lines 12-14 previously.
newPlan?.value === Plans.USERS_TEAMM || | ||
newPlan?.value === Plans.USERS_TEAMY | ||
) { | ||
if (newPlan?.isTeamPlan) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More stuff we can fix with the updates to useAvailablePlans
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 #3610 +/- ##
==========================================
- Coverage 99.00% 99.00% -0.01%
==========================================
Files 810 810
Lines 14549 14524 -25
Branches 4150 4129 -21
==========================================
- Hits 14404 14379 -25
Misses 138 138
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 #3610 +/- ##
==========================================
- Coverage 99.00% 99.00% -0.01%
==========================================
Files 810 810
Lines 14549 14524 -25
Branches 4150 4129 -21
==========================================
- Hits 14404 14379 -25
Misses 138 138
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 #3610 +/- ##
==========================================
- Coverage 99.00% 99.00% -0.01%
==========================================
Files 810 810
Lines 14549 14524 -25
Branches 4143 4129 -14
==========================================
- Hits 14404 14379 -25
Misses 138 138
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
@@ -50,7 +47,7 @@ export function extractSeats({ | |||
|
|||
// if their on trial their seat count is around 1000 so this resets the | |||
// value to the minium value they would be going on if sentry or pro | |||
if (trialStatus === TrialStatuses.ONGOING && value === Plans.USERS_TRIAL) { | |||
if (trialStatus === TrialStatuses.ONGOING && isTrialPlan) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's one of the references we were still relying on the plan name for; changed to now use isTrialPlan
trialStatus === TrialStatuses.ONGOING && | ||
plan?.value === Plans.USERS_TRIAL | ||
) { | ||
if (trialStatus === TrialStatuses.ONGOING && plan?.isTrialPlan) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other reference that has been changed to use isTrialPlan
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3610 +/- ##
==========================================
- Coverage 99.00% 99.00% -0.01%
==========================================
Files 810 810
Lines 14549 14524 -25
Branches 4150 4129 -21
==========================================
- Hits 14404 14379 -25
Misses 138 138
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
Description
last bits of milestone 2 work (for now, I think near term we will need to consolidate these two plan representations into a single one since it's a little confusing still. But "IndividualPlan" is basically all the static plan information, while Plan has that information + owner specific information like hasSeatsLeft)
This PR updates IndividualPlan to have isTeamPlan and isSentryPlan, which are the only two values we need off that version of the plan and related hook atm. Didn't want to overcompensate here and add all the values to the GQL query when we don't actually need them.
This PR ALSO removes the isSentryPlan helper and swaps over to the isSentryPlan GQL value on the Plan type. That stuff is somewhat straightforward and similar to all the other migrations we've done.
In the process, I also saw a few places where we were still referencing the trial plan name directly and have made the updates to change those over since they were pretty small. I'll comment in the PR where those occur for whoever takes this review on
Closes codecov/engineering-team#3132
Closes codecov/engineering-team#3137
Screenshots
Confirming isSentryPlan / isTeamPlan getting passed through properly
Link to Sample Entry
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.