From ae94267bd11b346baa1466bf3d3c2ce38b2935dc Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 20 Dec 2023 17:55:13 +0000 Subject: [PATCH] fix(subscriptions): ensure that manually added subscriptions work correctly in all deployments (#3182) --- .github/actions/api-deploy-ecs/action.yml | 5 + api/app/utils.py | 11 +- api/conftest.py | 3 +- api/organisations/chargebee/chargebee.py | 2 +- api/organisations/models.py | 71 +++++--- api/organisations/subscriptions/constants.py | 1 + api/sales_dashboard/forms.py | 3 +- api/tests/unit/app/test_unit_app_utils.py | 8 + api/tests/unit/app/test_unit_app_views.py | 1 + .../test_unit_organisations_models.py | 158 +++++++++++++++--- .../test_unit_organisations_views.py | 14 +- 11 files changed, 224 insertions(+), 53 deletions(-) diff --git a/.github/actions/api-deploy-ecs/action.yml b/.github/actions/api-deploy-ecs/action.yml index 4e8c59dc61c1..e60692bb54f6 100644 --- a/.github/actions/api-deploy-ecs/action.yml +++ b/.github/actions/api-deploy-ecs/action.yml @@ -133,6 +133,11 @@ runs: echo ${{ github.sha }} > api/CI_COMMIT_SHA shell: bash + - name: Write file to indicate SaaS + run: | + echo '' > api/SAAS_DEPLOYMENT + shell: bash + - name: Configure AWS Credentials uses: aws-actions/configure-aws-credentials@v1-node16 with: diff --git a/api/app/utils.py b/api/app/utils.py index 9611bc331aca..a843cc6ce946 100644 --- a/api/app/utils.py +++ b/api/app/utils.py @@ -12,6 +12,14 @@ def create_hash(): return shortuuid.uuid() +def is_enterprise(): + return pathlib.Path("./ENTERPRISE_VERSION").exists() + + +def is_saas(): + return pathlib.Path("./SAAS_DEPLOYMENT").exists() + + def get_version_info() -> dict: """Reads the version info baked into src folder of the docker container""" version_json = {} @@ -27,7 +35,8 @@ def get_version_info() -> dict: version_json = version_json | { "ci_commit_sha": _get_file_contents("./CI_COMMIT_SHA"), "image_tag": image_tag, - "is_enterprise": pathlib.Path("./ENTERPRISE_VERSION").exists(), + "is_enterprise": is_enterprise(), + "is_saas": is_saas(), } return version_json diff --git a/api/conftest.py b/api/conftest.py index 24651534dc31..dd3c25ea94de 100644 --- a/api/conftest.py +++ b/api/conftest.py @@ -153,10 +153,11 @@ def xero_subscription(organisation): @pytest.fixture() -def chargebee_subscription(organisation): +def chargebee_subscription(organisation: Organisation) -> Subscription: subscription = Subscription.objects.get(organisation=organisation) subscription.payment_method = CHARGEBEE subscription.subscription_id = "subscription-id" + subscription.plan = "scale-up-v2" subscription.save() # refresh organisation to load subscription diff --git a/api/organisations/chargebee/chargebee.py b/api/organisations/chargebee/chargebee.py index 4444454e4896..faaf9b61a880 100644 --- a/api/organisations/chargebee/chargebee.py +++ b/api/organisations/chargebee/chargebee.py @@ -4,7 +4,7 @@ from datetime import datetime import chargebee -from chargebee import APIError as ChargebeeAPIError +from chargebee.api_error import APIError as ChargebeeAPIError from django.conf import settings from pytz import UTC diff --git a/api/organisations/models.py b/api/organisations/models.py index 7149826567f9..510a49ca320e 100644 --- a/api/organisations/models.py +++ b/api/organisations/models.py @@ -15,6 +15,7 @@ ) from simple_history.models import HistoricalRecords +from app.utils import is_enterprise, is_saas from organisations.chargebee import ( get_customer_id_from_subscription_id, get_max_api_calls_for_plan, @@ -31,10 +32,12 @@ from organisations.subscriptions.constants import ( CHARGEBEE, FREE_PLAN_ID, + FREE_PLAN_SUBSCRIPTION_METADATA, MAX_API_CALLS_IN_FREE_PLAN, MAX_SEATS_IN_FREE_PLAN, SUBSCRIPTION_BILLING_STATUSES, SUBSCRIPTION_PAYMENT_METHODS, + TRIAL_SUBSCRIPTION_ID, XERO, ) from organisations.subscriptions.exceptions import ( @@ -45,8 +48,6 @@ from users.utils.mailer_lite import MailerLite from webhooks.models import AbstractBaseExportableWebhookModel -TRIAL_SUBSCRIPTION_ID = "trial" - environment_cache = caches[settings.ENVIRONMENT_CACHE_NAME] @@ -234,6 +235,10 @@ def update_plan(self, plan_id): def can_auto_upgrade_seats(self) -> bool: return self.plan in settings.AUTO_SEAT_UPGRADE_PLANS + @property + def is_free_plan(self) -> bool: + return self.plan == FREE_PLAN_ID + @hook(AFTER_SAVE, when="cancellation_date", has_changed=True) @hook(AFTER_SAVE, when="subscription_id", has_changed=True) def update_mailer_lite_subscribers(self): @@ -311,33 +316,55 @@ def get_portal_url(self, redirect_url): return get_portal_url(self.customer_id, redirect_url) def get_subscription_metadata(self) -> BaseSubscriptionMetadata: - metadata = None + if self.is_free_plan: + # Free plan is the default everywhere, we should prevent + # increased access for all deployment types on the free + # plan. + return FREE_PLAN_SUBSCRIPTION_METADATA - if self.is_in_trial(): - metadata = BaseSubscriptionMetadata( - seats=self.max_seats, api_calls=self.max_api_calls - ) - elif self.payment_method == CHARGEBEE and self.subscription_id: - if self.organisation.has_subscription_information_cache(): - # Getting the data from the subscription information cache because - # data is guaranteed to be up to date by using a Chargebee webhook. - metadata = ChargebeeObjMetadata( - seats=self.organisation.subscription_information_cache.allowed_seats, - api_calls=self.organisation.subscription_information_cache.allowed_30d_api_calls, - projects=self.organisation.subscription_information_cache.allowed_projects, - chargebee_email=self.organisation.subscription_information_cache.chargebee_email, - ) - else: - metadata = get_subscription_metadata_from_id(self.subscription_id) + return ( + self._get_subscription_metadata_for_saas() + if is_saas() + else self._get_subscription_metadata_for_self_hosted() + ) + + def _get_subscription_metadata_for_saas(self) -> BaseSubscriptionMetadata: + if self.payment_method == CHARGEBEE and self.subscription_id: + return self._get_subscription_metadata_for_chargebee() elif self.payment_method == XERO and self.subscription_id: - metadata = XeroSubscriptionMetadata( + return XeroSubscriptionMetadata( seats=self.max_seats, api_calls=self.max_api_calls ) - return metadata or BaseSubscriptionMetadata( + # Default fall through here means this is a manually added subscription + # or for a payment method that is not covered above. In this situation + # we want the response to be what is stored in the Django database. + # Note that Free plans are caught in the parent method above. + return BaseSubscriptionMetadata( + seats=self.max_seats, api_calls=self.max_api_calls + ) + + def _get_subscription_metadata_for_chargebee(self) -> ChargebeeObjMetadata: + if self.organisation.has_subscription_information_cache(): + # Getting the data from the subscription information cache because + # data is guaranteed to be up to date by using a Chargebee webhook. + return ChargebeeObjMetadata( + seats=self.organisation.subscription_information_cache.allowed_seats, + api_calls=self.organisation.subscription_information_cache.allowed_30d_api_calls, + projects=self.organisation.subscription_information_cache.allowed_projects, + chargebee_email=self.organisation.subscription_information_cache.chargebee_email, + ) + + return get_subscription_metadata_from_id(self.subscription_id) + + def _get_subscription_metadata_for_self_hosted(self) -> BaseSubscriptionMetadata: + if not is_enterprise(): + return FREE_PLAN_SUBSCRIPTION_METADATA + + return BaseSubscriptionMetadata( seats=self.max_seats, api_calls=self.max_api_calls, - projects=settings.MAX_PROJECTS_IN_FREE_PLAN, + projects=None, ) def add_single_seat(self): diff --git a/api/organisations/subscriptions/constants.py b/api/organisations/subscriptions/constants.py index 204ca6c488eb..690120ec262c 100644 --- a/api/organisations/subscriptions/constants.py +++ b/api/organisations/subscriptions/constants.py @@ -39,6 +39,7 @@ projects=settings.MAX_PROJECTS_IN_FREE_PLAN, ) FREE_PLAN_ID = "free" +TRIAL_SUBSCRIPTION_ID = "trial" class SubscriptionCacheEntity(Enum): diff --git a/api/sales_dashboard/forms.py b/api/sales_dashboard/forms.py index b14b2bcbf2b7..4314866f7c90 100644 --- a/api/sales_dashboard/forms.py +++ b/api/sales_dashboard/forms.py @@ -7,11 +7,12 @@ from environments.models import Environment from features.models import Feature -from organisations.models import TRIAL_SUBSCRIPTION_ID, Organisation +from organisations.models import Organisation from organisations.subscriptions.constants import ( FREE_PLAN_ID, MAX_API_CALLS_IN_FREE_PLAN, MAX_SEATS_IN_FREE_PLAN, + TRIAL_SUBSCRIPTION_ID, ) from projects.models import Project from users.models import FFAdminUser diff --git a/api/tests/unit/app/test_unit_app_utils.py b/api/tests/unit/app/test_unit_app_utils.py index ef58d2baa96f..6fd992e91680 100644 --- a/api/tests/unit/app/test_unit_app_utils.py +++ b/api/tests/unit/app/test_unit_app_utils.py @@ -17,6 +17,9 @@ def path_side_effect(file_path: str) -> mocker.MagicMock: if file_path == "./ENTERPRISE_VERSION": mocked_path_object.exists.return_value = True + if file_path == "./SAAS_DEPLOYMENT": + mocked_path_object.exists.return_value = False + return mocked_path_object mocked_pathlib.Path.side_effect = path_side_effect @@ -35,6 +38,7 @@ def path_side_effect(file_path: str) -> mocker.MagicMock: "ci_commit_sha": "some_sha", "image_tag": "2.66.2", "is_enterprise": True, + "is_saas": False, "package_versions": {".": "2.66.2"}, } @@ -49,6 +53,9 @@ def path_side_effect(file_path: str) -> mocker.MagicMock: if file_path == "./ENTERPRISE_VERSION": mocked_path_object.exists.return_value = True + if file_path == "./SAAS_DEPLOYMENT": + mocked_path_object.exists.return_value = False + return mocked_path_object mocked_pathlib.Path.side_effect = path_side_effect @@ -62,4 +69,5 @@ def path_side_effect(file_path: str) -> mocker.MagicMock: "ci_commit_sha": "unknown", "image_tag": "unknown", "is_enterprise": True, + "is_saas": False, } diff --git a/api/tests/unit/app/test_unit_app_views.py b/api/tests/unit/app/test_unit_app_views.py index f59463ceca0c..fce9bae82f4f 100644 --- a/api/tests/unit/app/test_unit_app_views.py +++ b/api/tests/unit/app/test_unit_app_views.py @@ -16,4 +16,5 @@ def test_get_version_info(api_client: APIClient) -> None: "ci_commit_sha": "unknown", "image_tag": "unknown", "is_enterprise": False, + "is_saas": False, } diff --git a/api/tests/unit/organisations/test_unit_organisations_models.py b/api/tests/unit/organisations/test_unit_organisations_models.py index 8a498bd1b4b7..41e4e9ac0b96 100644 --- a/api/tests/unit/organisations/test_unit_organisations_models.py +++ b/api/tests/unit/organisations/test_unit_organisations_models.py @@ -2,6 +2,7 @@ from unittest import mock import pytest +from django.conf import settings from django.test import TestCase from pytest_mock import MockerFixture from rest_framework.test import override_settings @@ -9,7 +10,6 @@ from environments.models import Environment from organisations.chargebee.metadata import ChargebeeObjMetadata from organisations.models import ( - TRIAL_SUBSCRIPTION_ID, Organisation, OrganisationRole, OrganisationSubscriptionInformationCache, @@ -19,6 +19,9 @@ CHARGEBEE, FREE_PLAN_ID, FREE_PLAN_SUBSCRIPTION_METADATA, + MAX_API_CALLS_IN_FREE_PLAN, + MAX_SEATS_IN_FREE_PLAN, + TRIAL_SUBSCRIPTION_ID, XERO, ) from organisations.subscriptions.exceptions import ( @@ -289,42 +292,48 @@ def test_organisation_is_paid_returns_false_if_cancelled_subscription_exists( def test_subscription_get_subscription_metadata_returns_cb_metadata_for_cb_subscription( - organisation, - mocker, + organisation: Organisation, + mocker: MockerFixture, ): # Given seats = 10 api_calls = 50000000 + projects = 10 OrganisationSubscriptionInformationCache.objects.create( - organisation=organisation, allowed_seats=seats, allowed_30d_api_calls=api_calls + organisation=organisation, + allowed_seats=seats, + allowed_30d_api_calls=api_calls, + allowed_projects=projects, ) - expected_metadata = ChargebeeObjMetadata( - seats=seats, api_calls=api_calls, projects=10 + seats=seats, api_calls=api_calls, projects=projects ) - mock_cb_get_subscription_metadata = mocker.patch( - "organisations.models.Subscription.get_subscription_metadata" + mocker.patch("organisations.models.is_saas", return_value=True) + Subscription.objects.filter(organisation=organisation).update( + plan="scale-up-v2", + subscription_id="subscription-id", + payment_method=CHARGEBEE, ) - mock_cb_get_subscription_metadata.return_value = expected_metadata + organisation.subscription.refresh_from_db() # When subscription_metadata = organisation.subscription.get_subscription_metadata() # Then - mock_cb_get_subscription_metadata.assert_called_once_with() - assert subscription_metadata == expected_metadata -def test_subscription_get_subscription_metadata_returns_xero_metadata_for_xero_sub(): +def test_subscription_get_subscription_metadata_returns_xero_metadata_for_xero_sub( + mocker: MockerFixture, +): # Given subscription = Subscription( - payment_method=XERO, subscription_id="xero-subscription" + payment_method=XERO, subscription_id="xero-subscription", plan="enterprise" ) - expected_metadata = XeroSubscriptionMetadata( seats=subscription.max_seats, api_calls=subscription.max_api_calls ) + mocker.patch("organisations.models.is_saas", return_value=True) # When subscription_metadata = subscription.get_subscription_metadata() @@ -344,24 +353,125 @@ def test_subscription_get_subscription_metadata_returns_free_plan_metadata_for_n assert subscription_metadata == FREE_PLAN_SUBSCRIPTION_METADATA -def test_subscription_get_subscription_metadata_for_trial(): +@pytest.mark.parametrize( + "subscription_id, plan, max_seats, expected_seats, expected_projects", + ( + ( + None, + "free", + 10, + MAX_SEATS_IN_FREE_PLAN, + settings.MAX_PROJECTS_IN_FREE_PLAN, + ), + ("anything", "enterprise", 20, 20, None), + (TRIAL_SUBSCRIPTION_ID, "enterprise", 20, 20, None), + ), +) +def test_get_subscription_metadata_for_enterprise_self_hosted_licenses( + organisation: Organisation, + subscription_id: str | None, + plan: str, + max_seats: int, + expected_seats: int, + expected_projects: int | None, + mocker: MockerFixture, +) -> None: + """ + Specific test to make sure that we can manually add subscriptions to + enterprise self-hosted deployments and the values stored in the django + database will be correctly used. + """ # Given - max_seats = 10 - max_api_calls = 1000000 - subscription = Subscription( - subscription_id=TRIAL_SUBSCRIPTION_ID, + Subscription.objects.filter(organisation=organisation).update( + subscription_id=subscription_id, plan=plan, max_seats=max_seats + ) + organisation.subscription.refresh_from_db() + mocker.patch("organisations.models.is_saas", return_value=False) + mocker.patch("organisations.models.is_enterprise", return_value=True) + + # When + subscription_metadata = organisation.subscription.get_subscription_metadata() + + # Then + assert subscription_metadata.projects == expected_projects + assert subscription_metadata.seats == expected_seats + + +@pytest.mark.parametrize( + "subscription_id, plan, max_seats, max_api_calls, expected_seats, " + "expected_api_calls, expected_projects", + ( + ( + None, + "free", + 10, + 5000000, + MAX_SEATS_IN_FREE_PLAN, + MAX_API_CALLS_IN_FREE_PLAN, + settings.MAX_PROJECTS_IN_FREE_PLAN, + ), + ("anything", "enterprise", 20, 5000000, 20, 5000000, None), + (TRIAL_SUBSCRIPTION_ID, "enterprise", 20, 5000000, 20, 5000000, None), + ), +) +def test_get_subscription_metadata_for_manually_added_enterprise_saas_licenses( + organisation: Organisation, + subscription_id: str | None, + plan: str, + max_seats: int, + max_api_calls: int, + expected_seats: int, + expected_api_calls: int, + expected_projects: int | None, + mocker: MockerFixture, +) -> None: + """ + Specific test to make sure that we can manually add subscriptions to + the SaaS platform and the values stored in the Django database will + be correctly used. + """ + # Given + Subscription.objects.filter(organisation=organisation).update( + subscription_id=subscription_id, + plan=plan, max_seats=max_seats, max_api_calls=max_api_calls, - payment_method=None, ) + organisation.subscription.refresh_from_db() + mocker.patch("organisations.models.is_saas", return_value=True) # When - subscription_metadata = subscription.get_subscription_metadata() + subscription_metadata = organisation.subscription.get_subscription_metadata() # Then - assert subscription_metadata.seats == max_seats - assert subscription_metadata.api_calls == max_api_calls - assert subscription_metadata.projects is None + assert subscription_metadata.projects == expected_projects + assert subscription_metadata.seats == expected_seats + assert subscription_metadata.api_calls == expected_api_calls + + +def test_get_subscription_metadata_for_self_hosted_open_source( + organisation: Organisation, mocker: MockerFixture +) -> None: + """ + Open source should ignore the details provided in the + subscription and always return the free plan metadata. + """ + # Given + Subscription.objects.filter(organisation=organisation).update( + max_seats=100, + max_api_calls=10000000000, + plan="enterprise", + subscription_id="subscription-id", + ) + organisation.subscription.refresh_from_db() + mocker.patch("organisations.models.is_enterprise", return_value=False) + mocker.patch("organisations.models.is_saas", return_value=False) + + # When + subscription_metadata = organisation.subscription.get_subscription_metadata() + + # Then + assert subscription_metadata == FREE_PLAN_SUBSCRIPTION_METADATA def test_subscription_add_single_seat_calls_correct_chargebee_method_for_upgradable_plan( diff --git a/api/tests/unit/organisations/test_unit_organisations_views.py b/api/tests/unit/organisations/test_unit_organisations_views.py index ef048900f589..def822ac1603 100644 --- a/api/tests/unit/organisations/test_unit_organisations_views.py +++ b/api/tests/unit/organisations/test_unit_organisations_views.py @@ -840,11 +840,13 @@ def test_trigger_sample_webhook_calls_trigger_sample_webhook_method_with_correct def test_get_subscription_metadata_when_subscription_information_cache_exist( - organisation, admin_client, chargebee_subscription + organisation: Organisation, + admin_client: APIClient, + chargebee_subscription: Subscription, + mocker: MagicMock, ): # Given expected_seats = 10 - expected_projects = 5 expected_projects = 3 expected_api_calls = 100 expected_chargebee_email = "test@example.com" @@ -862,6 +864,8 @@ def test_get_subscription_metadata_when_subscription_information_cache_exist( args=[organisation.pk], ) + mocker.patch("organisations.models.is_saas", return_value=True) + # When response = admin_client.get(url) @@ -877,7 +881,10 @@ def test_get_subscription_metadata_when_subscription_information_cache_exist( def test_get_subscription_metadata_when_subscription_information_cache_does_not_exist( - mocker, organisation, admin_client, chargebee_subscription + mocker: MockerFixture, + organisation: Organisation, + admin_client: APIClient, + chargebee_subscription: Subscription, ): # Given expected_seats = 10 @@ -885,6 +892,7 @@ def test_get_subscription_metadata_when_subscription_information_cache_does_not_ expected_api_calls = 100 expected_chargebee_email = "test@example.com" + mocker.patch("organisations.models.is_saas", return_value=True) get_subscription_metadata = mocker.patch( "organisations.models.get_subscription_metadata_from_id", return_value=ChargebeeObjMetadata(