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..0a8bde1c 100755 --- a/enterprise_access/apps/api/v1/views/subsidy_access_policy.py +++ b/enterprise_access/apps/api/v1/views/subsidy_access_policy.py @@ -746,11 +746,16 @@ def can_redeem(self, request, enterprise_customer_uuid): resolved_policy = None list_price = 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 = { + redemption: 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 ] @@ -785,25 +790,17 @@ def can_redeem(self, request, enterprise_customer_uuid): if redeemable_policies: resolved_policy = SubsidyAccessPolicy.resolve_policy(redeemable_policies) - try: - if resolved_policy: - list_price = 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) - except ContentPriceNullException as exc: - raise RedemptionRequestException( - detail=f'Could not determine list price for content_key: {content_key}', - ) from exc + if resolved_policy: + list_price = resolved_policy.get_list_price(lms_user_id, content_key) + elif successful_redemptions: + # Get the policy used at time of successful 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, hence the need for this. + successfully_redeemed_policy = policy_by_redemption[successful_redemptions[0]] + list_price = successfully_redeemed_policy.get_list_price(lms_user_id, content_key) + else: + content_metadata = get_and_cache_content_metadata(content_key) + list_price = extract_price_from_metadata... element_response = { "content_key": content_key, diff --git a/enterprise_access/apps/api_client/enterprise_catalog_client.py b/enterprise_access/apps/api_client/enterprise_catalog_client.py index d1941530..cb304631 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): + self.content_metadata_endpoint = urljoin(self.api_base_url, 'content-metadata/') + super().__init__() + + @backoff.on_exception(wait_gen=backoff.expo, exception=autoretry_for_exceptions) + def content_metadata(self, content_id): + """ + 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 = {'content_identifiers': content_id} + endpoint = self.content_metadata_endpoint + response = self.client.get(endpoint, params=query_params) + response.raise_for_status() + response_json = response.json() + if results := response_json.get('results'): + return results[0] + return None diff --git a/enterprise_access/apps/content_metadata/api.py b/enterprise_access/apps/content_metadata/api.py index ea9a5513..4c24e156 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,73 @@ 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, + timeout=settings.CONTENT_METADATA_CACHE_TIMEOUT, + **kwargs, +): + """ + Returns the metadata corresponding to the requested + ``content_keys`` within the provided ``enterprise_catalog_uuid``, + as told by the enterprise-access service. Utilizes a cache per-content-record, + that is, each combination of (enterprise_catalog_uuid, key) for key in content_keys + is cached independently. + + Returns: A list of dictionaries containing content metadata for the given keys. + Raises: An HTTPError if there's a problem getting the content metadata + via the enterprise-catalog service. + """ + cache_key = versioned_cache_key('get_and_cache_content_metadata', content_identifier) + cached_response = TieredCache.get_cached_response(cache_key) + if cached_response.is_found: + return cached_response.value + + content_metadata = EnterpriseCatalogApiV1Client().content_metadata( + content_identifier, + **kwargs, + ) + 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 + + +def _fetch_content_metadata_with_client(enterprise_catalog_uuid, content_keys): + """ + Helper to isolate the task of fetching content metadata via our client. + """ + client = EnterpriseCatalogApiClient() + 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 diff --git a/enterprise_access/apps/subsidy_access_policy/subsidy_api.py b/enterprise_access/apps/subsidy_access_policy/subsidy_api.py index 3dc0152c..ec63babe 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,14 +146,19 @@ 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) - else: + try: + matching_policy = next( + policy for policy in policies_with_subsidy + if str(policy.uuid) == subsidy_access_policy_uuid + ) + except StopIteration: logger.warning( f"Transaction {transaction_uuid} has unmatched policy uuid for subsidy {subsidy_uuid}: " f"Found policy uuid {subsidy_access_policy_uuid} that is no longer tied to this subsidy." ) + result[content_key][matching_policy].append(redemption) + return result