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 8fd75389..a95af162 100755 --- 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 @@ -1784,6 +1784,13 @@ def setup_mocks(self): lms_client_instance = lms_client.return_value lms_client_instance.get_enterprise_user.return_value = TEST_USER_RECORD + catalog_get_and_cache_content_metadata_patcher = mock.patch( + 'enterprise_access.apps.api.v1.views.subsidy_access_policy.get_and_cache_content_metadata', + ) + self.mock_catalog_get_and_cache_content_metadata = catalog_get_and_cache_content_metadata_patcher.start() + self.mock_catalog_get_and_cache_content_metadata.return_value = {} + + self.addCleanup(catalog_get_and_cache_content_metadata_patcher.stop) self.addCleanup(lms_client_patcher.stop) self.addCleanup(subsidy_client_patcher.stop) self.addCleanup(contains_key_patcher.stop) @@ -1956,6 +1963,12 @@ def test_can_redeem_policy_none_redeemable( 'unit': 'usd_cents', 'all_transactions': [], } + self.mock_catalog_get_and_cache_content_metadata.return_value = { + 'normalized_metadata': { + 'content_price': 50, # normalized_metadata always serializes USD. + }, + 'normalized_metadata_by_run': {}, + } test_content_key_1 = "course-v1:edX+Privacy101+3T2020" test_content_key_2 = "course-v1:edX+Privacy101+3T2020_2" test_content_key_1_metadata_price = 29900 @@ -1996,9 +2009,9 @@ def mock_get_subsidy_content_data(*args, **kwargs): # Check the response for the first content_key given. assert response_list[0]["content_key"] == test_content_key_1 - # We should not assume that a list price is fetchable if the - # content cant' be redeemed - the content may not be in any catalog for any policy. - assert response_list[0]["list_price"] is None + # When there's no redeemable policy, the price returned is the current + # fixed price fetched directly from catalog. + assert response_list[0]["list_price"] == {'usd': 50.0, 'usd_cents': 5000} assert len(response_list[0]["redemptions"]) == 0 assert response_list[0]["has_successful_redemption"] is False assert response_list[0]["redeemable_subsidy_access_policy"] is None @@ -2031,7 +2044,7 @@ def mock_get_subsidy_content_data(*args, **kwargs): # Check the response for the second content_key given. assert response_list[1]["content_key"] == test_content_key_2 - assert response_list[1]["list_price"] is None + assert response_list[1]["list_price"] == {'usd': 50.0, 'usd_cents': 5000} assert len(response_list[1]["redemptions"]) == 0 assert response_list[1]["has_successful_redemption"] is False @@ -2263,6 +2276,12 @@ def test_can_redeem_policy_beyond_enrollment_deadline(self, mock_lms_client, moc "content_price": 1234, "enroll_by_date": '2001-01-01T00:00:00Z', } + self.mock_catalog_get_and_cache_content_metadata.return_value = { + 'normalized_metadata': { + 'content_price': 12.34, # normalized_metadata always serializes USD. + }, + 'normalized_metadata_by_run': {}, + } with mock.patch( 'enterprise_access.apps.subsidy_access_policy.content_metadata_api.get_and_cache_content_metadata', @@ -2368,6 +2387,13 @@ def test_can_redeem_lms_user_id_override_for_staff( 'content_price': 19900, } self.mock_get_content_metadata.return_value = mock_subsidy_content_data + self.mock_catalog_get_and_cache_content_metadata.return_value = { + 'normalized_metadata': { + 'content_price': 199.00, # normalized_metadata always serializes USD. + }, + 'normalized_metadata_by_run': {}, + } + query_params = {'content_key': test_content_key} if lms_user_id_override: query_params['lms_user_id'] = lms_user_id_override @@ -2793,6 +2819,12 @@ def test_can_redeem_no_assignment_for_content( "content_price": content_key_for_redemption_metadata_price, } self.mock_get_content_metadata.return_value = mock_get_subsidy_content_data + self.mock_catalog_get_and_cache_content_metadata.return_value = { + 'normalized_metadata': { + 'content_price': content_key_for_redemption_metadata_price / 100, + }, + 'normalized_metadata_by_run': {}, + } # It's an unredeemable response, so mock out some admin users to return mock_lms_client.return_value.get_enterprise_customer_data.return_value = { @@ -2816,7 +2848,10 @@ def test_can_redeem_no_assignment_for_content( # Check the response for the first content_key given. assert response_list[0]["content_key"] == content_key_for_redemption - assert response_list[0]["list_price"] is None + assert response_list[0]["list_price"] == { + 'usd': content_key_for_redemption_metadata_price / 100.0, + 'usd_cents': content_key_for_redemption_metadata_price, + } assert response_list[0]["redemptions"] == [] assert response_list[0]["has_successful_redemption"] is False assert response_list[0]["redeemable_subsidy_access_policy"] is None 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 b0a3b463..6c314a2f 100755 --- a/enterprise_access/apps/api/v1/views/subsidy_access_policy.py +++ b/enterprise_access/apps/api/v1/views/subsidy_access_policy.py @@ -27,6 +27,7 @@ from enterprise_access.apps.api.mixins import UserDetailsFromJwtMixin from enterprise_access.apps.api_client.lms_client import LmsApiClient from enterprise_access.apps.content_assignments.api import AllocationException +from enterprise_access.apps.content_metadata.api import get_and_cache_content_metadata from enterprise_access.apps.core.constants import ( SUBSIDY_ACCESS_POLICY_ALLOCATION_PERMISSION, SUBSIDY_ACCESS_POLICY_READ_PERMISSION, @@ -53,6 +54,7 @@ MissingSubsidyAccessReasonUserMessages, TransactionStateChoices ) +from enterprise_access.apps.subsidy_access_policy.content_metadata_api import make_list_price_dict from enterprise_access.apps.subsidy_access_policy.exceptions import ( ContentPriceNullException, MissingAssignment, @@ -744,13 +746,18 @@ def can_redeem(self, request, enterprise_customer_uuid): redeemable_policies = [] non_redeemable_policies = [] resolved_policy = None - list_price = None + list_price_dict = None - redemptions_by_policy_uuid = redemptions_by_content_and_policy[content_key] + redemptions_by_policy = redemptions_by_content_and_policy[content_key] + policy_by_redemption_uuid = { + redemption['uuid']: policy + for policy, redemptions in redemptions_by_policy.items() + for redemption in redemptions + } # Flatten dict of lists because the response doesn't need to be bucketed by policy_uuid. redemptions = [ redemption - for redemptions in redemptions_by_policy_uuid.values() + for redemptions in redemptions_by_policy.values() for redemption in redemptions ] @@ -787,19 +794,32 @@ def can_redeem(self, request, enterprise_customer_uuid): try: if resolved_policy: - list_price = resolved_policy.get_list_price(lms_user_id, content_key) + list_price_dict = resolved_policy.get_list_price(lms_user_id, content_key) elif successful_redemptions: - # Get the policy record used at time of successful redemption. - # [2023-12-05] TODO: consider cleaning this up. - # This is kind of silly, b/c we only need this policy to compute the - # list price, and it's really only *necessary* to fetch that price - # from within the context of a *policy record* for cases where that successful - # policy was assignment-based (because the list price for assignments might - # slightly different from the current list price in the canonical content metadata). - successfully_redeemed_policy = self.get_queryset().filter( - uuid=successful_redemptions[0]['subsidy_access_policy_uuid'], - ).first() - list_price = successfully_redeemed_policy.get_list_price(lms_user_id, content_key) + # Get the policy used for redemption and use that to compute the price. If the redemption was the + # result of assignment, the historical assignment price might differ from the canonical price. We + # prefer to display the redeemed price to avoid confusion. + successfully_redeemed_policy = policy_by_redemption_uuid[successful_redemptions[0]['uuid']] + list_price_dict = successfully_redeemed_policy.get_list_price(lms_user_id, content_key) + else: + # In the case where the learner cannot redeem and has never redeemed this content, bypass the + # subsidy metadata endpoint and go straight to the source (enterprise-catalog) to find normalized + # price data. In this case, the list price returned rarely actually drives the display price in the + # learner portal frontend, but we still need to maintain a non-null list price for a few reasons: + # * Enterprise customers that leverage the API directly always expect a non-null price. + # * On rare occasion, this price actually does drive the price display in learner-portal. We think + # this can happen when courses are searchable and there is an assignment-based policy, but nothing + # has been assigned. + # * Long-term, we will use can_redeem for all subsidy types, at which point we will also rely on + # this list_price for price display 100% of the time. + course_metadata = get_and_cache_content_metadata(content_key, coerce_to_parent_course=True) + decimal_dollars = ( + course_metadata['normalized_metadata_by_run'].get(content_key, {}).get('content_price') or + course_metadata['normalized_metadata'].get('content_price') + ) + if decimal_dollars is None: + raise ContentPriceNullException('Failed to obtain content price from enterprise-catalog.') + list_price_dict = make_list_price_dict(decimal_dollars=decimal_dollars) except ContentPriceNullException as exc: raise RedemptionRequestException( detail=f'Could not determine list price for content_key: {content_key}', @@ -807,7 +827,7 @@ def can_redeem(self, request, enterprise_customer_uuid): element_response = { "content_key": content_key, - "list_price": list_price, + "list_price": list_price_dict, "redemptions": redemptions, "has_successful_redemption": bool(successful_redemptions), "redeemable_subsidy_access_policy": resolved_policy, diff --git a/enterprise_access/apps/api_client/enterprise_catalog_client.py b/enterprise_access/apps/api_client/enterprise_catalog_client.py index d1941530..a1592e84 100644 --- a/enterprise_access/apps/api_client/enterprise_catalog_client.py +++ b/enterprise_access/apps/api_client/enterprise_catalog_client.py @@ -1,6 +1,8 @@ """ API client for enterprise-catalog service. """ +from urllib.parse import urljoin + import backoff from django.conf import settings @@ -10,10 +12,14 @@ class EnterpriseCatalogApiClient(BaseOAuthClient): """ - API client for calls to the enterprise catalog service. + V2 API client for calls to the enterprise catalog service. """ - api_base_url = settings.ENTERPRISE_CATALOG_URL + '/api/v2/' - enterprise_catalog_endpoint = api_base_url + 'enterprise-catalogs/' + api_version = 'v2' + + def __init__(self): + self.api_base_url = urljoin(settings.ENTERPRISE_CATALOG_URL, f'api/{self.api_version}/') + self.enterprise_catalog_endpoint = urljoin(self.api_base_url, 'enterprise-catalogs/') + super().__init__() @backoff.on_exception(wait_gen=backoff.expo, exception=autoretry_for_exceptions) def contains_content_items(self, catalog_uuid, content_ids): @@ -84,3 +90,37 @@ def get_content_metadata_count(self, catalog_uuid): response = self.client.get(endpoint) response.raise_for_status() return response.json()['count'] + + def content_metadata(self, content_id): + raise NotImplementedError('There is currently no v2 API implementation for this endpoint.') + + +class EnterpriseCatalogApiV1Client(EnterpriseCatalogApiClient): + """ + V1 API client for calls to the enterprise catalog service. + """ + api_version = 'v1' + + def __init__(self): + super().__init__() + self.content_metadata_endpoint = urljoin(self.api_base_url, 'content-metadata/') + + @backoff.on_exception(wait_gen=backoff.expo, exception=autoretry_for_exceptions) + def content_metadata(self, content_id, coerce_to_parent_course=False): + """ + Fetch catalog-/customer-agnostic content metadata. + + Arguments: + content_id (str): ID of content to fetch. + + Returns: + dict: serialized content metadata, or None if not found. + """ + query_params = {} + if coerce_to_parent_course: + query_params |= {'coerce_to_parent_course': True} + kwargs = {'params': query_params} if query_params else {} + endpoint = self.content_metadata_endpoint + content_id + response = self.client.get(endpoint, **kwargs) + response.raise_for_status() + return response.json() diff --git a/enterprise_access/apps/api_client/tests/test_enterprise_catalog_client.py b/enterprise_access/apps/api_client/tests/test_enterprise_catalog_client.py index 9405c429..2703f1f1 100644 --- a/enterprise_access/apps/api_client/tests/test_enterprise_catalog_client.py +++ b/enterprise_access/apps/api_client/tests/test_enterprise_catalog_client.py @@ -5,11 +5,15 @@ from unittest import mock from uuid import uuid4 +import ddt from django.test import TestCase from requests import Response from requests.exceptions import HTTPError -from enterprise_access.apps.api_client.enterprise_catalog_client import EnterpriseCatalogApiClient +from enterprise_access.apps.api_client.enterprise_catalog_client import ( + EnterpriseCatalogApiClient, + EnterpriseCatalogApiV1Client +) class TestEnterpriseCatalogApiClient(TestCase): @@ -111,3 +115,56 @@ def test_get_content_metadata_count(self, mock_oauth_client): mock_oauth_client.return_value.get.assert_called_with( f'http://enterprise-catalog.example.com/api/v2/enterprise-catalogs/{catalog_uuid}/get_content_metadata/', ) + + +@ddt.ddt +class TestEnterpriseCatalogApiV1Client(TestCase): + """ + Test EnterpriseCatalogApiV1Client. + """ + + @ddt.data( + {'coerce_to_parent_course': False}, + {'coerce_to_parent_course': True}, + ) + @ddt.unpack + @mock.patch('enterprise_access.apps.api_client.base_oauth.OAuthAPIClient') + def test_content_metadata(self, mock_oauth_client, coerce_to_parent_course): + content_key = 'course+A' + mock_response_json = { + 'key': content_key, + 'other_metadata': 'foo', + } + + request_response = Response() + request_response.status_code = 200 + mock_oauth_client.return_value.get.return_value.json.return_value = mock_response_json + + client = EnterpriseCatalogApiV1Client() + fetched_metadata = client.content_metadata(content_key, coerce_to_parent_course=coerce_to_parent_course) + + self.assertEqual(fetched_metadata, mock_response_json) + expected_query_params_kwarg = {} + if coerce_to_parent_course: + expected_query_params_kwarg |= {'params': {'coerce_to_parent_course': True}} + mock_oauth_client.return_value.get.assert_called_with( + f'http://enterprise-catalog.example.com/api/v1/content-metadata/{content_key}', + **expected_query_params_kwarg, + ) + + @mock.patch('enterprise_access.apps.api_client.base_oauth.OAuthAPIClient') + def test_content_metadata_raises_http_error(self, mock_oauth_client): + content_key = 'course+A' + request_response = Response() + request_response.status_code = 400 + + mock_oauth_client.return_value.get.return_value = request_response + + client = EnterpriseCatalogApiV1Client() + + with self.assertRaises(HTTPError): + client.content_metadata(content_key) + + mock_oauth_client.return_value.get.assert_called_with( + f'http://enterprise-catalog.example.com/api/v1/content-metadata/{content_key}', + ) diff --git a/enterprise_access/apps/content_metadata/api.py b/enterprise_access/apps/content_metadata/api.py index ea9a5513..b1fe8d62 100644 --- a/enterprise_access/apps/content_metadata/api.py +++ b/enterprise_access/apps/content_metadata/api.py @@ -7,11 +7,11 @@ from django.conf import settings from django.core.cache import cache -from requests.exceptions import HTTPError +from edx_django_utils.cache import TieredCache from enterprise_access.cache_utils import versioned_cache_key -from ..api_client.enterprise_catalog_client import EnterpriseCatalogApiClient +from ..api_client.enterprise_catalog_client import EnterpriseCatalogApiClient, EnterpriseCatalogApiV1Client logger = logging.getLogger(__name__) @@ -64,7 +64,7 @@ def get_and_cache_catalog_content_metadata( # Here's the list of results fetched from the catalog service fetched_metadata = [] if keys_to_fetch: - fetched_metadata = _fetch_metadata_with_client(enterprise_catalog_uuid, keys_to_fetch) + fetched_metadata = _fetch_catalog_content_metadata_with_client(enterprise_catalog_uuid, keys_to_fetch) # Do a bulk set into the cache of everything we just had to fetch from the catalog service content_metadata_to_cache = {} @@ -91,22 +91,57 @@ def get_and_cache_catalog_content_metadata( return metadata_results_list -def _fetch_metadata_with_client(enterprise_catalog_uuid, content_keys): +def _fetch_catalog_content_metadata_with_client(enterprise_catalog_uuid, content_keys): """ Helper to isolate the task of fetching content metadata via our client. """ client = EnterpriseCatalogApiClient() - try: - response_payload = client.catalog_content_metadata( - enterprise_catalog_uuid, - list(content_keys), - ) - results = response_payload['results'] - logger.info( - 'Fetched content metadata in catalog %s for the following content keys: %s', - enterprise_catalog_uuid, - [record.get('key') for record in results], + response_payload = client.catalog_content_metadata( + enterprise_catalog_uuid, + list(content_keys), + ) + results = response_payload['results'] + logger.info( + 'Fetched content metadata in catalog %s for the following content keys: %s', + enterprise_catalog_uuid, + [record.get('key') for record in results], + ) + return results + + +def get_and_cache_content_metadata( + content_identifier, + coerce_to_parent_course=False, + timeout=settings.CONTENT_METADATA_CACHE_TIMEOUT, +): + """ + Fetch & cache content metadata from the enterprise-catalog catalog-/customer-agnostic endoint. + + Returns: + dict: Serialized content metadata from the enterprise-catalog API. + + Raises: + HTTPError: If there's a problem calling the enterprise-catalog API. + """ + cache_key = versioned_cache_key( + 'get_and_cache_content_metadata', + content_identifier, + f'coerce_to_parent_course={coerce_to_parent_course}', + ) + cached_response = TieredCache.get_cached_response(cache_key) + if cached_response.is_found: + return cached_response.value + + content_metadata = EnterpriseCatalogApiV1Client().content_metadata( + content_identifier, + coerce_to_parent_course=coerce_to_parent_course, + ) + if content_metadata: + TieredCache.set_all_tiers( + cache_key, + content_metadata, + django_cache_timeout=timeout, ) - return results - except HTTPError as exc: - raise exc + else: + logger.warning('Could not fetch metadata for content %s', content_identifier) + return content_metadata diff --git a/enterprise_access/apps/content_metadata/tests/test_api.py b/enterprise_access/apps/content_metadata/tests/test_api.py index 3e32f113..721244b4 100644 --- a/enterprise_access/apps/content_metadata/tests/test_api.py +++ b/enterprise_access/apps/content_metadata/tests/test_api.py @@ -6,6 +6,7 @@ from uuid import uuid4 from django.test import TestCase +from edx_django_utils.cache import TieredCache from requests.exceptions import HTTPError from enterprise_access.apps.content_metadata import api @@ -15,10 +16,15 @@ class TestContentMetadataApi(TestCase): """ Tests the content_metadata/api.py functions. """ + def tearDown(self): + super().tearDown() + # Clear the cache to prevent cache bleed across tests. + TieredCache.dangerous_clear_all_tiers() + @mock.patch('enterprise_access.apps.content_metadata.api.EnterpriseCatalogApiClient', autospec=True) - def test_get_and_cache_metadata_happy_path(self, mock_client_class): + def test_get_and_cache_catalog_content_metadata_happy_path(self, mock_client_class): content_keys = ['course+A', 'course+B'] - customer_uuid = uuid4() + catalog_uuid = uuid4() # Mock results from the catalog content metadata API endpoint. mock_result = { 'count': 2, @@ -29,17 +35,17 @@ def test_get_and_cache_metadata_happy_path(self, mock_client_class): mock_client = mock_client_class.return_value mock_client.catalog_content_metadata.return_value = mock_result - metadata_list = api.get_and_cache_catalog_content_metadata(customer_uuid, content_keys) + metadata_list = api.get_and_cache_catalog_content_metadata(catalog_uuid, content_keys) self.assertEqual(mock_client.catalog_content_metadata.call_count, 1) # tease apart the call args, because the client is passed a set() of content keys to fetch call_args, _ = mock_client.catalog_content_metadata.call_args_list[0] - self.assertEqual(call_args[0], customer_uuid) + self.assertEqual(call_args[0], catalog_uuid) self.assertEqual(sorted(call_args[1]), sorted(content_keys)) self.assertEqual(metadata_list, mock_result['results']) # ask again to hit the cache, ensure that we're still at only one client fetch - metadata_list = api.get_and_cache_catalog_content_metadata(customer_uuid, content_keys) + metadata_list = api.get_and_cache_catalog_content_metadata(catalog_uuid, content_keys) self.assertEqual(mock_client.catalog_content_metadata.call_count, 1) self.assertEqual(metadata_list, mock_result['results']) @@ -60,13 +66,13 @@ def test_get_and_cache_metadata_happy_path(self, mock_client_class): ], } - new_metadata_list = api.get_and_cache_catalog_content_metadata(customer_uuid, new_content_keys) + new_metadata_list = api.get_and_cache_catalog_content_metadata(catalog_uuid, new_content_keys) # Should only have requested courses C and D via the client # tease apart the call args, because the client is passed a set() of content keys to fetch self.assertEqual(mock_client.catalog_content_metadata.call_count, 2) call_args, _ = mock_client.catalog_content_metadata.call_args_list[1] - self.assertEqual(call_args[0], customer_uuid) + self.assertEqual(call_args[0], catalog_uuid) self.assertEqual(sorted(call_args[1]), ['course+C', 'course+D']) # And there will be results for courses B and C, but no result for course D @@ -79,16 +85,67 @@ def test_get_and_cache_metadata_happy_path(self, mock_client_class): ) @mock.patch('enterprise_access.apps.content_metadata.api.EnterpriseCatalogApiClient', autospec=True) - def test_get_and_cache_metadata_http_error(self, mock_client_class): + def test_get_and_cache_catalog_content_metadata_http_error(self, mock_client_class): content_keys = ['course+A', 'course+B'] - customer_uuid = uuid4() + catalog_uuid = uuid4() mock_client = mock_client_class.return_value mock_client.catalog_content_metadata.side_effect = HTTPError('oh barnacles') with self.assertRaisesRegex(HTTPError, 'oh barnacles'): - api.get_and_cache_catalog_content_metadata(customer_uuid, content_keys) + api.get_and_cache_catalog_content_metadata(catalog_uuid, content_keys) # tease apart the call args, because the client is passed a set() of content keys to fetch call_args, _ = mock_client.catalog_content_metadata.call_args_list[0] - self.assertEqual(call_args[0], customer_uuid) + self.assertEqual(call_args[0], catalog_uuid) self.assertEqual(sorted(call_args[1]), sorted(content_keys)) + + @mock.patch('enterprise_access.apps.content_metadata.api.EnterpriseCatalogApiV1Client', autospec=True) + def test_get_and_cache_content_metadata_happy_path(self, mock_client_class): + content_key = 'course+A' + # Mock results from the content metadata API endpoint. + mock_result = { + 'key': 'course+A', + 'data': 'things', + } + mock_client = mock_client_class.return_value + mock_client.content_metadata.return_value = mock_result + + metadata = api.get_and_cache_content_metadata(content_key) + + self.assertEqual(mock_client.content_metadata.call_count, 1) + call_args, _ = mock_client.content_metadata.call_args_list[0] + self.assertEqual(call_args[0], content_key) + self.assertEqual(metadata, mock_result) + + # ask again to hit the cache, ensure that we're still at only one client fetch + metadata = api.get_and_cache_content_metadata(content_key) + + self.assertEqual(mock_client.content_metadata.call_count, 1) + self.assertEqual(metadata, mock_result) + + # Now get-and-cache again, but this time request key is different. + new_content_key = 'course+B' + new_mock_result = { + 'key': 'course+B', + 'data': 'things', + } + mock_client.content_metadata.return_value = new_mock_result + + new_metadata = api.get_and_cache_content_metadata(new_content_key) + + self.assertEqual(mock_client.content_metadata.call_count, 2) + call_args, _ = mock_client.content_metadata.call_args_list[-1] + self.assertEqual(call_args[0], new_content_key) + self.assertEqual(new_metadata, new_mock_result) + + @mock.patch('enterprise_access.apps.content_metadata.api.EnterpriseCatalogApiV1Client', autospec=True) + def test_get_and_cache_content_metadata_http_error(self, mock_client_class): + content_key = 'course+A' + mock_client = mock_client_class.return_value + mock_client.content_metadata.side_effect = HTTPError('oh barnacles') + + with self.assertRaisesRegex(HTTPError, 'oh barnacles'): + api.get_and_cache_content_metadata(content_key) + + call_args, _ = mock_client.content_metadata.call_args_list[0] + self.assertEqual(call_args[0], content_key) diff --git a/enterprise_access/apps/subsidy_access_policy/content_metadata_api.py b/enterprise_access/apps/subsidy_access_policy/content_metadata_api.py index c3288ad9..2a5c2be9 100644 --- a/enterprise_access/apps/subsidy_access_policy/content_metadata_api.py +++ b/enterprise_access/apps/subsidy_access_policy/content_metadata_api.py @@ -111,21 +111,36 @@ def get_list_price_for_content(enterprise_customer_uuid, content_key, content_me raise ContentPriceNullException(f'Could not determine list price for {content_key}') from exc # Note that the "content_price" key is guaranteed to exist, but the value may be None. - return list_price_dict_from_usd_cents(content_metadata['content_price']) + return make_list_price_dict(integer_cents=content_metadata['content_price']) -def list_price_dict_from_usd_cents(list_price_integer_cents): +def make_list_price_dict(decimal_dollars=None, integer_cents=None): """ - Helper to compute a list price dictionary given the non-negative price of the content in USD cents. + Helper to compute a list price dictionary given the non-negative price of the content. + + Usage: + list_price_dict = make_list_price_dict(decimal_dollars=100.00) + list_price_dict = make_list_price_dict(integer_cents=10000) + + Args: + decimal_dollars (float): Content price in USD dollars. Must only be specified if ``integer_cents`` is None. + integer_cents (int): Content price in USD cents. Must only be specified if ``decimal_dollars`` is None. + + Raises: + ValueError if neither OR both arguments were specified. """ - list_price_decimal_dollars = None - if list_price_integer_cents is not None: - list_price_decimal_dollars = Decimal(list_price_integer_cents) / 100 - - return { - "usd": list_price_decimal_dollars, - "usd_cents": list_price_integer_cents, - } + if decimal_dollars is None and integer_cents is not None: + return { + "usd": Decimal(integer_cents) / 100, + "usd_cents": integer_cents, + } + elif decimal_dollars is not None and integer_cents is None: + return { + "usd": decimal_dollars, + "usd_cents": int(Decimal(decimal_dollars) * 100), + } + else: + raise ValueError("Only one of `decimal_dollars` or `integer_cents` can be passed.") def enroll_by_datetime(content_metadata): diff --git a/enterprise_access/apps/subsidy_access_policy/models.py b/enterprise_access/apps/subsidy_access_policy/models.py index ddad7e99..1d9bdfb5 100644 --- a/enterprise_access/apps/subsidy_access_policy/models.py +++ b/enterprise_access/apps/subsidy_access_policy/models.py @@ -52,7 +52,7 @@ get_and_cache_catalog_contains_content, get_and_cache_content_metadata, get_list_price_for_content, - list_price_dict_from_usd_cents + make_list_price_dict ) from .customer_api import get_and_cache_enterprise_learner_record from .exceptions import ( @@ -1391,7 +1391,7 @@ def get_list_price(self, lms_user_id, content_key): return super().get_list_price(lms_user_id, content_key) # an assignment's content_quantity is always <= 0 to express the fact # that value has been consumed from a subsidy (though not necessarily fulfilled) - return list_price_dict_from_usd_cents(found_assignment.content_quantity * -1) + return make_list_price_dict(integer_cents=found_assignment.content_quantity * -1) def can_redeem( self, lms_user_id, content_key, diff --git a/enterprise_access/apps/subsidy_access_policy/subsidy_api.py b/enterprise_access/apps/subsidy_access_policy/subsidy_api.py index 3dc0152c..b213e475 100644 --- a/enterprise_access/apps/subsidy_access_policy/subsidy_api.py +++ b/enterprise_access/apps/subsidy_access_policy/subsidy_api.py @@ -134,7 +134,7 @@ def get_redemptions_by_content_and_policy_for_learner(policies, lms_user_id): """ policies_by_subsidy_uuid = defaultdict(set) for policy in policies: - policies_by_subsidy_uuid[policy.subsidy_uuid].add(str(policy.uuid)) + policies_by_subsidy_uuid[policy.subsidy_uuid].add(policy) result = defaultdict(lambda: defaultdict(list)) @@ -146,8 +146,15 @@ def get_redemptions_by_content_and_policy_for_learner(policies, lms_user_id): content_key = redemption['content_key'] subsidy_access_policy_uuid = redemption['subsidy_access_policy_uuid'] - if subsidy_access_policy_uuid in policies_with_subsidy: - result[content_key][subsidy_access_policy_uuid].append(redemption) + matching_policies = [ + policy for policy in policies_with_subsidy + if str(policy.uuid) == subsidy_access_policy_uuid + ] + # We can assume there's at most one matching policy because the ``policies`` arg passed into this function + # should not contain duplicates. + matching_policy = matching_policies[0] if matching_policies else None + if matching_policy: + result[content_key][matching_policy].append(redemption) else: logger.warning( f"Transaction {transaction_uuid} has unmatched policy uuid for subsidy {subsidy_uuid}: " diff --git a/enterprise_access/apps/subsidy_access_policy/tests/test_content_metadata_api.py b/enterprise_access/apps/subsidy_access_policy/tests/test_content_metadata_api.py new file mode 100644 index 00000000..126a48d0 --- /dev/null +++ b/enterprise_access/apps/subsidy_access_policy/tests/test_content_metadata_api.py @@ -0,0 +1,67 @@ +""" +Test content_metadata_api.py +""" +import contextlib + +import ddt +from django.test import TestCase + +from enterprise_access.apps.subsidy_access_policy.content_metadata_api import make_list_price_dict + + +@ddt.ddt +class ContentMetadataApiTests(TestCase): + """ + Test various functions inside content_metadata_api.py + """ + + @ddt.data( + # Standard happy path. + { + "decimal_dollars": 10.5, + "integer_cents": None, + "expected_result": {"usd": 10.5, "usd_cents": 1050}, + }, + # Standard happy path. + { + "decimal_dollars": None, + "integer_cents": 1050, + "expected_result": {"usd": 10.5, "usd_cents": 1050}, + }, + # Weird precision input just gets passed along as-is. + { + "decimal_dollars": 10.503, + "integer_cents": None, + "expected_result": {"usd": 10.503, "usd_cents": 1050}, + }, + # All None. + { + "decimal_dollars": None, + "integer_cents": None, + "expect_raises": ValueError, + }, + # All defined. + { + "decimal_dollars": 10.5, + "integer_cents": 1050, + "expect_raises": ValueError, + }, + ) + @ddt.unpack + def test_make_list_price_dict( + self, + decimal_dollars, + integer_cents, + expected_result=None, + expect_raises=None, + ): + cm = contextlib.nullcontext() + if expect_raises: + cm = self.assertRaises(expect_raises) + with cm: + actual_result = make_list_price_dict( + decimal_dollars=decimal_dollars, + integer_cents=integer_cents, + ) + if not expect_raises: + assert actual_result == expected_result diff --git a/enterprise_access/apps/subsidy_access_policy/tests/test_subsidy_api.py b/enterprise_access/apps/subsidy_access_policy/tests/test_subsidy_api.py index 476299f4..c31e4193 100644 --- a/enterprise_access/apps/subsidy_access_policy/tests/test_subsidy_api.py +++ b/enterprise_access/apps/subsidy_access_policy/tests/test_subsidy_api.py @@ -145,9 +145,9 @@ def test_redemptions_by_content_and_policy(self, mock_transaction_cache): self.assertEqual( { - 'content-1': {str(cherry_policy.uuid): [mock_pie_transactions[0]]}, - 'content-2': {str(apple_policy.uuid): [mock_pie_transactions[1]]}, - 'content-3': {str(german_chocolate_policy.uuid): [mock_cake_transactions[0]]}, + 'content-1': {cherry_policy: [mock_pie_transactions[0]]}, + 'content-2': {apple_policy: [mock_pie_transactions[1]]}, + 'content-3': {german_chocolate_policy: [mock_cake_transactions[0]]}, }, result, )