-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: ability to include unlisted courses in catalogs #767
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,13 +34,15 @@ | |
from enterprise_catalog.apps.catalog.constants import ( | ||
COURSE, | ||
COURSE_RUN, | ||
DISCOVERY_COURSE_KEY_BATCH_SIZE, | ||
DISCOVERY_PROGRAM_KEY_BATCH_SIZE, | ||
FORCE_INCLUSION_METADATA_TAG_KEY, | ||
LEARNER_PATHWAY, | ||
PROGRAM, | ||
REINDEX_TASK_BATCH_SIZE, | ||
TASK_BATCH_SIZE, | ||
) | ||
from enterprise_catalog.apps.catalog.content_metadata_utils import ( | ||
transform_course_metadata_to_visible, | ||
) | ||
from enterprise_catalog.apps.catalog.models import ( | ||
CatalogQuery, | ||
ContentMetadata, | ||
|
@@ -83,17 +85,7 @@ | |
Returns: | ||
list of dict: Returns a list of dictionaries where each dictionary represents the course data from discovery. | ||
""" | ||
courses = [] | ||
discovery_client = DiscoveryApiClient() | ||
|
||
# Batch the course keys into smaller chunks so that we don't send too big of a request to discovery | ||
batched_course_keys = batch(course_keys, batch_size=DISCOVERY_COURSE_KEY_BATCH_SIZE) | ||
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)} | ||
courses.extend(discovery_client.get_courses(query_params=query_params)) | ||
|
||
return courses | ||
return DiscoveryApiClient().fetch_courses_by_keys(course_keys) | ||
|
||
|
||
def _fetch_programs_by_keys(program_keys): | ||
|
@@ -105,17 +97,7 @@ | |
Returns: | ||
list of dict: Returns a list of dictionaries where each dictionary represents the program data from discovery. | ||
""" | ||
programs = [] | ||
discovery_client = DiscoveryApiClient() | ||
|
||
# Batch the program keys into smaller chunks so that we don't send too big of a request to discovery | ||
batched_program_keys = batch(program_keys, batch_size=DISCOVERY_PROGRAM_KEY_BATCH_SIZE) | ||
for program_keys_chunk in batched_program_keys: | ||
# Discovery expects the uuids param to be in the format ?uuids=program1,program2,... | ||
query_params = {'uuids': ','.join(program_keys_chunk)} | ||
programs.extend(discovery_client.get_programs(query_params=query_params)) | ||
|
||
return programs | ||
return DiscoveryApiClient().fetch_programs_by_keys(program_keys) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved this logic into the client, left the old signature to maintain tests |
||
|
||
|
||
def unready_tasks(celery_task, time_delta): | ||
|
@@ -394,6 +376,10 @@ | |
metadata_record.json_metadata['reviews_count'] = review.get('reviews_count') | ||
metadata_record.json_metadata['avg_course_rating'] = review.get('avg_course_rating') | ||
|
||
# johnnagro ENT-8212 need to retransform after full pull | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when we initially force-include the content, we need to mark it somehow so that the "full content metadata update job" knows to re-transform it on the full content metadata object it retrieves after the initial catalog crawl |
||
if metadata_record.json_metadata.get(FORCE_INCLUSION_METADATA_TAG_KEY): | ||
metadata_record.json_metadata = transform_course_metadata_to_visible(metadata_record.json_metadata) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we probably do want to add/update a test to cover this line. |
||
|
||
modified_content_metadata_records.append(metadata_record) | ||
program_content_keys = create_course_associated_programs( | ||
course_metadata_dict.get('programs', []), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,15 @@ | |
from celery.exceptions import SoftTimeLimitExceeded | ||
from django.conf import settings | ||
|
||
from enterprise_catalog.apps.catalog.constants import ( | ||
DISCOVERY_COURSE_KEY_BATCH_SIZE, | ||
DISCOVERY_PROGRAM_KEY_BATCH_SIZE, | ||
) | ||
from enterprise_catalog.apps.catalog.content_metadata_utils import ( | ||
tansform_force_included_courses, | ||
) | ||
from enterprise_catalog.apps.catalog.utils import batch | ||
|
||
from .base_oauth import BaseOAuthClient | ||
from .constants import ( | ||
DISCOVERY_COURSE_REVIEWS_ENDPOINT, | ||
|
@@ -209,6 +218,17 @@ | |
) | ||
raise exc | ||
|
||
try: | ||
# NOTE johnnagro this ONLY supports courses at the moment (NOT programs, leanerpathways, etc) | ||
if forced_aggregation_keys := catalog_query.content_filter.get('enterprise_force_include_aggregation_keys'): | ||
forced_courses = self.fetch_courses_by_keys(forced_aggregation_keys) | ||
results += tansform_force_included_courses(forced_courses) | ||
except Exception as exc: | ||
LOGGER.exception( | ||
f'unable to add unlisted courses for catalog_id: {catalog_query.id}' | ||
) | ||
raise exc | ||
|
||
return results | ||
|
||
def _retrieve_courses(self, offset, request_params): | ||
|
@@ -326,6 +346,48 @@ | |
|
||
return programs | ||
|
||
def fetch_courses_by_keys(self, course_keys): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved from |
||
""" | ||
Fetches course data from discovery's /api/v1/courses endpoint for the provided course keys. | ||
|
||
Args: | ||
course_keys (list of str): Content keys for Course ContentMetadata objects. | ||
Returns: | ||
list of dict: Returns a list of dictionaries where each dictionary represents the course | ||
data from discovery. | ||
""" | ||
courses = [] | ||
|
||
# Batch the course keys into smaller chunks so that we don't send too big of a request to discovery | ||
batched_course_keys = batch(course_keys, batch_size=DISCOVERY_COURSE_KEY_BATCH_SIZE) | ||
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)} | ||
courses.extend(self.get_courses(query_params=query_params)) | ||
|
||
return courses | ||
|
||
def fetch_programs_by_keys(self, program_keys): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved from |
||
""" | ||
Fetches program data from discovery's /api/v1/programs endpoint for the provided program keys. | ||
|
||
Args: | ||
program_keys (list of str): Content keys for Program ContentMetadata objects. | ||
Returns: | ||
list of dict: Returns a list of dictionaries where each dictionary represents the program | ||
data from discovery. | ||
""" | ||
programs = [] | ||
|
||
# Batch the program keys into smaller chunks so that we don't send too big of a request to discovery | ||
batched_program_keys = batch(program_keys, batch_size=DISCOVERY_PROGRAM_KEY_BATCH_SIZE) | ||
for program_keys_chunk in batched_program_keys: | ||
# Discovery expects the uuids param to be in the format ?uuids=program1,program2,... | ||
query_params = {'uuids': ','.join(program_keys_chunk)} | ||
programs.extend(self.get_programs(query_params=query_params)) | ||
|
||
return programs | ||
|
||
|
||
class CatalogQueryMetadata: | ||
""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,6 +102,8 @@ | |
'Certificación Profesional': 'Certificación Profesional', | ||
} | ||
|
||
FORCE_INCLUSION_METADATA_TAG_KEY = 'enterprise_force_included' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. magic field which marks content metadata as having been force-included |
||
|
||
|
||
def json_serialized_course_modes(): | ||
""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
""" | ||
Utility functions for manipulating content metadata. | ||
""" | ||
|
||
from logging import getLogger | ||
|
||
from .constants import FORCE_INCLUSION_METADATA_TAG_KEY | ||
|
||
|
||
LOGGER = getLogger(__name__) | ||
|
||
|
||
def tansform_force_included_courses(courses): | ||
""" | ||
Transform a list of forced/unlisted course metadata | ||
ENT-8212 | ||
""" | ||
results = [] | ||
for course_metadata in courses: | ||
results += transform_course_metadata_to_visible(course_metadata) | ||
return results | ||
|
||
|
||
def transform_course_metadata_to_visible(course_metadata): | ||
""" | ||
Transform an individual forced/unlisted course metadata | ||
so that it is visible/available/published in our metadata | ||
ENT-8212 | ||
""" | ||
course_metadata[FORCE_INCLUSION_METADATA_TAG_KEY] = True | ||
advertised_course_run_uuid = course_metadata.get('advertised_course_run_uuid') | ||
course_run_statuses = [] | ||
for course_run in course_metadata.get('course_runs', []): | ||
if course_run.get('uuid') == advertised_course_run_uuid: | ||
course_run['status'] = 'published' | ||
course_run['availability'] = 'Current' | ||
course_run_statuses.append(course_run.get('status')) | ||
course_metadata['course_run_statuses'] = course_run_statuses | ||
return course_metadata |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
from uuid import uuid4 | ||
|
||
from django.test import TestCase | ||
|
||
from enterprise_catalog.apps.catalog.content_metadata_utils import ( | ||
tansform_force_included_courses, | ||
transform_course_metadata_to_visible, | ||
) | ||
|
||
|
||
class ContentMetadataUtilsTests(TestCase): | ||
""" | ||
Tests for content metadata utils. | ||
""" | ||
|
||
def test_transform_course_metadata_to_visible(self): | ||
advertised_course_run_uuid = str(uuid4()) | ||
content_metadata = { | ||
'advertised_course_run_uuid': advertised_course_run_uuid, | ||
'course_runs': [ | ||
{ | ||
'uuid': advertised_course_run_uuid, | ||
'status': 'unpublished', | ||
'availability': 'Coming Soon', | ||
} | ||
], | ||
'course_run_statuses': [ | ||
'unpublished' | ||
] | ||
} | ||
transform_course_metadata_to_visible(content_metadata) | ||
assert content_metadata['course_runs'][0]['status'] == 'published' | ||
assert content_metadata['course_runs'][0]['availability'] == 'Current' | ||
assert content_metadata['course_run_statuses'][0] == 'published' | ||
|
||
def test_tansform_force_included_courses(self): | ||
advertised_course_run_uuid = str(uuid4()) | ||
content_metadata = { | ||
'advertised_course_run_uuid': advertised_course_run_uuid, | ||
'course_runs': [ | ||
{ | ||
'uuid': advertised_course_run_uuid, | ||
'status': 'unpublished', | ||
'availability': 'Coming Soon', | ||
} | ||
], | ||
'course_run_statuses': [ | ||
'unpublished' | ||
] | ||
} | ||
courses = [content_metadata] | ||
tansform_force_included_courses(courses) | ||
assert courses[0]['course_runs'][0]['status'] == 'published' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved this logic into the client, left the old signature to maintain tests