-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: latest attempt endpoint fixes #158
Conversation
from edx_exams.apps.core.models import Exam, ExamAttempt, ProctoringProvider | ||
from edx_exams.apps.core.statuses import ExamAttemptStatus | ||
from edx_exams.apps.core.test_utils.factories import UserFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this util to core since we really wouldn't want the lti/core apps to depend on the api app.
@@ -38,14 +38,11 @@ def get_attempt_by_id(attempt_id): | |||
return attempt | |||
|
|||
|
|||
def get_latest_attempt_for_user(user_id): | |||
def get_active_attempt_for_user(user_id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed this since it's behavior isn't really fetching 'latest' anymore
edx_exams/apps/api/v1/views.py
Outdated
else: | ||
attempt = get_active_attempt_for_user(user.id) | ||
|
||
serialized_attempt = StudentAttemptSerializer(attempt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be more efficient for this to be below the if statement block below? I'm thinking we might otherwise end up serializing this attempt twice for no reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call, there's something odd about that logic here
edx_exams/apps/api/v1/views.py
Outdated
return Response( | ||
status=status.HTTP_200_OK, | ||
data=serialized_attempt.data if latest_attempt is not None else {} | ||
data=serialized_attempt.data if attempt is not None else {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it also be worth maybe checking if attempt is not None here, serializing the data, then sending it back in the response?
efb00a1
to
8522887
Compare
JIRA: MST-1950
Frontend PR: openedx/frontend-lib-special-exams#113
Alter the behavior of this endpoint such that:
The story calls for using exam_id here but that could mislead devs troubleshooting or looking at logs in the future since an exam id could be duplicated between edx-exams and edx-proctoring. I chose content id instead since it's unique.
Hindsight, this probably would be less messy as two endpoints but I didn't want to go back on the decisions we've already made or rework the UI. Once edx-proctoring goes away so does 90% of this logic. I've left a note in code and opened https://2u-internal.atlassian.net/browse/MST-2047