feat: add exec ed course uuid to learner home serializer - #451
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.
69a1d0a to
4aa20a7
Compare
brobro10000
left a comment
There was a problem hiding this comment.
Please regen the migration file with the Django command.
4aa20a7 to
aa8a0ba
Compare
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.