From 76b0b88a4e2a7518373373d975bcf9e87cff934d Mon Sep 17 00:00:00 2001 From: James Kiger Date: Thu, 21 Nov 2024 10:51:05 -0500 Subject: [PATCH 1/7] Update is_mmo method to check new stripe metadata fields, add unit test, adjust existing tests --- kobo/apps/organizations/models.py | 9 ++- .../stripe/tests/test_organization_usage.py | 56 +++++++++++++------ kobo/apps/stripe/tests/utils.py | 22 ++++---- kpi/tests/test_usage_calculator.py | 4 +- 4 files changed, 60 insertions(+), 31 deletions(-) diff --git a/kobo/apps/organizations/models.py b/kobo/apps/organizations/models.py index a51d2662af..c2c74cca59 100644 --- a/kobo/apps/organizations/models.py +++ b/kobo/apps/organizations/models.py @@ -179,7 +179,14 @@ def is_mmo(self): If the override is enabled, it takes precedence over the subscription status """ - return self.mmo_override or bool(self.active_subscription_billing_details()) + if self.mmo_override: + return True + + if billing_details := self.active_subscription_billing_details(): + if product_metadata := billing_details.get('product_metadata'): + return product_metadata.get('mmo_enabled') == 'true' + + return False @cache_for_request def is_admin_only(self, user: 'User') -> bool: diff --git a/kobo/apps/stripe/tests/test_organization_usage.py b/kobo/apps/stripe/tests/test_organization_usage.py index e07773be59..a7f3c20a23 100644 --- a/kobo/apps/stripe/tests/test_organization_usage.py +++ b/kobo/apps/stripe/tests/test_organization_usage.py @@ -23,7 +23,7 @@ from kobo.apps.organizations.models import Organization, OrganizationUser from kobo.apps.stripe.constants import USAGE_LIMIT_MAP from kobo.apps.stripe.tests.utils import ( - generate_enterprise_subscription, + generate_mmo_subscription, generate_plan_subscription, ) from kobo.apps.stripe.utils import get_organization_plan_limit @@ -107,7 +107,7 @@ def test_usage_for_plans_with_org_access(self): when viewing /service_usage/{organization_id}/ """ - generate_enterprise_subscription(self.organization) + generate_mmo_subscription(self.organization) # the user should see usage for everyone in their org response = self.client.get(self.detail_url) @@ -138,7 +138,7 @@ def test_endpoint_speed(self): # get the average request time for 10 hits to the endpoint single_user_time = timeit.timeit(lambda: self.client.get(self.detail_url), number=10) - generate_enterprise_subscription(self.organization) + generate_mmo_subscription(self.organization) # get the average request time for 10 hits to the endpoint multi_user_time = timeit.timeit(lambda: self.client.get(self.detail_url), number=10) @@ -151,7 +151,7 @@ def test_endpoint_is_cached(self): """ Test that multiple hits to the endpoint from the same origin are properly cached """ - generate_enterprise_subscription(self.organization) + generate_mmo_subscription(self.organization) first_response = self.client.get(self.detail_url) assert first_response.data['total_submission_count']['current_month'] == self.expected_submissions_multi @@ -438,7 +438,7 @@ def test_user_not_member_of_organization(self): assert response.status_code == status.HTTP_400_BAD_REQUEST def test_successful_retrieval(self): - generate_enterprise_subscription(self.organization) + generate_mmo_subscription(self.organization) create_mock_assets([self.anotheruser]) response = self.client.get(self.detail_url) assert response.status_code == status.HTTP_200_OK @@ -446,8 +446,8 @@ def test_successful_retrieval(self): assert response.data['results'][0]['asset__name'] == 'test' assert response.data['results'][0]['deployment_status'] == 'deployed' - def test_aggregates_usage_for_enterprise_org(self): - generate_enterprise_subscription(self.organization) + def test_aggregates_usage_for_mmo(self): + generate_mmo_subscription(self.organization) self.organization.add_user(self.newuser) # create 2 additional assets, one per user create_mock_assets([self.anotheruser, self.newuser]) @@ -455,14 +455,6 @@ def test_aggregates_usage_for_enterprise_org(self): assert response.status_code == status.HTTP_200_OK assert response.data['count'] == 2 - def test_users_without_enterprise_see_only_their_usage(self): - generate_plan_subscription(self.organization) - self.organization.add_user(self.newuser) - create_mock_assets([self.anotheruser, self.newuser]) - response = self.client.get(self.detail_url) - assert response.status_code == status.HTTP_200_OK - assert response.data['count'] == 1 - @ddt class OrganizationsUtilsTestCase(BaseTestCase): @@ -478,7 +470,7 @@ def setUp(self): self.organization.add_user(self.anotheruser, is_admin=True) def test_get_plan_community_limit(self): - generate_enterprise_subscription(self.organization) + generate_mmo_subscription(self.organization) limit = get_organization_plan_limit(self.organization, 'seconds') assert limit == 2000 # TODO get the limits from the community plan, overrides limit = get_organization_plan_limit(self.organization, 'characters') @@ -509,3 +501,35 @@ def test_get_suscription_limit_unlimited(self, usage_type): generate_plan_subscription(self.organization, metadata=product_metadata) limit = get_organization_plan_limit(self.organization, usage_type) assert limit == float('inf') + + +class OrganizationsModelIntegrationTestCase(BaseTestCase): + fixtures = ['test_data'] + + def setUp(self): + self.someuser = User.objects.get(username='someuser') + self.organization = self.someuser.organization + + def test_is_mmo_subscription_logic(self): + self.organization.mmo_override = True + self.organization.save() + assert self.organization.is_mmo is True + + self.organization.mmo_override = False + self.organization.save() + assert self.organization.is_mmo is False + + product_metadata = { + 'mmo_enabled': 'false', + } + subscription = generate_plan_subscription( + self.organization, metadata=product_metadata + ) + assert self.organization.is_mmo is False + subscription.status = 'canceled' + subscription.ended_at = timezone.now() + subscription.save() + + product_metadata['mmo_enabled'] = 'true' + generate_plan_subscription(self.organization, metadata=product_metadata) + assert self.organization.is_mmo is True diff --git a/kobo/apps/stripe/tests/utils.py b/kobo/apps/stripe/tests/utils.py index 55bb9e62af..e7dd249d86 100644 --- a/kobo/apps/stripe/tests/utils.py +++ b/kobo/apps/stripe/tests/utils.py @@ -2,7 +2,7 @@ from dateutil.relativedelta import relativedelta from django.utils import timezone -from djstripe.models import Customer, Product, SubscriptionItem, Subscription, Price +from djstripe.models import Customer, Price, Product, Subscription, SubscriptionItem from model_bakery import baker from kobo.apps.organizations.models import Organization @@ -17,7 +17,6 @@ def generate_plan_subscription( ) -> Subscription: """Create a subscription for a product with custom metadata""" created_date = timezone.now() - relativedelta(days=age_days) - price_id = 'price_sfmOFe33rfsfd36685657' if not customer: customer = baker.make(Customer, subscriber=organization, livemode=False) @@ -30,14 +29,12 @@ def generate_plan_subscription( product_metadata = {**product_metadata, **metadata} product = baker.make(Product, active=True, metadata=product_metadata) - if not (price := Price.objects.filter(id=price_id).first()): - price = baker.make( - Price, - active=True, - id=price_id, - recurring={'interval': interval}, - product=product, - ) + price = baker.make( + Price, + active=True, + recurring={'interval': interval}, + product=product, + ) period_offset = relativedelta(weeks=2) @@ -59,5 +56,6 @@ def generate_plan_subscription( ) -def generate_enterprise_subscription(organization: Organization, customer: Customer = None): - return generate_plan_subscription(organization, {'plan_type': 'enterprise'}, customer) +def generate_mmo_subscription(organization: Organization, customer: Customer = None): + product_metadata = {'mmo_enabled': 'true', 'plan_type': 'enterprise'} + return generate_plan_subscription(organization, product_metadata, customer) diff --git a/kpi/tests/test_usage_calculator.py b/kpi/tests/test_usage_calculator.py index e8a86fe7b8..82c3afc6f7 100644 --- a/kpi/tests/test_usage_calculator.py +++ b/kpi/tests/test_usage_calculator.py @@ -12,7 +12,7 @@ from kobo.apps.kobo_auth.shortcuts import User from kobo.apps.organizations.models import Organization from kobo.apps.stripe.constants import USAGE_LIMIT_MAP -from kobo.apps.stripe.tests.utils import generate_enterprise_subscription +from kobo.apps.stripe.tests.utils import generate_mmo_subscription from kobo.apps.trackers.models import NLPUsageCounter from kpi.models import Asset from kpi.tests.base_test_case import BaseAssetTestCase @@ -201,7 +201,7 @@ def test_organization_setup(self): organization = baker.make(Organization, id='org_abcd1234', mmo_override=True) organization.add_user(user=self.anotheruser, is_admin=True) organization.add_user(user=self.someuser, is_admin=True) - generate_enterprise_subscription(organization) + generate_mmo_subscription(organization) calculator = ServiceUsageCalculator(self.someuser, organization) submission_counters = calculator.get_submission_counters() From bd98b845106b8ae9767ccc6e28bb957137c05701 Mon Sep 17 00:00:00 2001 From: James Kiger Date: Thu, 21 Nov 2024 11:35:55 -0500 Subject: [PATCH 2/7] Fix organization api tests --- kobo/apps/organizations/tests/test_organizations_api.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/kobo/apps/organizations/tests/test_organizations_api.py b/kobo/apps/organizations/tests/test_organizations_api.py index c97ebef66e..8b8cff438a 100644 --- a/kobo/apps/organizations/tests/test_organizations_api.py +++ b/kobo/apps/organizations/tests/test_organizations_api.py @@ -34,6 +34,7 @@ class OrganizationApiTestCase(BaseTestCase): 'current_period_start': '2024-01-01', 'current_period_end': '2024-12-31' } + MMO_SUBSCRIPTION_DETAILS = {'product_metadata': {'mmo_enabled': 'true'}} def setUp(self): self.user = User.objects.get(username='someuser') @@ -116,13 +117,13 @@ def test_api_response_includes_is_mmo_with_mmo_override(self): @patch.object( Organization, 'active_subscription_billing_details', - return_value=DEFAULT_SUBSCRIPTION_DETAILS + return_value=MMO_SUBSCRIPTION_DETAILS, ) def test_api_response_includes_is_mmo_with_subscription( self, mock_active_subscription ): """ - Test that is_mmo is True when there is an active subscription. + Test that is_mmo is True when there is an active MMO subscription. """ self._insert_data(mmo_override=False) response = self.client.get(self.url_detail) @@ -149,14 +150,14 @@ def test_api_response_includes_is_mmo_with_no_override_and_no_subscription( @patch.object( Organization, 'active_subscription_billing_details', - return_value=DEFAULT_SUBSCRIPTION_DETAILS + return_value=MMO_SUBSCRIPTION_DETAILS, ) def test_api_response_includes_is_mmo_with_override_and_subscription( self, mock_active_subscription ): """ Test that is_mmo is True when both mmo_override and active - subscription is present. + MMO subscription is present. """ self._insert_data(mmo_override=True) response = self.client.get(self.url_detail) From 95cf9b6bfffa783665bc380c441b0fccb78b7114 Mon Sep 17 00:00:00 2001 From: James Kiger Date: Thu, 21 Nov 2024 14:00:13 -0500 Subject: [PATCH 3/7] Add stripe_enabled settings override for new test class --- kobo/apps/stripe/tests/test_organization_usage.py | 1 + 1 file changed, 1 insertion(+) diff --git a/kobo/apps/stripe/tests/test_organization_usage.py b/kobo/apps/stripe/tests/test_organization_usage.py index a7f3c20a23..390d8d74d6 100644 --- a/kobo/apps/stripe/tests/test_organization_usage.py +++ b/kobo/apps/stripe/tests/test_organization_usage.py @@ -503,6 +503,7 @@ def test_get_suscription_limit_unlimited(self, usage_type): assert limit == float('inf') +@override_settings(STRIPE_ENABLED=True) class OrganizationsModelIntegrationTestCase(BaseTestCase): fixtures = ['test_data'] From a1d5052941aad4316e27fc9f44e252f0924559cd Mon Sep 17 00:00:00 2001 From: Guillermo Date: Fri, 22 Nov 2024 18:51:46 -0600 Subject: [PATCH 4/7] Remove obsolete lines of code --- kobo/apps/organizations/views.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/kobo/apps/organizations/views.py b/kobo/apps/organizations/views.py index 9be7689a27..29414441f2 100644 --- a/kobo/apps/organizations/views.py +++ b/kobo/apps/organizations/views.py @@ -3,8 +3,6 @@ from django.db.models import QuerySet from django.utils.decorators import method_decorator from django.utils.http import http_date -from django.views.decorators.cache import cache_page -from django_dont_vary_on.decorators import only_vary_on from rest_framework import status, viewsets from rest_framework.decorators import action from rest_framework.request import Request @@ -63,10 +61,6 @@ def get_queryset(self, *args, **kwargs): raise NotImplementedError -@method_decorator(cache_page(settings.ENDPOINT_CACHE_DURATION), name='service_usage') -# django uses the Vary header in its caching, and each middleware can potentially add more Vary headers -# we use this decorator to remove any Vary headers except 'origin' (we don't want to cache between different installs) -@method_decorator(only_vary_on('Origin'), name='service_usage') class OrganizationViewSet(viewsets.ModelViewSet): """ Organizations are groups of users with assigned permissions and configurations From 02b489b867e2f5f2dd20e96463fff46127016f2f Mon Sep 17 00:00:00 2001 From: Guillermo Date: Sun, 24 Nov 2024 22:32:09 -0600 Subject: [PATCH 5/7] Fix linter error --- kobo/apps/organizations/views.py | 1 - 1 file changed, 1 deletion(-) diff --git a/kobo/apps/organizations/views.py b/kobo/apps/organizations/views.py index 29414441f2..3c5f44b18f 100644 --- a/kobo/apps/organizations/views.py +++ b/kobo/apps/organizations/views.py @@ -1,7 +1,6 @@ from django.conf import settings from django.contrib.postgres.aggregates import ArrayAgg from django.db.models import QuerySet -from django.utils.decorators import method_decorator from django.utils.http import http_date from rest_framework import status, viewsets from rest_framework.decorators import action From 9eaa6cd8b5b171559dcd841bdb2d4ba57cdc77b9 Mon Sep 17 00:00:00 2001 From: James Kiger Date: Wed, 27 Nov 2024 10:36:08 -0500 Subject: [PATCH 6/7] Changes from PR review --- kobo/apps/organizations/models.py | 3 ++- kobo/apps/stripe/tests/test_organization_usage.py | 8 -------- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/kobo/apps/organizations/models.py b/kobo/apps/organizations/models.py index c2c74cca59..fedff3754a 100644 --- a/kobo/apps/organizations/models.py +++ b/kobo/apps/organizations/models.py @@ -175,7 +175,8 @@ def is_mmo(self): This returns True if: - A superuser has enabled the override (`mmo_override`), or - - The organization has an active subscription. + - The organization has an active subscription to a plan with + mmo_enabled set to 'true' in the Stripe product metadata. If the override is enabled, it takes precedence over the subscription status """ diff --git a/kobo/apps/stripe/tests/test_organization_usage.py b/kobo/apps/stripe/tests/test_organization_usage.py index 390d8d74d6..5a5929e931 100644 --- a/kobo/apps/stripe/tests/test_organization_usage.py +++ b/kobo/apps/stripe/tests/test_organization_usage.py @@ -512,14 +512,6 @@ def setUp(self): self.organization = self.someuser.organization def test_is_mmo_subscription_logic(self): - self.organization.mmo_override = True - self.organization.save() - assert self.organization.is_mmo is True - - self.organization.mmo_override = False - self.organization.save() - assert self.organization.is_mmo is False - product_metadata = { 'mmo_enabled': 'false', } From 8b1322549e05bf197a2abecd1e45c5580419f085 Mon Sep 17 00:00:00 2001 From: James Kiger Date: Wed, 27 Nov 2024 10:47:05 -0500 Subject: [PATCH 7/7] Remove unused import after merge conflict fix --- kobo/apps/organizations/views.py | 1 - 1 file changed, 1 deletion(-) diff --git a/kobo/apps/organizations/views.py b/kobo/apps/organizations/views.py index 6e87cd141c..9c3d4634ee 100644 --- a/kobo/apps/organizations/views.py +++ b/kobo/apps/organizations/views.py @@ -9,7 +9,6 @@ OuterRef, ) from django.db.models.expressions import Exists -from django.utils.decorators import method_decorator from django.utils.http import http_date from rest_framework import status, viewsets from rest_framework.decorators import action