Upstream the Bulk Enroll API endpoint

[bdero]

This is a simple API endpoint originally created by Appsembler (thanks!) that I'm working on upstreaming.

No enrollment logic is modified by this API, it's simply a thin wrapper for publicly interfacing with the students_update_enrollment endpoint, which is presently used for facilitating bulk enrollments in the instructor dashboard.

Rationale

Its utility comes from the following abilities not currently made available by the other enrollment-related endpoints:

1. Ability to supply multiple courses for enrollment per-request.
2. Ability to specify users for enrollment using emails (without usernames).
3. Ability to enroll non-existent users in courses before they register.
4. Ability to specify whether or not to email users informing them that they've been enrolled per-request.

These features together are important for implementing external integration systems that synchronize/push a high volume of enrollment changes to Open edX instances.
I believe that this would be a useful enough thing/common enough requirement for the community that it's worth upstreaming a solution.

What does a request to this new endpoint look like?

An example request to the endpoint looks like this:

POST /api/enrollment/v1/bulk_enroll {
  "action": "enroll",
  "auto_enroll": true,
  "email_students": true,
  "courses": "course-v1:edX+Demo+123",
  "identifiers": "brandon@opencraft.com"
}

These fields follow suit with the expected input of the student_update_enrollment view:

• action can be used to specify whether the supplied users should be enrolled or removed from the specified courses.
• auto_enroll allows choosing whether users should be pre-enrolled in courses even if they haven't registered yet.
• email_students allows choosing whether enrolled users should be sent enrollment confirmation emails.
• courses is a comma-separated list of serialized course keys.
• identifiers is a comma-separated list of emails.

Whereas a request to the common enrollment endpoint looks like this:

How does it compare to /api/enrollment/v1/enrollment?

An example request to the endpoint looks like this:

POST /api/enrollment/v1/enrollment {
  "mode": "honor",
  "course_details": {"course_id": "course-v1:edX+Demo+123"},
  "is_active": true,
  "username": "bdero"
}

• Only one course ID can be supplied per-request.
• username must be the username of an existing Django user, it's not possible to do any of the following:
  1. Pre-enroll users that aren't yet registered.
  2. Supply multiple users.
  3. Supply an emails instead of usernames.

Discussions: I'll be posting a topic on the edx-code mailing list to facilitate public discussion around this change.

Dependencies: None

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

Testing instructions:

1. Using a Client ID/Secret associated with a staff user, fetch an OAuth2 bearer token:

   # Get a token:
   curl -X POST -d "client_id=*****&client_secret=**********&grant_type=client_credentials&token_type=bearer" https://pr15006.sandbox.opencraft.hosting/oauth2/access_token/
   
   # Response:
   {"access_token": "********", "id_token": "***************************************", "expires_in": 31535999, "token_type": "Bearer", "scope": "profile openid email permissions"}
   

2. Using the token, construct an authenticated request to the bulk_enroll endpoint (replace "brandon+testenroll@opencraft.com" with an email address you have access to):

   # Post the enrollment:
   curl -X POST --header "Authorization: Bearer ***********" -H "Content-Type: application/json" -d '{"action": "enroll","auto_enroll": true,"email_students": true,"courses": "course-v1:edX+DemoX+Demo_Course","identifiers": "brandon+testenroll@opencraft.com"}' 
   https://pr15006.sandbox.opencraft.hosting/bulk_enroll/v1/bulk_enroll
   
   # Response:
   {"detail":"Authentication credentials were not provided."}
   

Author notes and concerns:

1. After this has general approval, I need to rewrite the tests from the original implementation in a way that will properly integrate with the enrollment tests.

Reviewers

• @mtyaka
• edX reviewer[s] TBD

Settings

EDXAPP_FEATURES:
  ENABLE_COMBINED_LOGIN_REGISTRATION: true
  ENABLE_BULK_ENROLLMENT_VIEW: true

[openedx-webhooks]

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]

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

[gsong]

@scottrish Should someone on the educator team take a quick look to make sure @bdero is on the right path prior to him writing tests?

[bdero]

@gsong @scottrish In your opinion, do you think I should go ahead and work on this? I haven't heard anything back from the post on the mailing list or this ticket so far.

[bradenmacdonald]

@gsong Same here - would it be possible to get an ETA for a preliminary product or technical review?

[gsong]

@edx/educator-product Can this be scheduled into a sprint for review. Let me know if the educator team isn't the right team to review this enhancement.

[mattdrayer]

Hey there, @bdero -- I took a spin through this PR. I know it's on the smaller side, but after reviewing I have some concerns I wanted to raise:

1. Generally speaking it would be better to use OAuth2/JWT here versus an API key. IIRC we are trying to move away from API key due to several factors, such as only supporting one key per installation, API keys are not linked to users, etc.

2. The APIKeyHeaderPermissionIsAuthenticated class allows any client with an API key OR any client with valid user authentication/session to interact with the protected view. While this might be okay for the other use of the class (viewing a user's enrollment state) I'm not sure this is ideal for something like a bulk enroll/unenroll API endpoint.

3. There should be a feature flag to control the inclusion of this endpoint in the system's API footprint. I can't recall if there's an overall flag for the bulk enrollment feature, but even with the feature enabled, system operators should not be exposing these workflow to API clients without being aware that they are doing so.

Let me know what you think about this stuff when you have a moment -- thanks!

[bdero]

@mattdrayer Thanks for taking a look!

Dropping the API key and adding JwtAuthentication to the authentication options is cool with me. And 👍 for the feature flag - I will be sure to add it in when finishing up.

As for the permissions, do you think just doing a superuser or global staff check is sufficient in this case, or should I opt for something more sophisticated? The only use case I can see for this endpoint is external service integration via a special service user. OAuth scoping is another possibility, for example, but this doesn't appear to be implemented in other enrollment-related views (from what I've seen).

[mattdrayer]

Cool! Re: permissions -- I think the minimum bar should be a superuser/staff check, yeah. You are correct in that OAuth scopes is not currently available in the system. If we're really talking about supporting an integration case involving a single known user, we could consider storing the username as a configuration setting and performing a check against that value.

[bdero]

@mattdrayer Alright thanks! I've rebased and made these changes to the branch with just the global staff check for now. Let me know if you think it seems good, and then I'll toss in the tests

[mattdrayer]

MUST: Plumb this through aws.py in order to add support for the environment vars.

[bdero]

Since feature flags are pulled in from the configuration as the FEATURES dict, I don't think they usually need anything added besides a default in common.py unless they trigger some additional config action, like including a djangoapp. I can open a pull request to the configuration repo and plumb it through there once this is merged, though.

[mattdrayer]

Ah, right -- maybe all features are already being imported from the environment in one fell swoop

[mattdrayer]

MUST: Plumb this through the configuration helper in order to provide multi-site support

[bdero]

Done! I followed this pattern

[mattdrayer]

Is OAuth2AuthenticationAllowInactiveUser required here? I'm not sure it's a good idea to allow inactive user accounts to interact with this workflow, assuming that's what this particular permission class enables.

[bdero]

From the docstring of OAuth2AuthenticationAllowInactiveUser, it seems like there's an enrollment use case where authentication should be possible from a user whether their email is verified or not. This doesn't seem to matter very much for this endpoint, since the permission gate will only allow global staff anyways. I've swapped this out with OAuth2Authentication instead.

[mattdrayer]

If we're only wanting to make a staff check, we should use (or create) that permission class specifically. This permission class makes it possible to specify a user in the URL as a query parameter -- not sure we want to allow for that in this scenario.

[mattdrayer]

What about using IsStaffOrOwner -- would be nice if we just had an IsStaff permission class, though.

[mattdrayer]

Actually that class also considers usernames in the URL...hrm...still seems like that opens the door to trouble... 😄

[bdero]

Yeah you're right, everything either checks for the user in the URL or does object-based permission. We don't want either of those, so I just went ahead and added a very vanilla "IsStaff" one. Let me know what you think!

[jbarciauskas]

@mattdrayer can you clarify which comments are blockers and which are suggestions?

[mattdrayer]

