Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: new "retired" field on policy models #386

Merged
merged 1 commit into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 nice

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
Loading