diff --git a/docs/decisions/0016-policy-retirement.rst b/docs/decisions/0016-policy-retirement.rst index 4be93f41..16bf45ba 100644 --- a/docs/decisions/0016-policy-retirement.rst +++ b/docs/decisions/0016-policy-retirement.rst @@ -60,12 +60,12 @@ historical spend exists. The desired outcome is that content should NOT redeemab Decision ======== -We will add configuration flexibility by adding a new field ``redeemability_disabled_at`` to the policy model which +We will add configuration flexibility by adding a new field ``retired`` to the policy model which signals that it is not redeemable BUT should remain visible to enterprise admins: +------------------+-----------------+---------------------+-----------------------------+-------------------------------+-----------------------+ | Subsidy expired? | Subsidy | SubsidyAccessPolicy | **SubsidyAccessPolicy | Budget visibility on “Learner | Content redeemability | -| | is_soft_deleted | active | redeemability_disabled_at** | Credit Management” page | via budget | +| | is_soft_deleted | active | retired** | Credit Management” page | via budget | +==================+=================+=====================+=============================+===============================+=======================+ | No | FALSE | TRUE | null | ✅ visible | ✅ redeemable | +------------------+-----------------+---------------------+-----------------------------+-------------------------------+-----------------------+ @@ -78,21 +78,20 @@ In summary: * The budget/policy is ALWAYS VISIBLE to enterprise admins when subsidy is not deleted and the policy is active. * The budget/policy is ONLY REDEEMABLE when the subsidy is not expired, the subsidy is not deleted, the policy is - active, **and the policy does not have redeemability disabled**. + active, **and the policy is not retired**. Impacted use cases: +-----------------------------------------------------+----------------------------------------------------------------+ | Use Case | Actions | +=====================================================+================================================================+ -| Need to change learner credit plan from 2 to 1 | 1. Set ``SubsidyAccessPolicy.redeemability_disabled_at`` = | -| budgets. Spend exists. | ``now()`` on unwanted budget. | +| Need to change learner credit plan from 2 to 1 | 1. Set ``SubsidyAccessPolicy.retired`` = ``TRUE`` | +| budgets. Spend exists. | on unwanted budget. | | | 2. Update ``SubsidyAccessPolicy.spend_limit`` on remaining | | | budget(s) as needed. | +-----------------------------------------------------+----------------------------------------------------------------+ -| Need to change the distribution mode of one budget. | 1. Set ``SubsidyAccessPolicy.redeemability_disabled_at`` = | -| Spend exists. | ``now()``. | -| | 2. Create a new SubsidyAccessPolicy with a different | +| Need to change the distribution mode of one budget. | 1. Set ``SubsidyAccessPolicy.retired`` = ``TRUE`` | +| Spend exists. | 2. Create a new SubsidyAccessPolicy with a different | | | distribution mode. | +-----------------------------------------------------+----------------------------------------------------------------+ | Need to change learner credit plan from 2 to 1 | 1. Set ``SubsidyAccessPolicy.active`` = ``FALSE`` on unwanted | @@ -117,7 +116,7 @@ their behavior is not fully described by their name. Rejected Alternatives ===================== -Renaming ``active`` -> ``is_soft_deleted`` in addition to adding ``redeemability_disabled_at`` +Renaming ``active`` -> ``is_soft_deleted`` in addition to adding ``retired`` ---------------------------------------------------------------------------------------------- This ADR reinforces the concept that ``SubsidyAccessPolicy.active`` mirrors the intended behavior of an "is soft deleted" diff --git a/enterprise_access/apps/api/serializers/subsidy_access_policy.py b/enterprise_access/apps/api/serializers/subsidy_access_policy.py index 0f285dae..adc6f45f 100644 --- a/enterprise_access/apps/api/serializers/subsidy_access_policy.py +++ b/enterprise_access/apps/api/serializers/subsidy_access_policy.py @@ -161,6 +161,7 @@ class Meta: 'display_name', 'description', 'active', + 'retired', 'enterprise_customer_uuid', 'catalog_uuid', 'subsidy_uuid', @@ -194,6 +195,7 @@ class Meta: 'display_name', 'description', 'active', + 'retired', 'enterprise_customer_uuid', 'catalog_uuid', 'subsidy_uuid', @@ -395,6 +397,7 @@ class Meta: 'display_name', 'description', 'active', + 'retired', 'catalog_uuid', 'subsidy_uuid', 'access_method', @@ -422,6 +425,10 @@ class Meta: 'allow_null': False, 'required': False, }, + 'retired': { + 'allow_null': True, + 'required': False, + }, 'catalog_uuid': { 'allow_null': False, 'required': False, diff --git a/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py b/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py index 49b10c11..48b612e1 100644 --- a/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py +++ b/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py @@ -270,6 +270,7 @@ def test_detail_view(self, role_context_dict): self.assertEqual({ 'access_method': 'direct', 'active': True, + 'retired': False, 'catalog_uuid': str(self.redeemable_policy.catalog_uuid), 'display_name': self.redeemable_policy.display_name, 'description': 'A generic description', @@ -361,6 +362,7 @@ def test_list_view(self, role_context_dict): { 'access_method': 'direct', 'active': True, + 'retired': False, 'catalog_uuid': str(self.non_redeemable_policy.catalog_uuid), 'display_name': self.non_redeemable_policy.display_name, 'description': 'A generic description', @@ -387,6 +389,7 @@ def test_list_view(self, role_context_dict): { 'access_method': 'direct', 'active': True, + 'retired': False, 'catalog_uuid': str(self.redeemable_policy.catalog_uuid), 'display_name': self.redeemable_policy.display_name, 'description': 'A generic description', @@ -480,6 +483,7 @@ def test_destroy_view(self, request_payload, expected_change_reason): expected_response = { 'access_method': 'direct', 'active': False, + 'retired': False, 'catalog_uuid': str(self.redeemable_policy.catalog_uuid), 'display_name': self.redeemable_policy.display_name, 'description': 'A generic description', @@ -526,6 +530,7 @@ def test_destroy_view(self, request_payload, expected_change_reason): 'description': 'the new description', 'display_name': 'new display_name', 'active': True, + 'retired': True, 'catalog_uuid': str(uuid4()), 'subsidy_uuid': str(uuid4()), 'access_method': AccessMethods.ASSIGNED, @@ -540,6 +545,7 @@ def test_destroy_view(self, request_payload, expected_change_reason): 'description': 'the new description', 'display_name': 'new display_name', 'active': True, + 'retired': True, 'catalog_uuid': str(uuid4()), 'subsidy_uuid': str(uuid4()), 'access_method': AccessMethods.ASSIGNED, @@ -586,6 +592,7 @@ def test_update_views(self, is_patch, request_payload): # Fields that we officially support PATCHing. 'access_method': policy_for_edit.access_method, 'active': policy_for_edit.active, + 'retired': policy_for_edit.retired, 'catalog_uuid': str(policy_for_edit.catalog_uuid), 'display_name': policy_for_edit.display_name, 'description': policy_for_edit.description, @@ -802,12 +809,13 @@ def test_create_view(self, policy_type, extra_fields, expected_response_code, ex }, ]) - # Test the retrieve endpoint + # Test the create endpoint payload = { 'policy_type': policy_type, 'display_name': 'created policy', 'description': 'test description', 'active': True, + 'retired': False, 'enterprise_customer_uuid': str(TEST_ENTERPRISE_UUID), 'catalog_uuid': str(uuid4()), 'subsidy_uuid': str(uuid4()), @@ -863,6 +871,7 @@ def test_idempotent_create_view(self, policy_type, extra_fields, expected_respon 'display_name': 'new policy', 'description': 'test description', 'active': True, + 'retired': False, 'enterprise_customer_uuid': enterprise_customer_uuid, 'catalog_uuid': catalog_uuid, 'subsidy_uuid': subsidy_uuid, @@ -1288,6 +1297,11 @@ def test_credits_available_endpoint( enterprise_customer_uuid=self.enterprise_uuid, active=False, ) + # The following policy should never be returned as it has redeemability disabled. + PerLearnerEnrollmentCapLearnerCreditAccessPolicyFactory( + enterprise_customer_uuid=self.enterprise_uuid, + retired=True, + ) # The following policy should never be returned as it's had more spend than the `spend_limit`. PerLearnerSpendCapLearnerCreditAccessPolicyFactory( enterprise_customer_uuid=self.enterprise_uuid, diff --git a/enterprise_access/apps/api/v1/views/subsidy_access_policy.py b/enterprise_access/apps/api/v1/views/subsidy_access_policy.py index de90d994..bf25b5f4 100644 --- a/enterprise_access/apps/api/v1/views/subsidy_access_policy.py +++ b/enterprise_access/apps/api/v1/views/subsidy_access_policy.py @@ -388,12 +388,11 @@ def get_permission_object(self): def get_queryset(self): """ - Base queryset that returns all active policies associated + Base queryset that returns all active & redeemable policies associated with the customer uuid requested by the client. """ - return SubsidyAccessPolicy.objects.filter( + return SubsidyAccessPolicy.policies_with_redemption_enabled().filter( enterprise_customer_uuid=self.enterprise_customer_uuid, - active=True, ).order_by('-created') def evaluate_policies(self, enterprise_customer_uuid, lms_user_id, content_key): diff --git a/enterprise_access/apps/subsidy_access_policy/admin.py b/enterprise_access/apps/subsidy_access_policy/admin.py index fdfc3095..130fa46b 100644 --- a/enterprise_access/apps/subsidy_access_policy/admin.py +++ b/enterprise_access/apps/subsidy_access_policy/admin.py @@ -10,6 +10,7 @@ from pygments import highlight from pygments.formatters import HtmlFormatter # pylint: disable=no-name-in-module from pygments.lexers import JsonLexer # pylint: disable=no-name-in-module +from simple_history.admin import SimpleHistoryAdmin from enterprise_access.apps.api.serializers.subsidy_access_policy import SubsidyAccessPolicyResponseSerializer from enterprise_access.apps.subsidy_access_policy import constants, models @@ -39,7 +40,7 @@ def cents_to_usd_string(cents): return "${:,.2f}".format(float(cents) / constants.CENTS_PER_DOLLAR) -class BaseSubsidyAccessPolicyMixin(admin.ModelAdmin): +class BaseSubsidyAccessPolicyMixin(SimpleHistoryAdmin): """ Mixin for common admin properties on subsidy access policy models. """ @@ -47,11 +48,18 @@ class BaseSubsidyAccessPolicyMixin(admin.ModelAdmin): 'uuid', 'modified', 'active', + 'retired', 'display_name_or_short_description', 'policy_spend_limit_dollars', ) + history_list_display = ( + 'active', + 'retired', + 'spend_limit', + ) list_filter = ( 'active', + 'retired', 'access_method', ) ordering = ['-modified'] @@ -123,6 +131,9 @@ class PerLearnerEnrollmentCreditAccessPolicy(DjangoQLSearchMixin, BaseSubsidyAcc list_display = BaseSubsidyAccessPolicyMixin.list_display + ( 'per_learner_enrollment_limit', ) + history_list_display = BaseSubsidyAccessPolicyMixin.history_list_display + ( + 'per_learner_enrollment_limit', + ) search_fields = ( 'uuid', 'display_name', @@ -140,6 +151,7 @@ class PerLearnerEnrollmentCreditAccessPolicy(DjangoQLSearchMixin, BaseSubsidyAcc 'display_name', 'description', 'active', + 'retired', 'catalog_uuid', 'subsidy_uuid', 'created', @@ -168,6 +180,9 @@ class PerLearnerSpendCreditAccessPolicy(DjangoQLSearchMixin, BaseSubsidyAccessPo list_display = BaseSubsidyAccessPolicyMixin.list_display + ( 'per_learner_spend_limit_dollars', ) + history_list_display = BaseSubsidyAccessPolicyMixin.history_list_display + ( + 'per_learner_spend_limit_dollars', + ) readonly_fields = BaseSubsidyAccessPolicyMixin.readonly_fields + ( 'per_learner_spend_limit_dollars', ) @@ -188,6 +203,7 @@ class PerLearnerSpendCreditAccessPolicy(DjangoQLSearchMixin, BaseSubsidyAccessPo 'display_name', 'description', 'active', + 'retired', 'catalog_uuid', 'subsidy_uuid', 'created', @@ -244,6 +260,7 @@ class LearnerContentAssignmentAccessPolicy(DjangoQLSearchMixin, BaseSubsidyAcces 'display_name', 'description', 'active', + 'retired', 'catalog_uuid', 'subsidy_uuid', 'assignment_configuration', diff --git a/enterprise_access/apps/subsidy_access_policy/migrations/0021_subsidyaccesspolicy_retired.py b/enterprise_access/apps/subsidy_access_policy/migrations/0021_subsidyaccesspolicy_retired.py new file mode 100644 index 00000000..06852638 --- /dev/null +++ b/enterprise_access/apps/subsidy_access_policy/migrations/0021_subsidyaccesspolicy_retired.py @@ -0,0 +1,33 @@ +# Generated by Django 4.2.9 on 2024-01-17 23:44 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('subsidy_access_policy', '0020_alter_historicalassignedlearnercreditaccesspolicy_options_and_more'), + ] + + operations = [ + migrations.AddField( + model_name='historicalsubsidyaccesspolicy', + name='retired', + field=models.BooleanField(default=False, help_text="True means redeemability of content using this policy has been enabled. Set this to False to deactivate the policy but keep it visible from an admin's perspective (useful when you want to just expire a policy without expiring the whole plan)."), + ), + migrations.AddField( + model_name='subsidyaccesspolicy', + name='retired', + field=models.BooleanField(default=False, help_text="True means redeemability of content using this policy has been enabled. Set this to False to deactivate the policy but keep it visible from an admin's perspective (useful when you want to just expire a policy without expiring the whole plan)."), + ), + migrations.AlterField( + model_name='historicalsubsidyaccesspolicy', + name='active', + field=models.BooleanField(default=False, help_text='Set to FALSE to deactivate and hide this policy. Use this when you want to disable redemption and make it disappear from all frontends, effectively soft-deleting it. Default is False (deactivated).'), + ), + migrations.AlterField( + model_name='subsidyaccesspolicy', + name='active', + field=models.BooleanField(default=False, help_text='Set to FALSE to deactivate and hide this policy. Use this when you want to disable redemption and make it disappear from all frontends, effectively soft-deleting it. Default is False (deactivated).'), + ), + ] diff --git a/enterprise_access/apps/subsidy_access_policy/models.py b/enterprise_access/apps/subsidy_access_policy/models.py index dc3ef461..99d70517 100644 --- a/enterprise_access/apps/subsidy_access_policy/models.py +++ b/enterprise_access/apps/subsidy_access_policy/models.py @@ -137,7 +137,18 @@ class Meta: ) active = models.BooleanField( default=False, - help_text='Whether this policy is active, defaults to false.', + help_text=( + 'Set to FALSE to deactivate and hide this policy. Use this when you want to disable redemption and make ' + 'it disappear from all frontends, effectively soft-deleting it. Default is False (deactivated).' + ), + ) + retired = models.BooleanField( + default=False, + help_text=( + "True means redeemability of content using this policy has been enabled. " + "Set this to False to deactivate the policy but keep it visible from an admin's perspective " + "(useful when you want to just expire a policy without expiring the whole plan)." + ), ) catalog_uuid = models.UUIDField( db_index=True, @@ -200,6 +211,23 @@ class Meta: # ProxyAwareHistoricalRecords docstring for more info. history = ProxyAwareHistoricalRecords(inherit=True) + @classmethod + def policies_with_redemption_enabled(cls): + """ + Return all policies which have redemption enabled. + """ + return cls.objects.filter( + active=True, + retired=False, + ) + + @property + def is_redemption_enabled(self): + """ + Return True if this policy has redemption enabled. + """ + return self.active and not self.retired + @property def subsidy_active_datetime(self): """ @@ -543,7 +571,7 @@ def can_redeem(self, lms_user_id, content_key, skip_customer_user_check=False): * third a list of any transactions representing existing redemptions (any state). """ # inactive policy - if not self.active: + if not self.is_redemption_enabled: return (False, REASON_POLICY_EXPIRED, []) # learner not associated to enterprise @@ -622,7 +650,7 @@ def credit_available(self, lms_user_id, skip_customer_user_check=False): * Whether the transactions associated with policy have exceeded the policy-wide spend limit. """ # inactive policy - if not self.active: + if not self.is_redemption_enabled: logger.info('[credit_available] policy %s inactive', self.uuid) return False @@ -1244,7 +1272,7 @@ def can_allocate(self, number_of_learners, content_key, content_price_cents): self.validate_requested_allocation_price(content_key, content_price_cents) # inactive policy - if not self.active: + if not self.is_redemption_enabled: return (False, REASON_POLICY_EXPIRED) # no content key in catalog diff --git a/enterprise_access/apps/subsidy_access_policy/tests/test_models.py b/enterprise_access/apps/subsidy_access_policy/tests/test_models.py index b1c7bdb4..e6d1f900 100644 --- a/enterprise_access/apps/subsidy_access_policy/tests/test_models.py +++ b/enterprise_access/apps/subsidy_access_policy/tests/test_models.py @@ -119,6 +119,10 @@ def setUpClass(cls): per_learner_enrollment_limit=5, active=False, ) + cls.active_non_redeemable_per_learner_enroll_policy = PerLearnerEnrollmentCapLearnerCreditAccessPolicyFactory( + per_learner_enrollment_limit=5, + retired=True, + ) cls.per_learner_spend_policy = PerLearnerSpendCapLearnerCreditAccessPolicyFactory( uuid=ACTIVE_LEARNER_SPEND_CAP_POLICY_UUID, per_learner_spend_limit=500, @@ -128,6 +132,10 @@ def setUpClass(cls): per_learner_spend_limit=500, active=False, ) + cls.active_non_redeemable_per_learner_spend_policy = PerLearnerSpendCapLearnerCreditAccessPolicyFactory( + per_learner_spend_limit=500, + retired=True, + ) def tearDown(self): """ @@ -197,7 +205,7 @@ def test_object_creation_with_policy_type_in_kwarg(self, *args): # Happy path: content in catalog, learner in enterprise, subsidy has value, # existing transactions for learner and policy below the policy limits. # Expected can_redeem result: True - 'policy_is_active': True, + 'policy_active_type': 'active', 'catalog_contains_content': True, 'enterprise_contains_learner': True, 'subsidy_is_redeemable': {'can_redeem': True, 'active': True}, @@ -208,7 +216,7 @@ def test_object_creation_with_policy_type_in_kwarg(self, *args): { # Content not in catalog, every other check would succeed. # Expected can_redeem result: False - 'policy_is_active': True, + 'policy_active_type': 'active', 'catalog_contains_content': False, 'enterprise_contains_learner': True, 'subsidy_is_redeemable': {'can_redeem': True, 'active': True}, @@ -221,7 +229,7 @@ def test_object_creation_with_policy_type_in_kwarg(self, *args): { # Learner is not in the enterprise, every other check would succeed. # Expected can_redeem result: False - 'policy_is_active': True, + 'policy_active_type': 'active', 'catalog_contains_content': True, 'enterprise_contains_learner': False, 'subsidy_is_redeemable': {'can_redeem': True, 'active': True}, @@ -234,7 +242,7 @@ def test_object_creation_with_policy_type_in_kwarg(self, *args): { # The subsidy is not redeemable, every other check would succeed. # Expected can_redeem result: False - 'policy_is_active': True, + 'policy_active_type': 'active', 'catalog_contains_content': True, 'enterprise_contains_learner': True, 'subsidy_is_redeemable': {'can_redeem': False, 'active': True}, @@ -246,7 +254,7 @@ def test_object_creation_with_policy_type_in_kwarg(self, *args): # The subsidy is redeemable, but the learner has already enrolled more than the limit. # Every other check would succeed. # Expected can_redeem result: False - 'policy_is_active': True, + 'policy_active_type': 'active', 'catalog_contains_content': True, 'enterprise_contains_learner': True, 'subsidy_is_redeemable': {'can_redeem': True, 'active': True}, @@ -266,7 +274,7 @@ def test_object_creation_with_policy_type_in_kwarg(self, *args): # The subsidy is redeemable, but another redemption would exceed the policy-wide ``spend_limit``. # Every other check would succeed. # Expected can_redeem result: False - 'policy_is_active': True, + 'policy_active_type': 'active', 'catalog_contains_content': True, 'enterprise_contains_learner': True, 'subsidy_is_redeemable': {'can_redeem': True, 'active': True}, @@ -285,7 +293,20 @@ def test_object_creation_with_policy_type_in_kwarg(self, *args): { # The subsidy access policy is not active, every other check would succeed. # Expected can_redeem result: False - 'policy_is_active': False, + 'policy_active_type': 'inactive', + 'catalog_contains_content': True, + 'enterprise_contains_learner': True, + 'subsidy_is_redeemable': {'can_redeem': True, 'active': True}, + 'transactions_for_learner': {'transactions': [], 'aggregates': {}}, + 'transactions_for_policy': {'results': [], 'aggregates': {'total_quantity': -200}}, + 'expected_policy_can_redeem': (False, REASON_POLICY_EXPIRED, []), + 'expect_content_metadata_fetch': False, + 'expect_transaction_fetch': False, + }, + { + # The subsidy access policy is not redeemable, every other check would succeed. + # Expected can_redeem result: False + 'policy_active_type': 'non_redeemable', 'catalog_contains_content': True, 'enterprise_contains_learner': True, 'subsidy_is_redeemable': {'can_redeem': True, 'active': True}, @@ -298,7 +319,7 @@ def test_object_creation_with_policy_type_in_kwarg(self, *args): { # The subsidy is not active, every other check would succeed. # Expected can_redeem result: False - 'policy_is_active': True, + 'policy_active_type': 'active', 'catalog_contains_content': True, 'enterprise_contains_learner': True, 'subsidy_is_redeemable': {'can_redeem': True, 'active': False}, @@ -310,7 +331,7 @@ def test_object_creation_with_policy_type_in_kwarg(self, *args): @ddt.unpack def test_learner_enrollment_cap_policy_can_redeem( self, - policy_is_active, + policy_active_type, catalog_contains_content, enterprise_contains_learner, subsidy_is_redeemable, @@ -333,8 +354,10 @@ def test_learner_enrollment_cap_policy_can_redeem( self.mock_subsidy_client.list_subsidy_transactions.return_value = transactions_for_policy policy_record = self.inactive_per_learner_enroll_policy - if policy_is_active: + if policy_active_type == 'active': policy_record = self.per_learner_enroll_policy + elif policy_active_type == 'non_redeemable': + policy_record = self.active_non_redeemable_per_learner_enroll_policy can_redeem_result = policy_record.can_redeem(self.lms_user_id, self.course_id)