Skip to content

Commit

Permalink
feat: new "retired" field on policy models
Browse files Browse the repository at this point in the history
Implements ADR 0016-policy-retirement, and also updates it to reflect
the decision to rename the field to "retired" and change it to a
boolean.

Note this also adds extra visibility into the historical values of
various policy fields via the django admin record history interface.
Previously, changes were only noted (e.g. "active field changed"), but
now the actual historical value of the field is visible.

ENT-8206
  • Loading branch information
pwnage101 committed Jan 18, 2024
1 parent 6bfbfbe commit a96819a
Show file tree
Hide file tree
Showing 8 changed files with 148 additions and 28 deletions.
17 changes: 8 additions & 9 deletions docs/decisions/0016-policy-retirement.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
+------------------+-----------------+---------------------+-----------------------------+-------------------------------+-----------------------+
Expand All @@ -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 |
Expand All @@ -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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ class Meta:
'display_name',
'description',
'active',
'retired',
'enterprise_customer_uuid',
'catalog_uuid',
'subsidy_uuid',
Expand Down Expand Up @@ -194,6 +195,7 @@ class Meta:
'display_name',
'description',
'active',
'retired',
'enterprise_customer_uuid',
'catalog_uuid',
'subsidy_uuid',
Expand Down Expand Up @@ -395,6 +397,7 @@ class Meta:
'display_name',
'description',
'active',
'retired',
'catalog_uuid',
'subsidy_uuid',
'access_method',
Expand Down Expand Up @@ -422,6 +425,10 @@ class Meta:
'allow_null': False,
'required': False,
},
'retired': {
'allow_null': True,
'required': False,
},
'catalog_uuid': {
'allow_null': False,
'required': False,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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',
Expand All @@ -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',
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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()),
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 2 additions & 3 deletions enterprise_access/apps/api/v1/views/subsidy_access_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
19 changes: 18 additions & 1 deletion enterprise_access/apps/subsidy_access_policy/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -39,19 +40,26 @@ 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.
"""
list_display = (
'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']
Expand Down Expand Up @@ -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',
Expand All @@ -140,6 +151,7 @@ class PerLearnerEnrollmentCreditAccessPolicy(DjangoQLSearchMixin, BaseSubsidyAcc
'display_name',
'description',
'active',
'retired',
'catalog_uuid',
'subsidy_uuid',
'created',
Expand Down Expand Up @@ -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',
)
Expand All @@ -188,6 +203,7 @@ class PerLearnerSpendCreditAccessPolicy(DjangoQLSearchMixin, BaseSubsidyAccessPo
'display_name',
'description',
'active',
'retired',
'catalog_uuid',
'subsidy_uuid',
'created',
Expand Down Expand Up @@ -244,6 +260,7 @@ class LearnerContentAssignmentAccessPolicy(DjangoQLSearchMixin, BaseSubsidyAcces
'display_name',
'description',
'active',
'retired',
'catalog_uuid',
'subsidy_uuid',
'assignment_configuration',
Expand Down
Original file line number Diff line number Diff line change
@@ -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).'),
),
]
36 changes: 32 additions & 4 deletions enterprise_access/apps/subsidy_access_policy/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,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,
Expand Down Expand Up @@ -195,6 +206,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):
"""
Expand Down Expand Up @@ -526,7 +554,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
Expand Down Expand Up @@ -605,7 +633,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

Expand Down Expand Up @@ -1227,7 +1255,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
Expand Down
Loading

0 comments on commit a96819a

Please sign in to comment.