Subscriptions: avoid double deletion

[stsewd]

Subscription is set to cascade delete
when an organization is deleted,
with this additional signal
we are cancelling the subscription twice,
and stripe is given an error.

[humitos]

Where are we canceling the subscription on Stripe when the organization is deleted?

[stsewd]

@humitos

readthedocs.org/readthedocs/subscriptions/signals.py

Lines 16 to 17 in 97ee9ba

	@receiver(pre_delete, sender=Subscription)
	def remove_stripe_subscription(sender, instance, using, **kwargs):

[humitos]

I'm not sure to understand these changes.

Subscription is set to cascade delete when an organization is deleted

If the Subscription is set to CASCADE, it won't trigger Django signals and then

readthedocs.org/readthedocs/subscriptions/signals.py

Lines 16 to 17 in 97ee9ba

	@receiver(pre_delete, sender=Subscription)
	def remove_stripe_subscription(sender, instance, using, **kwargs):

won't be called either. Am I correct?

I think we need to connect remove_stripe_subscription to pre_delete on Organization. Otherwise, it won't be executed.

If I'm not correct, can you please explain to me the flow happening when an organization is deleted by the user?

[stsewd]

If the Subscription is set to CASCADE, it won't trigger Django signals and then

It will, when you delete an org, all their related objects will be deleted as you manually called .delete() on each one, that's why tests are passing.

[humitos]

@stsewd

It will, when you delete an org, all their related objects will be deleted as you manually called .delete() on each one, that's why tests are passing.

I think you are confused about this, .delete() is not called. Take a look at the Django documentation https://docs.djangoproject.com/en/4.0/ref/models/fields/#django.db.models.CASCADE. It explicitly says that it does not call .delete()

However, the last sentence says,

but the pre_delete and post_delete signals are sent for all deleted objects

I understand that "for all deleted objects" only applies to the original objects, not the CASCADEd ones because Django does not really know what are those objects, so it can't trigger those signals for them.

I want to be 100% sure that we are understanding exactly how it works because I want to prevent any issues with the subscriptions that are hard to detect and affect directly our income and onboarding UX + support time, etc

[stsewd]

@humitos

When Django deletes an object, by default it emulates the behavior of the SQL constraint ON DELETE CASCADE – in other words, any objects which had foreign keys pointing at the object to be deleted will be deleted along with it. For example:

https://docs.djangoproject.com/en/4.0/topics/db/queries/#topics-db-queries-delete

In any way, we have tests for this at .com (se also #1436 on .com)

[humitos]

but the pre_delete and post_delete signals are sent for all deleted objects

I understand that "for all deleted objects" only applies to the original objects, not the CASCADEd ones because Django does not really know what are those objects, so it can't trigger those signals for them.

These signals are sent to all deleted objects, including the CASCADEd ones. I just confirmed that by doing a local test. This is the answer I was expecting from you with my original comment asking about how it's the flow of execution calls: #9341 (review)

---

I executed this code:

o = Organization.objects.create(slug='1')
s = Subscription.objects.create(organization=o, plan=Plan.objects.get(pk=3))
o.delete()

Without this PR's changes:

[info     ] Removing organization completely [readthedocs.organizations.signals] organization_slug=1
[error    ] Called remove_subscription     [readthedocs.subscriptions.signals] organization_slug=1
[error    ] Called remove_stripe_subscription [readthedocs.subscriptions.signals] organization_slug=1

With this PR's changes:

[error    ] Called remove_stripe_subscription [readthedocs.subscriptions.signals]
[info     ] Removing organization completely [readthedocs.organizations.signals] organization_slug=1

[humitos]

It would be good to update this PR to a mergeable state since we are still hitting this issue.

[humitos]

I tested this by myself to find out the answer to my question. Everything looks good, I think. Hopefully, we don't screw things related to subscriptions on production again 🙃