feat: add endpoint to get value of unified translations toggle #87
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a new REST API endpoint to check whether the unified site translations feature is enabled for a course. The PR also refactors the toggle from WaffleFlag to CourseWaffleFlag to support course-level overrides.
Changes:
- Adds a new GET endpoint at
/api/courses/<course_id>/unified-translations/enabledthat returns{'enabled': bool} - Changes
ENABLE_UNIFIED_SITE_AND_TRANSLATION_LANGUAGEfromWaffleFlagtoCourseWaffleFlagto support course-level granularity - Adds a helper function
unified_site_and_translation_language_is_enabled()intoggles.pyfor consistency with other toggle checks
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lms/urls.py | Registers the new endpoint URL pattern for unified translations feature status |
| lms/djangoapps/courseware/views/views.py | Implements the view function and imports the new toggle helper |
| lms/djangoapps/courseware/toggles.py | Updates toggle implementation to CourseWaffleFlag and adds helper function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @ddt.data(True, False) | ||
| def test_view(self, enabled): | ||
| course_id = 'course-v1:org+course+run' | ||
| url = reverse('unified_translations_enabled_view', args=[course_id]) | ||
| with override_waffle_flag(ENABLE_UNIFIED_SITE_AND_TRANSLATION_LANGUAGE, enabled): | ||
| response = self.client.get(url) | ||
| assert response.json()['enabled'] == enabled |
There was a problem hiding this comment.
The test coverage is missing a test case for invalid course_id handling. There should be a test that verifies the endpoint returns an appropriate error response when provided with an invalid course identifier.
| urlpatterns += [ | ||
| re_path( | ||
| fr'^courses/{settings.COURSE_ID_PATTERN}/unified-translations/enabled/$', | ||
| courseware_views.unified_site_and_translation_language_enabled, | ||
| name='unified_translations_enabled_view' | ||
| ) | ||
| ] |
There was a problem hiding this comment.
The new URL pattern should be grouped with the related courseware endpoints (lines 746-759) instead of being in a separate urlpatterns block. This improves code organization and maintainability by keeping related endpoints together.
| except InvalidKeyError: | ||
| return JsonResponse({"error": "Invalid course_id"}) |
There was a problem hiding this comment.
The error response for an invalid course_id should include an appropriate HTTP status code. Currently, it returns a 200 status code by default, which is semantically incorrect for an error condition. Consider returning a 400 (Bad Request) status code to properly indicate a client error.
| response = self.client.get(url) | ||
| assert response.json()['enabled'] == enabled |
There was a problem hiding this comment.
The test should verify the HTTP status code of the response. The test currently only checks the response body but doesn't assert that the status code is 200, which is important for API contract validation.
Adds an endpoint
LMS_BASE/api/courses/<course_id>/unified-translations/enabledthat returns{'enabled': bool}to identify if the unified site translations feature is enabled or not.