-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: update initialize
endpoint and create assessments/feedback
endpoint in ORA Staff Grader
#33632
feat: update initialize
endpoint and create assessments/feedback
endpoint in ORA Staff Grader
#33632
Conversation
Thanks for the pull request, @BryanttV! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
6621aa7
to
25d2b74
Compare
25d2b74
to
d137652
Compare
initialize
endpoint and create assessment/feedback
endpoint in ORA Staff Grader
initialize
endpoint and create assessment/feedback
endpoint in ORA Staff Graderinitialize
endpoint and create assessments/feedback
endpoint in ORA Staff Grader
96d0f4d
to
ff88db5
Compare
44188a0
to
4a641da
Compare
Hi @BryanttV! Is this ready for review? |
Hi @mphilbrick211!. Yes, it's ready for review |
aae1092
to
09a10d1
Compare
09a10d1
to
f26c6f8
Compare
7e20bd3
to
b5fc878
Compare
5a3bb34
to
82ae819
Compare
Hi @mariajgrimaldi, I addressed your comments, can you check again? thanks! |
@@ -147,6 +150,121 @@ def get(self, request, ora_location, *args, **kwargs): | |||
return UnknownErrorResponse() | |||
|
|||
|
|||
class AssessmentFeedbackToView(StaffGraderBaseView): |
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.
FYI, it is possible to declare a single DRF view with multiple "GET" endpoints: https://stackoverflow.com/a/70301277
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.
Hi @pomegranited, I didn't know that was possible, thanks for commenting! I have already made the necessary changes to use a single view. Could you check it again?
feat: generate_assessment_data handler created fix: style and functions rename fix: test submission metadata serializers upgraded fix: style and some test and docs improvements fix: HTTPStatus conditional chore: update assessment_filter argument and update docstring chore: fix quality errors chore: remove inline comments and improve docstring of serializers refactor: rename assessment_filter to assessment_type
6ef3209
to
ecb3fe1
Compare
except Exception as ex: | ||
log.exception(ex) | ||
return UnknownErrorResponse() |
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.
@BryanttV I'm hitting this general Exception clause when I try accessing these endpoints as a student. Traceback below; is there a better way to catch xblock handler permission errors?
edx.devstack.lms | 2024-02-18 12:05:23,986 ERROR 983 [lms.djangoapps.ora_staff_grader.views] [user 6] [ip 172.19.0.1] views.py:243 - Expecting value: line 1 column 1 (char 0)
edx.devstack.lms | Traceback (most recent call last):
edx.devstack.lms | File "/edx/app/edxapp/edx-platform/lms/djangoapps/ora_staff_grader/views.py", line 230, in _get_assessments
edx.devstack.lms | assessments_data = {"assessments": getter_func(request, ora_location, submission_uuid)}
edx.devstack.lms | File "/edx/app/edxapp/edx-platform/lms/djangoapps/ora_staff_grader/ora_api.py", line 63, in get_assessments_to
edx.devstack.lms | return json.loads(response.content)
edx.devstack.lms | File "/usr/lib/python3.8/json/__init__.py", line 357, in loads
edx.devstack.lms | return _default_decoder.decode(s)
edx.devstack.lms | File "/usr/lib/python3.8/json/decoder.py", line 337, in decode
edx.devstack.lms | obj, end = self.raw_decode(s, idx=_w(s, 0).end())
edx.devstack.lms | File "/usr/lib/python3.8/json/decoder.py", line 355, in raw_decode
edx.devstack.lms | raise JSONDecodeError("Expecting value", s, err.value) from None
edx.devstack.lms | json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
edx.devstack.lms | Internal Server Error: /api/ora_staff_grader/assessments/feedback/to/
edx.devstack.lms | 2024-02-18 12:05:24,056 ERROR 983 [django.request] [user None] [ip None] log.py:241 - Internal Server Error: /api/ora_staff_grader/assessments/feedback/to/
edx.devstack.lms | [18/Feb/2024 12:05:24] "GET /api/ora_staff_grader/assessments/feedback/to/?oraLocation=block-v1%3AedX%2BDemoX%2BDemo_Course%2Btype%40openassessment%2Bblock%40bce6c7ff830a473f9971204964424221&submissionUUID=90fe5b5b-03fa-4371-af26-2ee49939483f HTTP/1.1" 500 19098
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.
Hi @pomegranited, I was doing some research and found the following:
-
Currently, this is how the other ORA Staff Grader endpoints behave.
-
The error is generated from the following line. When there is a permissions error from ORA the following is returned in the
response.content
:b'<div>You do not have permission to access ORA staff grading.</div>\n
When trying to parse this string to a dict using
json.loads()
it breaks 💥
I think that this permission error should be raised by the XBlockInternalError
, but since the status_code
of the response always returns a 200
, the error would never be raised, even though it exists. In my opinion, if there is any type of error from the handler, the most appropriate thing to do is to return the corresponding 4xx
code.
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.
I see that also happens with the existing initialize
endpoint. The views don't check for permissions, but it waits for the ORA handlers to return:
> response.content
b'<div>You do not have permission to access ORA staff grading.</div>\n'.
So maybe it'd be enough to return this error, but avoiding the unknown error by wrapping json.loads(response.content)
in a try-except block.
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.
By doing this change:
try:
return json.loads(response.content)
except json.JSONDecodeError as exc:
raise XBlockInternalError(
context={"handler": handler_name, "details": response.content}
) from exc
The response looks like this:
{
"error": "ERR_INTERNAL",
"handler": "list_assessments_to",
"details": "<div>You do not have permission to access ORA staff grading.</div>\n"
}
And the log looks like this:
tutor_dev-lms-1 | 2024-02-20 17:49:21,564 ERROR 31 [lms.djangoapps.ora_staff_grader.views] [user 6] [ip 172.18.0.1] views.py:243 - {'handler': 'list_assessments_to', 'details': b'<div>You do not have permission to access ORA staff grading.</div>\n'}
tutor_dev-lms-1 | Internal Server Error: /api/ora_staff_grader/assessments/feedback/to/
tutor_dev-lms-1 | 2024-02-20 17:49:21,668 ERROR 31 [django.request] [user None] [ip None] log.py:241 - Internal Server Error: /api/ora_staff_grader/assessments/feedback/to/
tutor_dev-lms-1 | [20/Feb/2024 17:49:21] "GET /api/ora_staff_grader/assessments/feedback/to/?oraLocation=block-v1%3Aedunext%2BDemo%2BORA%2Btype%40openassessment%2Bblock%4075685668559f451dabd6ebe6375ad080&submissionUUID=29872ddb-4dfd-4f17-b4bb-459ce1167fcc HTTP/1.1" 500 137
A Django response object containing serialized assessment data or an error response. | ||
""" | ||
try: | ||
assessments_data = {"assessments": getter_func(request, ora_location, submission_uuid)} |
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.
The only difference between get_assessments_from
and get_assessments_to
now is the handler_name
.. you could consolidate them and pass the handler instead of a Callable to this method?
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.
You are right! I'll make the changes
Hi @mariajgrimaldi, @pomegranited. All the requested changes are in place. Could you please check again? Thank you very much! |
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.
This is working as expected. Thank you!
If @pomegranited agrees, we can merge this one. Thank you folks!
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.
LGTM, thank you @BryanttV @mariajgrimaldi !
@BryanttV 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
For this PR we have the following goals:
/api/ora_staff_grader/assessments/feedback/from
endpoint to list the assessment received for a specific submission of a learner./api/ora_staff_grader/assessments/feedback/to
endpoint to list the assessment given by a learner based on their submission ID./api/ora_staff_grader/initialize
endpoint to add thefullname
andemail
fields.Note
This PR in the ORA Grading MFE will use these endpoints to display a more detailed table to the instructor.
Dependencies
Important
This upgrade will consist of two PRs: one for ORA and the other for edx-platform. This is because the main serializer of the service resides in the platform. To test it, you must fetch and set up the ORA version with the new handlers.
initialize
endpoint and createassessments/feedback
endpoint in ORA Staff Grader edx-ora2#2101What changed?
1. New endpoints
/api/ora_staff_grader/assessments/feedback/from
This endpoint fetches assessment details for a given:
/api/ora_staff_grader/assessments/feedback/to
This endpoint fetches assessment details for a given:
Both endpoints return the scorer information and scoring details to fill the following Staff Grade MFE table:
The following is an example of the response from either of the two endpoints:
2. Updated endpoint
/api/ora_staff_grader/initialize
This is a pre-existing endpoint to which two extra fields are added (
fullname
,email
) to fill the following table:The following is an example of the response from this endpoint:
Testing Instructions
To test this, you can use Postman:
Get an auth token from
/oauth2/access_token/
with the following x-www-form-urlencoded content type body:From the response, copy the
access_token
, which must be sent in the headers of the endpoints.Test the endpoints:
/api/ora_staff_grader/initialize
/api/ora_staff_grader/assessments/feedback/from
/api/ora_staff_grader/assessments/feedback/to
NOTE: From the MFE ora-grading you can get the
oraLocation
from here:And the
submissionUUID
from the response of theinitialize
endpoint here:Make sure that the
oraLocation
value is encoded.