Skip to content

Fix the BulkEnrollSerializer courses field to internally behave with strings and lists - #15579

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

Fix the BulkEnrollSerializer courses field to internally behave with strings and lists#15579
brittneyexline merged 1 commit into
openedx:masterfrom
open-craft:bdero/bulk-enroll

Conversation

@bdero

@bdero bdero commented Jul 14, 2017

Copy link
Copy Markdown
Contributor

Follow up to https://github.com/edx/edx-platform/pull/15006: Internally, DRF may pass data into the value conversion method of ListFields to_internal_value as a list instead of a string in some cases, so StringListField in the Bulk Enroll API needs to work with both. This change adds this support and tests.

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

Test setup instructions:

  1. Add a new OAuth2 client:
    1. For the "User", set it to the staff user (likely the user ID is 5 if on a devstack)
    2. For the "Url", set it to anything: http://example.com/
    3. For the "Redirect uri", set it to anything: http://example.com/callback
    4. Copy the "Client id" and "Client secret" for use when constructing requests
    5. For "Client type", select "Confidential (Web applications)"
    6. Click "Save"

Testing instructions:
Replace the "client_id" and "client_secret" with the respective keys copied suring the test setup instructions, and run the following to obtain an access token:

# Get a token:
curl -X POST -d "client_id=6f663d9f3425fade127c&client_secret=8a3a91e83f61fd54e9f7c074acfed909cbcc196e&grant_type=client_credentials&token_type=bearer" http://localhost:8000/oauth2/access_token/

# Example Response:
{"access_token": "773b8ef9fb8747e62360b0215f133c2793ed267a", "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJmYW1pbHlfbmFtZSI6IkFkYW1hIiwiYWRtaW5pc3RyYXRvciI6dHJ1ZSwic3ViIjoiNjZjNmQxNzE4MTZlZjgxZGM4ODg1OWYwZTg3YWU3MzIiLCJpc3MiOiJodHRwOi8vMTI3LjAuMC4xOjgwMDAvb2F1dGgyIiwidXNlcl90cmFja2luZ19pZCI6NSwicHJlZmVycmVkX3VzZXJuYW1lIjoic3RhZmYiLCJuYW1lIjoic3RhZmYiLCJsb2NhbGUiOiJlbiIsImdpdmVuX25hbWUiOiJCaWxsIiwiZXhwIjoxNTAwMDczNDQwLCJpYXQiOjE1MDAwNjk4NDAsImVtYWlsIjoic3RhZmZAZXhhbXBsZS5jb20iLCJhdWQiOiI2ZjY2M2Q5ZjM0MjVmYWRlMTI3YyJ9.xO187GzdbvFqDuju_URXWewX7Dn-foQH60srrR1jHa0", "expires_in": 31535999, "token_type": "Bearer", "scope": "profile openid email permissions"}

Using the access token in the response from the previous step, construct an authenticated request to the bulk_enroll endpoint (replace "course-v1:asdf+asdf+asdf" with a course on your devstack):

# Post the enrollment:
curl -X POST --header "Authorization: Bearer 773b8ef9fb8747e62360b0215f133c2793ed267a" -H "Content-Type: application/json" -d '{"action": "enroll","auto_enroll": true,"email_students": true,"courses": "course-v1:asdf+asdf+asdf","identifiers": "staff@example.com"}' http://localhost:8000/api/bulk_enroll/v1/bulk_enroll

# Example Response:
{"action":"enroll","courses":{"course-v1:asdf+asdf+asdf":{"action":"enroll","results":[{"identifier":"staff@example.com","after":{"enrollment":true,"allowed":false,"user":true,"auto_enroll":false},"before":{"enrollment":true,"allowed":false,"user":true,"auto_enroll":false}}],"auto_enroll":true}},"email_students":true,"auto_enroll":true}

Settings

EDXAPP_FEATURES:
  ENABLE_COMBINED_LOGIN_REGISTRATION: true
  ENABLE_BULK_ENROLLMENT_VIEW: true

@bdero
bdero requested a review from brittneyexline July 14, 2017 23:38
@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-1823 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 Jul 14, 2017
@bdero

bdero commented Jul 15, 2017

Copy link
Copy Markdown
Contributor Author

jenkins run a11y

@brittneyexline

Copy link
Copy Markdown
Contributor

I still haven't done testing on this - I'll deploy it to the business sandbox so we can get that going.

@brittneyexline

Copy link
Copy Markdown
Contributor

I've tested this on the business sandbox, so this is good to go.

@brittneyexline
brittneyexline merged commit 583719c into openedx:master Jul 18, 2017
@bdero

bdero commented Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

@brittneyexline Hah, just when I got the OC sandbox build to complete too. 😛

Thanks!

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

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@bradenmacdonald
bradenmacdonald deleted the bdero/bulk-enroll branch July 19, 2017 16:02
@bdero
bdero restored the bdero/bulk-enroll branch July 20, 2017 22:00
@bdero
bdero deleted the bdero/bulk-enroll branch July 20, 2017 22:00
@pomegranited

Copy link
Copy Markdown
Contributor

@bdero Could we get this and https://github.com/edx/edx-platform/pull/15006 into Ginkgo?

@bdero

bdero commented Jul 28, 2017

Copy link
Copy Markdown
Contributor Author

@pomegranited I think that would be an excellent thing - should I make a PR against the Ginko RC to do this?

@pomegranited

Copy link
Copy Markdown
Contributor

@bdero Yep, if you could submit a PR against open-release/ginkgo.master, ping @nedbat and @gsong, and add it to the Confluence page under Things to Cherry Pick into GInkgo, that should about cover it 😄

Thank you!

@pomegranited

Copy link
Copy Markdown
Contributor

@bdero Sorry, I should take this back.. @bradenmacdonald reminded me that we're concentrating on bug fixes not new features for the ginkgo rc right now. Apologies for the noise!

@bdero

bdero commented Jul 29, 2017

Copy link
Copy Markdown
Contributor Author

@pomegranited Ok, thanks for letting me know :)

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants