Skip to content

Commit

Permalink
feat: sync full metadata for restricted courses
Browse files Browse the repository at this point in the history
ENT-9597
  • Loading branch information
iloveagent57 committed Oct 23, 2024
1 parent 0eb18b0 commit e203d42
Show file tree
Hide file tree
Showing 8 changed files with 314 additions and 86 deletions.
65 changes: 55 additions & 10 deletions enterprise_catalog/apps/api/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
FORCE_INCLUSION_METADATA_TAG_KEY,
LEARNER_PATHWAY,
PROGRAM,
QUERY_FOR_RESTRICTED_RUNS,
REINDEX_TASK_BATCH_SIZE,
TASK_BATCH_SIZE,
VIDEO,
Expand Down Expand Up @@ -73,7 +74,7 @@
EXPLORE_CATALOG_TITLES = ['A la carte', 'Subscription']


def _fetch_courses_by_keys(course_keys):
def _fetch_courses_by_keys(course_keys, extra_query_params=None):
"""
Fetches course data from discovery's /api/v1/courses endpoint for the provided course keys.
Expand All @@ -82,7 +83,7 @@ def _fetch_courses_by_keys(course_keys):
Returns:
list of dict: Returns a list of dictionaries where each dictionary represents the course data from discovery.
"""
return DiscoveryApiClient().fetch_courses_by_keys(course_keys)
return DiscoveryApiClient().fetch_courses_by_keys(course_keys, extra_query_params=extra_query_params)


def _fetch_programs_by_keys(program_keys):
Expand Down Expand Up @@ -283,6 +284,14 @@ def _update_full_content_metadata_course(content_keys, dry_run=False):
)
modified_content_metadata_records.append(modified_course_record)

program_content_keys = create_course_associated_programs(
course_metadata_dict.get('programs', []),
modified_course_record,
)
_update_full_content_metadata_program(program_content_keys, dry_run)

_update_full_restricted_course_metadata(modified_course_record, course_review, dry_run)

if dry_run:
logger.info('dry_run=True, not updating course metadata')
else:
Expand All @@ -307,6 +316,44 @@ def _update_full_content_metadata_course(content_keys, dry_run=False):
)


def _update_full_restricted_course_metadata(modified_metadata_record, course_review, dry_run):
"""
For all restricted courses whose parent is ``modified_metadata_record``, does a full
update of metadata and restricted run relationships.
"""
restricted_courses = list(modified_metadata_record.restricted_courses.all())
if not restricted_courses:
return

# Fetch from /api/v1/courses with restricted content included
metadata_list = _fetch_courses_by_keys(
[modified_metadata_record.content_key],
extra_query_params=QUERY_FOR_RESTRICTED_RUNS,
)
if not metadata_list:
raise Exception(
f'No restricted course metadata could be fetched for {modified_metadata_record.content_key}'
)

full_restricted_metadata = metadata_list[0]

for restricted_course in restricted_courses:
# 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.
_update_single_full_course_record(
full_restricted_metadata, restricted_course, course_review, dry_run, skip_json_metadata_update=True,
)
restricted_course.save()


