Instructor Dashboard - API Updates#38458
Instructor Dashboard - API Updates#38458brianjbuck-wgu wants to merge 9 commits intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @brianjbuck-wgu! 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. |
There was a problem hiding this comment.
Pull request overview
Updates the Instructor API v2 “special exams” endpoints to expose additional exam attempt metadata and support client-driven ordering, while also making the due-date-change “reason” field optional/blank-safe.
Changes:
- Allow
reasonto be omitted or blank in the due date change payload. - Add
exam_typeto exam attempt responses (derived from proctored exam flags). - Add
orderingsupport (via in-memory sort) to course allowances and course exam attempts list endpoints, with accompanying tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
lms/djangoapps/instructor/views/serializers_v2.py |
Makes due-date reason accept blank/omitted and adds derived exam_type to exam attempts. |
lms/djangoapps/instructor/views/api_v2.py |
Introduces _sort_in_memory and wiring for ordering on allowances/attempts endpoints. |
lms/djangoapps/instructor/tests/views/test_special_exams_api_v2.py |
Adds coverage for exam_type and new ordering query param behavior. |
lms/djangoapps/instructor/tests/test_api.py |
Adds regression test ensuring due-date changes work when reason is omitted/blank. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Updates Instructor Dashboard v2 APIs to support optional “reason” on due date extensions, expose an exam_type field on exam attempts, and add query-param sorting for special-exams allowances/attempts endpoints.
Changes:
- Make
reasonoptional (including blank) for the due date extension v2 serializer and add coverage for omitted/blank values. - Add
exam_typetoExamAttemptSerializer, derived from proctored/practice flags. - Add
orderingsupport for course allowances and course exam attempts endpoints, backed by a shared in-memory sort helper, with associated tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
lms/djangoapps/instructor/views/serializers_v2.py |
Makes due-date reason optional/blankable and adds derived exam_type to exam attempt serialization. |
lms/djangoapps/instructor/views/api_v2.py |
Introduces _sort_in_memory and wires ordering into allowances + attempts list APIs. |
lms/djangoapps/instructor/tests/views/test_special_exams_api_v2.py |
Adds/extends tests for exam_type and ordering support on special-exams APIs. |
lms/djangoapps/instructor/tests/test_api.py |
Adds regression test ensuring due-date change works with omitted/blank reason. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c2c97fe to
d960f85
Compare
d960f85 to
d07978b
Compare
d07978b to
b8a53d2
Compare
wgu-taylor-payne
left a comment
There was a problem hiding this comment.
Nice set of changes — clean structure overall. A few suggestions below.
Review assisted by Kiro
| ORDERING_FIELDS = { | ||
| 'username': 'user.username', | ||
| 'user.username': 'user.username', | ||
| 'email': 'user.email', | ||
| 'user.email': 'user.email', | ||
| 'exam_name': 'proctored_exam.exam_name', | ||
| 'proctored_exam.exam_name': 'proctored_exam.exam_name', | ||
| 'time_limit': 'proctored_exam.time_limit_mins', | ||
| 'proctored_exam.time_limit_mins': 'proctored_exam.time_limit_mins', | ||
| 'started_at': 'started_at', | ||
| 'completed_at': 'completed_at', | ||
| 'status': 'status', | ||
| } |
There was a problem hiding this comment.
The serializer outputs start_time and end_time (via source='started_at'/source='completed_at'), but the ordering params only accept started_at/completed_at. An API consumer sees start_time in the response JSON but has to know the internal field name to sort by it — and since unrecognized ordering values silently no-op, ?ordering=start_time would just return unsorted results with no error.
Consider accepting the serializer field names as aliases:
| ORDERING_FIELDS = { | |
| 'username': 'user.username', | |
| 'user.username': 'user.username', | |
| 'email': 'user.email', | |
| 'user.email': 'user.email', | |
| 'exam_name': 'proctored_exam.exam_name', | |
| 'proctored_exam.exam_name': 'proctored_exam.exam_name', | |
| 'time_limit': 'proctored_exam.time_limit_mins', | |
| 'proctored_exam.time_limit_mins': 'proctored_exam.time_limit_mins', | |
| 'started_at': 'started_at', | |
| 'completed_at': 'completed_at', | |
| 'status': 'status', | |
| } | |
| ORDERING_FIELDS = { | |
| 'username': 'user.username', | |
| 'user.username': 'user.username', | |
| 'email': 'user.email', | |
| 'user.email': 'user.email', | |
| 'exam_name': 'proctored_exam.exam_name', | |
| 'proctored_exam.exam_name': 'proctored_exam.exam_name', | |
| 'time_limit': 'proctored_exam.time_limit_mins', | |
| 'proctored_exam.time_limit_mins': 'proctored_exam.time_limit_mins', | |
| 'start_time': 'started_at', | |
| 'started_at': 'started_at', | |
| 'end_time': 'completed_at', | |
| 'completed_at': 'completed_at', | |
| 'status': 'status', | |
| } |
Same idea for the docstring example (ordering=-started_at → ordering=-start_time).
| def _get_exam_type(self, attempt): | ||
| """Derive exam type string for sorting purposes.""" | ||
| exam = attempt.get('proctored_exam', {}) | ||
| if exam.get('is_practice_exam'): | ||
| return 'practice' | ||
| if exam.get('is_proctored'): | ||
| return 'proctored' | ||
| return 'timed' |
There was a problem hiding this comment.
This is identical to ExamAttemptSerializer.get_exam_type in serializers_v2.py. If the derivation logic changes, one could be updated without the other. Consider extracting a shared helper:
| def _get_exam_type(self, attempt): | |
| """Derive exam type string for sorting purposes.""" | |
| exam = attempt.get('proctored_exam', {}) | |
| if exam.get('is_practice_exam'): | |
| return 'practice' | |
| if exam.get('is_proctored'): | |
| return 'proctored' | |
| return 'timed' | |
| @staticmethod | |
| def _get_exam_type(attempt): | |
| """Derive exam type string for sorting purposes.""" | |
| return ExamAttemptSerializer.get_exam_type_from_dict( | |
| attempt.get('proctored_exam', {}) | |
| ) |
…or a module-level function both can call. Not blocking, just a maintenance consideration.
| # Verify ascending and descending both succeed | ||
| for prefix in ('', '-'): | ||
| response = self.client.get(self._url(), {'ordering': f'{prefix}{ordering}'}) | ||
| assert response.status_code == status.HTTP_200_OK | ||
| assert len(response.json()['results']) == 2 |
There was a problem hiding this comment.
This only asserts status 200 and count — it would still pass if ordering were completely ignored. test_sort_allowances has a stronger pattern (checking asc_results[0] == desc_results[1]). Consider mirroring that here:
| # Verify ascending and descending both succeed | |
| for prefix in ('', '-'): | |
| response = self.client.get(self._url(), {'ordering': f'{prefix}{ordering}'}) | |
| assert response.status_code == status.HTTP_200_OK | |
| assert len(response.json()['results']) == 2 | |
| # Verify ascending and descending both succeed and reverse order | |
| asc_response = self.client.get(self._url(), {'ordering': ordering}) | |
| desc_response = self.client.get(self._url(), {'ordering': f'-{ordering}'}) | |
| assert asc_response.status_code == status.HTTP_200_OK | |
| asc_results = asc_response.json()['results'] | |
| desc_results = desc_response.json()['results'] | |
| assert len(asc_results) == 2 | |
| assert asc_results[0] == desc_results[1] | |
| assert asc_results[1] == desc_results[0] |
Description
Makes several tweaks to the Instructor API v2 endpoints:
edx_proctoringAPIs return lists instead of querysets.Impacted user roles: Course Staff, Instructors.
Deadline
None
Other information