feat: Changing ENT-11900 to non nullable SSPproduct#202
feat: Changing ENT-11900 to non nullable SSPproduct#202rthota-sonata-hue wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #202 +/- ##
==========================================
+ Coverage 86.24% 86.25% +0.01%
==========================================
Files 153 153
Lines 12648 12663 +15
Branches 1210 1211 +1
==========================================
+ Hits 10908 10923 +15
Misses 1425 1425
Partials 315 315 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the customer_billing checkout intent data model to enforce a non-null ssp_product relationship (defaulting to teams-yearly) and adds a uniqueness constraint intended to prevent duplicate (user, ssp_product) combinations.
Changes:
- Made
CheckoutIntent.ssp_productnon-nullable and introduced a defaultteams-yearlyvalue. - Added a DB uniqueness constraint on
(user, ssp_product). - Updated model behavior and tests to accommodate the non-null/defaulted
ssp_product.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| enterprise_access/apps/customer_billing/models.py | Makes ssp_product non-null with a default, adds a uniqueness constraint, and adds save() logic for default assignment. |
| enterprise_access/apps/customer_billing/migrations/0037_alter_checkoutintent_sspproduct_nonnull_and_unique.py | Alters the FK field and adds the uniqueness constraint at the DB level. |
| enterprise_access/apps/customer_billing/tests/test_models.py | Updates model tests to reflect default assignment behavior for ssp_product. |
| migrations.AlterField( | ||
| model_name='checkoutintent', | ||
| name='ssp_product', | ||
| field=models.ForeignKey(default='teams-yearly', on_delete=django.db.models.deletion.PROTECT, to='customer_billing.sspproduct'), | ||
| ), |
| def save(self, *args, **kwargs): | ||
| """Ensure a default `ssp_product` exists before saving to satisfy non-null constraint.""" | ||
| if not getattr(self, 'ssp_product_id', None): | ||
| try: | ||
| default_product = SspProduct.objects.get(slug='teams-yearly') | ||
| except SspProduct.DoesNotExist: | ||
| default_product, _ = SspProduct.objects.get_or_create( | ||
| slug='teams-yearly', | ||
| defaults={ | ||
| 'stripe_price_lookup_key': 'teams_yearly_price', | ||
| 'catalog_query_uuid': uuid4(), | ||
| }, | ||
| ) | ||
| self.ssp_product = default_product | ||
| super().save(*args, **kwargs) |
| class Meta: | ||
| verbose_name = "Enterprise Checkout Intent" | ||
| verbose_name_plural = "Enterprise Checkout Intents" | ||
| constraints = [ | ||
| models.UniqueConstraint(fields=['user', 'ssp_product'], name='unique_user_ssp_product'), |
There was a problem hiding this comment.
This is a fair point and we should do it in this PR. The tech spec didn't explicitly call it out, sorry about that.
There was a problem hiding this comment.
We're still missing the change to make the user field a ForeignKey instead of OneToOne.
| products = [] | ||
| for slug in slugs: | ||
| product, _ = SspProduct.objects.get_or_create( | ||
| slug=slug, | ||
| defaults={ | ||
| 'stripe_price_lookup_key': f"{slug.replace('-', '_')}_price", | ||
| 'catalog_query_uuid': uuid4(), | ||
| }, | ||
| ) | ||
| products.append(product) | ||
|
|
||
| # Create one intent without specifying a product; model should assign default | ||
| intent_without_product = CheckoutIntent.objects.create( | ||
| user=cast(AbstractUser, self.user1), | ||
| enterprise_slug='nullable-product-enterprise', |
shravani-sonata-gottapu
force-pushed
the
shravani/ENT-11902-non-nullable
branch
from
77b2950 to
4a53c7d
Compare
iloveagent57
left a comment
iloveagent57
left a comment
There was a problem hiding this comment.
Looks good, just two small changes requested.
| if not getattr(self, 'ssp_product_id', None): | ||
| # assign the default SSP product slug; let FK constraint enforce existence | ||
| # pylint: disable=attribute-defined-outside-init | ||
| self.ssp_product_id = 'teams-yearly' |
There was a problem hiding this comment.
why do we need this if the field defines the same default?
| blank=True, | ||
| null=False, | ||
| blank=False, | ||
| default='teams-yearly', |
There was a problem hiding this comment.
the teams-yearly value should be changed to a django setting value so that we're able to change this default across environments, and also to avoid using a magic string
| class Meta: | ||
| verbose_name = "Enterprise Checkout Intent" | ||
| verbose_name_plural = "Enterprise Checkout Intents" | ||
| constraints = [ | ||
| models.UniqueConstraint(fields=['user', 'ssp_product'], name='unique_user_ssp_product'), |
There was a problem hiding this comment.
This is a fair point and we should do it in this PR. The tech spec didn't explicitly call it out, sorry about that.
|
|
||
| def get_default_ssp_product_slug(): | ||
| """Return the default SSP product slug from Django settings.""" | ||
| return getattr(settings, 'SSP_DEFAULT_PRODUCT_SLUG', 'teams-yearly') |
There was a problem hiding this comment.
if you're adding it as a base setting, you don't need to be this defensive about getting the value - you can use settings.SSP_DEFAULT_PRODUCT_SLUG directly (thus avoiding use of a magic string as the getattr() default value).
| class Meta: | ||
| verbose_name = "Enterprise Checkout Intent" | ||
| verbose_name_plural = "Enterprise Checkout Intents" | ||
| constraints = [ | ||
| models.UniqueConstraint(fields=['user', 'ssp_product'], name='unique_user_ssp_product'), |
There was a problem hiding this comment.
We're still missing the change to make the user field a ForeignKey instead of OneToOne.
Ticket : https://2u-internal.atlassian.net/browse/ENT-11902
Description :
Alter ssp_product FK from null=True to null=False
Add unique_together constraint on (user, ssp_product)
Add migration guard to fail if NULL rows still exist
Update tests for non-nullable field and unique constraint