-
Notifications
You must be signed in to change notification settings - Fork 6
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
PlanService updates for GitLab #461
base: main
Are you sure you want to change the base?
Conversation
shared/plan/constants.py
Outdated
SENTRY_MONTHLY = "users-sentrym" | ||
SENTRY_YEARLY = "users-sentryy" | ||
SENTRY_MONTHLY = "users-sentrym" # not in BillingPlan | ||
SENTRY_YEARLY = "users-sentryy" # not in BillingPlan |
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.
does anyone know why these Plans aren't in the BillingPlan enum? Seems like an oversight...
@ajay-sentry @RulaKhaled since you two have been in the PlanService lately 🙏
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.
Talked with Ajay, this is an oversight, adding to BillingPlan
31ae72f
to
3a445a9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #461 +/- ##
==========================================
- Coverage 90.55% 89.96% -0.59%
==========================================
Files 404 324 -80
Lines 12562 9189 -3373
Branches 2107 1633 -474
==========================================
- Hits 11375 8267 -3108
+ Misses 1078 859 -219
+ Partials 109 63 -46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
shared/billing/__init__.py
Outdated
@@ -37,6 +37,7 @@ def is_enterprise_cloud_plan(plan: BillingPlan) -> bool: | |||
|
|||
|
|||
def is_pr_billing_plan(plan: str) -> bool: | |||
# use is_pr_billing_plan() in PlanService instead of accessing this directly |
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.
Maybe it's worth to mark it as deprecated?
self.current_org = current_org | ||
if ( | ||
current_org.service == Service.GITLAB.value | ||
and current_org.parent_service_id |
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 doesn't seem correct... you are checking for current_org.parent_service_id
not being None
, but self.current_org
is getting the value of current_org.root_organization
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.
it's correct, just confusing 🙃
We want the root org. The way you know if the current_org is the root org is parent_service_id
field.
if current_org.parent_service_id
, it means it is not the root org.
parent and root are not necessarily the same org - GL orgs can have multiple levels of groups and subgroups.
So if you know that you don't have the root org (by checking current_org.parent_service_id), then get the root org with .root_organization
, a helper function on the model.
3a445a9
to
9e55b8a
Compare
…_billing_plan into PlanService
9e55b8a
to
990c398
Compare
update PlanService so GitLab orgs use the root org's plan, fold is_pr_billing_plan into PlanService
IF an OwnerOrg is a GitLab subgroup OR has an Account, the
plan
on the OwnerOrg is not the one we want to use! We want theplan
on the root Org or Account object instead.PlanService already had the smarts to check for an
Account
and get the Accountplan
, but now it also has the smarts to check whether a GitLab OwnerOrg has a parent, then use theplan
from the root Org.Part of codecov/engineering-team#2710