From 1d696b33a37a70351a808aa1227c14a21ae638c3 Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Wed, 22 Jan 2025 14:50:08 -0800 Subject: [PATCH] fix: use coerce_to_parent_course when fetching content for default enrollment intentions ENT-9840 --- enterprise/api_client/enterprise_catalog.py | 22 ++-- enterprise/content_metadata/api.py | 10 +- enterprise/models.py | 1 + .../api_client/test_enterprise_catalog.py | 117 ++++++++++++++++++ 4 files changed, 138 insertions(+), 12 deletions(-) diff --git a/enterprise/api_client/enterprise_catalog.py b/enterprise/api_client/enterprise_catalog.py index f56d88fab..a4c89589b 100644 --- a/enterprise/api_client/enterprise_catalog.py +++ b/enterprise/api_client/enterprise_catalog.py @@ -7,8 +7,12 @@ from logging import getLogger from urllib.parse import urljoin -from requests.exceptions import ConnectionError, RequestException, Timeout # pylint: disable=redefined-builtin -from rest_framework.exceptions import NotFound +from requests.exceptions import ( # pylint: disable=redefined-builtin + ConnectionError, + HTTPError, + RequestException, + Timeout, +) from django.conf import settings @@ -399,7 +403,7 @@ def get_customer_content_metadata_content_identifier(self, enterprise_uuid, cont response = self.client.get(api_url) response.raise_for_status() return response.json() - except NotFound as exc: + except HTTPError as exc: LOGGER.exception( "No matching content found in catalog for customer: [%s] or content_id: [%s], Error: %s", enterprise_uuid, @@ -418,22 +422,24 @@ def get_customer_content_metadata_content_identifier(self, enterprise_uuid, cont return {} @UserAPIClient.refresh_token - def get_content_metadata_content_identifier(self, content_id): + def get_content_metadata_content_identifier(self, content_id, coerce_to_parent_course=False): """ Return the content metadata for the content_id. """ try: api_url = self.get_api_url(self.CONTENT_METADATA_IDENTIFIER_ENDPOINT) - query_params = {"content_identifiers": content_id} + query_params = {} + if coerce_to_parent_course: + query_params["coerce_to_parent_course"] = True response = self.client.get( - api_url, + api_url + content_id, params=query_params, ) response.raise_for_status() return response.json() - except NotFound as exc: + except HTTPError as exc: LOGGER.exception( - "No matching content found in catalog for content_id: [%s], Error: %s", + "No matching content found for content_id: [%s], Error: %s", content_id, str(exc), ) diff --git a/enterprise/content_metadata/api.py b/enterprise/content_metadata/api.py index 59ab35d7e..52222ab6f 100644 --- a/enterprise/content_metadata/api.py +++ b/enterprise/content_metadata/api.py @@ -16,7 +16,7 @@ DEFAULT_CACHE_TIMEOUT = getattr(settings, 'CONTENT_METADATA_CACHE_TIMEOUT', 60 * 5) -def get_and_cache_content_metadata(content_key, timeout=None): +def get_and_cache_content_metadata(content_key, timeout=None, coerce_to_parent_course=False): """ Returns the metadata corresponding to the requested ``content_key`` REGARDLESS of catalog/customer associations. @@ -28,15 +28,16 @@ def get_and_cache_content_metadata(content_key, timeout=None): Raises: An HTTPError if there's a problem getting the content metadata via the enterprise-catalog service. """ - cache_key = versioned_cache_key('get_content_metadata_content_identifier', content_key) + cache_key = versioned_cache_key('get_content_metadata_content_identifier', content_key, coerce_to_parent_course) cached_response = TieredCache.get_cached_response(cache_key) if cached_response.is_found: - logger.info(f'cache hit for content_key {content_key}') + logger.info(f'cache hit for content_key {content_key} and coerce_to_parent_course={coerce_to_parent_course}') return cached_response.value try: result = EnterpriseCatalogApiClient().get_content_metadata_content_identifier( content_id=content_key, + coerce_to_parent_course=coerce_to_parent_course, ) except HTTPError as exc: raise exc @@ -46,8 +47,9 @@ def get_and_cache_content_metadata(content_key, timeout=None): return {} logger.info( - 'Fetched catalog for content_key %s. Result = %s', + 'Fetched catalog for content_key %s and coerce_to_parent_course=%s. Result = %s', content_key, + coerce_to_parent_course, result, ) TieredCache.set_all_tiers(cache_key, result, timeout or DEFAULT_CACHE_TIMEOUT) diff --git a/enterprise/models.py b/enterprise/models.py index ae637c3b3..912cbc149 100644 --- a/enterprise/models.py +++ b/enterprise/models.py @@ -2554,6 +2554,7 @@ def content_metadata_for_content_key(self): try: return get_and_cache_content_metadata( content_key=self.content_key, + coerce_to_parent_course=True, ) except HTTPError as e: LOGGER.error( diff --git a/tests/test_enterprise/api_client/test_enterprise_catalog.py b/tests/test_enterprise/api_client/test_enterprise_catalog.py index 98be0c5f1..83135a05e 100644 --- a/tests/test_enterprise/api_client/test_enterprise_catalog.py +++ b/tests/test_enterprise/api_client/test_enterprise_catalog.py @@ -9,6 +9,7 @@ import responses from pytest import mark, raises from requests.exceptions import ConnectionError, RequestException, Timeout # pylint: disable=redefined-builtin +from testfixtures import LogCapture from enterprise.api_client import enterprise_catalog from enterprise.models import EnterpriseCustomerCatalog @@ -652,3 +653,119 @@ def test_get_content_metadata_without_enterprise_catalogs(): request_url = responses.calls[0][0].url assert url == request_url + + +@responses.activate +@mark.django_db +@mock.patch('enterprise.api_client.client.JwtBuilder', mock.Mock()) +def test_get_customer_content_metadata_content_identifier(): + responses.reset() + client = enterprise_catalog.EnterpriseCatalogApiClient('staff-user-goes-here') + + # Set up a mock response for requesting a course (via run key). + url = _url(f"enterprise-customer/{TEST_ENTERPRISE_ID}/content-metadata/course-v1:org+course+run/") + expected_response = { + 'content_type': 'course', + 'key': 'org+course', + 'data': 'foo', + } + responses.add(responses.GET, url, json=expected_response) + + # Set up a mock response for requesting content that does not exist. + url = _url(f"enterprise-customer/{TEST_ENTERPRISE_ID}/content-metadata/course-v1:does+not+exist/") + responses.add(responses.GET, url, status=404) + + # Test requesting the course (via run key) directly. + results = client.get_customer_content_metadata_content_identifier(TEST_ENTERPRISE_ID, 'course-v1:org+course+run') + expected_results = { + 'content_type': 'course', + 'key': 'org+course', + 'data': 'foo', + } + assert results == expected_results + + # Test requesting non-existing content. + with LogCapture(level=logging.INFO) as log_capture: + results = client.get_customer_content_metadata_content_identifier( + TEST_ENTERPRISE_ID, + 'course-v1:does+not+exist', + ) + expected_results = {} + assert results == expected_results + logging_messages = [log_msg.getMessage() for log_msg in log_capture.records] + assert any('No matching content found' in message for message in logging_messages) + + # Test making a request, but the connection fails. + with LogCapture(level=logging.INFO) as log_capture: + results = client.get_customer_content_metadata_content_identifier( + TEST_ENTERPRISE_ID, + 'course-v1:does+not+match', + ) + expected_results = {} + assert results == expected_results + logging_messages = [log_msg.getMessage() for log_msg in log_capture.records] + assert any('Exception raised in' in message for message in logging_messages) + + +@responses.activate +@mark.django_db +@mock.patch('enterprise.api_client.client.JwtBuilder', mock.Mock()) +def test_get_content_metadata_content_identifier(): + responses.reset() + client = enterprise_catalog.EnterpriseCatalogApiClient('staff-user-goes-here') + + # Set up a mock response for requesting a run directly. + url = _url('content-metadata/course-v1:org+course+run') + expected_response = { + 'content_type': 'courserun', + 'key': 'course-v1:org+course+run', + 'data': 'foo', + } + responses.add(responses.GET, url, json=expected_response) + + # Set up a mock response for requesting a run, but asking it to be coerced to a parent course. + url = _url('content-metadata/course-v1:org+course+run/?coerce_to_parent_course=true') + expected_response = { + 'content_type': 'course', + 'key': 'org+course', + 'data': 'foo', + } + responses.add(responses.GET, url, json=expected_response) + + # Set up a mock response for requesting content that does not exist. + url = _url('content-metadata/course-v1:does+not+exist') + responses.add(responses.GET, url, status=404) + + # Test requesting the run directly. + results = client.get_content_metadata_content_identifier('course-v1:org+course+run') + expected_results = { + 'content_type': 'courserun', + 'key': 'course-v1:org+course+run', + 'data': 'foo', + } + assert results == expected_results + + # Test requesting the run, asking it to be coerced to a parent course. + results = client.get_content_metadata_content_identifier('course-v1:org+course+run', coerce_to_parent_course=True) + expected_results = { + 'content_type': 'courserun', + 'key': 'course-v1:org+course+run', + 'data': 'foo', + } + assert results == expected_results + + # Test requesting non-existing content. + with LogCapture(level=logging.INFO) as log_capture: + results = client.get_content_metadata_content_identifier('course-v1:does+not+exist') + expected_results = {} + assert results == expected_results + logging_messages = [log_msg.getMessage() for log_msg in log_capture.records] + assert any('No matching content found' in message for message in logging_messages) + + # Test making a request, but the connection fails. + with LogCapture(level=logging.INFO) as log_capture: + results = client.get_content_metadata_content_identifier('course-v1:does+not+match') + expected_results = {} + assert results == expected_results + logging_messages = [log_msg.getMessage() for log_msg in log_capture.records] + assert any('Exception raised in' in message for message in logging_messages)