feat: add exec ed course uuid to learner home serializer - #428
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the Learner Home CourseRunSerializer API response to include a courseUuid value (sourced from the Catalog service) for Executive Education (GetSmarter/Titan) enrollments, enabling the frontend to construct Titan redirect URLs with the needed catalog identifier.
Changes:
- Added a
courseUuidserializer method field, populated only for Executive Education enrollment modes. - Introduced an
EXECUTIVE_EDUCATION_MODESallowlist to gate when the catalog lookup occurs. - Updated and expanded serializer tests to verify the field is populated only for Executive Education modes and that non-ExecEd modes do not query the catalog.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
lms/djangoapps/learner_home/serializers.py |
Adds courseUuid to CourseRunSerializer, conditionally fetching the catalog UUID for ExecEd modes only. |
lms/djangoapps/learner_home/test_serializers.py |
Updates tests to cover ExecEd/non-ExecEd behavior and ensures catalog lookup is avoided when not needed. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
74efaac to
11b5ce0
Compare
There was a problem hiding this comment.
🟡 Changes recommended
courseUuid is currently computed for all enrollments (triggering catalog lookup on the common path) and the described gating/no-call behavior isn’t enforced by tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
| def get_courseUuid(self, instance): | ||
| course_uuid = get_course_uuid_for_course(instance.course_id) | ||
| return str(course_uuid) if course_uuid else None |
| @mock.patch("lms.djangoapps.learner_home.serializers.get_course_uuid_for_course") | ||
| def test_with_data(self, mock_get_course_uuid_for_course): | ||
| # Tests courseUuid is populated | ||
| mock_get_course_uuid_for_course.return_value = uuid4() | ||
| input_data = self.create_test_enrollment(course_mode=CourseMode.EXECUTIVE_EDUCATION) |
11b5ce0 to
a86c2ed
Compare
a86c2ed to
fea4a9d
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
courseUuid lookup currently runs for all enrollments (not just ExecEd) and the updated tests don’t yet enforce the intended ExecEd-only behavior and “no catalog call” constraint for non-ExecEd.
Review details
Suppressed comments (4)
lms/djangoapps/learner_home/serializers.py:121
courseUuidis currently looked up via Catalog for every enrollment, which both contradicts the PR description (only ExecEd enrollments should trigger a lookup) and adds an avoidable external call for audit/verified/etc. Please gate the lookup to Executive Education enrollment modes and return None otherwise.
def get_courseUuid(self, instance):
course_uuid = get_course_uuid_for_course(instance.course_id)
return str(course_uuid) if course_uuid else None
lms/djangoapps/learner_home/test_serializers.py:234
test_course_uuidcurrently uses the default audit enrollment, but the new field is intended to be populated only for ExecEd enrollments. This test should create an Executive Education enrollment so it matches the intended behavior and remains stable ifcourseUuidis gated by mode.
@mock.patch("lms.djangoapps.learner_home.serializers.get_course_uuid_for_course")
def test_course_uuid(self, mock_get_course_uuid_for_course):
# Given the catalog has a UUID for this course
course_uuid = uuid4()
mock_get_course_uuid_for_course.return_value = course_uuid
lms/djangoapps/learner_home/test_serializers.py:210
test_with_datanow patchesget_course_uuid_for_coursebut doesn't configure the mock and still uses the default (audit) enrollment; oncecourseUuidis correctly only populated for ExecEd enrollments, this test will start failing (courseUuid should be None for audit). Update the test to exercise an ExecEd enrollment and set an explicit UUID return value so the assertions are meaningful.
This issue also appears in the following locations of the same file:
- line 230
- line 245
def test_with_data(self):
input_data = self.create_test_enrollment()
input_context = self.create_test_context(input_data.course.id)
output_data = CourseRunSerializer(input_data, context=input_context).data
lms/djangoapps/learner_home/test_serializers.py:249
- The tests added for
courseUuiddon't currently assert the key requirement that non-ExecEd enrollments should not call the Catalog lookup and should returnNoneforcourseUuid. Also,test_missing_course_uuidshould use an ExecEd enrollment sincecourseUuidshould be null-by-design for normal enrollments regardless of catalog configuration.
def is_progress_url_matching_course_home_mfe_progress_tab_is_active(self):
"""
Compares the progress URL generated by CourseRunSerializer to the expected progress URL.
:return: True if the generated progress URL matches the expected, False otherwise.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The serializer currently performs the catalog UUID lookup for all enrollments and the added test asserts UUID population for a non-ExecEd enrollment, conflicting with the stated requirement that only ExecEd modes should populate courseUuid and trigger the lookup.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
lms/djangoapps/learner_home/serializers.py:121
courseUuidis currently looked up for every enrollment, which contradicts the PR description (catalog lookup should only fire for Executive Education modes) and can add an unnecessary external/cached API call in the common (audit/verified/etc.) case. Gate the lookup on the enrollment's mode and returnNonewithout calling the catalog for non-ExecEd enrollments.
def get_courseUuid(self, instance):
course_uuid = get_course_uuid_for_course(instance.course_id)
return str(course_uuid) if course_uuid else None
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| @mock.patch("lms.djangoapps.learner_home.serializers.get_course_uuid_for_course") | ||
| def test_course_uuid(self, mock_get_course_uuid_for_course): | ||
| # Given the catalog has a UUID for this course | ||
| course_uuid = uuid4() | ||
| mock_get_course_uuid_for_course.return_value = course_uuid | ||
| input_data = self.create_test_enrollment() | ||
| input_context = self.create_test_context(input_data.course.id) | ||
|
|
||
| # When I serialize | ||
| output_data = CourseRunSerializer(input_data, context=input_context).data | ||
|
|
||
| # Then courseUuid is the stringified catalog UUID, looked up by course_id | ||
| self.assertEqual(output_data["courseUuid"], str(course_uuid)) | ||
| mock_get_course_uuid_for_course.assert_called_once_with(input_data.course_id) | ||
|
|
Description
Adds a
courseUuidfield to the Learner HomeCourseRunSerializer, resolved via the existingget_course_uuid_for_course()catalog utility (already used bycommon.djangoapps.entitlements) for every enrollment — not gated to Executive Education modes. It'snullwhenever the catalog can't resolve a UUID (e.g. no catalog integration configured, which is the common case in most test/local environments).This mirrors how
homeUrland other delegating fields on this serializer are handled: no mode-specific branching, just a straight lookup.Consumers decide whether they care about the value — the actual Executive Education gating lives downstream, in
frontend-app-learner-dashboard's existingisExecEd2UCoursecheck, which is what decides whether to appendcourseUuidas acourse_idquery param on the dashboard CTA at all.This is step 1 of a 3-repo change for ENT-9254 (see below). Today, a learner clicking "Start Course" on their edx.org dashboard for an enrolled ExecEd/GetSmarter presentation lands on a generic Titan/GetSmarter profile page and has to click "View Course" a second time to reach the OLC (Online Learning Campus). Titan can skip that extra hop and redirect straight into the OLC if it's given an identifier for the course.
Supporting information
Related changes in other repos:
frontend-app-learner-dashboard: extendsuseCardExecEdTrackingParamto appendcourseUuidas acourse_idquery param on the "Start/Resume Course" link, alongside the existingorg_id— #26titan: reads that param inSpree::UsersControllerDecorator#showand redirects straight to the OLC instead of rendering the profile page, when it matches a course — #3903This PR is safe to merge standalone — it only adds a new field to an existing API response; nothing consumes it yet.
Testing instructions
/api/learner_home/initor equivalent) and confirm the correspondingcourseRunentry has acourseUuid(non-null whenever the catalog service can resolve one for that course, regardless of mode).Covered by
lms/djangoapps/learner_home/test_serializers.py::TestCourseRunSerializer::test_with_data, which mocks the catalog call socourseUuidis populated, same treatment as every other field on this serializer (no dedicated test, matchinghomeUrl— neither has branching behavior worth testing beyond the general smoke test).Verified with a real test run:
lms/djangoapps/learner_home/(112 tests) passes in full.Other information