Add feature to send an e-mail to staff when a student enrolls

[clemente]

This sends an e-mail to a specified e-mail address each time a student enrolls to a course.

• a new lms.env.json variable, ENROLLMENT_NOTIFICATION_EMAIL, contains the destination e-mail. Leave it to blank to disable the feature
• a SiteConfiguration variable of the same name and function can be used to enable/disable the feature from admin. If present it will have precedence over the lms.env.json one
• this feature is disabled by default

JIRA tickets: None

Discussions: None

Dependencies: None
Documentation PR not required because this doesn't need a feature flag (FEATURES in lms.env.json), but a normal lms.env.json variable.

Screenshots:

Sandbox URL: TBD - sandbox is being provisioned.

Partner information: None

Deployment targets: N/A

Merge deadline: None

Testing instructions:

1. In lms.env.json, set ENROLLMENT_NOTIFICATION_EMAIL to your e-mail.
2. Enroll to a course
3. The specified address should receive an e-mail informing of which student enrolled into which course
4. Try to unenroll, no e-mail expected
5. In admin, add a SiteConfiguration object with { "ENROLLMENT_NOTIFICATION_EMAIL":"setfromsiteconf@example.com" }
6. Restart LMS or try the next steps many times, because SiteConfiguration changes sometimes take a short time to be updated
7. Try again the enrollment. You should see the e-mail sent to the new address
8. Try to use '' in SiteConfiguration to disable the feature, while it's enabled in lms.env.json. Retry, it should disable the feature
9. Remove both SiteConfiguration and the lms.env.json setting and retry the enrollment, check that by default it doesn't send e-mail.

Author notes and concerns:

The SiteConfiguration variable allows to do this change from admin without restarting, whereas the lms.env.json one is there because many other e-mail addresses are defined there.

Reviewers

• @tomaszgy
• @UmanShahzad
• @iloveagent57

Settings

[openedx-webhooks]

Thanks for the pull request, @clemente! 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.

[tomaszgy]

I've left few small comments - please have a look if they make sense to you :)

[tomaszgy]

When get_full_name() returns empty string it gives a bit strange effect, e.g. tomasz (), so maybe brackets could be conditional?

[tomaszgy]

Maybe: by setting the e-mail of the staff in?

[tomaszgy]

Maybe: Please specify the destination e-mail address.?

[tomaszgy]

Shouldn't this section be ordered alphabetically?

[tomaszgy]

👍 after comments are addressed (those that @clemente agrees with)

• I tested this:
  I followed the testing instructions from @clemente. All worked as expected.
• I read through the code and left few comments.
• I checked for accessibility issues
• Includes documentation

[UmanShahzad]

Nit: Does this really warrant a separate comment? This pattern is used throughout the codebase a lot.

[UmanShahzad]

