Skip to content

Remove request.POST save - incompatible with DRF v3.6.3. - #15584

Merged
doctoryes merged 1 commit into
masterfrom
jeskew/fix_drf_upgrade_incompat
Jul 17, 2017
Merged

Remove request.POST save - incompatible with DRF v3.6.3.#15584
doctoryes merged 1 commit into
masterfrom
jeskew/fix_drf_upgrade_incompat

Conversation

@doctoryes

Copy link
Copy Markdown
Contributor

DRF v3.6.3 doesn't allow request.POST to be changed for DRF Requests. This change makes the Python unittests pass again.

@bdero Was there a reason for this statement? The request isn't accessed further in this method.

@doctoryes

Copy link
Copy Markdown
Contributor Author

As you can see by the code below, modifying request.POST for a rest_framework.request.Request is non-standard and requires a workaround:
https://github.com/edx/edx-platform/blob/master/openedx/core/djangoapps/user_api/helpers.py#L411-L417
We should avoid doing so if possible.

@doctoryes
doctoryes requested a review from haikuginger July 17, 2017 17:18
@doctoryes

Copy link
Copy Markdown
Contributor Author

The request.POST attribute changed to a @property between releases 3.2.4 and 3.3.1 - here's the PR: encode/django-rest-framework#3592

def post(self, request):
serializer = BulkEnrollmentSerializer(data=request.data)
if serializer.is_valid():
request.POST = request.data

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 that self.request gets passed in to students_update_enrollment below, and that function accesses request.POST. however, i don't understand if self.request and request are the same object?

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.

It appears they are:

ipdb> id(request)
140610886734992
ipdb> id(self.request)
140610886734992

but it also appears that request.POST is the same as request.data:

ipdb> p request.POST
<QueryDict: {u'action': [u'enroll'], u'courses': [u'org.1/course_1/Run_1'], u'identifiers': [u'robot+test+5@edx.org'], u'email_students': [u'False']}>
ipdb> p request.data
<QueryDict: {u'action': [u'enroll'], u'courses': [u'org.1/course_1/Run_1'], u'identifiers': [u'robot+test+5@edx.org'], u'email_students': [u'False']}>
ipdb> id(request.POST)
140610886285712
ipdb> id(request.data)
140610886285712

So I'm unsure if there's a reason why this line exists.

@bdero

bdero commented Jul 17, 2017

Copy link
Copy Markdown
Contributor

@doctoryes I'm currently verifying this change

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

thanks for the clarification on that @doctoryes. approving this, if something comes up from @bdero's verification, i suggest addressing those issues in https://github.com/edx/edx-platform/pull/15579.

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

It's unclear to me why this was even done in the first place.

@bdero

bdero commented Jul 17, 2017

Copy link
Copy Markdown
Contributor

@doctoryes @brittneyexline I've tested this carefully, although it's difficult to test some aspects completely with this bug still present. 👍 I don't see a reason why this should be necessary as well.

@macdiesel This feature was initially built by another dev group and I didn't notice this line during the initial rebasing. It's possible that this was needed for a prior DRF version as the blame says it was introduced in a commit that fixes the feature for a version range of DRF.

@doctoryes
doctoryes merged commit 0365712 into master Jul 17, 2017
@doctoryes
doctoryes deleted the jeskew/fix_drf_upgrade_incompat branch July 17, 2017 17:54
@doctoryes

Copy link
Copy Markdown
Contributor Author

Thanks for the prompt review, @bdero .

@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 Tuesday, July 18, 2017.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants