Merge pull request #2314 from openedx/pwnage101/ENT-9840

fix: enrollment intention saves should not be blocked on catalog inclusion

docs/decisions/0015-default-enrollments.rst

@@ -65,7 +65,7 @@ will always primarily be associated with the course run associated with the ``De
 * If the content_key is a specific course run, we'll always try to enroll in the explicitly
   configured course run key (not the advertised course run).
 
-This content_key will be passed to the ``EnterpriseCatalogApiClient().get_content_metadata_content_identifier``
+This content_key will be passed to the ``EnterpriseCatalogApiClient().get_customer_content_metadata_content_identifier``
 method. When passing a course run key to this endpoint, it'll actually return the top-level *course* metadata
 associated with the course run, not just the specified course run's metadata
 (this is primarily due to catalogs containing courses, not course runs, and we generally say that

enterprise/api_client/discovery.py

@@ -283,7 +283,7 @@ def get_course_run(self, course_run_id):
             course_run_id(string): Course run ID (aka Course Key) in string format.
 
         Returns:
-            dict: Course run data provided by Course Catalog API.
+            dict: Course run data provided by Course Catalog API, or empty dict if not found.
 
         """
         return self._load_data(

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
 
@@ -29,8 +33,9 @@ class EnterpriseCatalogApiClient(UserAPIClient):
     REFRESH_CATALOG_ENDPOINT = ENTERPRISE_CATALOG_ENDPOINT + '/{}/refresh_metadata'
     CATALOG_DIFF_ENDPOINT = ENTERPRISE_CATALOG_ENDPOINT + '/{}/generate_diff'
     ENTERPRISE_CUSTOMER_ENDPOINT = 'enterprise-customer'
-    CONTENT_METADATA_IDENTIFIER_ENDPOINT = ENTERPRISE_CUSTOMER_ENDPOINT + \
+    CUSTOMER_CONTENT_METADATA_IDENTIFIER_ENDPOINT = ENTERPRISE_CUSTOMER_ENDPOINT + \
         "/{}/content-metadata/" + "{}"
+    CONTENT_METADATA_IDENTIFIER_ENDPOINT = "content-metadata/"
     CATALOG_QUERIES_ENDPOINT = 'catalog-queries'
     GET_CONTENT_FILTER_HASH_ENDPOINT = CATALOG_QUERIES_ENDPOINT + '/get_content_filter_hash'
     GET_QUERY_BY_HASH_ENDPOINT = CATALOG_QUERIES_ENDPOINT + '/get_query_by_hash?hash={}'
@@ -386,26 +391,59 @@ def enterprise_contains_content_items(self, enterprise_uuid, content_ids):
         return response.json()
 
     @UserAPIClient.refresh_token
-    def get_content_metadata_content_identifier(self, enterprise_uuid, content_id):
+    def get_customer_content_metadata_content_identifier(self, enterprise_uuid, content_id):
         """
         Return all content metadata contained in the catalogs associated with the
         given EnterpriseCustomer and content_id.
         """
         try:
             api_url = self.get_api_url(
-                f"{self.CONTENT_METADATA_IDENTIFIER_ENDPOINT.format(enterprise_uuid, content_id)}"
+                f"{self.CUSTOMER_CONTENT_METADATA_IDENTIFIER_ENDPOINT.format(enterprise_uuid, content_id)}"
             )
             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,
                 content_id,
                 str(exc),
             )
             return {}
+        except (RequestException, ConnectionError, Timeout) as exc:
+            LOGGER.exception(
+                (
+                    "Exception raised in "
+                    "EnterpriseCatalogApiClient::get_customer_content_metadata_content_identifier: [%s]"
+                ),
+                str(exc),
+            )
+            return {}
+
+    @UserAPIClient.refresh_token
+    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 = {}
+            if coerce_to_parent_course:
+                query_params["coerce_to_parent_course"] = True
+            response = self.client.get(
+                api_url + content_id,
+                params=query_params,
+            )
+            response.raise_for_status()
+            return response.json()
+        except HTTPError as exc:
+            LOGGER.exception(
+                "No matching content found for content_id: [%s], Error: %s",
+                content_id,
+                str(exc),
+            )
+            return {}
         except (RequestException, ConnectionError, Timeout) as exc:
             LOGGER.exception(
                 "Exception raised in EnterpriseCatalogApiClient::get_content_metadata_content_identifier: [%s]",

enterprise/content_metadata/api.py

@@ -16,6 +16,46 @@
 DEFAULT_CACHE_TIMEOUT = getattr(settings, 'CONTENT_METADATA_CACHE_TIMEOUT', 60 * 5)
 
 
+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.
+
+    The response is cached in a ``TieredCache`` (meaning in both the RequestCache,
+    _and_ the django cache for the configured expiration period).
+
+    Returns: A dict with content metadata for the given key.
+    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, 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} 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
+
+    if not result:
+        logger.warning('No content found for content_key %s', content_key)
+        return {}
+
+    logger.info(
+        '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)
+    return result
+
+
 def get_and_cache_customer_content_metadata(enterprise_customer_uuid, content_key, timeout=None):
     """
     Returns the metadata corresponding to the requested
@@ -28,14 +68,18 @@ def get_and_cache_customer_content_metadata(enterprise_customer_uuid, content_ke
     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', enterprise_customer_uuid, content_key)
+    cache_key = versioned_cache_key(
+        'get_customer_content_metadata_content_identifier',
+        enterprise_customer_uuid,
+        content_key,
+    )
     cached_response = TieredCache.get_cached_response(cache_key)
     if cached_response.is_found:
         logger.info(f'cache hit for enterprise customer {enterprise_customer_uuid} and content {content_key}')
         return cached_response.value
 
     try:
-        result = EnterpriseCatalogApiClient().get_content_metadata_content_identifier(
+        result = EnterpriseCatalogApiClient().get_customer_content_metadata_content_identifier(
             enterprise_uuid=enterprise_customer_uuid,
             content_id=content_key,
         )

enterprise/models.py

@@ -62,7 +62,7 @@
     json_serialized_course_modes,
 )
 from enterprise.content_metadata.api import (
-    get_and_cache_customer_content_metadata,
+    get_and_cache_content_metadata,
     get_and_cache_enterprise_contains_content_items,
 )
 from enterprise.errors import LinkUserToEnterpriseError
@@ -2543,12 +2543,18 @@ class Meta:
     @property
     def content_metadata_for_content_key(self):
         """
-        Retrieves the content metadata for the instance's enterprise customer and content key.
+        Retrieves the content metadata for the instance's content key.
+
+        NOTE (ENT-9840): Prior versions of this method used `get_and_cache_customer_content_metadata()` instead of
+        `get_and_cache_content_metadata()`. The goal was to ensure that model saves only succeed when the requested
+        content is actually contained in the customer's catalogs.  However, as part of ENT-9840 we are relaxing this
+        requirement because delays in discovery->catalog replication can easily result in default intentions being
+        un-saveable when simultaneously modifying catalog definitions to include the content.
         """
         try:
-            return get_and_cache_customer_content_metadata(
-                enterprise_customer_uuid=self.enterprise_customer.uuid,
+            return get_and_cache_content_metadata(
                 content_key=self.content_key,
+                coerce_to_parent_course=True,
             )
         except HTTPError as e:
             LOGGER.error(
@@ -2640,6 +2646,8 @@ def applicable_enterprise_catalog_uuids(self):
             enterprise_customer_uuid=self.enterprise_customer.uuid,
             content_keys=[self.course_run_key],
         )
+        if not contains_content_items_response.get('contains_content_items'):
+            return []
         return contains_content_items_response.get('catalog_list', [])
 
     def determine_content_type(self):
@@ -2695,6 +2703,11 @@ def clean(self):
         if not self.course_run:
             # NOTE: This validation check also acts as an inferred check on the derived content_type
             # from the content metadata.
+            # NOTE 2: This check does NOT assert that the content is actually contained in the
+            # customer's catalogs. ADR 0015, as written, would _like_ for that check to exist in
+            # this clean() method, but that has proven infeasible due to the nature of how data
+            # replication delays from discovery->catalog cause contains_content_items calls to be
+            # temporarily unreliable. Instead this only checks that the content key exists at all.
             raise ValidationError({
                 'content_key': _('The content key did not resolve to a valid course run.')
             })

integrated_channels/degreed2/client.py

@@ -349,7 +349,7 @@ def _fetch_and_assign_skills_to_course(self, external_id):
         # We need to do 2 steps here:
         # 1. Fetch skills from enterprise-catalog
 
-        metadata = self.enterprise_catalog_api_client.get_content_metadata_content_identifier(
+        metadata = self.enterprise_catalog_api_client.get_customer_content_metadata_content_identifier(
             enterprise_uuid=self.enterprise_configuration.enterprise_customer.uuid,
             content_id=external_id)
         LOGGER.info(

tests/test_enterprise/api/test_default_enrollment_views.py

@@ -200,7 +200,6 @@ def test_default_enterprise_enrollment_intentions_not_in_catalog(self, mock_cata
         enrollment_intention = create_mock_default_enterprise_enrollment_intention(
             enterprise_customer=self.enterprise_customer,
             mock_catalog_api_client=mock_catalog_api_client,
-            contains_content_items=False,
             catalog_list=[],
         )
         query_params = f'enterprise_customer_uuid={str(self.enterprise_customer.uuid)}'
@@ -370,7 +369,6 @@ def test_default_enrollment_intentions_learner_status_content_not_in_catalog(
         enrollment_intention = create_mock_default_enterprise_enrollment_intention(
             enterprise_customer=self.enterprise_customer,
             mock_catalog_api_client=mock_catalog_api_client,
-            contains_content_items=False,
             catalog_list=[],
         )
         mock_get_best_mode_from_course_key.return_value = VERIFIED_COURSE_MODE

tests/test_enterprise/api/test_views.py

@@ -16,6 +16,7 @@
 import ddt
 import pytz
 import responses
+from edx_django_utils.cache import TieredCache
 from edx_toggles.toggles.testutils import override_waffle_flag
 from faker import Faker
 from oauth2_provider.models import get_application_model
@@ -177,19 +178,18 @@ def create_mock_default_enterprise_enrollment_intention(
     enterprise_customer,
     mock_catalog_api_client,
     content_metadata=None,
-    contains_content_items=False,
     catalog_list=None,
 ):
     """
     Create a mock default enterprise enrollment intention.
     """
     mock_content_metadata = content_metadata or fake_catalog_api.FAKE_COURSE
-    mock_contains_content_items = contains_content_items
     mock_catalog_list = (
         catalog_list
         if catalog_list is not None
         else [fake_catalog_api.FAKE_CATALOG_RESULT.get('uuid')]
     )
+    mock_contains_content_items = bool(mock_catalog_list)
 
     mock_catalog_api_client.return_value = mock.Mock(
         get_content_metadata_content_identifier=mock.Mock(
@@ -239,6 +239,13 @@ def setUp(self):
         super().setUp()
         self.set_jwt_cookie(ENTERPRISE_OPERATOR_ROLE, ALL_ACCESS_CONTEXT)
 
+    def tearDown(self):
+        """
+        Clears TieredCache so that mock catalog API calls do not bleed across tests.
+        """
+        super().tearDown()
+        TieredCache.dangerous_clear_all_tiers()
+
     @staticmethod
     def create_user(username=TEST_USERNAME, password=TEST_PASSWORD, **kwargs):
         # AED 2024-06-12: For simplicity, I've tried to refactor the test setup in the base APITest