Nit: can we use django.core.mail.message.EmailMessage (example: https://github.com/edx/edx-platform/pull/16624/files#diff-5a31e91b78b2599dc5ace9c2a3b31d8cR776)? It may allow a little more flexibility later.

[UmanShahzad]

I think we can just use smtplib.SMTPException.

[UmanShahzad]

Nit: log -> LOGGER.

[UmanShahzad]

Question: I haven't actually looked, but is this the best way to generate these URLs? Wondering if there's any prebuilt function for this as I'd imagine this is a relatively common thing to do.

[clemente]

The Django ways to get a URL are in https://stackoverflow.com/questions/2345708/how-can-i-get-the-full-absolute-url-with-domain-in-django, but in this case we don't have a {{request}} object and the offered solutions are also several lines long.
There might be an openedx way. But I didn't find a {{get_absolute_url}} in CourseLocator, nor a method in CourseMode. The methods that return a path return relative URLs. Grepping for course∩absolute∩url didn't show any code for this.

[UmanShahzad]

Any particular reason this isn't in setUp?

[UmanShahzad]

We should capitalize learner ('Learner').

[UmanShahzad]

We should capitalize course ('Course').

[UmanShahzad]

Not sure of the UX involved here with a raw course URL being pasted in. Depending on that, this may have to have an HTML alternative where 'Course' is clickable or something.

[clemente]

I have sent me a test e-mail in text/plain and verified that e.g. gmail makes URLs clickable. As long as we don't need design, text/plain is fine for URLs.
I also don't know the UX requirements, maybe this needs a logo, header, footer etc. I expect the client or edx to come back with changes to this template.

[UmanShahzad]

See above about the raw URL.

[clemente]

@tomaszgy @UmanShahzad Thanks for the comments and the testing. I applied all your comments except the 2 where I replied.

[pomegranited]

All text needs to be translated, e.g. https://github.com/edx/edx-platform/blob/master/common/templates/emails/sync_learner_profile_data_email_change_subject.txt

[UmanShahzad]

👍 And note that if you do this, you'll have to strip newlines. See https://github.com/edx/edx-platform/blob/master/common/djangoapps/third_party_auth/pipeline.py#L778-L782

[pomegranited]

Same here: text should be translated.

[pomegranited]

I signed up using the client's themed registration form, with matching registration form config, and this offers only the "Full Name" UserProfile field. So unfortunately, the Django User.get_full_name() method never returns anything.

So I think you need to do something like this instead.

[pomegranited]

@clemente I hate to be fussy about this, but I think upstream will be too, so it's better to sort it out now.

When translating a line of text, you need to include all of the pieces so they can be translated all together. Some languages may switch the order around completely, or change punctuation, or anything.

So for example, this file should be:

${_("Learner: {username} ({full_name})").format(
    username=user.username,
    full_name=user_full_name,
)}
${_("Course: {course_url}").format(course_url=course_url)}

${_("More learner data in admin: {user_admin_url}").format(user_admin_url=user_admin_url}

See e.g. https://github.com/edx/edx-platform/blob/6d2468d0dcf782a7561ecc61946146d6fae2762f/lms/templates/sysadmin_dashboard_gitlogs.html#L133
https://github.com/edx/edx-platform/blob/c0c740ad1d98b3b40fb3ce91620ec7faa7460b93/lms/templates/dashboard.html#L231

[UmanShahzad]

Nit: it could be useful to factor out this context and send it into both subject and message so that some overriding theme could potentially use some detail (like user's full name) in the subject.

[UmanShahzad]

Nit: this could be 1 line:

user_full_name = user.profile.name if hasattr(user, 'profile') else None

[UmanShahzad]

Nit: now that we're using pytest, we can do assert len(mail.outbox) == 1 and get all the benefits of self.assertEqual. Same goes for the others, but up to you to change it.

[UmanShahzad]

Thanks @clemente! I have just some nits + what @pomegranited said, then we should be good. Note I have not had time to manually test this, though; mostly just doing a code review.

[clemente]

@UmanShahzad @pomegranited I fixed your comments.

[openedx-webhooks]

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

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

[pomegranited]

@clemente 👍

• I tested this on my master devstack using the instructions provided, and it worked as expected.
• I read through the code
• I checked for accessibility issues N/A
• Includes documentation - docstrings are sufficient.

[nedbat]

@edx/learner-product I think this should get an OK from product first.

[marcotuts]

I've pinged @sstack22 on this, and will update the ticket to be associated with the educator team. Thanks for flagging this Ned.

[clemente]

Squashed commits and rebased on master.

[nedbat]

@sstack22 Any thoughts on this?

[sstack22]

Thanks for the ping @nedbat. @clemente - this isn't something we'd want enabled for edX. I would recommend considering a daily email in general though, as individual emails may be overwhelming.

[bradenmacdonald]

@sstack22 Good point about the digest. In this case, I believe the use case we built this for would be sending the email to salesforce or some other automated CRM workflow, so a digest would be less useful in that case. Certainly a good option to add in the future though, and I can see that making this much more widely useful.

[nedbat]

Should this PR be closed then?

[clemente]

@iloveagent57 Thanks, fixed. I also rebased on top of master.

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[openedx-webhooks]

@clemente 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

[iloveagent57]

@clemente Thank you for your patience. I've talked with @nedbat and others here about ways that we can make this type of thing better in the future, mostly by allowing you (or any other community contributor) to extend the functionality of edx-platform without having to write a PR against it (and wait months and months for a review/approval). For this type of PR, where you're adding a downstream signal receiver, I think there are two approaches that would work:

1. You guys create a catch-all edx extensions library using the Django App Plugin functionality. It would define a set of signals to listen for, do whatever needs to be done based on those signals, and install this library into whatever installation relies on this extended functionality.
2. You/we/someone adds another endpoint to the enrollments API like /api/enrollments/v1/course-enrollments that responds with a paginated set of users enrolled in a particular course, or optionally all courses, and optionally ordered by descending enrollment update time. This would let you run a cron via jenkins or whatever you want to send emails to whichever course staff wants an enrollment notification email.

What do you think of these approaches? I can talk to some folks here and see if there's a general preference between these alternatives.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Friday, August 03, 2018.

[edx-pipeline-bot]

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/STAGE_edxapp_M-D

[nasthagiri]

@iloveagent57 Option #2 is better implemented as a web-hook - where an external handler registers itself for notifications and gets called whenever an enrollment happens.

[edx-pipeline-bot]

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

[edx-pipeline-bot]

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

[nasthagiri]

To provide some context, we are adopting the Open-Closed principle for edx-platform as presented (slides) at the Open edX conference. So new features that are not "core" to the platform should be added as extensions or plugins for everyone to use.

Given that, to help us follow through on this PR, let me summarize the implementation options that we have discussed thus far.

1. Django App Plugin, in a separate repo, running in the LMS process

As also recommended on another recent PR, you can use the Django App Plugin design pattern for this feature. That design pattern will allow you to put this new feature in its own repo - without needing to contribute it back to our core platform.

For a reference implementation, you can take a look at how the Completion Django app is defined in its own repo and uses the Plugin pattern to insert itself in the LMS.

If you choose this option, you can put your new repo in the friendsofopenedx github organization. This is a new github organization that the open source community plans to use to share plugins with each other.

2. Real-time events using xAPI or Caliper

There is an upcoming feature to send real-time events to interested parties using standard protocols such as xAPI and Caliper. The design proposal is currently under review.

Once that feature is in place, you can integrate events, such as for enrollments, directly with external services.

3. Real-time notification with a Web Hook

Another alternative is to introduce a web hook within the edx-platform. There would be a new REST endpoint where interested listeners would register callback URLs for enrollment events. This would be a generic integration point that we could add to the core for this and future extensions.

4. Poll for new enrollments via a new Enrollment API

As @iloveagent57 suggests above, another option would be to add a REST API in the core where interested parties can get a list of all enrollments, ordered by date. This wouldn't be a real-time notification, but would be a generically useful integration point for various types of extensions.

I apologize for not presenting these options earlier. But given that we are now working on this feature, we can now focus on implementing it for its long-term benefit for open edX users.

[schenedx]

@clemente and @pomegranited, given the desired directions detailed above by @nasthagiri, please let us know which direction you will take, and the timeline for when the steps will be taken.
If we don't see a response from opencraft by next Wednesday, I will proceed to revert the changes in this PR from edx-platform code base.
I apologize for the timeline and prescribed action. We believe this is the most effective way to get right feature to be integrated into edx-platform.
If you have strong concerns with our approach, please let us know as soon as you can.

[pomegranited]

@schenedx I don't understand why this PR needs to be reverted. This PR was submitted almost a full year ago, and it has been merged, and deployed to production without issue, correct?

For our future PRs, we will certainly use the django-plugin approach, and we're pleased to see it in operation.

But for this change, we are out of client budget, and cannot take more time to reimplement it. So yes, we have strong concerns about reverting this now.

CC @clemente @bradenmacdonald @nasthagiri @nedbat

[pomegranited]

My apologies @schenedx -- I was surprised to see the word "revert" used here after such a long review process! Have had a chat with @nasthagiri , and we'll get back to you soon about what next steps our client budget can support, if any.

[schenedx]

@pomegranited Your surprise is understandable. I will connect again on Wednesday to check if you and opencraft made any decisions.

[clemente]

@schenedx We weren't up to date about the latest requirements, sorry. We'll accept if this is unmerged in the near future, so you can proceed with the revert now.

For the long-term future, we're still getting to a conclusion (to be shared tomorrow).

Notifying instructors that enrollments happened can still be a useful feature. Could I suggest as another option, to implement this feature as part of [notifier|https://github.com/edx/notifier/], which currently sends digest e-mails of forum posts?

[clemente]

@schenedx And the decision is… we're working on it, we still need to experiment a bit more to find a long-term solution. We'd prefer not to use Django plugin because they depend on the internals.
For now, reverting this PR is ok.

Obviously we'd want to upstream features that may useful to everyone, even if it's new APIs to do these extensions.

Also see comments about implementing it as part of notifier.

[schenedx]

@clemente Sounds good. Here is the revert PR at https://github.com/edx/edx-platform/pull/18762. Will merge that when test passes

[clemente]

@nasthagiri
We started testing out how this development could be reimplemented within the new direction of using Django plugins to extend the platform. We started a new plugin that detects enrollments via a signal, same as what was done in this PR.

However, plugins have the same risk of code drift, because they use and depend on internal features that may change. Therefore we want to use only public APIs in our plugins, and work on contributing these public APIs to Open edX if they aren't there.

Our plugin now depends on two other systems (maybe more, correct me if I'm wrong):

1. we depend on a signal, ENROLL_STATUS_CHANGE, to detect the enrollment. I consider signals as a form of API. But we're not sure that a signal can be considered a public API just because of being a signal (in that case, all signals would be public APIs). But we don't know how else we can differentiate public vs. internals APIs. A decorator? A different package name? Some comments guaranteeing that it's fine to use these from plugins and that they won't change?
2. we depend on edx-ace to send the e-mails. I guess this is closer to being a stable API, but how can we make it explicit? Should it say that the interface is stable? Should it be included in a central whitelist of stable interfaces?

We would like to find out the best way to proceed here during the transition to the new system, and how could we help.
After the direction is clear we'll finish the plugin (adding tests, better templates and a new setting to hold the destination e-mail).
Thanks.

CC @pomegranited

[clemente]

@nasthagiri This isn't urgent at all, but it would be interesting to know how our approach follows edX's guidelines (see previous message); thanks.

[clemente]

@nasthagiri Another ping to see whether we can discuss the public APIs mentioned in the previous message, or know about the latest guidelines or about this particular case (Django signals for enrollment). Thanks.

[clemente]

@nasthagiri About the APIs: our window to develop the APIs for this feature has now closed, since the client project is over. That means we won't be reimplementing this feature (enrollment e-mails).
We still have many other cases where we'll need to agree on public APIs to create plugins, but we'll all need to be more reactive in order to define the APIs quicker, to avoid more contributions stalling in the same way. Thanks.
CC @antoviaque @pomegranited