feat: add plan tier service #150
Conversation
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 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 ReportAll modified lines are covered by tests ✅ 📢 Thoughts on this report? Let us know!. |
| @@ -23,6 +23,7 @@ type Owner { | |||
| orgUploadToken: String | |||
| defaultOrgUsername: String | |||
| isCurrentUserActivated: Boolean! | |||
| tier: TierName | |||
There was a problem hiding this comment.
The return type of the resolver is TierName which indicates that it can't be null. Should this be TierName! to match? Or should the return type of the resolver be Optional[TierName]?
There was a problem hiding this comment.
Good catch, I'll make this non-nullable
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.
Would it make sense to just include a tier with each plan representation? Then we wouldn't need to keep these lists in sync and might simplify the implementation of TierService
There was a problem hiding this comment.
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=<some tier>,
...
),
}
}
class PlanService():
...
def tier(self) -> TierName:
return self.plan_data.tier
| @@ -47,6 +50,8 @@ class PlanPrice(enum.Enum): | |||
| CODECOV_FREE = 0 | |||
| CODECOV_BASIC = 0 | |||
| CODECOV_TRIAL = 0 | |||
| LITE_MONTHLY = 6 | |||
| LITE_YEARLY = 8 | |||
There was a problem hiding this comment.
These values are not defined yet and subject to change
| @@ -84,6 +97,7 @@ class PlanData: | |||
| "Unlimited private repositories", | |||
| "Priority Support", | |||
| ], | |||
| tier_name=TierName.PRO.value, | |||
There was a problem hiding this comment.
I think the types are slightly off here. TierName.PRO has type TierName but TierName.PRO.value has type str
| @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.
This is where you could call .value on the enum. The return type would be str.
| @@ -6,6 +6,7 @@ type Plan { | |||
| pretrialUsersCount: Int | |||
| marketingName: String! | |||
| planName: String! | |||
| tierName: String! | |||
There was a problem hiding this comment.
Do we use the GraphQL enum anywhere? Should this be tierName: TierName! instead?
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.