Skip to content

Commit

Permalink
fix: restricted run relationships should not be updated during update…
Browse files Browse the repository at this point in the history 
…_full_content_metadata

ENT-9831
  • Loading branch information
@pwnage101
pwnage101 committed Dec 9, 2024
1 parent 782e196 commit 01f5367
Show file tree
Hide file tree
Showing 4 changed files with 311 additions and 71 deletions.
4 changes: 0 additions & 4 deletions enterprise_catalog/apps/api/tasks.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
214 changes: 148 additions & 66 deletions enterprise_catalog/apps/api/tests/test_tasks.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2416,68 +2416,90 @@ 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,
mock_create_course_associated_programs,
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()
course_run_1_uuid = uuid.uuid4()
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': {
Expand All @@ -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,
Expand All @@ -2513,25 +2542,87 @@ 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',
},
],
}
)

# 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
Expand All @@ -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')
Expand Down
2 changes: 1 addition & 1 deletion enterprise_catalog/apps/catalog/models.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 []
Expand Down
Loading

0 comments on commit 01f5367

Please sign in to comment.