Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport the upstreamed Bulk Enroll API #64

Merged
merged 2 commits into from
Jul 23, 2017

Conversation

bdero added 2 commits July 19, 2017 02:22
… behave with strings and lists

(cherry picked from commit 43e161f)
@bdero bdero force-pushed the bdero/backport-bulk-enroll branch from 9cbfb4c to e3e8dbe Compare July 20, 2017 22:31
@bdero
Copy link
Author

bdero commented Jul 20, 2017

Note: I removed this fix, which is not compatible with the version of DRF used in Ficus: https://github.com/edx/edx-platform/pull/15584

See this PR for details!: https://github.com/edx/edx-platform/pull/15631

CC @pomegranited

@pomegranited
Copy link
Member

@bdero Does it make sense to include both https://github.com/edx/edx-platform/pull/15584 and https://github.com/edx/edx-platform/pull/15631 in this change?

Since setting the request.POST parameters explicitly is non-standard, it seems like letting the JSON data come in as a POST parameter is a cleaner solution for all versions of django-rest-framework.

@bdero
Copy link
Author

bdero commented Jul 21, 2017

@pomegranited There was some pretty significant refactoring that happened between the version of DRF that Cloudera is using and edx:edx-platform/master. Since the DRF request POST parameters are not a property in the Cloudera version of DRF, I suspect that setting the content type in the underlying Django request won't result in the POST params being pulled from the data dict. (It's possible this behavior still exists through a dunder hook or similar, but I couldn't find anything relevant in the code)

@pomegranited
Copy link
Member

@bdero No worries, thanks for your explanation.
👍

  • I tested this on the appserver deployed from this branch.
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation - cherry-picked code, no extra docs required.

@bdero bdero merged commit 7566bac into opencraft-release/ficus.3-cloudera Jul 23, 2017
@bdero
Copy link
Author

bdero commented Jul 23, 2017

@pomegranited Thank you!

@bdero bdero deleted the bdero/backport-bulk-enroll branch July 23, 2017 23:06
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.

2 participants