fix: Excessive 404s on subscription metadata (#2985)

api/organisations/exceptions.py

@@ -1,11 +1,6 @@
 from rest_framework.exceptions import APIException
 
 
-class OrganisationHasNoSubscription(APIException):
+class OrganisationHasNoPaidSubscription(APIException):
     status_code = 400
     default_detail = "Organisation has no subscription"
-
-
-class SubscriptionNotFound(APIException):
-    status_code = 404
-    default_detail = "Subscription Not found"

api/organisations/migrations/0048_add_default_subscription_to_orphaned_organisations.py

@@ -0,0 +1,32 @@
+# Generated by Django 3.2.23 on 2023-11-16 16:01
+
+from django.db import migrations
+
+
+def create_default_subscription(apps, schema_editor):
+    Organisation = apps.get_model("organisations", "Organisation")
+    Subscription = apps.get_model("organisations", "Subscription")
+
+    organisations_without_subscription = Organisation.objects.filter(
+        subscription__isnull=True
+    )
+
+    subscriptions_to_create = []
+    for organisation in organisations_without_subscription:
+        subscriptions_to_create.append(Subscription(organisation=organisation))
+
+    Subscription.objects.bulk_create(subscriptions_to_create)
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('organisations', '0047_organisation_force_2fa'),
+    ]
+
+    operations = [
+        migrations.RunPython(
+            create_default_subscription,
+            reverse_code=migrations.RunPython.noop,
+        )
+    ]

api/organisations/models.py

@@ -94,7 +94,9 @@ def get_unique_slug(self):
     def num_seats(self):
         return self.users.count()
 
-    def has_subscription(self) -> bool:
+    def has_paid_subscription(self) -> bool:
+        # Includes subscriptions that are canceled.
+        # See is_paid for active paid subscriptions only.
         return hasattr(self, "subscription") and bool(self.subscription.subscription_id)
 
     def has_subscription_information_cache(self) -> bool:
@@ -104,10 +106,12 @@ def has_subscription_information_cache(self) -> bool:
 
     @property
     def is_paid(self):
-        return self.has_subscription() and self.subscription.cancellation_date is None
+        return (
+            self.has_paid_subscription() and self.subscription.cancellation_date is None
+        )
 
     def over_plan_seats_limit(self, additional_seats: int = 0):
-        if self.has_subscription():
+        if self.has_paid_subscription():
             susbcription_metadata = self.subscription.get_subscription_metadata()
             return self.num_seats + additional_seats > susbcription_metadata.seats
 
@@ -127,7 +131,7 @@ def is_auto_seat_upgrade_available(self) -> bool:
 
     @hook(BEFORE_DELETE)
     def cancel_subscription(self):
-        if self.has_subscription():
+        if self.has_paid_subscription():
             self.subscription.cancel()
 
     @hook(AFTER_CREATE)
@@ -186,6 +190,8 @@ class Meta:
 
 
 class Subscription(LifecycleModelMixin, SoftDeleteExportableModel):
+    # Even though it is not enforced at the database level,
+    # every organisation has a subscription.
     organisation = models.OneToOneField(
         Organisation, on_delete=models.CASCADE, related_name="subscription"
     )

api/organisations/tests/test_models.py

@@ -40,7 +40,7 @@ def test_can_create_organisation_with_and_without_webhook_notification_email(sel
         self.assertTrue(organisation_1.name)
         self.assertTrue(organisation_2.name)
 
-    def test_has_subscription_true(self):
+    def test_has_paid_subscription_true(self):
         # Given
         organisation = Organisation.objects.create(name="Test org")
         Subscription.objects.filter(organisation=organisation).update(
@@ -51,14 +51,14 @@ def test_has_subscription_true(self):
         organisation.refresh_from_db()
 
         # Then
-        assert organisation.has_subscription()
+        assert organisation.has_paid_subscription()
 
-    def test_has_subscription_missing_subscription_id(self):
+    def test_has_paid_subscription_missing_subscription_id(self):
         # Given
         organisation = Organisation.objects.create(name="Test org")
 
         # Then
-        assert not organisation.has_subscription()
+        assert not organisation.has_paid_subscription()
 
     @mock.patch("organisations.models.cancel_chargebee_subscription")
     def test_cancel_subscription_cancels_chargebee_subscription(

api/organisations/views.py

@@ -19,10 +19,7 @@
 from rest_framework.response import Response
 from rest_framework.throttling import ScopedRateThrottle
 
-from organisations.exceptions import (
-    OrganisationHasNoSubscription,
-    SubscriptionNotFound,
-)
+from organisations.exceptions import OrganisationHasNoPaidSubscription
 from organisations.models import (
     Organisation,
     OrganisationRole,
@@ -183,19 +180,15 @@ def update_subscription(self, request, pk):
     )
     def get_subscription_metadata(self, request, pk):
         organisation = self.get_object()
-        if not organisation.has_subscription():
-            raise SubscriptionNotFound()
-
         subscription_details = organisation.subscription.get_subscription_metadata()
         serializer = self.get_serializer(instance=subscription_details)
-
         return Response(serializer.data)
 
     @action(detail=True, methods=["GET"], url_path="portal-url")
     def get_portal_url(self, request, pk):
         organisation = self.get_object()
-        if not organisation.has_subscription():
-            raise OrganisationHasNoSubscription()
+        if not organisation.has_paid_subscription():
+            raise OrganisationHasNoPaidSubscription()
         redirect_url = get_current_site(request)
         serializer = self.get_serializer(
             data={"url": organisation.subscription.get_portal_url(redirect_url)}
@@ -210,8 +203,8 @@ def get_portal_url(self, request, pk):
     )
     def get_hosted_page_url_for_subscription_upgrade(self, request, pk):
         organisation = self.get_object()
-        if not organisation.has_subscription():
-            raise OrganisationHasNoSubscription()
+        if not organisation.has_paid_subscription():
+            raise OrganisationHasNoPaidSubscription()
         serializer = self.get_serializer(
             data={
                 "subscription_id": organisation.subscription.subscription_id,

api/sales_dashboard/templates/sales_dashboard/home.html

@@ -75,7 +75,7 @@ <h1 class="h2">Organisations</h1>
           <tbody>
             {% for org in object_list %}
             <tr class="
-              {% if org.has_subscription and not org.is_paid %}table-danger
+              {% if org.has_paid_subscription and not org.is_paid %}table-danger
               {% elif org.num_users > org.subscription_information_cache.allowed_seats %}table-warning
               {% elif org.subscription_information_cache.api_calls_30d > org.subscription_information_cache.allowed_30d_api_calls %}table-warning
               {% endif %}"

api/sales_dashboard/templates/sales_dashboard/organisation.html

@@ -12,7 +12,7 @@
                 <h1 class="h2 float-left">
                     Organisation: <strong>{{organisation.name}}</strong>
                     - Plan: <strong>{{ organisation.subscription.plan|default:"Free"}}</strong>
-                    {% if organisation.has_subscription and not organisation.is_paid %}<span class="badge badge-danger">Subscription Cancelled</button>{% endif %} 
+                    {% if organisation.has_paid_subscription and not organisation.is_paid %}<span class="badge badge-danger">Subscription Cancelled</button>{% endif %}
                 </h1>
                 <div class="float-right"><a href="/admin/organisations/organisation/{{organisation.id}}/change">Django Admin</a></div>
                 <div class="float-right">

api/tests/unit/organisations/test_unit_organisation_views.py

@@ -12,6 +12,7 @@
 from django.db.models import Model
 from django.urls import reverse
 from freezegun import freeze_time
+from pytest_mock import MockerFixture
 from pytz import UTC
 from rest_framework import status
 from rest_framework.test import APIClient, override_settings
@@ -433,15 +434,15 @@ def test_update_subscription_gets_subscription_data_from_chargebee(
         # Since subscription is created using organisation.id rather than
         # organisation and we already have evaluated subscription(by add_organisation)
         # attribute of organisation(before we created the subscription) we need to
-        # refresh organisation for `has_subscription` to work properly
+        # refresh organisation for `has_paid_subscription` to work properly
         organisation.refresh_from_db()
 
         # and
         mock_get_subscription_data.assert_called_with(hosted_page_id=hosted_page_id)
 
         # and
         assert (
-            organisation.has_subscription()
+            organisation.has_paid_subscription()
             and organisation.subscription.subscription_id == subscription_id
             and organisation.subscription.customer_id == customer_id
         )
@@ -904,14 +905,14 @@ def test_get_subscription_metadata_when_subscription_information_cache_does_not_
     )
 
 
-def test_get_subscription_metadata_returns_404_if_the_organisation_have_no_subscription(
-    mocker, organisation, admin_client
-):
+def test_get_subscription_metadata_returns_200_if_the_organisation_have_no_paid_subscription(
+    mocker: MockerFixture, organisation: Organisation, admin_client: APIClient
+) -> None:
     # Given
     get_subscription_metadata = mocker.patch(
         "organisations.models.get_subscription_metadata_from_id"
     )
-
+    assert organisation.subscription.subscription_id is None
     url = reverse(
         "api-v1:organisations:organisation-get-subscription-metadata",
         args=[organisation.pk],
@@ -921,7 +922,15 @@ def test_get_subscription_metadata_returns_404_if_the_organisation_have_no_subsc
     response = admin_client.get(url)
 
     # Then
-    assert response.status_code == status.HTTP_404_NOT_FOUND
+    assert response.status_code == status.HTTP_200_OK
+    assert response.data == {
+        "chargebee_email": None,
+        "max_api_calls": 50000,
+        "max_projects": 1,
+        "max_seats": 1,
+        "payment_source": None,
+    }
+
     get_subscription_metadata.assert_not_called()