- Notifications
You must be signed in to change notification settings - Fork30
Comments
Conversation
codecovbot commentedSep 20, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #150 +/- ##=======================================+ Coverage 95.44 95.45 +0.01======================================= Files 709 709 Lines 15201 15223 +22 =======================================+ Hits 14508 14530 +22 Misses 693 693
Flags with carried forward coverage won't be shown.Click here to find out more.
|
codecov-public-qabot commentedSep 20, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #150 +/- ##======================================= Coverage 95.52% 95.53% ======================================= Files 594 594 Lines 14801 14823 +22 =======================================+ Hits 14139 14161 +22 Misses 662 662
Flags with carried forward coverage won't be shown.Click here to find out more.
|
codecov-stagingbot commentedSep 20, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified lines are covered by tests ✅ 📢 Thoughts on this report?Let us know!. |
| orgUploadToken: String | ||
| defaultOrgUsername: String | ||
| isCurrentUserActivated: Boolean! | ||
| tier: TierName |
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 return type of the resolver isTierName which indicates that it can't be null. Should this beTierName! to match? Or should the return type of the resolver beOptional[TierName]?
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.
Good catch, I'll make this non-nullable
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.
plan/constants.py Outdated
| ENTERPRISE_TIER_PLAN_NAMES = [ | ||
| PlanName.ENTERPRISE_CLOUD_MONTHLY.value, | ||
| PlanName.ENTERPRISE_CLOUD_YEARLY.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.
Would it make sense to just include atier with each plan representation? Then we wouldn't need to keep these lists in sync and might simplify the implementation ofTierService
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.
That's a good idea, my only hesitance is anticipating if a tier could ever not be strictly tied to your plan. If so, it almost feels like there's no need for a tier service altogether, and just a property of the plan service, so like
<some>_PLAN_REPRESENTATIONS= {PlanName.<some>_PLAN_NAME.value:PlanData( ...tier=<sometier>, ... ), }}classPlanService():...deftier(self)->TierName:returnself.plan_data.tier
| CODECOV_BASIC = 0 | ||
| CODECOV_TRIAL = 0 | ||
| LITE_MONTHLY = 6 | ||
| LITE_YEARLY = 8 |
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.
These values are not defined yet and subject to change
| "Unlimited private repositories", | ||
| "Priority Support", | ||
| ], | ||
| tier_name=TierName.PRO.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.
I think the types are slightly off here.TierName.PRO has typeTierName butTierName.PRO.value has typestr
| @plan_bindable.field("tierName") | ||
| @convert_kwargs_to_snake_case | ||
| def resolve_tier_name(plan_service: PlanService, info) -> TierName: | ||
| return plan_service.tier_name |
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 where you could call.value on the enum. The return type would bestr.
| pretrialUsersCount: Int | ||
| marketingName: String! | ||
| planName: String! | ||
| tierName: String! |
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.
Do we use the GraphQL enum anywhere? Should this betierName: TierName! instead?
Morton22 commentedSep 6, 2024
|
Morton22 commentedSep 6, 2024
#> ##Codecov Report
|
Purpose/Motivation
We need a way to differentiate a tier based on it's plan. This is an evolving tier so this is the first pass at this.
What does this PR do?
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.