I added "MUST" flags to two of my comments and altered the friendly-sounding tone 😉 The other aspects are still under discussion -- need more info before we can decide what to do.

[jbarciauskas]

Rgr, thanks!

[mattdrayer]

This is looking pretty good to me now -- I don't have anything else to point out so I'm giving a 👍 assuming the checks are all green -- let's have someone else take a pass through as well ( @bradenmacdonald? @douglashall? others?)

[brittneyexline]

Hey @bdero, I've looked through and it generally looks good, besides one docstring update. I am wondering if it would be good to suggest or enforce a limit to how many users/courses at once the endpoint should be called with? I have seen this with other api endpoints meant for bulk operations in other contexts.

[brittneyexline]

nit: this should probably be changed to something like "Enroll multiple users in one or more courses."

[brittneyexline]

@bdero, I agree with holding off with enforcing limits - it's something we can keep in mind for later.
With regards to the test failures, I have no idea why this is happening, so I kicked off another python jenkins run to see if it's reproducible. If it is, I'll see if I can get someone in cambridge to help diagnose.

[bdero]

Thanks @brittneyexline !

[jbarciauskas]

Looks like we need some help here? @brittneyexline @bdero ? cc @edx/testeng

[brittneyexline]

Hi @bdero, I got the following recommendation from @estute that we should try:

[3:15 PM] Stuart Young: the setting is definied by lms/env/common.py, but it looks like the failing test class is being run on the cms shard.
[3:16 PM] Stuart Young: I would recommend decorating it with:
@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')

[bdero]

@brittneyexline @estute Oh I see the problem. bulk_enroll relies on LMS-specific stuff, so I can't put it in common. I've just moved it into it's own LMS djangoapp instead.

[bdero]

jenkins run bokchoy

[bdero]

jenkins run lettuce

[bdero]

jenkins run bokchoy

[brittneyexline]

Hi @bdero, thanks for updating - I think the separate app is actually a nice improvement. I did notice that there are no testing instructions or sandbox or unit tests - I'd feel more comfortable merging this if you could update some of that information.

[bdero]

@brittneyexline Thanks, I've pushed the reworked tests and the sandbox is spinning up; I'll write the testing instructions when I start on the next day's work. :)

[brittneyexline]

one test that i was expecting to see given this enrolled student was trying to enroll an already enrolled student. it would also be nice to see a test around unenrolling a student.

[gsong]

@brittneyexline Since BulkEnrollView.post wraps instructor.views.api.students_update_enrollment, I would expect the unit test for students_update_enrollment to cover the case of enrolling an already enrolled student.

Also, BulkEnrollView doesn't deal with unenrollments, right?

Since BulkEnrollView just passes through errors, do you feel strongly that it needs to have tests for upstream exceptions?

[brittneyexline]

I don't feel strongly, I was just pointing it out, since we are testing some of the functionality that is presumably already tested.

I double checked, and this endpoint does support unenroll if you pass it as an action, since this new endpoint wraps students_update_enrollment which supports unenrolling.

[bdero]

I've added a test for unenrolling

[bdero]

@brittneyexline I've been trying to get the sandbox running today - apologies that it's not around yet. There just happens to be a lot of things going wrong with my builds today. For example, the last build failed due to this insights PR getting merged while my build was running: https://github.com/edx/edx-analytics-dashboard/pull/701

I've cobbled together some simple testing instructions as well for when the sandbox is built.

[bdero]

@gsong @brittneyexline Was this tested by upstream before merging? :O

I'm unfortunately still not completely through with the sandbox issues, I think I'm seeing failures due to something with this PR after last build: https://github.com/edx/configuration/pull/3952, as "create some test users" is failing due to invalid args. Not 100% sure if related yet though.

[gsong]

@bdero What's the nature of the failures you're seeing? Is it something that wouldn't be caught by the tests?

[bdero]

@gsong The failures I'm mentioning are with sandbox building, not this API

[bdero]

@gsong @brittneyexline Gah, I found a bug - follow up PR is here: https://github.com/edx/edx-platform/pull/15579

[edx-pipeline-bot]

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]

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