def _get_course_records_by_key(fetched_course_keys):
"""
Helper to fetch a dict of course `ContentMetadata` records by content key.
Expand All @@ -320,14 +367,17 @@ def _get_course_records_by_key(fetched_course_keys):
}


def _update_single_full_course_record(course_metadata_dict, metadata_record, course_review, dry_run):
def _update_single_full_course_record(
course_metadata_dict, metadata_record, course_review, dry_run, skip_json_metadata_update=False,
):
"""
Given a fetched dictionary of course content metadata and an option `course_review` record,
updates an existing course `ContentMetadata` instance with the "full" dictionary of `json_metadata`
for that course.
"""
# Merge the full metadata from discovery's /api/v1/courses into the local metadata object.
metadata_record._json_metadata.update(course_metadata_dict) # pylint: disable=protected-access
if not skip_json_metadata_update:
# Merge the full metadata from discovery's /api/v1/courses into the local metadata object.
metadata_record._json_metadata.update(course_metadata_dict) # pylint: disable=protected-access

_normalize_metadata_record(metadata_record)

Expand All @@ -344,11 +394,6 @@ def _update_single_full_course_record(course_metadata_dict, metadata_record, cou
metadata_record.content_key, json.dumps(metadata_record.json_metadata)
))

program_content_keys = create_course_associated_programs(
course_metadata_dict.get('programs', []),
metadata_record,
)
_update_full_content_metadata_program(program_content_keys, dry_run)
return metadata_record


Expand Down
99 changes: 95 additions & 4 deletions enterprise_catalog/apps/api/tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@
from enterprise_catalog.apps.catalog.constants import (
COURSE,
COURSE_RUN,
COURSE_RUN_RESTRICTION_TYPE_KEY,
EXEC_ED_2U_COURSE_TYPE,
FORCE_INCLUSION_METADATA_TAG_KEY,
LEARNER_PATHWAY,
PROGRAM,
QUERY_FOR_RESTRICTED_RUNS,
)
from enterprise_catalog.apps.catalog.models import CatalogQuery, ContentMetadata
from enterprise_catalog.apps.catalog.serializers import (
Expand Down Expand Up @@ -2377,6 +2379,7 @@ 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,
Expand All @@ -2388,8 +2391,18 @@ def test_update_full_content_metadata_course(
# 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'}
{
'key': 'course1',
'title': 'Course 1',
FORCE_INCLUSION_METADATA_TAG_KEY: True,
'course_runs': [
{'key': 'course-run-1'},
]
},
{
'key': 'course2',
'title': 'Course 2',
},
]
reviews_for_courses_dict = {
'course1': {'reviews_count': 10, 'avg_course_rating': 4.5},
Expand All @@ -2399,15 +2412,66 @@ def test_update_full_content_metadata_course(
content_metadata_2 = ContentMetadataFactory(content_type=COURSE, content_key='course2')
metadata_records_for_fetched_keys = [content_metadata_1, content_metadata_2]

restricted_course_dicts = [
{
'key': 'course1',
'title': 'Course 1',
'other': 'stuff',
FORCE_INCLUSION_METADATA_TAG_KEY: True,
'course_runs': [
{'key': 'course-run-1'},
{'key': 'course-run-1-restricted', COURSE_RUN_RESTRICTION_TYPE_KEY: 'anything'},
{'key': 'another-restricted-run', COURSE_RUN_RESTRICTION_TYPE_KEY: 'anything'},
]
},
]
catalog_query = CatalogQueryFactory(
content_filter={
'restricted_runs_allowed': {
'course:course1': [
'course-run-1-restricted',
],
},
},
)
restricted_course = RestrictedCourseMetadataFactory(
unrestricted_parent=content_metadata_1,
catalog_query=catalog_query,
content_type=COURSE,
content_key='course1',
_json_metadata={
'course_runs': [
{'key': 'course-run-1'},
],
}
)
canonical_restricted_course = RestrictedCourseMetadataFactory(
unrestricted_parent=content_metadata_1,
catalog_query=None,
content_type=COURSE,
content_key='course1',
_json_metadata={
'course_runs': [
{'key': 'course-run-1'},
],
}
)

# Configure mock objects
mock_fetch_courses_by_keys.return_value = full_course_dicts
mock_fetch_courses_by_keys.side_effect = [
full_course_dicts,
restricted_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_fetch_courses_by_keys.assert_has_calls([
mock.call(content_keys),
mock.call([restricted_course.content_key], extra_query_params=QUERY_FOR_RESTRICTED_RUNS),
])
mock_get_course_reviews.assert_called_once_with(['course1', 'course2'])
mock_filter.assert_called_once_with(content_key__in=['course1', 'course2'])

Expand All @@ -2421,6 +2485,33 @@ 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.
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
canonical_restricted_course.refresh_from_db()
self.assertEqual(
{run['key'] for run in canonical_restricted_course.json_metadata['course_runs']},
{'course-run-1', 'course-run-1-restricted', 'another-restricted-run'},
)
# 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')
@mock.patch('enterprise_catalog.apps.api.tasks.ContentMetadata.objects.filter')
Expand Down
4 changes: 3 additions & 1 deletion enterprise_catalog/apps/api_client/discovery.py
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ def get_programs(self, query_params=None):

return programs

def fetch_courses_by_keys(self, course_keys):
def fetch_courses_by_keys(self, course_keys, extra_query_params=None):
"""
Fetches course data from discovery's /api/v1/courses endpoint for the provided course keys.
Expand All @@ -529,6 +529,8 @@ def fetch_courses_by_keys(self, course_keys):
for course_keys_chunk in batched_course_keys:
# Discovery expects the keys param to be in the format ?keys=course1,course2,...
query_params = {'keys': ','.join(course_keys_chunk)}
if extra_query_params:
query_params.update(extra_query_params)
courses.extend(self.get_courses(query_params=query_params))

return courses
Expand Down
1 change: 1 addition & 0 deletions enterprise_catalog/apps/catalog/algolia_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -1131,6 +1131,7 @@ def _get_course_run(course, course_run):
'content_price': normalized_content_metadata.get('content_price'),
'is_active': _get_is_active_course_run(course_run),
'is_late_enrollment_eligible': _get_is_late_enrollment_eligible(course_run),
'restriction_type': course_run.get('restriction_type'),
}
return course_run

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Generated by Django 4.2.16 on 2024-10-23 15:22

from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):

dependencies = [
('catalog', '0042_alter_restrictedcoursemetadata_display_name'),
]

operations = [
migrations.AddField(
model_name='restrictedcoursemetadata',
name='restricted_run_allowed_for_restricted_course',
field=models.ManyToManyField(through='catalog.RestrictedRunAllowedForRestrictedCourse', to='catalog.contentmetadata'),
),
migrations.AlterField(
model_name='restrictedrunallowedforrestrictedcourse',
name='course',
field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, to='catalog.restrictedcoursemetadata'),
),
]
Loading

0 comments on commit e203d42

Please sign in to comment.