From 01f5367309ee25093e414b0fd3498a48ec575073 Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Fri, 6 Dec 2024 16:12:18 -0800 Subject: [PATCH] fix: restricted run relationships should not be updated during update_full_content_metadata ENT-9831 --- enterprise_catalog/apps/api/tasks.py | 4 - .../apps/api/tests/test_tasks.py | 214 ++++++++++++------ enterprise_catalog/apps/catalog/models.py | 2 +- .../apps/catalog/tests/test_models.py | 162 +++++++++++++ 4 files changed, 311 insertions(+), 71 deletions(-) diff --git a/enterprise_catalog/apps/api/tasks.py b/enterprise_catalog/apps/api/tasks.py index 2ac33754..122686dc 100644 --- a/enterprise_catalog/apps/api/tasks.py +++ b/enterprise_catalog/apps/api/tasks.py @@ -341,10 +341,6 @@ def _update_full_restricted_course_metadata(modified_metadata_record, course_rev # First, update the restricted course record's json metadata to use the full metadata # from above. restricted_course.update_metadata(full_restricted_metadata, is_full_update=True) - # Second, if it's not the canonical copy of the restricted course, - # we have to reset the restricted course *run* relationships. - if restricted_course.catalog_query: - restricted_course.update_course_run_relationships() # Last, run the "standard" transformations below to update with the full # course metadata, do normalization, etc. diff --git a/enterprise_catalog/apps/api/tests/test_tasks.py b/enterprise_catalog/apps/api/tests/test_tasks.py index 3f686094..e8194b13 100644 --- a/enterprise_catalog/apps/api/tests/test_tasks.py +++ b/enterprise_catalog/apps/api/tests/test_tasks.py @@ -2416,7 +2416,6 @@ def test_index_algolia_dry_run(self, mock_search_client): @mock.patch('enterprise_catalog.apps.api.tasks.ContentMetadata.objects.filter') @mock.patch('enterprise_catalog.apps.api.tasks.create_course_associated_programs') @mock.patch('enterprise_catalog.apps.api.tasks._update_full_content_metadata_program') - @override_settings(SHOULD_FETCH_RESTRICTED_COURSE_RUNS=True) def test_update_full_content_metadata_course( self, mock_update_content_metadata_program, @@ -2424,6 +2423,56 @@ def test_update_full_content_metadata_course( mock_filter, mock_get_course_reviews, mock_fetch_courses_by_keys + ): + # Mock data + content_keys = ['course1', 'course2'] + full_course_dicts = [ + {'key': 'course1', 'title': 'Course 1', FORCE_INCLUSION_METADATA_TAG_KEY: True}, + {'key': 'course2', 'title': 'Course 2'} + ] + reviews_for_courses_dict = { + 'course1': {'reviews_count': 10, 'avg_course_rating': 4.5}, + 'course2': {'reviews_count': 5, 'avg_course_rating': 3.8} + } + content_metadata_1 = ContentMetadataFactory(content_type=COURSE, content_key='course1') + content_metadata_2 = ContentMetadataFactory(content_type=COURSE, content_key='course2') + metadata_records_for_fetched_keys = [content_metadata_1, content_metadata_2] + + # Configure mock objects + mock_fetch_courses_by_keys.return_value = full_course_dicts + mock_get_course_reviews.return_value = reviews_for_courses_dict + mock_filter.return_value = metadata_records_for_fetched_keys + + # Call the function + tasks._update_full_content_metadata_course(content_keys) # pylint: disable=protected-access + + mock_fetch_courses_by_keys.assert_called_once_with(content_keys) + mock_get_course_reviews.assert_called_once_with(['course1', 'course2']) + mock_filter.assert_called_once_with(content_key__in=['course1', 'course2']) + + content_metadata_1.refresh_from_db() + content_metadata_2.refresh_from_db() + assert content_metadata_1.json_metadata.get('reviews_count') == 10 + assert content_metadata_1.json_metadata.get('avg_course_rating') == 4.5 + assert content_metadata_2.json_metadata.get('reviews_count') == 5 + assert content_metadata_2.json_metadata.get('avg_course_rating') == 3.8 + + self.assertEqual(mock_update_content_metadata_program.call_count, 2) + self.assertEqual(mock_create_course_associated_programs.call_count, 2) + + @mock.patch('enterprise_catalog.apps.api.tasks._fetch_courses_by_keys') + @mock.patch('enterprise_catalog.apps.api.tasks.DiscoveryApiClient.get_course_reviews') + @mock.patch('enterprise_catalog.apps.api.tasks.ContentMetadata.objects.filter') + @mock.patch('enterprise_catalog.apps.api.tasks.create_course_associated_programs') + @mock.patch('enterprise_catalog.apps.api.tasks._update_full_content_metadata_program') + @override_settings(SHOULD_FETCH_RESTRICTED_COURSE_RUNS=True) + def test_update_full_content_metadata_course_with_restricted_runs( + self, + mock_update_content_metadata_program, + mock_create_course_associated_programs, + mock_filter, + mock_get_course_reviews, + mock_fetch_courses_by_keys ): # Mock data course_1_uuid = uuid.uuid4() @@ -2431,53 +2480,26 @@ def test_update_full_content_metadata_course( restricted_run_1_uuid = uuid.uuid4() restricted_run_2_uuid = uuid.uuid4() content_keys = ['course1', 'course2'] - full_course_dicts = [ - { - 'key': 'course1', - 'uuid': course_1_uuid, - 'aggregation_key': 'course:course1', - 'title': 'Course 1', - FORCE_INCLUSION_METADATA_TAG_KEY: True, - 'course_runs': [ - { - 'key': 'course-run-1', 'uuid': course_1_uuid, - 'aggregation_key': 'courserun:course1', - }, - ] - }, - { - 'key': 'course2', - 'title': 'Course 2', - }, - ] reviews_for_courses_dict = { 'course1': {'reviews_count': 10, 'avg_course_rating': 4.5}, 'course2': {'reviews_count': 5, 'avg_course_rating': 3.8} } content_metadata_1 = ContentMetadataFactory(content_type=COURSE, content_key='course1') content_metadata_2 = ContentMetadataFactory(content_type=COURSE, content_key='course2') + content_metadata_run_1 = ContentMetadataFactory( # pylint: disable=unused-variable + content_type=COURSE_RUN, + content_key='course-run-1', + ) + content_metadata_restricted_run_1 = ContentMetadataFactory( + content_type=COURSE_RUN, + content_key='course-run-1-restricted' + ) + content_metadata_restricted_run_2 = ContentMetadataFactory( # pylint: disable=unused-variable + content_type=COURSE_RUN, + content_key='course-run-2-restricted' + ) metadata_records_for_fetched_keys = [content_metadata_1, content_metadata_2] - restricted_course_dicts = [ - { - 'key': 'course1', - 'uuid': course_1_uuid, - 'title': 'Course 1', - 'other': 'stuff', - FORCE_INCLUSION_METADATA_TAG_KEY: True, - 'course_runs': [ - {'key': 'course-run-1', 'uuid': course_run_1_uuid, 'aggregation_key': 'courserun:course-run-1'}, - { - 'key': 'course-run-1-restricted', COURSE_RUN_RESTRICTION_TYPE_KEY: 'anything', - 'aggregation_key': 'courserun:course1', 'uuid': restricted_run_1_uuid, - }, - { - 'key': 'another-restricted-run', COURSE_RUN_RESTRICTION_TYPE_KEY: 'anything', - 'aggregation_key': 'courserun:course1', 'uuid': restricted_run_2_uuid, - }, - ], - }, - ] catalog_query = CatalogQueryFactory( content_filter={ 'restricted_runs_allowed': { @@ -2495,16 +2517,23 @@ def test_update_full_content_metadata_course( _json_metadata={ 'course_runs': [ { - 'key': 'course-run-1', 'uuid': course_run_1_uuid, + 'key': 'course-run-1', + 'uuid': course_run_1_uuid, 'aggregation_key': 'courserun:course1', }, { - 'key': 'course-run-1-restricted', COURSE_RUN_RESTRICTION_TYPE_KEY: 'anything', - 'aggregation_key': 'courserun:course1', 'uuid': restricted_run_1_uuid, + 'key': 'course-run-1-restricted', + 'uuid': restricted_run_1_uuid, + 'aggregation_key': 'courserun:course1', + COURSE_RUN_RESTRICTION_TYPE_KEY: 'anything', }, ] } ) + RestrictedRunAllowedForRestrictedCourseFactory( + course=restricted_course, + run=content_metadata_restricted_run_1, + ) canonical_restricted_course = RestrictedCourseMetadataFactory( unrestricted_parent=content_metadata_1, catalog_query=None, @@ -2513,16 +2542,21 @@ def test_update_full_content_metadata_course( _json_metadata={ 'course_runs': [ { - 'key': 'course-run-1', 'uuid': course_run_1_uuid, + 'key': 'course-run-1', + 'uuid': course_run_1_uuid, 'aggregation_key': 'courserun:course1', }, { - 'key': 'course-run-1-restricted', COURSE_RUN_RESTRICTION_TYPE_KEY: 'anything', - 'aggregation_key': 'courserun:course1', 'uuid': restricted_run_1_uuid, + 'key': 'course-run-1-restricted', + 'aggregation_key': 'courserun:course1', + 'uuid': restricted_run_1_uuid, + COURSE_RUN_RESTRICTION_TYPE_KEY: 'anything', }, { - 'key': 'another-restricted-run', COURSE_RUN_RESTRICTION_TYPE_KEY: 'anything', - 'aggregation_key': 'courserun:course1', 'uuid': restricted_run_2_uuid, + 'key': 'course-run-2-restricted', + 'uuid': restricted_run_2_uuid, + 'aggregation_key': 'courserun:course1', + COURSE_RUN_RESTRICTION_TYPE_KEY: 'anything', }, ], } @@ -2530,8 +2564,65 @@ def test_update_full_content_metadata_course( # Configure mock objects mock_fetch_courses_by_keys.side_effect = [ - full_course_dicts, - restricted_course_dicts, + # First call to /api/v1/courses originates from the top of + # _update_full_content_metadata_course(), so it does NOT pass the + # `?include_restricted=` query parameter. Therefore, no restricted + # runs should be included in the mock response. + [ + { + 'key': 'course1', + 'uuid': course_1_uuid, + 'aggregation_key': 'course:course1', + 'title': 'Course 1', + FORCE_INCLUSION_METADATA_TAG_KEY: True, + 'course_runs': [ + { + 'key': 'course-run-1', + 'uuid': course_1_uuid, + 'aggregation_key': 'courserun:course1', + }, + ] + }, + { + 'key': 'course2', + 'title': 'Course 2', + }, + ], + # Second call to /api/v1/courses originates from + # _update_full_restricted_course_metadata(), so it passess the + # `?include_restricted=` query parameter to include all the + # restricted runs for the course. + [ + { + 'key': 'course1', + 'uuid': course_1_uuid, + 'title': 'Course 1', + FORCE_INCLUSION_METADATA_TAG_KEY: True, + 'course_runs': [ + { + 'key': 'course-run-1', + 'uuid': course_run_1_uuid, + 'aggregation_key': 'courserun:course1' + }, + { + 'key': 'course-run-1-restricted', + 'uuid': restricted_run_1_uuid, + 'aggregation_key': 'courserun:course1', + COURSE_RUN_RESTRICTION_TYPE_KEY: 'anything', + }, + { + 'key': 'course-run-2-restricted', + 'uuid': restricted_run_2_uuid, + 'aggregation_key': 'courserun:course1', + COURSE_RUN_RESTRICTION_TYPE_KEY: 'anything', + }, + ], + # Throw in this extra key to the mock response, so that + # later we can validate that it gets merged into the final + # metadata for the restricted course. + 'other': 'stuff', + }, + ] ] mock_get_course_reviews.return_value = reviews_for_courses_dict mock_filter.return_value = metadata_records_for_fetched_keys @@ -2556,32 +2647,23 @@ def test_update_full_content_metadata_course( self.assertEqual(mock_update_content_metadata_program.call_count, 2) self.assertEqual(mock_create_course_associated_programs.call_count, 2) - # Test that the restricted course for our catalog query contains - # only the one restricted run, because that's all the query definition allows. + # Test that the extra keys from the /api/v1/courses response were + # appropriately merged into the restricted course metadata, and the + # restricted runs were preserved. restricted_course.refresh_from_db() - restricted_run = ContentMetadata.objects.get( - content_type=COURSE_RUN, - parent_content_key='course1', - content_key='course-run-1-restricted', - ) - related_run = restricted_course.restricted_run_allowed_for_restricted_course.filter( - content_key=restricted_run.content_key, - ).first() - self.assertEqual(1, restricted_course.restricted_run_allowed_for_restricted_course.all().count()) - self.assertEqual(related_run, restricted_run) self.assertEqual('stuff', restricted_course.json_metadata['other']) self.assertEqual( {run['key'] for run in restricted_course.json_metadata['course_runs']}, {'course-run-1', 'course-run-1-restricted'}, ) - # The canonical restricted course record should contain two restricted runs and a non-restricted run + # The canonical restricted course record should STILL contain two + # restricted runs and a non-restricted run, as well as the extra key. canonical_restricted_course.refresh_from_db() + self.assertEqual('stuff', canonical_restricted_course.json_metadata['other']) self.assertEqual( {run['key'] for run in canonical_restricted_course.json_metadata['course_runs']}, - {'course-run-1', 'course-run-1-restricted', 'another-restricted-run'}, + {'course-run-1', 'course-run-1-restricted', 'course-run-2-restricted'}, ) - # But the canonical course will *not* have any direct relationship to restricted run ContentMetadtata records. - self.assertEqual(0, canonical_restricted_course.restricted_run_allowed_for_restricted_course.all().count()) @mock.patch('enterprise_catalog.apps.api.tasks._fetch_courses_by_keys') @mock.patch('enterprise_catalog.apps.api.tasks.DiscoveryApiClient.get_course_reviews') diff --git a/enterprise_catalog/apps/catalog/models.py b/enterprise_catalog/apps/catalog/models.py index de154b35..aaaf7999 100644 --- a/enterprise_catalog/apps/catalog/models.py +++ b/enterprise_catalog/apps/catalog/models.py @@ -1572,7 +1572,7 @@ def update_contentmetadata_from_discovery(catalog_query, dry_run=False): def synchronize_restricted_content(catalog_query, dry_run=False): """ - Fetch and assoicate any permitted restricted couress for the given catalog_query. + Fetch and assoicate any permitted restricted courses for the given catalog_query. """ if not getattr(settings, 'SHOULD_FETCH_RESTRICTED_COURSE_RUNS', False): return [] diff --git a/enterprise_catalog/apps/catalog/tests/test_models.py b/enterprise_catalog/apps/catalog/tests/test_models.py index 99b4024f..03b7367f 100644 --- a/enterprise_catalog/apps/catalog/tests/test_models.py +++ b/enterprise_catalog/apps/catalog/tests/test_models.py @@ -407,6 +407,168 @@ def test_contentmetadata_update_from_discovery_dry_run_update(self, mock_client) json.dumps(course_metadata, sort_keys=True), ) + @override_settings(DISCOVERY_CATALOG_QUERY_CACHE_TIMEOUT=0) + @override_settings(SHOULD_FETCH_RESTRICTED_COURSE_RUNS=True) + @mock.patch('enterprise_catalog.apps.api_client.discovery.DiscoveryApiClient') + @mock.patch('enterprise_catalog.apps.catalog.models.DiscoveryApiClient') + def test_contentmetadata_update_from_discovery_with_restricted_runs( + self, + mock_client2, + mock_client, + ): + """ + update_contentmetadata_from_discovery should update or create ContentMetadata + objects from the discovery service /search/all api call and create all + the necessary records and relationships for restricted runs. + """ + course_run_unrestricted_1_metadata = { + 'aggregation_key': 'courserun:edX+testX', + 'key': 'course-v1:edX+testX+unrestricted.1', + 'title': 'test course run', + 'status': 'published', + 'first_enrollable_paid_seat_price': 99, + } + course_run_restricted_1_metadata = { + 'aggregation_key': 'courserun:edX+testX', + 'key': 'course-v1:edX+testX+restricted.1', + 'title': 'test course run (restricted 1)', + 'status': 'published', + 'first_enrollable_paid_seat_price': 99, + COURSE_RUN_RESTRICTION_TYPE_KEY: RESTRICTION_FOR_B2B, + } + course_run_restricted_2_metadata = { + 'aggregation_key': 'courserun:edX+testX', + 'key': 'course-v1:edX+testX+restricted.2', + 'title': 'test course run (restricted 2)', + 'status': 'published', + 'first_enrollable_paid_seat_price': 99, + COURSE_RUN_RESTRICTION_TYPE_KEY: RESTRICTION_FOR_B2B, + } + course_metadata = { + 'aggregation_key': 'course:edX+testX', + 'key': 'edX+testX', + 'title': 'test course', + 'content_type': 'course', + 'course_runs': [ + course_run_unrestricted_1_metadata, + ], + } + course_metadata_restricted_canonical = { + 'aggregation_key': 'course:edX+testX', + 'key': 'edX+testX', + 'title': 'test course', + 'content_type': 'course', + 'course_runs': [ + course_run_unrestricted_1_metadata, + course_run_restricted_1_metadata, + course_run_restricted_2_metadata, + ], + } + # This is only used for output validation, not as a mock API response. + course_metadata_restricted_for_catalog = { + 'aggregation_key': 'course:edX+testX', + 'key': 'edX+testX', + 'title': 'test course', + 'content_type': 'course', + 'course_runs': [ + course_run_unrestricted_1_metadata, + course_run_restricted_1_metadata, + ], + 'course_run_keys': [ + course_run_unrestricted_1_metadata['key'], + course_run_restricted_1_metadata['key'], + ], + 'course_run_statuses': [ + 'published', + ], + 'first_enrollable_paid_seat_price': 99, + } + catalog_query = factories.CatalogQueryFactory( + content_filter={ + 'restricted_runs_allowed': { + course_metadata['key']: [ + # Only the first restricted run will be associated with our catalog. + course_run_restricted_1_metadata['key'], + ], + }, + }, + ) + catalog = factories.EnterpriseCatalogFactory(catalog_query=catalog_query) + + # First Discovery API call originates from + # update_contentmetadata_from_discovery() and hits /api/v1/search/all to + # get only the plain unrestricted version of the course. + mock_client.return_value.get_metadata_by_query.return_value = [ + course_metadata, + # Catalogs almost never actually include runs directly, so excluding + # the runs here is more prodlike. + ] + # Second and third Discovery API calls originate from + # synchronize_restricted_content() and hit /api/v1/search/all to get the + # restricted stuff. + mock_client2.return_value.retrieve_metadata_for_content_filter.side_effect = [ + # First call fetches the canonical restricted course metadata: + [course_metadata_restricted_canonical], + # Second call fetches the restricted run requested by the content filter: + [course_run_restricted_1_metadata], + ] + + self.assertEqual(ContentMetadata.objects.count(), 0) + update_contentmetadata_from_discovery(catalog.catalog_query) + mock_client.assert_called_once() + self.assertEqual(ContentMetadata.objects.count(), 2) + + associated_metadata = catalog.content_metadata + + # Check that the unrestricted course variant is stored without any + # restricted runs, and that it is associated with the catalog. + contentmetadata_course = ContentMetadata.objects.get(content_key=course_metadata['key']) + self.assertEqual(contentmetadata_course.content_type, COURSE) + self.assertEqual(contentmetadata_course.parent_content_key, None) + self.assertEqual(contentmetadata_course.json_metadata, course_metadata) + assert contentmetadata_course in associated_metadata + + # Check that the unrestricted run is NOT stored. + contentmetadata_unrestricted_course_run = ContentMetadata.objects.filter( + content_key=course_run_unrestricted_1_metadata['key'] + ) + self.assertEqual(contentmetadata_unrestricted_course_run.count(), 0) + + # Check that the requested restricted run is stored, but NOT associated + # with any catalog. + contentmetadata_restricted_course_run = ContentMetadata.objects.get( + content_key=course_run_restricted_1_metadata['key'] + ) + self.assertEqual(contentmetadata_restricted_course_run.content_type, COURSE_RUN) + self.assertEqual(contentmetadata_restricted_course_run.parent_content_key, course_metadata['key']) + self.assertEqual(contentmetadata_restricted_course_run.json_metadata, course_run_restricted_1_metadata) + assert contentmetadata_restricted_course_run not in associated_metadata + + # Check that the OTHER restricted run is NOT stored. + contentmetadata_other_restricted_course_run = ContentMetadata.objects.filter( + content_key=course_run_restricted_2_metadata['key'] + ) + self.assertEqual(contentmetadata_other_restricted_course_run.count(), 0) + + # Check that two variants of RestrictedCourseMetadata were created: one + # canonical variant and another one for the catalog. + restrictedcoursemetadata_canonical = RestrictedCourseMetadata.objects.get( + content_key=course_metadata['key'], + catalog_query=None, + ) + assert restrictedcoursemetadata_canonical.json_metadata == course_metadata_restricted_canonical + assert restrictedcoursemetadata_canonical.restricted_run_allowed_for_restricted_course.count() == 0 + + restrictedcoursemetadata_for_catalog = RestrictedCourseMetadata.objects.get( + content_key=course_metadata['key'], + catalog_query=catalog_query, + ) + assert restrictedcoursemetadata_for_catalog.json_metadata == course_metadata_restricted_for_catalog + self.assertEqual( + restrictedcoursemetadata_for_catalog.restricted_run_allowed_for_restricted_course.first(), + contentmetadata_restricted_course_run, + ) + @contextmanager def _mock_enterprise_customer_cache( self,