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

xitij2000 · 2021-04-13T08:29:04Z

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:

https://github.com/edx/edx-platform/pull/19635
https://github.com/edx/edx-platform/pull/24873
This PR is following the pattern detailed in this ADR PR:https://github.com/edx/edx-platform/pull/27129

Sandbox URL: TBD - sandbox is being provisioned.

Merge deadline: "None"

Testing instructions:

paer test_system -t lms/djangoapps/instructor/tests/test_api.py
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.
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/
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.
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.
Call the list_report_downloads endpoint. It should include the newly-generated report.

Reviewers

@arjunsinghy96
edX reviewer[s] TBD

Settings

EDXAPP_FEATURES:
  ENABLE_COMBINED_LOGIN_REGISTRATION: true

openedx-webhooks · 2021-04-13T08:29:09Z

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.

lms/djangoapps/instructor/apps.py

xitij2000 · 2021-04-13T11:31:36Z

lms/djangoapps/instructor/tests/test_api.py

+    'instructor_api_v0:list_instructor_tasks',
+    'instructor_api_v0:list_report_downloads',


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.

xitij2000 · 2021-04-13T11:32:58Z

lms/djangoapps/instructor/tests/test_api.py

@@ -179,6 +181,7 @@
    'students_update_enrollment',
    'update_forum_role_membership',
    'override_problem_score',
+    'instructor_api_v0:generate_problem_responses'


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

xitij2000 · 2021-04-13T11:34:26Z

lms/djangoapps/instructor/tests/test_api.py

        mock_problem_key.course_key = self.course.id
        with patch.object(UsageKey, 'from_string') as patched_method:
            patched_method.return_value = mock_problem_key
-            self._access_endpoint('get_problem_responses', {}, 200, msg)
+            self._access_endpoint(endpoint, {}, 200, msg)


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

xitij2000 · 2021-04-13T11:35:10Z

lms/djangoapps/instructor/urls.py

+urlpatterns = [
+    url(rf'^courses/{COURSE_ID_PATTERN}/instructor/api/', include(api_urls.urlpatterns)),
+    url(rf'^instructor/api/v0/courses/{COURSE_ID_PATTERN}/',
+        include((api_urls.v0_api_urls, 'lms.djangoapps.instructor'), namespace='instructor_api_v0')),
+]


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

xitij2000 · 2021-04-13T11:36:28Z

lms/djangoapps/instructor/views/api.py

+        """
+        Initiates asynchronous problem response report generation.
+        """
+        return get_problem_responses(request=request, course_id=course_id)


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

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.

xitij2000 · 2021-04-13T11:38:03Z

lms/djangoapps/instructor/views/api.py

@@ -1024,9 +1095,9 @@ def get_problem_responses(request, course_id):
    # The name of the POST parameter is `problem_location` (not pluralised) in
    # order to preserve backwards compatibility with existing third-party
    # scripts.
-    problem_locations = request.POST.get('problem_location', '')
+    problem_locations = getattr(request, 'data', request.POST).get('problem_location', '')


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.

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

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

xitij2000 · 2021-04-13T11:44:34Z

lms/djangoapps/instructor/views/api.py

 @require_POST
 @ensure_csrf_cookie
-@cache_control(no_cache=True, no_store=True, must_revalidate=True)
-@require_course_permission(permissions.SHOW_TASKS)
 def list_instructor_tasks(request, course_id):
    """
    List instructor tasks.

-    Takes optional query paremeters.
+    Takes optional query parameters.
        - With no arguments, lists running tasks.
        - `problem_location_str` lists task history for problem
        - `problem_location_str` and `unique_student_identifier` lists task
            history for problem AND student (intersection)
    """
+    return _list_instructor_tasks(request=request, course_id=course_id)
+
+
+@cache_control(no_cache=True, no_store=True, must_revalidate=True)
+@require_course_permission(permissions.SHOW_TASKS)
+def _list_instructor_tasks(request, course_id):


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.

xitij2000 · 2021-04-13T11:47:14Z

lms/djangoapps/instructor/views/api_urls.py

+v0_api_urls = [
+    url(r'^tasks$', api.InstructorTasks.as_view(), name='list_instructor_tasks', ),
+    url(r'^reports$', api.ReportDownloads.as_view(), name='list_report_downloads', ),
+    url(r'^generate_report/problem_responses$', api.ProblemResponseReportInitiate.as_view(),
+        name='generate_problem_responses', ),
+]
+


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.

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

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 · 2021-04-13T15:37:14Z

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

xitij2000 · 2021-04-14T19:05:15Z

@natabene Sure, will do.

bradenmacdonald · 2021-04-14T22:14:34Z

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

arjunsinghy96

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 · 2021-04-15T14:03:28Z

@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 · 2021-04-16T17:50:10Z

@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?

lms/djangoapps/instructor/urls.py

natabene · 2021-04-26T15:00:35Z

@bradenmacdonald Sounds like this is yours to review.

bradenmacdonald

@xitij2000 Nice work, thanks 👍🏻

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

…can be driven by MFEs.

xitij2000 · 2021-05-05T04:52:06Z

@bradenmacdonald Done!

edx-status-bot · 2021-05-05T14:48:52Z

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

openedx-webhooks · 2021-05-06T18:05:17Z

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

edx-pipeline-bot · 2021-05-06T19:12:49Z

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

edx-pipeline-bot · 2021-05-06T19:37:41Z

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

xitij2000 · 2021-05-07T06:09:02Z

Thanks!

…can be driven by MFEs. (openedx#27313) (cherry picked from commit 161e356)

…can be driven by MFEs. (openedx#27313) (#381) (cherry picked from commit 161e356)

alfredchavez · 2021-10-26T23:41:22Z

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

…can be driven by MFEs. (openedx#27313) (cherry picked from commit 161e356)

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

xitij2000 force-pushed the kshitij/bb-2853 branch from 673c6cd to d366719 Compare April 13, 2021 10:47

xitij2000 commented Apr 13, 2021

View reviewed changes

lms/djangoapps/instructor/apps.py Show resolved Hide resolved

xitij2000 commented Apr 13, 2021

View reviewed changes

xitij2000 force-pushed the kshitij/bb-2853 branch from d366719 to dffa06c Compare April 14, 2021 20:57

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

arjunsinghy96 approved these changes Apr 15, 2021

View reviewed changes

xitij2000 mentioned this pull request Apr 15, 2021

Add OAuth2 support for problem response reports endpoints #24873

Closed

2 tasks

jramnai reviewed Apr 18, 2021

View reviewed changes

lms/djangoapps/instructor/urls.py Outdated Show resolved Hide resolved

xitij2000 force-pushed the kshitij/bb-2853 branch 4 times, most recently from 5dd4a86 to c994306 Compare April 22, 2021 05:47

openedx-webhooks added the engineering review label Apr 26, 2021

bradenmacdonald approved these changes May 4, 2021

View reviewed changes

feat: Add REST endpoints for problem response report generation that …

ade9582

…can be driven by MFEs.

xitij2000 force-pushed the kshitij/bb-2853 branch from df8ede5 to ade9582 Compare May 5, 2021 04:51

xitij2000 force-pushed the kshitij/bb-2853 branch from 2391876 to ade9582 Compare May 5, 2021 14:02

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

openedx-webhooks added merged and removed engineering review labels May 6, 2021

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

xitij2000 mentioned this pull request Jul 27, 2021

feat: Add REST endpoints for problem response report generation that can be driven by MFEs. (#27313) open-craft/edx-platform#381

Merged

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

feat: Add REST endpoints for problem response report generation that …

3a76d23

…can be driven by MFEs. (openedx#27313) (cherry picked from commit 161e356)

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

feat: Add REST endpoints for problem response report generation that …

6386407

…can be driven by MFEs. (openedx#27313) (#381) (cherry picked from commit 161e356)

xitij2000 added a commit to open-craft/edx-platform that referenced this pull request Nov 22, 2021

feat: Add REST endpoints for problem response report generation that …

cdf4463

…can be driven by MFEs. (openedx#27313) (cherry picked from commit 161e356)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

xitij2000 commented Apr 13, 2021 •

edited

Loading

openedx-webhooks commented Apr 13, 2021

xitij2000 Apr 13, 2021

xitij2000 Apr 13, 2021

xitij2000 Apr 13, 2021

xitij2000 Apr 13, 2021

xitij2000 Apr 13, 2021

bradenmacdonald Apr 29, 2021

xitij2000 Apr 13, 2021

bradenmacdonald Apr 29, 2021

xitij2000 Apr 30, 2021

xitij2000 Apr 13, 2021

xitij2000 Apr 13, 2021

bradenmacdonald Apr 14, 2021

xitij2000 Apr 15, 2021

natabene commented Apr 13, 2021

xitij2000 commented Apr 14, 2021

bradenmacdonald commented Apr 14, 2021

arjunsinghy96 left a comment

xitij2000 commented Apr 15, 2021

bradenmacdonald commented Apr 16, 2021

natabene commented Apr 26, 2021

bradenmacdonald left a comment

xitij2000 commented May 5, 2021

edx-status-bot commented May 5, 2021

openedx-webhooks commented May 6, 2021

edx-pipeline-bot commented May 6, 2021

edx-pipeline-bot commented May 6, 2021

xitij2000 commented May 7, 2021

alfredchavez commented Oct 26, 2021 •

edited

Loading

		'instructor_api_v0:list_instructor_tasks',
		'instructor_api_v0:list_report_downloads',

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

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

Conversation

xitij2000 commented Apr 13, 2021 • edited Loading

openedx-webhooks commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natabene commented Apr 13, 2021

xitij2000 commented Apr 14, 2021

bradenmacdonald commented Apr 14, 2021

arjunsinghy96 left a comment

Choose a reason for hiding this comment

xitij2000 commented Apr 15, 2021

bradenmacdonald commented Apr 16, 2021

natabene commented Apr 26, 2021

bradenmacdonald left a comment

Choose a reason for hiding this comment

xitij2000 commented May 5, 2021

edx-status-bot commented May 5, 2021

openedx-webhooks commented May 6, 2021

edx-pipeline-bot commented May 6, 2021

edx-pipeline-bot commented May 6, 2021

xitij2000 commented May 7, 2021

alfredchavez commented Oct 26, 2021 • edited Loading

xitij2000 commented Apr 13, 2021 •

edited

Loading

alfredchavez commented Oct 26, 2021 •

edited

Loading