Repository navigation
feat: add certificate proctoring status API endpoint - #447
rahulkanneri-2u wants to merge 2 commits into
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||
|
|
||
| authentication_classes = (JwtAuthentication,) | ||
| permission_classes = (ServiceUserPermissions,) |
There was a problem hiding this comment.
ServiceUserPermissions is the same check as DRF's IsAdminUser (bool(request.user and request.user.is_staff)). The built-in would let us drop the new class from permissions.py and the import here (add IsAdminUser to the rest_framework.permissions import):
| permission_classes = (ServiceUserPermissions,) | |
| permission_classes = (IsAuthenticated, IsAdminUser) |
| latest_by_exam = {attempt.exam_id: attempt for attempt in attempts} | ||
|
|
||
| return [(exam, latest_by_exam.get(exam.id)) for exam in exams] |
There was a problem hiding this comment.
check_if_exam_timed_out only runs when the learner reopens the exam, so a timed-out attempt can sit in started indefinitely and the LMS shows the wrong reason. get_exam_attempt_time_remaining is a pure function, so the effective status can be reported without writing anything:
| latest_by_exam = {attempt.exam_id: attempt for attempt in attempts} | |
| return [(exam, latest_by_exam.get(exam.id)) for exam in exams] | |
| latest_by_exam = {attempt.exam_id: attempt for attempt in attempts} | |
| # Report an in-progress attempt whose time has run out as submitted, without saving it: | |
| # check_if_exam_timed_out only runs when the learner reopens the exam, so the stored | |
| # status can stay "started" indefinitely. | |
| for attempt in latest_by_exam.values(): | |
| if ( | |
| attempt.status in ExamAttemptStatus.in_progress_statuses | |
| and get_exam_attempt_time_remaining(attempt) == 0 | |
| ): | |
| attempt.status = ExamAttemptStatus.submitted | |
| return [(exam, latest_by_exam.get(exam.id)) for exam in exams] |
abhalsod-sonata
left a comment
There was a problem hiding this comment.
Clean, read-only and batched (one query for exams, one for attempts) instead of get_current_exam_attempt per exam. Two suggestions, both inline:
- Timed-out attempts. Because this skips the timeout transition, an attempt whose time ran out keeps reporting
starteduntil the learner reopens the exam, and nothing else advances it. edx/edx-platform#522 treatsstartedas incomplete, so the learner is told to "complete your required proctored exam" when they can't, instead of seeing the "under review" message. Reporting it assubmittedwithout saving keeps the endpoint read-only and matches whatcheck_if_exam_timed_outwould store. - Permission class.
ServiceUserPermissionsis DRF'sIsAdminUser. Using the built-in drops a class whose name implies tighter scoping than it has (your own concern in the description).
Others LGTM.
Pin created/id as the source of truth for the latest attempt: an older attempt with a higher attempt_number or newer modified time must not win.
JIRA:
Description: Adds a read-only endpoint,
GET /api/v1/certificate_proctoring/course_id/{course_id}/status?lms_user_id={id}, that lets the LMS fetch a learner's proctoring status for certificate decisions. For each active, non-practice proctored exam in the course it returns the learner's latest attempt (id and status), or nulls when there is no attempt. Access is limited to service accounts and global staff through a newServiceUserPermissionsclass.Author concerns:
ServiceUserPermissionsonly checksis_staff, so any global staff user passes, not just the LMS service account. It matches the existingis_staffbehaviour, but the name and docstring imply tighter scoping.user__lms_user_id, which is indexed but not unique. If duplicate user rows share an id, the newest attempt across them is returned.submittedtransition that learner-facing views perform. A timed-outstartedattempt is therefore reported asstarted, which the LMS needs to account for.Dependencies:
edx/edx-platform#522
Installation instructions: None. There are no migrations or new settings.
Testing instructions:
pytest edx_exams/apps/api/v1/tests/test_views.py -k CertificateProctoringStatusViewTestsGET /api/v1/certificate_proctoring/course_id/<course_id>/status?lms_user_id=<id>.course_id,lms_user_idand anexamslist, one entry per active proctored exam, each with the latestattempt_idandstatus.lms_user_idor pass a non-integer value and expect 400.Merge checklist:
Post merge: