fix: adjust studio_grading_url to use the correct URL#38415
fix: adjust studio_grading_url to use the correct URL#38415dwong2708 wants to merge 14 commits intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @dwong2708! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
134b1cd to
d07678c
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the Instructor API v2 course metadata response to better support micro-frontend (MFE) URL configuration, including org-specific overrides, and adds a new admin_console_url field gated by instructor-level access.
Changes:
- Add
admin_console_urltoCourseInformationSerializerV2, returning a configured Admin Console authz URL only for users with instructor access. - Update
studio_grading_urlto readCOURSE_AUTHORING_MICROFRONTEND_URLfromMFE_CONFIG, with org-level override support via site configuration helpers. - Update API docs example response and extend/modernize tests (pytest-style assertions) for the new/updated fields.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
lms/djangoapps/instructor/views/serializers_v2.py |
Adds admin_console_url and switches studio_grading_url to org-aware config lookup. |
lms/djangoapps/instructor/views/api_v2.py |
Updates the documented example response to include admin_console_url. |
lms/djangoapps/instructor/tests/test_api_v2.py |
Updates assertions and adds coverage for admin_console_url / studio_grading_url. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Tested on openedx/frontend-app-instructor-dashboard#180 works as expected 👍 |
6a40994 to
df9e5fd
Compare
2a4fbba to
17d04df
Compare
wgu-taylor-payne
left a comment
There was a problem hiding this comment.
Nice work on the admin_console_url feature — the access gating and test coverage across instructor, staff, and data researcher roles look solid.
Two things I think we should address before merging, both around how the MFE URL settings are sourced.
Review assisted by Kiro
| mfe_base_url = configuration_helpers.get_value( | ||
| 'MFE_CONFIG', | ||
| getattr(settings, 'MFE_CONFIG', None) | ||
| ) | ||
| mfe_base_url = mfe_base_url.get('COURSE_AUTHORING_MICROFRONTEND_URL') if mfe_base_url else None |
There was a problem hiding this comment.
I think this should probably use the standalone COURSE_AUTHORING_MICROFRONTEND_URL setting rather than pulling it out of MFE_CONFIG. Based on what I can tell, every other MFE URL consumed by LMS backend code (CATALOG_MICROFRONTEND_URL, ACCOUNT_MICROFRONTEND_URL, LEARNING_MICROFRONTEND_URL, DISCUSSIONS_MICROFRONTEND_URL, PROFILE_MICROFRONTEND_URL, WRITABLE_GRADEBOOK_URL, etc.) reads from a standalone setting — none of them go through MFE_CONFIG. MFE_CONFIG is the config surface served to frontends via the MFE Config API; it's not really intended as a settings source for backend logic. (There is one instance in course_groups/rest_api/views.py that reads STUDIO_BASE_URL from MFE_CONFIG, but that key has no standalone equivalent — different situation.)
tutor-mfe does happen to inject COURSE_AUTHORING_MICROFRONTEND_URL into MFE_CONFIG in its LMS settings patches, so this works for Tutor deployments. But I don't think we should rely on deployment tooling for that contract.
Using configuration_helpers.get_value with the standalone setting name is the right call for site-config override support — we'd just want to change what it's looking up:
| mfe_base_url = configuration_helpers.get_value( | |
| 'MFE_CONFIG', | |
| getattr(settings, 'MFE_CONFIG', None) | |
| ) | |
| mfe_base_url = mfe_base_url.get('COURSE_AUTHORING_MICROFRONTEND_URL') if mfe_base_url else None | |
| mfe_base_url = configuration_helpers.get_value( | |
| 'COURSE_AUTHORING_MICROFRONTEND_URL', | |
| getattr(settings, 'COURSE_AUTHORING_MICROFRONTEND_URL', None) | |
| ) |
Since both LMS and CMS now need this setting, it should probably be moved from cms/envs/common.py to openedx/envs/common.py — that's where the other shared MFE URLs live (ACCOUNT_MICROFRONTEND_URL, LEARNING_MICROFRONTEND_URL, DISCUSSIONS_MICROFRONTEND_URL).
| mfe_base_url = configuration_helpers.get_value( | ||
| 'ADMIN_CONSOLE_MICROFRONTEND_URL', | ||
| getattr(settings, 'ADMIN_CONSOLE_MICROFRONTEND_URL', None) |
There was a problem hiding this comment.
The configuration_helpers.get_value usage here looks right, but ADMIN_CONSOLE_MICROFRONTEND_URL doesn't appear to be defined in any edx-platform settings module currently — the getattr fallback to None keeps it safe, but deployers won't know this setting exists without reading the serializer code.
I think we should add it to openedx/envs/common.py (defaulting to None) with the standard annotation block (setting_name, setting_description, setting_creation_date, setting_use_cases, etc.). The admin console MFE is pretty CMS-centric, and LMS needs the URL now — putting it in the shared settings avoids a future move or duplication if CMS needs it too. This would follow the same pattern as the other MFE URL settings.
17d04df to
0ae3b6a
Compare
Summary
Add
admin_console_urlfield to the Course Metadata API (v2) with instructor-level access control and organization-specific MFE configuration support.What Changed
New Feature
admin_console_urlfield to theCourseInformationSerializerV2that returns the URL to the admin console's authorization management pageADMIN_CONSOLE_MICROFRONTEND_URLis configured (supports org-specific configuration)nullif either condition is not metConfiguration Enhancement
studio_grading_urllogic to use organization-specific configuration viaconfiguration_helpers.get_value_for_org()MFE_CONFIG['COURSE_AUTHORING_MICROFRONTEND_URL']with org-level override support instead of directly usingsettings.COURSE_AUTHORING_MICROFRONTEND_URLTest Improvements
self.assertEqual()withassert ==self.assertIn()withassert inself.assertIsInstance()withassert isinstance()self.assertGreaterEqual()withassert >=# noqa: PT009comments (no longer needed with pytest assertions)admin_console_urlfield:nullfor non-instructor users (data researcher test case)@override_settingsdecorators to properly configure MFE URLs in testsTechnical Details
API Response Changes:
{ "admin_console_url": "http://localhost:2025/admin-console/authz", "studio_grading_url": "http://localhost:2001/authoring/course/{course_id}/settings/grading" }Access Control:
instructoraccess level receive a non-nulladmin_console_urlnullConfiguration:
nullif MFE URL not configuredTesting
test_admin_console_url_requires_instructor_access)Files Modified
lms/djangoapps/instructor/views/serializers_v2.py- Added new field and updated configuration logiclms/djangoapps/instructor/views/api_v2.py- Updated API documentation examplelms/djangoapps/instructor/tests/test_api_v2.py- Enhanced tests with new coverage and pytest assertionsDependencies
ADMIN_CONSOLE_MICROFRONTEND_URLsetting to be configured for the feature to be activeopenedx.core.djangoapps.site_configuration.helpersfor org-specific configurationNote: This change is backward compatible - the new field is always present in the response but returns
nullwhen not applicable, ensuring existing API consumers continue to work without modification.