-
Notifications
You must be signed in to change notification settings - Fork 29
feat: Add is___plan fields to plan resolver #1039
Conversation
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 #1039 +/- ##
=======================================
Coverage 96.00% 96.00%
=======================================
Files 828 828
Lines 19373 19397 +24
=======================================
+ Hits 18599 18623 +24
Misses 774 774
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
❌ 2 Tests Failed:
View the top 2 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
❌ 2 Tests Failed:
View the top 2 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
| @plan_bindable.field("isEnterprisePlan") | ||
| @sync_to_async | ||
| def resolve_is_enterprise_plan(plan_service: PlanService, info) -> bool: | ||
| return plan_service.is_enterprise_plan |
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.
where are these PlanService.is_* attributes defined, I couldn't find them in shared.
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.
LOL I totally forgot that we swapped over that stuff to shared, let me open up a new PR for that part. It worked on my local because I modified shared directly
| isProPlan: Boolean! | ||
| isSentryPlan: Boolean! | ||
| isTeamPlan: Boolean! | ||
| isTrialPlan: 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.
How are all these boolean fields different from using a enum for plan 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.
A couple aspects to it; first being there are multiple pro / sentry / team / enterprise / free plans so we can't rely on just the name of the plan to uniquely identify the plan type
Secondly, we could use the 'tier name' attribute, and I was thinking about that route as well, but then in the gazebo code instead of having something like
BEFORE
isFreePlan(accountDetails?.plan?.value)
AFTER
accountDetails?.plan?.isFreePlan
POTENTIAL AFTER
accountDetails?.plan?.tier == 'free'
which then kind of leaves us in a similar situation as currently where we have some business logic in the FE. I could go either way tbh, I was just slightly leaning toward keeping gazebo more presentational
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.
Ok got it
there are multiple pro / sentry / team / enterprise / free plans
does this mean multiple of these is_* booleans can be true for a given org?
…lue than the key, GQL doesnt like that
|
This PR includes changes to |
| trialEndDate: DateTime | ||
| trialTotalDays: Int | ||
| pretrialUsersCount: Int | ||
| baseUnitPrice: Int! |
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.
just organizing this in alphabetical
Purpose/Motivation
As part of Milestone 2 for making the developer plan like the team plan, we want to move all the business logic from Gazebo to API; making Gazebo the presentation layer it should've been in the first place.
Currently Gazebo has these helper functions:
Each of which needs to be updated / modified in tandem with any plan changes that occur on the API side. We basically have 2 sources of Truth for this information, and anytime we want to update that information on one end we end up having to "remember" to update it on the other to avoid breaking something.
The goal of this PR is to tackle creating all the new resolvers and graphQL types we need for the is____Plan helpers. The find___Plan helpers will be tackled in a separate PR.
Links to relevant tickets
Related ticket -- #2996
What does this PR do?
So in this PR, I declare a couple extra properties on the plan service, and just do a check for the current plan name being in any of the existing representations. In the future, when we create the new plan configuration table, we may need to update these functions one more time to just pull the properties from the table directly (depending on how the columns end up shaking up there)
Notes to Reviewer
Anything to note to the team? Any tips on how to review, or where to start?
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.