From 9e08cc8c30ea56d74740cffa530f6b6ea13a60c5 Mon Sep 17 00:00:00 2001 From: John Nagro Date: Fri, 18 Aug 2023 13:12:56 +0000 Subject: [PATCH] feat: multi-policy resolution --- .../api/v1/views/subsidy_access_policy.py | 3 +- .../apps/subsidy_access_policy/models.py | 41 +++++- .../tests/test_models.py | 125 +++++++++++++++++- 3 files changed, 161 insertions(+), 8 deletions(-) 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 02c53ef6..abfaecdc 100644 --- a/enterprise_access/apps/api/v1/views/subsidy_access_policy.py +++ b/enterprise_access/apps/api/v1/views/subsidy_access_policy.py @@ -613,9 +613,8 @@ def can_redeem(self, request, enterprise_customer_uuid): non_redeemable_policies )) - # TODO: Arbitrarily select one redeemable policy for now. if redeemable_policies: - resolved_policy = redeemable_policies[0] + resolved_policy = SubsidyAccessPolicy.resolve_policy(redeemable_policies) if resolved_policy or has_successful_redemption: list_price = self._get_list_price(enterprise_customer_uuid, content_key) diff --git a/enterprise_access/apps/subsidy_access_policy/models.py b/enterprise_access/apps/subsidy_access_policy/models.py index d43a2e20..9b8d27f1 100644 --- a/enterprise_access/apps/subsidy_access_policy/models.py +++ b/enterprise_access/apps/subsidy_access_policy/models.py @@ -6,6 +6,7 @@ from uuid import UUID, uuid4 import requests +from django.conf import settings from django.core.cache import cache as django_cache from django.db import models from django_extensions.db.models import TimeStampedModel @@ -29,7 +30,13 @@ from .content_metadata_api import get_and_cache_catalog_contains_content, get_and_cache_content_metadata from .exceptions import ContentPriceNullException, SubsidyAccessPolicyLockAttemptFailed, SubsidyAPIHTTPError from .subsidy_api import get_and_cache_transactions_for_learner -from .utils import ProxyAwareHistoricalRecords, create_idempotency_key_for_transaction, get_versioned_subsidy_client +from .utils import ( + ProxyAwareHistoricalRecords, + create_idempotency_key_for_transaction, + get_versioned_subsidy_client, + request_cache, + versioned_cache_key +) POLICY_LOCK_RESOURCE_NAME = "subsidy_access_policy" @@ -220,7 +227,26 @@ def __new__(cls, *args, **kwargs): return super().__new__(proxy_class) # pylint: disable=lost-exception def subsidy_record(self): - return self.subsidy_client.retrieve_subsidy(subsidy_uuid=self.subsidy_uuid) + """ + Retrieve this policy's corresponding subsidy record + """ + # don't utilize the cache unless this experimental feature is enabled + if not getattr(settings, 'MULTI_POLICY_RESOLUTION_ENABLED', False): + return self.subsidy_client.retrieve_subsidy(subsidy_uuid=self.subsidy_uuid) + + cache_key = versioned_cache_key( + 'get_subsidy_record', + self.enterprise_customer_uuid, + self.subsidy_uuid, + ) + cached_response = request_cache().get_cached_response(cache_key) + if cached_response.is_found: + return cached_response.value + + result = self.subsidy_client.retrieve_subsidy(subsidy_uuid=self.subsidy_uuid) + request_cache().set(cache_key, result) + + return result def subsidy_balance(self): """ @@ -573,13 +599,20 @@ def resolve_policy(cls, redeemable_policies): Returns: SubsidyAccessPolicy: one policy selected from the input list. """ + # gate for experimental functionality to resolve multiple policies + if not getattr(settings, 'MULTI_POLICY_RESOLUTION_ENABLED', False): + return redeemable_policies[0] + if len(redeemable_policies) == 1: return redeemable_policies[0] - # For now, we inefficiently make one call per subsidy record. + # resolve policies by: + # - priority (of type) + # - expiration, sooner to expire first + # - balance, lower balance first sorted_policies = sorted( redeemable_policies, - key=lambda p: (p.priority, p.subsidy_balance()), + key=lambda p: (p.priority, p.subsidy_expiration_datetime, p.subsidy_balance()), ) return sorted_policies[0] 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 74172f20..9ea684d2 100644 --- a/enterprise_access/apps/subsidy_access_policy/tests/test_models.py +++ b/enterprise_access/apps/subsidy_access_policy/tests/test_models.py @@ -2,13 +2,13 @@ Tests for subsidy_access_policy models. """ from datetime import datetime, timedelta -from unittest.mock import patch +from unittest.mock import PropertyMock, patch from uuid import uuid4 import ddt import pytest from django.core.cache import cache as django_cache -from django.test import TestCase +from django.test import TestCase, override_settings from enterprise_access.apps.subsidy_access_policy.constants import ( REASON_CONTENT_NOT_IN_CATALOG, @@ -557,3 +557,124 @@ def test_mock_subsidy_datetimes(self): assert policy.subsidy_active_datetime == mock_subsidy.get('active_datetime') assert policy.subsidy_expiration_datetime == mock_subsidy.get('expiration_datetime') assert policy.is_subsidy_active == mock_subsidy.get('is_active') + + +class SubsidyAccessPolicyResolverTests(TestCase): + """ SubsidyAccessPolicy.resolve_policy() tests. """ + + def setUp(self): + """ + Initialize mocked service clients. + """ + super().setUp() + yesterday = datetime.utcnow() - timedelta(days=1) + tomorrow = datetime.utcnow() + timedelta(days=1) + day_after_tomorrow = datetime.utcnow() + timedelta(days=2) + self.mock_subsidy_one = { + 'id': 1, + 'active_datetime': yesterday, + 'expiration_datetime': tomorrow, + 'is_active': True, + 'current_balance': 100, + } + self.mock_subsidy_two = { + 'id': 2, + 'active_datetime': yesterday, + 'expiration_datetime': tomorrow, + 'is_active': True, + 'current_balance': 50, + } + self.mock_subsidy_three = { + 'id': 3, + 'active_datetime': yesterday, + 'expiration_datetime': day_after_tomorrow, + 'is_active': True, + 'current_balance': 50, + } + self.mock_subsidy_four = { + 'id': 4, + 'active_datetime': yesterday, + 'expiration_datetime': tomorrow, + 'is_active': True, + 'current_balance': 100, + } + + self.policy_one = PerLearnerEnrollmentCapLearnerCreditAccessPolicyFactory.create() + self.policy_two = PerLearnerEnrollmentCapLearnerCreditAccessPolicyFactory.create() + self.policy_three = PerLearnerEnrollmentCapLearnerCreditAccessPolicyFactory.create() + self.policy_four = PerLearnerSpendCapLearnerCreditAccessPolicyFactory.create() + + policy_one_subsity_patcher = patch.object( + self.policy_one, 'subsidy_record' + ) + self.mock_policy_one_subsidy_record = policy_one_subsity_patcher.start() + self.mock_policy_one_subsidy_record.return_value = self.mock_subsidy_one + + policy_two_subsity_patcher = patch.object( + self.policy_two, 'subsidy_record' + ) + self.mock_policy_two_subsidy_record = policy_two_subsity_patcher.start() + self.mock_policy_two_subsidy_record.return_value = self.mock_subsidy_two + + policy_three_subsity_patcher = patch.object( + self.policy_three, 'subsidy_record' + ) + self.mock_policy_three_subsidy_record = policy_three_subsity_patcher.start() + self.mock_policy_three_subsidy_record.return_value = self.mock_subsidy_three + + policy_four_subsity_patcher = patch.object( + self.policy_four, 'subsidy_record' + ) + self.mock_policy_four_subsidy_record = policy_four_subsity_patcher.start() + self.mock_policy_four_subsidy_record.return_value = self.mock_subsidy_four + + self.addCleanup(policy_one_subsity_patcher.stop) + self.addCleanup(policy_two_subsity_patcher.stop) + self.addCleanup(policy_three_subsity_patcher.stop) + self.addCleanup(policy_four_subsity_patcher.stop) + + def test_setup(self): + """ + Ensure each policy has the correctly mocked subsidy object + """ + assert self.policy_one.subsidy_record() == self.mock_subsidy_one + assert self.policy_two.subsidy_record() == self.mock_subsidy_two + assert self.policy_three.subsidy_record() == self.mock_subsidy_three + + @override_settings(MULTI_POLICY_RESOLUTION_ENABLED=True) + def test_resolve_one_policy(self): + """ + Test resolve given a single policy + """ + policies = [self.policy_one] + assert SubsidyAccessPolicy.resolve_policy(policies) == self.policy_one + + @override_settings(MULTI_POLICY_RESOLUTION_ENABLED=True) + def test_resolve_two_policies_by_balance(self): + """ + Test resolve given a two policies with different balances, same expiration + the smaller balance policy should be returned. + """ + policies = [self.policy_one, self.policy_two] + assert SubsidyAccessPolicy.resolve_policy(policies) == self.policy_two + + @override_settings(MULTI_POLICY_RESOLUTION_ENABLED=True) + def test_resolve_two_policies_by_expiration(self): + """ + Test resolve given a two policies with different balances, differet expiration + the sooner expiration policy should be returned. + """ + policies = [self.policy_one, self.policy_three] + assert SubsidyAccessPolicy.resolve_policy(policies) == self.policy_one + + @override_settings(MULTI_POLICY_RESOLUTION_ENABLED=True) + def test_resolve_two_policies_by_type_priority(self): + """ + Test resolve given a two policies with same balances, same expiration + but different type-priority. + """ + policies = [self.policy_four, self.policy_one] + # artificially set the priority attribute higher on one of the policies (lower priority takes precident) + with patch.object(PerLearnerSpendCreditAccessPolicy, 'priority', new_callable=PropertyMock) as mock: + mock.return_value = 100 + assert SubsidyAccessPolicy.resolve_policy(policies) == self.policy_one