Skip to content

Patch the bulk_enroll request to use a form content type - #15631

Merged
brittneyexline merged 1 commit into
openedx:masterfrom
open-craft:bdero/bulk-enroll-content-type
Jul 21, 2017
Merged

Patch the bulk_enroll request to use a form content type#15631
brittneyexline merged 1 commit into
openedx:masterfrom
open-craft:bdero/bulk-enroll-content-type

Conversation

@bdero

@bdero bdero commented Jul 20, 2017

Copy link
Copy Markdown
Contributor

The plot thickens with with yet another follow-up fix (related to https://github.com/edx/edx-platform/pull/15584). :) It turns out that the line removed there was necessary for older versions of DRF. I neglected to notice that buried in the response that I received back during testing was actually an error from the downstream student_update_enrollment view. Because that view directly pulls the parameters out of POST data, certain conditions need to be met in order for DRF to pull POST data from the "_data" dict.

From my inline comment:

Setting the content type to be form data makes Django Rest Framework v3.6.3 treat all passed JSON data as
POST parameters. This is necessary because this request is forwarded on to the student_update_enrollment
view, which requires all of the parameters to be passed in via POST parameters.

JIRA tickets: OSPR-1835

Dependencies: None

Sandbox URL: TBD - sandbox is being provisioned.

Testing instructions:

Follow the exact same testing instructions that are listed in #15579, except be sure to double-check that the response does NOT contain an error field.

Reviewers

Settings

EDXAPP_FEATURES:
  ENABLE_COMBINED_LOGIN_REGISTRATION: true
  ENABLE_BULK_ENROLLMENT_VIEW: true

Setting the content type to be form data makes Django Rest Framework v3.6.3 treat all passed JSON data as
POST parameters. This is necessary because this request is forwarded on to the student_update_enrollment
view, which requires all of the parameters to be passed in via POST parameters.
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @bdero! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

Create an OSPR issue for this pull request.

@openedx-webhooks

Copy link
Copy Markdown

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

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

@bdero 👍

  • I tested this on my devstack and the sandbox using the test instructions and feature settings on https://github.com/edx/edx-platform/pull/15579, and verified the absence of the error key in the responses.
  • I read through the code
  • I checked for accessibility issues Affects API calls, not GUI
  • Includes documentation - inline docs clearly explain the issue.

# POST parameters. This is necessary because this request is forwarded on to the student_update_enrollment
# view, which requires all of the parameters to be passed in via POST parameters.
metadata = request._request.META # pylint: disable=protected-access
metadata['CONTENT_TYPE'] = 'application/x-www-form-urlencoded'

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 see - so that was the origin of the previous line which modified the request. 👍 for the comment.

@brittneyexline
brittneyexline merged commit f86f599 into openedx:master Jul 21, 2017
@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 Monday, July 24, 2017.

@bdero

bdero commented Jul 21, 2017

Copy link
Copy Markdown
Contributor Author

@pomegranited @brittneyexline @doctoryes Thank you!

@bdero
bdero deleted the bdero/bulk-enroll-content-type branch July 21, 2017 17:10
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been rolled back from the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants