From 627a6fa0b3d3609ac6e12aae7f9b71220f2567af Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Thu, 16 Nov 2023 14:46:30 -0500 Subject: [PATCH] fix: Excessive 404s on subscription metadata (#2985) --- api/organisations/exceptions.py | 7 +--- ..._subscription_to_orphaned_organisations.py | 32 +++++++++++++++++++ api/organisations/models.py | 14 +++++--- api/organisations/tests/test_models.py | 8 ++--- api/organisations/views.py | 17 +++------- .../templates/sales_dashboard/home.html | 2 +- .../sales_dashboard/organisation.html | 2 +- .../test_unit_organisation_views.py | 23 +++++++++---- 8 files changed, 70 insertions(+), 35 deletions(-) create mode 100644 api/organisations/migrations/0048_add_default_subscription_to_orphaned_organisations.py diff --git a/api/organisations/exceptions.py b/api/organisations/exceptions.py index f447e2d739a0..3c401516d16c 100644 --- a/api/organisations/exceptions.py +++ b/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" diff --git a/api/organisations/migrations/0048_add_default_subscription_to_orphaned_organisations.py b/api/organisations/migrations/0048_add_default_subscription_to_orphaned_organisations.py new file mode 100644 index 000000000000..de2471360221 --- /dev/null +++ b/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, + ) + ] diff --git a/api/organisations/models.py b/api/organisations/models.py index aaec83e2eef0..349dbdee3116 100644 --- a/api/organisations/models.py +++ b/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" ) diff --git a/api/organisations/tests/test_models.py b/api/organisations/tests/test_models.py index c525c29c4d7b..5f938fb6092f 100644 --- a/api/organisations/tests/test_models.py +++ b/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( diff --git a/api/organisations/views.py b/api/organisations/views.py index 517072fda6d9..40842fde7726 100644 --- a/api/organisations/views.py +++ b/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, diff --git a/api/sales_dashboard/templates/sales_dashboard/home.html b/api/sales_dashboard/templates/sales_dashboard/home.html index 24a37f5a991d..35c44627b3ce 100644 --- a/api/sales_dashboard/templates/sales_dashboard/home.html +++ b/api/sales_dashboard/templates/sales_dashboard/home.html @@ -75,7 +75,7 @@

Organisations

{% for org in object_list %} Organisation: {{organisation.name}} - Plan: {{ organisation.subscription.plan|default:"Free"}} - {% if organisation.has_subscription and not organisation.is_paid %}Subscription Cancelled{% endif %} + {% if organisation.has_paid_subscription and not organisation.is_paid %}Subscription Cancelled{% endif %}
Django Admin
diff --git a/api/tests/unit/organisations/test_unit_organisation_views.py b/api/tests/unit/organisations/test_unit_organisation_views.py index 1dcea8cc96db..866af46d481d 100644 --- a/api/tests/unit/organisations/test_unit_organisation_views.py +++ b/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,7 +434,7 @@ 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 @@ -441,7 +442,7 @@ def test_update_subscription_gets_subscription_data_from_chargebee( # 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()