Skip to content

[BB-728] Add problem response report API - #19635

Merged
ormsbee merged 1 commit into
openedx:masterfrom
open-craft:giovanni/bb-728-add-problem-response-report-api-upstream
Feb 26, 2019
Merged

[BB-728] Add problem response report API#19635
ormsbee merged 1 commit into
openedx:masterfrom
open-craft:giovanni/bb-728-add-problem-response-report-api-upstream

Conversation

@giovannicimolin

@giovannicimolin giovannicimolin commented Jan 22, 2019

Copy link
Copy Markdown
Contributor

This PR enables endpoints related to the problem response reports to be accessible by OAuth clients instead of just the LMS internal views.

JIRA tickets: OSPR-3008

Dependencies: None

Sandbox URL: https://pr19635.sandbox.opencraft.hosting/

Merge deadline: None.

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:
    Request:
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:
    Request:
POST /courses/course-v1:edX+DemoX+Demo_Course/instructor/api/list_instructor_tasks
Body: {}
  1. Check list_report_downloads endpoint. Example:
    Request:
POST /courses/course-v1:edX+DemoX+Demo_Course/instructor/api/list_report_downloads
Body: {}

Author notes and concerns:

  1. This is a upstream of features from an old branch, so the code might use some different libs and conventions from upstream's.
  2. I'm not sure if the way I've used to move the endpoints was correct.
  3. I'm not sure of the best place to put permissions, I've tried edx-platform/openedx/core/lib/api/permissions.py but I needed to use some methods from the courseware module, and it didn't felt right to import them there. So until I figure out the best place to put the permissions, I've create a permissions.py inside the app.

Reviewers

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @giovannicimolin! I've created OSPR-3008 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email 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 still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Jan 22, 2019
@giovannicimolin giovannicimolin changed the title [BB-728] Add problem response report API [BB-728] Add problem response report API [WIP] Jan 22, 2019
@xitij2000

Copy link
Copy Markdown
Contributor

jenkins run all

@giovannicimolin
giovannicimolin force-pushed the giovanni/bb-728-add-problem-response-report-api-upstream branch from fd5ec25 to 5c07848 Compare January 22, 2019 16:13
@natabene

Copy link
Copy Markdown
Contributor

@giovannicimolin Thank you for your contribution, please let me know once tests pass.

@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 Jan 22, 2019
@giovannicimolin

Copy link
Copy Markdown
Contributor Author

@xitij2000 This is ready for review.

@giovannicimolin giovannicimolin changed the title [BB-728] Add problem response report API [WIP] [BB-728] Add problem response report API Jan 23, 2019
@giovannicimolin

Copy link
Copy Markdown
Contributor Author

jenkins run bokchoy

1 similar comment
@giovannicimolin

Copy link
Copy Markdown
Contributor Author

jenkins run bokchoy

@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'd like to see if we can organise the API similar to other APIs since we're changing things anyway.

So something like:

GET /api/instructor/v1/course/{}/tasks
GET /api/instructor/v1/course/{}/reports
POST /api/instructor/v1/course/{}/reports/problem_responses

etc.

I am not sure why some of these list endpoints need POST. Perhaps someone from edX can comment with more information or context on that.

If we make these changes, I think only the changed APIs should go under /api/instructor/v1 the rest should stay in their current location.

Comment thread lms/djangoapps/instructor/views/api.py Outdated
Comment thread openedx/core/lib/api/permissions.py Outdated
Comment thread lms/djangoapps/instructor/views/api.py Outdated
Comment thread lms/djangoapps/instructor/views/api.py Outdated
@giovannicimolin
giovannicimolin force-pushed the giovanni/bb-728-add-problem-response-report-api-upstream branch from 92c8a2b to 3c7aa6a Compare January 24, 2019 12:48
@giovannicimolin

Copy link
Copy Markdown
Contributor Author

@xitij2000 I've addressed most of your comments. I just left out the method change because I'm waiting on someone from edx commenting about this.

@giovannicimolin

Copy link
Copy Markdown
Contributor Author

@natabene This is ready for review.

