Shared: Migrate to Plan / Tier Tables #479
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,24 +5,16 @@ | |
| from shared.billing import is_pr_billing_plan | ||
| from shared.config import get_config | ||
| from shared.django_apps.codecov.commands.exceptions import ValidationError | ||
| from shared.django_apps.codecov_auth.models import Owner, Plan, Service | ||
| from shared.plan.constants import ( | ||
| TEAM_PLAN_MAX_USERS, | ||
| TRIAL_PLAN_SEATS, | ||
| PlanBillingRate, | ||
| PlanName, | ||
| TierName, | ||
| TrialDaysAmount, | ||
| TrialStatus, | ||
| convert_to_DTO, | ||
| ) | ||
| from shared.self_hosted.service import enterprise_has_seats_left, license_seats | ||
|
|
||
|
|
@@ -55,19 +47,20 @@ def __init__(self, current_org: Owner): | |
| self.current_org = current_org.root_organization | ||
| else: | ||
| self.current_org = current_org | ||
|
|
||
| if not Plan.objects.filter(name=self.current_org.plan).exists(): | ||
|
Contributor
There was a problem hiding this comment. Remind me, are we porting all plans, including "obsolete" plans like the super super old ones that we don't use? If so, changing those "if name not in USER_PLAN_REPRESENTATION" to this wouldn't be 100% equivalent
Contributor
Author
There was a problem hiding this comment. The full plan list we're porting over can be found here: https://www.notion.so/sentry/Plan-and-Tier-Table-Mapping-1758b10e4b5d801aa2d7e30198bcfc68 It's basically all the plans that were in USER_PLAN_REPRESENTATION, and seemingly the check should hold true even if the plans are no longer active in the future too, which is why I didn't add the is_active=True filter |
||
| raise ValueError("Unsupported plan") | ||
| self._plan_data = None | ||
|
|
||
| def update_plan(self, name: str, user_count: Optional[int]) -> None: | ||
| """Updates the organization's plan and user count.""" | ||
| if not Plan.objects.filter(name=name).exists(): | ||
| raise ValueError("Unsupported plan") | ||
| if not user_count: | ||
| raise ValueError("Quantity Needed") | ||
| self.current_org.plan = name | ||
| self.current_org.plan_user_count = user_count | ||
| self._plan_data = Plan.objects.get(name=name) | ||
| self.current_org.delinquent = False | ||
| self.current_org.save() | ||
|
|
||
|
|
@@ -89,25 +82,25 @@ def has_account(self) -> bool: | |
| return self.current_org.account is not None | ||
|
|
||
| @property | ||
| def plan_data(self) -> Plan: | ||
|
Contributor
There was a problem hiding this comment. Have you confirmed this implementation effectively acts as cacheing the field? Since this is now a db query, we'd want this to be cached - perhaps look into cached_property decorator
Contributor
Author
There was a problem hiding this comment. How can I confirm if its cached or not? Is there a way to check locally / on stage if we do a DB hit vs. use the cache with the new property? In any case I was able to update using that property in 7804571
Contributor
There was a problem hiding this comment. Setting the decorator should suffice, there's probably a way to check it programatically as well but can't think off the top of my head |
||
| """Returns the plan data for the organization, either from account or default.""" | ||
| if self._plan_data is None: | ||
| self._plan_data = Plan.objects.get( | ||
| name=self.current_org.account.plan | ||
| if self.has_account | ||
| else self.current_org.plan | ||
| ) | ||
| return self._plan_data | ||
|
|
||
| @plan_data.setter | ||
| def plan_data(self, plan_data: Optional[Plan]) -> None: | ||
| """Sets the plan data directly.""" | ||
| self._plan_data = plan_data | ||
|
|
||
| @property | ||
| def plan_name(self) -> str: | ||
| """Returns the name of the organization's current plan.""" | ||
| return self.plan_data.name | ||
|
|
||
| @property | ||
| def plan_user_count(self) -> int: | ||
|
|
@@ -159,29 +152,35 @@ def monthly_uploads_limit(self) -> Optional[int]: | |
| return self.plan_data.monthly_uploads_limit | ||
|
|
||
| @property | ||
| def tier_name(self) -> TierName: | ||
| """Returns the tier name of the plan.""" | ||
| return self.plan_data.tier.tier_name | ||
|
Contributor
There was a problem hiding this comment. Since this has a relationship to tier, might be worth in the query of plan_data to prefetch this relationship, otherwise you'll have an n+1
Contributor
Author
There was a problem hiding this comment. Added to all relevant touchpoints! Good call |
||
|
|
||
| def available_plans(self, owner: Owner) -> List[Plan]: | ||
| """Returns the available plans for the owner and organization.""" | ||
| available_plans = {Plan.objects.get(name=PlanName.BASIC_PLAN_NAME.value)} | ||
|
|
||
| curr_plan = Plan.objects.get(name=self.plan_name) | ||
|
||
| if not curr_plan.paid_plan: | ||
| available_plans.add(curr_plan) | ||
|
|
||
| # Build list of available tiers based on conditions | ||
| available_tiers = [TierName.PRO.value] | ||
|
|
||
| if is_sentry_user(owner): | ||
| available_tiers.append(TierName.SENTRY.value) | ||
|
|
||
| if ( | ||
| not self.plan_activated_users | ||
| or len(self.plan_activated_users) <= TEAM_PLAN_MAX_USERS | ||
| ): | ||
| available_tiers.append(TierName.TEAM.value) | ||
|
|
||
| available_plans.update( | ||
| Plan.objects.filter(tier__tier_name__in=available_tiers, is_active=True) | ||
|
||
| ) | ||
|
|
||
| return [convert_to_DTO(plan) for plan in available_plans] | ||
|
|
||
| def _start_trial_helper( | ||
| self, | ||
|
|
@@ -222,7 +221,7 @@ def start_trial(self, current_owner: Owner) -> None: | |
| """ | ||
| if self.trial_status != TrialStatus.NOT_STARTED.value: | ||
| raise ValidationError("Cannot start an existing trial") | ||
| if not Plan.objects.filter(name=self.plan_name, paid_plan=False).exists(): | ||
| raise ValidationError("Cannot trial from a paid plan") | ||
|
|
||
| self._start_trial_helper(current_owner) | ||
|
|
@@ -236,10 +235,12 @@ def start_trial_manually(self, current_owner: Owner, end_date: datetime) -> None | |
| No value | ||
| """ | ||
| # Start a new trial plan for free users currently not on trial | ||
|
|
||
| plan = Plan.objects.get(name=self.plan_name) | ||
|
||
| if plan.paid_plan is False: | ||
| self._start_trial_helper(current_owner, end_date, is_extension=False) | ||
| # Extend an existing trial plan for users currently on trial | ||
| elif plan.tier.tier_name == TierName.TRIAL.value: | ||
| self._start_trial_helper(current_owner, end_date, is_extension=True) | ||
| # Paying users cannot start a trial | ||
| else: | ||
|
|
@@ -324,30 +325,30 @@ def has_seats_left(self) -> bool: | |
|
|
||
| @property | ||
| def is_enterprise_plan(self) -> bool: | ||
| return self.plan_data.tier.tier_name == TierName.ENTERPRISE.value | ||
|
|
||
| @property | ||
| def is_free_plan(self) -> bool: | ||
| return self.plan_data.paid_plan is False and not self.is_org_trialing | ||
|
|
||
| @property | ||
| def is_pro_plan(self) -> bool: | ||
| return ( | ||
| self.plan_data.tier.tier_name == TierName.PRO.value | ||
| or self.plan_data.tier.tier_name == TierName.SENTRY.value | ||
| ) | ||
|
|
||
| @property | ||
| def is_sentry_plan(self) -> bool: | ||
| return self.plan_data.tier.tier_name == TierName.SENTRY.value | ||
|
|
||
| @property | ||
| def is_team_plan(self) -> bool: | ||
| return self.plan_data.tier.tier_name == TierName.TEAM.value | ||
|
|
||
| @property | ||
| def is_trial_plan(self) -> bool: | ||
| return self.plan_data.tier.tier_name == TierName.TRIAL.value | ||
|
|
||
| @property | ||
| def is_pr_billing_plan(self) -> bool: | ||
|
|
||
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.
Thoughts on typing this?
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 was getting a circular dependency trying to import the Plan class and type it that way, is there another way we can do it?
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.
So why is this a circular dependency? I suppose cause you'd import Plan, and plan imports from constants.
So, this being the only thing that imports from Plan, could this belong in plan instead? You'd have to make an instance of Plan to use this wherever you do though
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.
yeah exactly, I kinda wanted to avoid having to bind this function to a class when it's really just a "transformer" from our plan object to the plan object that the planService and subsequently GQL expects
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 see. I'll leave the implementation up to you, I think though that not typing a function because of circular imports feels a bit odd, but you're getting of the fn soon right? If so I wouldn't say it's a big deal