feat: add exec ed course uuid to learner home serializer - #451
Open
jono-booth wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Adding a per-enrollment Catalog UUID lookup in a high-traffic serializer can introduce N+1 external calls and noisy error-level logging when Catalog integration is disabled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the Learner Home API’s CourseRunSerializer to include a new courseUuid field, derived via the existing Catalog utility get_course_uuid_for_course(), and updates the serializer test to mock that Catalog lookup so the new field is populated during testing.
Changes:
- Add
courseUuidtoCourseRunSerializer, resolved viaopenedx.core.djangoapps.catalog.utils.get_course_uuid_for_course(). - Update
TestCourseRunSerializer.test_with_datato mock the catalog call socourseUuidis non-null in the “all fields populated” test.
File summaries
| File | Description |
|---|---|
| lms/djangoapps/learner_home/serializers.py | Adds courseUuid to the Learner Home course run payload, computed from Catalog. |
| lms/djangoapps/learner_home/test_serializers.py | Mocks the catalog UUID lookup so the serializer test continues to assert all fields are populated. |
Review details
Suppressed comments (1)
lms/djangoapps/learner_home/serializers.py:121
- Calling
get_course_uuid_for_course()here will emit an error-level log whenever CatalogIntegration is disabled (seecheck_catalog_integration_and_get_user()), and Learner Home can serialize many course runs per request. This can create noisy/error logs in deployments that don’t use the catalog service. Consider guarding the call behind a single integration-enabled check at request scope (and returningNonesilently when disabled) or adjusting the catalog utility’s logging behavior so “integration disabled” isn’t logged per serialized item.
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
💡 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 |
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, resolved via the existingget_course_uuid_for_course()catalog utility (already used bycommon.djangoapps.entitlements)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.
Tests
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.