@natabene

Copy link
Copy Markdown
Contributor

@giovannicimolin Thanks for letting me know. @ormsbee Can you give this a look or would you recommend someone else as a reviewer?

@natabene
natabene requested a review from ormsbee January 26, 2019 06:35
@openedx-webhooks openedx-webhooks added architecture review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jan 26, 2019
@ormsbee

ormsbee commented Feb 5, 2019

Copy link
Copy Markdown
Contributor

@giovannicimolin

Thank you for this PR! Quick question on compatibility before I start the main review:

The Instructor Dashboard URLs you're moving around aren't an officially supported API, but it's likely that our power users have scripts that rely on the current behavior (and use session auth to get their info). That's not necessarily a blocker to moving things, particularly if we message it properly for Juniper -- but I'd rather keep compatibility for another release if it's not too expensive to do so. Would it be difficult to maintain backwards compatibility (e.g. have the old URLs work, but also place them in the approved new location)? Again, this not necessarily a blocker, but I'd like to understand how difficult it would be to do something like that.

FYI @pdpinch ^

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed engineering review labels Feb 5, 2019
@giovannicimolin

Copy link
Copy Markdown
Contributor Author

@ormsbee All the checks are green now. Ping me if there's anything else for me to address 😁

@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 Feb 15, 2019
@pdpinch

pdpinch commented Feb 21, 2019

Copy link
Copy Markdown
Contributor

@ormsbee Thank you for the heads-up. It sounds like the old endpoints will be maintained, so I don't have any concern about this.

I'll have to remember to ask you all later about your plans for API lifecycles in a microfrontend future.

@natabene

Copy link
Copy Markdown
Contributor

@ormsbee I think it is ready for your review, can you give this a look?

@openedx-webhooks

Copy link
Copy Markdown

@giovannicimolin 🎉 Your pull request was merged!

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

@giovannicimolin
giovannicimolin deleted the giovanni/bb-728-add-problem-response-report-api-upstream branch February 26, 2019 12:34
@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 on Wednesday, February 27, 2019.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@ormsbee

ormsbee commented Feb 28, 2019

Copy link
Copy Markdown
Contributor

@giovannicimolin: We have reason to suspect this PR might have been the cause of a large spike in CSRF errors when rolled out to production. Investigating, but we may need to revert this.

@giovannicimolin

Copy link
Copy Markdown
Contributor Author

@ormsbee Ok, ping me if you need any help with this PR.

@ormsbee

ormsbee commented Feb 28, 2019

Copy link
Copy Markdown
Contributor

Okay, I'm beginning to suspect that it has to do with the removal of ensure_csrf_cookie...

@ormsbee

ormsbee commented Feb 28, 2019

Copy link
Copy Markdown
Contributor

Going to try to recreate this locally.

@giovannicimolin

Copy link
Copy Markdown
Contributor Author

That's strange. I didn't bump into any issue while testing it with my master devstack.
I'll also try to reproduce it locally and I'm ready to create a fix for this when we pinpoint the exact cause.

Sorry for the trouble and thanks for pinging me.

@ormsbee

ormsbee commented Feb 28, 2019

Copy link
Copy Markdown
Contributor

I agree that it's strange, and I'm having difficulty reproducing as well. I mean, I can reproduce trivially by killing my csrftoken but I can't get it so that the old code actually somehow recovers while the new one doesn't. To unblock release, I'm going to revert this. I'm continuing to investigate.

@giovannicimolin
giovannicimolin restored the giovanni/bb-728-add-problem-response-report-api-upstream branch February 28, 2019 18:14
@giovannicimolin

giovannicimolin commented Feb 28, 2019

Copy link
Copy Markdown
Contributor Author

@ormsbee Thanks for the investigation. Please, ping me if you find the root cause of the issue or if you need any help fixing this.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@xitij2000
xitij2000 deleted the giovanni/bb-728-add-problem-response-report-api-upstream branch August 6, 2020 19:47
@pcockwell

Copy link
Copy Markdown
Contributor

@ormsbee Did the investigation for this 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

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