Skip to content

[BB-2853] Add OAuth2-enabled endpoints for problem response report generation - #27313

Merged
kdmccormick merged 1 commit into
openedx:masterfrom
open-craft:kshitij/bb-2853
May 6, 2021
Merged

[BB-2853] Add OAuth2-enabled endpoints for problem response report generation#27313
kdmccormick merged 1 commit into
openedx:masterfrom
open-craft:kshitij/bb-2853

Conversation

@xitij2000

@xitij2000 xitij2000 commented Apr 13, 2021

Copy link
Copy Markdown
Contributor

Currently, the endpoints related to generation of problem response reports, only support session-based authentication, and cannot be easily used by MFE or other API clients.

A previous PR for this was reverted since it causes some issues. While we were not able to reproduce these issues, in the interest of disturbing as little of the existing code as possible, this PR retains the existing endpoints, but adds alternative endpoints that expose the same views but through a DRF setup that supports OAuth2-based authentication.

Discussions:

Sandbox URL: TBD - sandbox is being provisioned.

Merge deadline: "None"

Testing instructions:

  1. paer test_system -t lms/djangoapps/instructor/tests/test_api.py
  2. Ensure that the existing instructor dashboard report generation works by generating a problem response report, and seeing that it runs properly and creates a new report.
  3. Create an OAuth2 token. You can either create a new OAuth2 client, and use the OAuth2 api to generate a token. Or just add one manually here; /admin/oauth2_provider/accesstoken/add/
    Make sure to use a user that has permissions to generate a report, such as a staff user/
  4. Use postman/httpie/curl etc to call the generate_problem_responses endpoint for a course with a corresponding problem_location. This will return a task id.
  5. Call the list_instructor_task endpoint for the same course. Usually, it is hard to observe the task actually in progress in the devstack, without too much data. The endpoint should work with OAuth though, which is the main thing to test.
  6. Call the list_report_downloads endpoint. It should include the newly-generated report.

Reviewers

Settings

EDXAPP_FEATURES:
  ENABLE_COMBINED_LOGIN_REGISTRATION: true

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Apr 13, 2021
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @xitij2000! I've created OSPR-5734 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.

Comment thread lms/djangoapps/instructor/apps.py Outdated

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.

Originally, these were POST endpoints, however they are GET endpoints under the new API since they simply list items, and potentially a future API could add support for other operations.

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.

This remains a POST operation since it's not idempotent.

Comment on lines 503 to 505

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.

I've reused the existing tests here, and just pointed them to run against the new API as well.

Comment thread lms/djangoapps/instructor/urls.py Outdated
Comment on lines 11 to 17

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.

The base path is now set here instead of in the plugin config.

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

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.

I'm just directly using the existing view method since it already does what we want with the exception of the auth methods supported.

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.

It's a bit weird to use comma-separated strings in a JSON body, when we could even more easily use a proper list. But if that's what the existing API does, we better leave it that way.

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

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.

If we're coming to this method from the DRF view above, this data will be available in request.data, while for the Django view it will be in request.POST.

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.

^ This helpful comment should be added into the code itself, please.

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.

I've split the parsing of this part out separately so that the new API can use a better format.

Comment thread lms/djangoapps/instructor/views/api.py Outdated
Comment on lines 1984 to 2147

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.

The original API uses POST, while the new endpoint is GET, so I've created a very small wrapper to just add the post requirement and CSRF requirement.

Comment on lines 9 to 21

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.

They are v0 URLs only because they are just porting existing code. They've been around a long time and have stayed stable to might be good candidates for v1.

However, when the UI that uses these is ported to an MFE, these APIs can be reevaluated, and a decision can be made about whether these are good enough, or if we need new v1 APIs the are better driven by MFEs.

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.

^ This comment may be worth including inline in the code itself too?

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.

Actually, while writing that comment I convinced myself to just call these v1. With v0 the implication is that the API is still in development and may change in backwards-incompatible ways without bumping up the version number. That doesn't seem to be the case here. These APIs have been in use for a while without major changes.

@natabene

Copy link
Copy Markdown
Contributor

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

@xitij2000

Copy link
Copy Markdown
Contributor Author

@natabene Sure, will do.

@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 Apr 14, 2021
@bradenmacdonald

Copy link
Copy Markdown
Contributor

@xitij2000 Good idea to make these new endpoints. And thanks for adding these nice comments explaining your changes.

@arjunsinghy96 arjunsinghy96 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.

LGTM 👍

  • I tested this: Old endpoints work, task_id is returned on new endpoint and a new report is generated and list api returns the list of all tasks
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

@xitij2000 Thanks for the helpful comments.

@xitij2000

Copy link
Copy Markdown
Contributor Author

@natabene This is good to review now.

CC: @bradenmacdonald Is this a candidate for core committer review? If so I can mark you as reviewer and you could take it up next sprint?

@bradenmacdonald

Copy link
Copy Markdown
Contributor

@natabene @xitij2000 I would be happy to review this, but I'll give @ormsbee the option first, since he reviewed the earlier version of it and also had to revert it. @ormsbee do you want to review?

Comment thread lms/djangoapps/instructor/urls.py Outdated
@xitij2000
xitij2000 force-pushed the kshitij/bb-2853 branch 4 times, most recently from 5dd4a86 to c994306 Compare April 22, 2021 05:47
@natabene

Copy link
Copy Markdown
Contributor

@bradenmacdonald Sounds like this is yours to review.

@bradenmacdonald bradenmacdonald 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.

@xitij2000 Nice work, thanks 👍🏻

Please squash and fix the minor docs issues I just noticed:

@xitij2000

Copy link
Copy Markdown
Contributor Author

@bradenmacdonald Done!

@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. There were no failures.

@kdmccormick
kdmccormick merged commit 161e356 into openedx:master May 6, 2021
@openedx-webhooks

Copy link
Copy Markdown

@xitij2000 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

@bradenmacdonald
bradenmacdonald deleted the kshitij/bb-2853 branch May 6, 2021 18:20
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@xitij2000

Copy link
Copy Markdown
Contributor Author

Thanks!

xitij2000 added a commit to open-craft/openedx-platform that referenced this pull request Jul 30, 2021
xitij2000 added a commit to open-craft/openedx-platform that referenced this pull request Jul 30, 2021
@alfredchavez

alfredchavez commented Oct 26, 2021

Copy link
Copy Markdown
Contributor

@xitij2000 could you port this changes to lilac.master? as part of BB-4877
CC: @arbrandes for review

xitij2000 added a commit to open-craft/openedx-platform that referenced this pull request Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants