From 296dba1bef5e57516e9e10e7b18f0f3e785853bd Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Tue, 14 Jan 2025 14:32:49 -0800 Subject: [PATCH] fix: enrollment intention saves should not be blocked on catalog inclusion Before this commit, normal replication delays from discovery->catalog can easily result in enrollment intentions being un-saveable when simultaneously modifying catalog definitions to include the content. Conversely, normal replication delays can also allow saving an enrollment intention that would eventually become invalid due to content being removed from the catalog, creating a false sense of correctness. After this commit, we no longer assert during intention save() that the customer's catalogs actually contain the content. Instead, we assert that the content exists at all in the enterprise-catalog database by querying the customer- and catalog-agnostic `/api/v1/content-metadata/?content_identifiers=` endpoint. Subsequent commits/PRs may introduce dynamic warning messaging (non-blocking) on django admin pages for the customer and intention when the intention's content key is not contained in the customer's catalogs. ENT-9840 --- docs/decisions/0015-default-enrollments.rst | 2 +- enterprise/api_client/discovery.py | 2 +- enterprise/api_client/enterprise_catalog.py | 38 +++++++++++++-- enterprise/content_metadata/api.py | 46 ++++++++++++++++++- enterprise/models.py | 20 ++++++-- integrated_channels/degreed2/client.py | 2 +- .../api/test_default_enrollment_views.py | 2 - tests/test_enterprise/api/test_views.py | 11 ++++- .../test_degreed2/test_client.py | 12 ++--- tests/test_models.py | 24 +++++----- 10 files changed, 125 insertions(+), 34 deletions(-) diff --git a/docs/decisions/0015-default-enrollments.rst b/docs/decisions/0015-default-enrollments.rst index 2622860069..a65590a76a 100644 --- a/docs/decisions/0015-default-enrollments.rst +++ b/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 diff --git a/enterprise/api_client/discovery.py b/enterprise/api_client/discovery.py index 3e39ce973d..1bc1b2231c 100644 --- a/enterprise/api_client/discovery.py +++ b/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( diff --git a/enterprise/api_client/enterprise_catalog.py b/enterprise/api_client/enterprise_catalog.py index 1ac0111728..f56d88fab9 100644 --- a/enterprise/api_client/enterprise_catalog.py +++ b/enterprise/api_client/enterprise_catalog.py @@ -29,8 +29,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,14 +387,14 @@ 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() @@ -406,6 +407,37 @@ def get_content_metadata_content_identifier(self, 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): + """ + 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} + response = self.client.get( + api_url, + params=query_params, + ) + response.raise_for_status() + return response.json() + except NotFound as exc: + LOGGER.exception( + "No matching content found in catalog 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]", diff --git a/enterprise/content_metadata/api.py b/enterprise/content_metadata/api.py index ac109e5ce7..59ab35d7e5 100644 --- a/enterprise/content_metadata/api.py +++ b/enterprise/content_metadata/api.py @@ -16,6 +16,44 @@ DEFAULT_CACHE_TIMEOUT = getattr(settings, 'CONTENT_METADATA_CACHE_TIMEOUT', 60 * 5) +def get_and_cache_content_metadata(content_key, timeout=None): + """ + 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) + cached_response = TieredCache.get_cached_response(cache_key) + if cached_response.is_found: + logger.info(f'cache hit for content_key {content_key}') + return cached_response.value + + try: + result = EnterpriseCatalogApiClient().get_content_metadata_content_identifier( + content_id=content_key, + ) + 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. Result = %s', + content_key, + 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 +66,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, ) diff --git a/enterprise/models.py b/enterprise/models.py index 3971c56970..31382e054b 100644 --- a/enterprise/models.py +++ b/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 @@ -2541,11 +2541,16 @@ 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, ) except HTTPError as e: @@ -2638,6 +2643,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): @@ -2693,6 +2700,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.') }) diff --git a/integrated_channels/degreed2/client.py b/integrated_channels/degreed2/client.py index 5b7c0de751..2c410d34c6 100644 --- a/integrated_channels/degreed2/client.py +++ b/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( diff --git a/tests/test_enterprise/api/test_default_enrollment_views.py b/tests/test_enterprise/api/test_default_enrollment_views.py index 91d631805b..b5128246f4 100644 --- a/tests/test_enterprise/api/test_default_enrollment_views.py +++ b/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 diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index 87e501024b..bd654cd224 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/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 diff --git a/tests/test_integrated_channels/test_degreed2/test_client.py b/tests/test_integrated_channels/test_degreed2/test_client.py index 163175804e..4bae0124e0 100644 --- a/tests/test_integrated_channels/test_degreed2/test_client.py +++ b/tests/test_integrated_channels/test_degreed2/test_client.py @@ -256,7 +256,7 @@ def test_create_content_metadata_success(self): responses.add( responses.GET, EnterpriseCatalogApiClient.API_BASE_URL - + EnterpriseCatalogApiClient.CONTENT_METADATA_IDENTIFIER_ENDPOINT.format( + + EnterpriseCatalogApiClient.CUSTOMER_CONTENT_METADATA_IDENTIFIER_ENDPOINT.format( enterprise_config.enterprise_customer.uuid, "key/" ), json={"skill_names": ["Supply Chain", "Supply Chain Management"]}, @@ -316,7 +316,7 @@ def test_create_content_metadata_retry_success(self): responses.add( responses.GET, EnterpriseCatalogApiClient.API_BASE_URL - + EnterpriseCatalogApiClient.CONTENT_METADATA_IDENTIFIER_ENDPOINT.format( + + EnterpriseCatalogApiClient.CUSTOMER_CONTENT_METADATA_IDENTIFIER_ENDPOINT.format( enterprise_config.enterprise_customer.uuid, "key/" ), json={"skill_names": ["Supply Chain", "Supply Chain Management"]}, @@ -422,7 +422,7 @@ def test_create_content_metadata_course_exists(self): responses.add( responses.GET, EnterpriseCatalogApiClient.API_BASE_URL - + EnterpriseCatalogApiClient.CONTENT_METADATA_IDENTIFIER_ENDPOINT.format( + + EnterpriseCatalogApiClient.CUSTOMER_CONTENT_METADATA_IDENTIFIER_ENDPOINT.format( enterprise_config.enterprise_customer.uuid, "key/" ), json={"skill_names": ["Supply Chain", "Supply Chain Management"]}, @@ -479,7 +479,7 @@ def test_update_content_metadata_success(self, mock_fetch_degreed_course_id): responses.add( responses.GET, EnterpriseCatalogApiClient.API_BASE_URL - + EnterpriseCatalogApiClient.CONTENT_METADATA_IDENTIFIER_ENDPOINT.format( + + EnterpriseCatalogApiClient.CUSTOMER_CONTENT_METADATA_IDENTIFIER_ENDPOINT.format( enterprise_config.enterprise_customer.uuid, "key/" ), json={"skill_names": ["Supply Chain", "Supply Chain Management"]}, @@ -651,7 +651,7 @@ def test_update_content_metadata_retry_success(self, mock_fetch_degreed_course_i responses.add( responses.GET, EnterpriseCatalogApiClient.API_BASE_URL - + EnterpriseCatalogApiClient.CONTENT_METADATA_IDENTIFIER_ENDPOINT.format( + + EnterpriseCatalogApiClient.CUSTOMER_CONTENT_METADATA_IDENTIFIER_ENDPOINT.format( enterprise_config.enterprise_customer.uuid, "key/" ), json={"skill_names": ["Supply Chain", "Supply Chain Management"]}, @@ -730,7 +730,7 @@ def test_update_content_metadata_retry_exhaust(self, mock_fetch_degreed_course_i responses.add( responses.GET, EnterpriseCatalogApiClient.API_BASE_URL - + EnterpriseCatalogApiClient.CONTENT_METADATA_IDENTIFIER_ENDPOINT.format( + + EnterpriseCatalogApiClient.CUSTOMER_CONTENT_METADATA_IDENTIFIER_ENDPOINT.format( enterprise_config.enterprise_customer.uuid, "key/" ), json={"skill_names": ["Supply Chain", "Supply Chain Management"]}, diff --git a/tests/test_models.py b/tests/test_models.py index f8797f8513..94121e5b21 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -247,11 +247,11 @@ def tearDown(self): DefaultEnterpriseEnrollmentIntention.objects.all().delete() @mock.patch( - 'enterprise.models.get_and_cache_customer_content_metadata', + 'enterprise.models.get_and_cache_content_metadata', return_value=mock.MagicMock(), ) - def test_retrieve_course_run_advertised_course_run(self, mock_get_and_cache_customer_content_metadata): - mock_get_and_cache_customer_content_metadata.return_value = self.mock_course + def test_retrieve_course_run_advertised_course_run(self, mock_get_and_cache_content_metadata): + mock_get_and_cache_content_metadata.return_value = self.mock_course default_enterprise_enrollment_intention = DefaultEnterpriseEnrollmentIntention.objects.create( enterprise_customer=self.test_enterprise_customer_1, content_key='edX+DemoX', @@ -262,11 +262,11 @@ def test_retrieve_course_run_advertised_course_run(self, mock_get_and_cache_cust assert default_enterprise_enrollment_intention.content_type == DefaultEnterpriseEnrollmentIntention.COURSE @mock.patch( - 'enterprise.models.get_and_cache_customer_content_metadata', + 'enterprise.models.get_and_cache_content_metadata', return_value=mock.MagicMock(), ) - def test_retrieve_course_run_with_explicit_course_run(self, mock_get_and_cache_customer_content_metadata): - mock_get_and_cache_customer_content_metadata.return_value = self.mock_course + def test_retrieve_course_run_with_explicit_course_run(self, mock_get_and_cache_content_metadata): + mock_get_and_cache_content_metadata.return_value = self.mock_course default_enterprise_enrollment_intention = DefaultEnterpriseEnrollmentIntention.objects.create( enterprise_customer=self.test_enterprise_customer_1, content_key='course-v1:edX+DemoX+2T2023', @@ -277,12 +277,12 @@ def test_retrieve_course_run_with_explicit_course_run(self, mock_get_and_cache_c assert default_enterprise_enrollment_intention.content_type == DefaultEnterpriseEnrollmentIntention.COURSE_RUN @mock.patch( - 'enterprise.models.get_and_cache_customer_content_metadata', + 'enterprise.models.get_and_cache_content_metadata', return_value=mock.MagicMock(), ) - def test_validate_missing_course_run(self, mock_get_and_cache_customer_content_metadata): + def test_validate_missing_course_run(self, mock_get_and_cache_content_metadata): # Simulate a 404 Not Found by raising an HTTPError exception - mock_get_and_cache_customer_content_metadata.side_effect = HTTPError + mock_get_and_cache_content_metadata.side_effect = HTTPError with self.assertRaises(ValidationError) as exc_info: DefaultEnterpriseEnrollmentIntention.objects.create( enterprise_customer=self.test_enterprise_customer_1, @@ -291,12 +291,12 @@ def test_validate_missing_course_run(self, mock_get_and_cache_customer_content_m self.assertIn('The content key did not resolve to a valid course run.', str(exc_info.exception)) @mock.patch( - 'enterprise.models.get_and_cache_customer_content_metadata', + 'enterprise.models.get_and_cache_content_metadata', return_value=mock.MagicMock(), ) @override_settings(ROOT_URLCONF="test_utils.admin_urls") - def test_validate_existing_soft_deleted_record(self, mock_get_and_cache_customer_content_metadata): - mock_get_and_cache_customer_content_metadata.return_value = self.mock_course + def test_validate_existing_soft_deleted_record(self, mock_get_and_cache_content_metadata): + mock_get_and_cache_content_metadata.return_value = self.mock_course existing_record = DefaultEnterpriseEnrollmentIntention.objects.create( enterprise_customer=self.test_enterprise_customer_1, content_key='edX+DemoX',