feat: add exec ed course uuid to learner home serializer - #450
Closed
jono-booth wants to merge 1 commit into
Closed
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The serializer currently performs the catalog UUID lookup for all enrollments and the tests don’t validate the intended “ExecEd-only lookup / non-ExecEd no-call” behavior described in the PR.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a courseUuid field to the Learner Home CourseRunSerializer so Executive Education (GetSmarter/Titan) enrollments can include a catalog-backed UUID for downstream redirect/tracking use cases.
Changes:
- Added
courseUuidtoCourseRunSerializerviaSerializerMethodFieldand catalog lookup helper. - Updated Learner Home serializer unit tests to account for the new field.
File summaries
| File | Description |
|---|---|
| lms/djangoapps/learner_home/serializers.py | Introduces courseUuid on CourseRunSerializer using get_course_uuid_for_course. |
| lms/djangoapps/learner_home/test_serializers.py | Adds/updates tests around courseUuid serialization behavior. |
Review details
Suppressed comments (2)
lms/djangoapps/learner_home/test_serializers.py:259
- The new courseUuid tests don't currently verify the intended behavior from the PR description: non‑ExecEd enrollments should keep
courseUuidasNoneand must not trigger a catalog lookup.test_missing_course_uuidshould cover the non‑ExecEd no-call case (and optionally also the ExecEd missing-UUID case).
@mock.patch("lms.djangoapps.learner_home.serializers.get_course_uuid_for_course")
def test_missing_course_uuid(self, mock_get_course_uuid_for_course):
# Given the catalog has no UUID for this course (e.g. no catalog integration configured)
mock_get_course_uuid_for_course.return_value = None
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 None, which is allowed
self.assertIsNone(output_data["courseUuid"])
lms/djangoapps/learner_home/test_serializers.py:240
test_course_uuidcurrently uses the default audit enrollment; ifcourseUuidis correctly restricted to Executive Education modes, this test will fail because no lookup should occur for audit. Create an ExecEd enrollment in this test.
@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)
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+119
to
+121
| def get_courseUuid(self, instance): | ||
| course_uuid = get_course_uuid_for_course(instance.course_id) | ||
| return str(course_uuid) if course_uuid else None |
Comment on lines
+206
to
212
| @mock.patch("lms.djangoapps.learner_home.serializers.get_course_uuid_for_course") | ||
| def test_with_data(self, mock_get_course_uuid_for_course): | ||
| # Mocked so courseUuid is populated, same as every other field | ||
| mock_get_course_uuid_for_course.return_value = uuid4() | ||
| input_data = self.create_test_enrollment() | ||
| input_context = self.create_test_context(input_data.course.id) | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a
courseUuidfield to the Learner HomeCourseRunSerializer, populated for Executive Education (GetSmarter/Titan) enrollments.Supporting information
Planned follow-ups (not in this PR):
frontend-app-learner-dashboard: extenduseCardExecEdTrackingParamto appendcourseUuidas acourse_idquery param on the "Start/Resume Course" link, alongside the existingorg_id. feat: add exec ed course uuid to exec ed course cards cta button frontend-app-learner-dashboard#26titan: read that param inSpree::UsersControllerDecorator#showand redirect straight topartner_olc_link(product)instead of rendering the profile page, when it matches an allocation. https://github.com/getsmarter/titan/pull/3903Testing instructions
executive-education,paid-executive-education, orunpaid-executive-education)./api/learner_home/initor equivalent) and confirm the correspondingcourseRunentry has a non-nullcourseUuid.Covered by new/updated unit tests in
lms/djangoapps/learner_home/test_serializers.py:test_with_data(updated to exercise an enrollment)test_course_uuid(assers courseUUID is available after enrolment)test_missing_course_uuid(assertsNone)Other information