-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: upgrading list_instructor_tasks to DRF ( 10th ) #35332
Conversation
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.
A couple of comments that might require some re-work here.
@@ -59,3 +59,40 @@ def validate_unique_student_identifier(self, value): | |||
return None | |||
|
|||
return user | |||
|
|||
|
|||
class ListInstructorSerializer(serializers.Serializer): # pylint: disable=abstract-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.
Suggestion: I think a better name for this would be the ListInstructorTaskInputSerializer
.
course_id = CourseKey.from_string(course_id) | ||
params = getattr(request, 'query_params', request.POST) | ||
problem_location_str = strip_if_string(params.get('problem_location_str', False)) | ||
student = params.get('unique_student_identifier', None) | ||
|
||
# For the DRF POST method, retrieve the data from request.data | ||
if not student and not problem_location_str: |
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.
Why not just pass the serializer from the POST
method above? You've already parsed this data out, may as well use that rather than re-getting it from the post data.
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.
Done, now picking validated-data.
serializer_class = ListInstructorSerializer | ||
|
||
@method_decorator(ensure_csrf_cookie) | ||
def post(self, request, course_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.
It looks like this API was previously a POST method but with query string input params, Can we just update the instructor dash to call this as a GET
method instead since it doesn't actually modify any data?
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.
They are not sending any data. Its a just a post call without any payload. I just maintained the behaviour. May be some other front-end is using it this way ?
Also another similar api exists with GET
and its hitting same method for getting data. But its different version.
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.
Interesting, okay seems like we have some redundancy that we should cleanup but that can be part of some future work. I don't want to blow up the scope of this change.
serializer_class = ListInstructorSerializer | ||
|
||
@method_decorator(ensure_csrf_cookie) | ||
def post(self, request, course_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.
Interesting, okay seems like we have some redundancy that we should cleanup but that can be part of some future work. I don't want to blow up the scope of this change.
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. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
Issue
verify via Postman
expected result
You need some data in instructor tasks table for this.
Verify via intructor dashboard-datadownload