Skip to content

Add OAuth2 support for problem response reports endpoints - #24873

Closed
pcockwell wants to merge 1 commit into
openedx:masterfrom
open-craft:patrick/BB-2853-upstream-bb-728
Closed

Add OAuth2 support for problem response reports endpoints#24873
pcockwell wants to merge 1 commit into
openedx:masterfrom
open-craft:patrick/BB-2853-upstream-bb-728

Conversation

@pcockwell

@pcockwell pcockwell commented Aug 31, 2020

Copy link
Copy Markdown
Contributor

This PR attempts to re-add functionality that was originally included with https://github.com/edx/edx-platform/pull/19635.

JIRA tickets:
OSPR-4933

Sandbox URL: TBD - sandbox is being provisioned.

Testing instructions:

  1. Run tests related to this new endpoint:
paver test_system -t lms/djangoapps/instructor/tests/test_api.py
  1. Go to the DemoX edX Demonstration Course as staff user.
  2. Check if the generation of problem response reports is working correctly via the instructor interface.
  3. Create an Oauth client here if one does not exists for testing.
  4. Use the Oauth2 endpoint to get an auth token (cUrl, Postman or other tool).
  5. Check get_response_report endpoint. Example:
POST /courses/course-v1:edX+DemoX+Demo_Course/instructor/api/get_problem_responses
Body:
{
    "problem_location": "block-v1:edX+DemoX+Demo_Course+type@problem+block@c554538a57664fac80783b99d9d6da7c"
}
  1. Check list_instructor_tasks endpoint. Example:
POST /courses/course-v1:edX+DemoX+Demo_Course/instructor/api/list_instructor_tasks
Body: {}
  1. Check list_report_downloads endpoint. Example:
POST /courses/course-v1:edX+DemoX+Demo_Course/instructor/api/list_report_downloads
Body: {}

Author notes and concerns:

  1. The code from the original PR was cherry-picked to the current master branch and then conflicts were manually resolved.

Reviewers

@openedx-webhooks

openedx-webhooks commented Aug 31, 2020

Copy link
Copy Markdown

Thanks for the pull request, @pcockwell! I've created OSPR-4933 to keep track of it in JIRA, where we prioritize reviews. 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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Aug 31, 2020
@pcockwell

Copy link
Copy Markdown
Contributor Author

@ormsbee Did the investigation for #19635 produce any useful information as the cause of the problems? Could you describe what was happening such that these errors were occurring and/or how to reproduce them? Any information you have on this would be appreciated.

Right now, the requests made to these endpoints succeed when providing an Authorization header with the Bearer access token included, but they do not appear to require any CSRF Token for the request. My understanding of the Django Rest Framework APIView, is that when [SessionAuthentication](https://www.django-rest-framework.org/api-guide/authentication/#sessionauthentication) is included in authentication_classes, then any requests that use Session based credentials should require a CSRF Token. As of yet though, I can't seem to figure out how to test these endpoints using the CSRF token

Comment thread lms/djangoapps/instructor/views/api.py Outdated
Comment thread lms/djangoapps/instructor/views/api.py Outdated
Comment thread lms/djangoapps/instructor/views/instructor_dashboard.py Outdated
@xitij2000

Copy link
Copy Markdown
Contributor

jenkins run all

Comment thread lms/djangoapps/instructor/views/api.py Outdated

@xitij2000 xitij2000 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please look at the failing tests and rectify.

Comment thread lms/djangoapps/instructor/views/api.py Outdated
Comment thread lms/djangoapps/instructor/views/api.py Outdated
@pcockwell
pcockwell force-pushed the patrick/BB-2853-upstream-bb-728 branch from ec88404 to 1fa00bf Compare September 2, 2020 10:10
@natabene

natabene commented Sep 2, 2020

Copy link
Copy Markdown
Contributor

@pcockwell Thank you for your contribution. Please let me know once it is ready for our review.

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Sep 2, 2020

@xitij2000 xitij2000 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Seems to be working OK. I've just suggested a few minor corrections and updates. See to them and this is done. Doesn't need another review pass from me.

  • I tested this: tested using regular and via instructor dashboard.
  • I read through the code
  • Includes documentation

Comment thread lms/djangoapps/instructor/views/api.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assumes that request is in args[0]
Assumes that request is in args[0].

Comment thread lms/djangoapps/instructor/views/api.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
POST /api/instructor/v1/course/{}/tasks
POST /api/instructor/v1/course/{}/reports

Comment thread lms/djangoapps/instructor/views/api.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update this documentation to also point to the new API endpoints?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xitij2000

Is this what you meant?

Comment thread lms/djangoapps/instructor/views/api.py Outdated
Comment thread lms/djangoapps/instructor/views/api.py Outdated
Comment thread lms/djangoapps/instructor/views/api.py Outdated

@xitij2000 xitij2000 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

  • I tested this: tested endpoints using REST requests
  • I read through the code
  • Includes documentation

@pcockwell
pcockwell force-pushed the patrick/BB-2853-upstream-bb-728 branch from d7ed737 to f82500e Compare September 7, 2020 07:14
@pcockwell pcockwell changed the title Add OAuth2 support for problem response reports endpoints and move API Add OAuth2 support for problem response reports endpoints Sep 7, 2020
@pcockwell

Copy link
Copy Markdown
Contributor Author

@natabene This is now ready for edX review. Thanks!

@pcockwell

Copy link
Copy Markdown
Contributor Author

jenkins run py38

@natabene

natabene commented Sep 8, 2020

Copy link
Copy Markdown
Contributor

@pcockwell Thanks for letting me know, I am lining this up for our review.

@openedx-webhooks openedx-webhooks added awaiting prioritization and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Sep 8, 2020
@pcockwell

Copy link
Copy Markdown
Contributor Author

jenkins run python-3.8

@bradenmacdonald

Copy link
Copy Markdown
Contributor

@pcockwell If this PR is no longer WIP, could you please update the description to reflect its current status?

Please note this PR is a WIP. Functionality testing still needs to occur.

@pcockwell

Copy link
Copy Markdown
Contributor Author

@pcockwell If this PR is no longer WIP, could you please update the description to reflect its current status?

Please note this PR is a WIP. Functionality testing still needs to occur.

Done. Sorry about that

@xitij2000
xitij2000 force-pushed the patrick/BB-2853-upstream-bb-728 branch from bea09ba to f54cf80 Compare January 8, 2021 13:45
@xitij2000

Copy link
Copy Markdown
Contributor

jenkins run a11y

@xitij2000
xitij2000 force-pushed the patrick/BB-2853-upstream-bb-728 branch from 2ac60e7 to 5fb1e03 Compare March 23, 2021 10:56
@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. The following contexts failed:

  • jenkins/a11y
  • jenkins/quality
  • jenkins/python

@xitij2000

Copy link
Copy Markdown
Contributor

@natabene This can be closed since I've opened a new PR here: https://github.com/edx/edx-platform/pull/27313

@natabene natabene closed this Apr 19, 2021
@openedx-webhooks

Copy link
Copy Markdown

@pcockwell Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@xitij2000
xitij2000 deleted the patrick/BB-2853-upstream-bb-728 branch August 26, 2021 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U rejected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants