-
Notifications
You must be signed in to change notification settings - Fork 23
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: Migrate isFreePlan to GQL Field and remove Helper #3584
Conversation
if ( | ||
accountDetails?.activatedUserCount >= accountDetails?.plan?.quantity && | ||
!planData?.plan?.hasSeatsLeft && |
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.
We're using hasSeatsLeft in favor of accountDetails business logic here -- cleaning up the code a bit imo :)
@@ -63,7 +62,7 @@ function MembersList() { | |||
}) | |||
|
|||
const { activate } = useActivateUser({ owner, provider }) | |||
const { data: accountDetails } = useAccountDetails({ owner, provider }) | |||
const { data: planData } = usePlanData({ owner, provider }) |
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.
hot swap of useAccountDetails -> usePlanData
@@ -61,14 +60,14 @@ function ActivationStatus({ | |||
handleActivate, | |||
}: ActivationStatusProps) { | |||
const { provider, owner } = useParams<{ provider: string; owner: string }>() | |||
const { data: accountDetails } = useAccountDetails({ owner, provider }) | |||
const { data: planData } = usePlanData({ owner, provider }) |
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.
Hot swap of useAccountDetails -> usePlanData
|
||
render(<FreePlanCard plan={freePlan} />, { | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a bug fix test update
Bundle ReportChanges will increase total bundle size by 953 bytes (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
Bundle ReportChanges will increase total bundle size by 953 bytes (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
@@ -17,10 +17,10 @@ const UpdateButton: React.FC<BillingControlsProps> = ({ | |||
seats, | |||
}) => { | |||
const { provider, owner } = useParams<{ provider: string; owner: string }>() | |||
const { data: accountDetails } = useAccountDetails({ provider, owner }) | |||
const { data: planData } = usePlanData({ provider, owner }) |
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.
Hot swap of useAccountDetails -> usePlanData
@@ -281,7 +284,8 @@ export const useInfiniteTestResults = ({ | |||
data?.owner?.repository?.testAnalytics?.testResults?.totalCount ?? | |||
0, | |||
private: data?.owner?.repository?.private ?? null, | |||
plan: data?.owner?.plan?.value ?? null, | |||
planName: data?.owner?.plan?.value ?? null, |
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.
renaming this field to be more relevant to what it actually is
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 #3584 +/- ##
==========================================
- Coverage 98.90% 98.90% -0.01%
==========================================
Files 810 810
Lines 14563 14555 -8
Branches 4145 4150 +5
==========================================
- Hits 14404 14395 -9
- Misses 152 153 +1
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 #3584 +/- ##
==========================================
- Coverage 98.90% 98.90% -0.01%
==========================================
Files 810 810
Lines 14563 14555 -8
Branches 4145 4143 -2
==========================================
- Hits 14404 14395 -9
- Misses 152 153 +1
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 #3584 +/- ##
==========================================
- Coverage 98.90% 98.90% -0.01%
==========================================
Files 810 810
Lines 14563 14555 -8
Branches 4152 4143 -9
==========================================
- Hits 14404 14395 -9
- Misses 152 153 +1
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 #3584 +/- ##
==========================================
- Coverage 98.91% 98.90% -0.02%
==========================================
Files 806 810 +4
Lines 14478 14555 +77
Branches 4105 4150 +45
==========================================
+ Hits 14321 14395 +74
- Misses 150 153 +3
Partials 7 7
... and 28 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
@@ -62,7 +60,7 @@ function PlansActionsBilling({ plan }) { | |||
return ( | |||
<div className="flex self-start"> | |||
<Button to={{ pageName: 'upgradeOrgPlan' }} variant="primary"> | |||
{isSentryPlan(plan?.value) ? 'Manage plan' : 'Upgrade'} |
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.
BUG FIX: this wasn't the plan object but rather the plan name so this was never working
@@ -71,22 +69,17 @@ function PlansActionsBilling({ plan }) { | |||
return ( | |||
<div className="flex self-start"> | |||
<Button to={{ pageName: 'upgradeOrgPlan' }} variant="primary"> | |||
{isFreePlan(plan?.value) || isTrialPlan(plan?.value) |
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.
similar bugfix here
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.
lgtm in general just one question regarding nullable values
@@ -35,13 +35,6 @@ export interface Plan { | |||
quantity?: number | |||
} | |||
|
|||
export function isFreePlan(plan?: PlanName | null) { |
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.
beauty
@@ -65,6 +65,7 @@ const GetTestResultsSchema = z.object({ | |||
plan: z | |||
.object({ | |||
value: z.nativeEnum(Plans), | |||
isFreePlan: z.boolean(), |
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.
is this nullable?
@@ -181,7 +183,8 @@ interface UseTestResultsArgs { | |||
testResults: TestResult[] | |||
pageInfo: { endCursor: string | null; hasNextPage: boolean } | |||
private: boolean | null | |||
plan: PlanName | null | |||
planName: PlanName | null | |||
isFreePlan: boolean | null |
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.
because it says | null here
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.
guh good catch, I'll update. It shouldn't be nullable iirc
Description
This PR aims to remove the isFreePlan helper function in Gazebo in favor of the new GraphQL field for the same.
Similar PR to #3576.
One piece of milestone 2: codecov/engineering-team#2989
Closes codecov/engineering-team#2997
Clearly I lied when I said the isEnterprisePlan PR was going to be the largest, each one looks to be similarly difficult though now with 2 under our belts I think we know what each of these will entail for the rest.
Notes
Screenshots
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.