-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: Create plans and tiers tables #467
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #467 +/- ##
==========================================
- Coverage 90.55% 89.97% -0.58%
==========================================
Files 404 324 -80
Lines 12562 9198 -3364
Branches 2107 1633 -474
==========================================
- Hits 11375 8276 -3099
+ 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. |
|
||
class BillingRate(models.TextChoices): | ||
MONTHLY = "monthly" | ||
YEARLY = "yearly" |
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.
why not use PlanBillingRate
?
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 should be annually instead of yearly as well I think https://github.com/codecov/gazebo/blob/113a97f9e6a40ab9162576599308e123a25626aa/src/shared/utils/billing.ts#L29
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.
didn't know we have it but i think we can't use enum.Enum directly in the choices argument of a Django model field, we'll have to do [(tag.value, tag.name) for tag in PlanBillingRate]
which is not very clean.
I'll update to annually
tier = models.ForeignKey( | ||
"Tiers", on_delete=models.CASCADE, related_name="plans", db_index=True | ||
) |
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.
tier = models.ForeignKey( | |
"Tiers", on_delete=models.CASCADE, related_name="plans", db_index=True | |
) | |
tier = models.ForeignKey( | |
"Tiers", on_delete=models.SET_NULL, related_name="plans" | |
) |
I removed the index, seems unnecessary for a small table, but up to you
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.
Agreed with Nora, probably don't need an index here
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.
makes sense, tbh i always like to use indexes you never know how big the table will grow to be
tier = models.ForeignKey( | ||
"Tiers", on_delete=models.CASCADE, related_name="plans", db_index=True | ||
) | ||
base_unit_price = models.IntegerField(default=0) |
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.
base_unit_price = models.IntegerField(default=0) | |
base_unit_price = models.IntegerField(default=0, blank=True) |
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 this one should always have a value, assuming blank=True means 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.
blank true means it can be null in the DB but it can't be optional in a form
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 it makes sense if we are ever updating free plans
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.
no, blank=True
means you don't have to supply a value in a form/when you create the object, it's independent from nullable. it has to do with the field in code, not db. docs
if you have blank=False
, you need to manually include a value for this field even if it's the default value
if you have blank=True
, you don't need to explicitly set this field, if you leave it blank, it will use the default value. Because of this, I put blank=True
on almost any field that has a default value so that I get the behavior I want.
plan_name = models.CharField(max_length=255, unique=True) | ||
|
||
class Meta: | ||
db_table = "plans" |
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.
is this needed?
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.
what does this do? :O
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 just a clean way of naming the database
private_repo_support = models.BooleanField(default=False) | ||
|
||
class Meta: | ||
db_table = "tiers" |
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.
prob not needed for a new table unless there's naming overlap with another table
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.
looking good! left some comments, happy to discuss any if you want :)
I think @ajay-sentry said that there will be a lot of django admin use of these tables - I would make those pages now and test them to make sure your fields are the way you want. Same with factories.
max_seats = models.IntegerField(null=True, blank=True) | ||
monthly_uploads_limit = models.IntegerField(null=True, blank=True) | ||
paid_plan = models.BooleanField(default=False) | ||
plan_name = models.CharField(max_length=255, unique=True) |
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 a thought here, should we just call this "name?" since it's already on the plan 🤔
bundle_analysis = models.BooleanField(default=False) | ||
test_analytics = models.BooleanField(default=False) | ||
flaky_test_detection = models.BooleanField(default=False) | ||
patch_coverage = models.BooleanField(default=False) |
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 should be project coverage right?
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.
yes good catch
test_analytics = False | ||
flaky_test_detection = False | ||
project_coverage = False | ||
private_repo_support = False |
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.
if you had blank=True
on the bool fields on the model, you wouldn't need to name any of these fields, they would automatically be False
when you make an obj with the factory
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 it default to False or None in that case?
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 stack overflow page seems to give a mixed view -- https://stackoverflow.com/questions/8609192/what-is-the-difference-between-null-true-and-blank-true-in-django
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 realize now that we already had default=False on the model too 🤦♂️
To replace plans representation, we're making plans configurable through the DB.
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.