feat: welcome emails for xPRO Learners #3017
Conversation
mudassir-hafeez
force-pushed
the
mudassir/Welcome-emails-for-xpro-learners
branch
from
cf98a55 to
f26a2e3
Compare
mudassir-hafeez
force-pushed
the
mudassir/Welcome-emails-for-xpro-learners
branch
from
d8e766b to
29b61aa
Compare
| Dear | ||
| {{ user.name }}, | ||
| </p> | ||
| <p style="margin: 0 0 10px">Welcome to MIT xPRO!</p> |
There was a problem hiding this comment.
According to the document provided, there are spaces before this line
| <p style="margin: 0 0 10px">Welcome to MIT xPRO!</p> | |
| <p style="margin: 0 70 10px">Welcome to MIT xPRO!</p> |
There was a problem hiding this comment.
I have set the left margin to 50 for this line as it closely matches what is in the document.
| <p style="margin: 0 0 10px">Welcome to MIT xPRO!</p> | ||
| <p style="margin: 0 0 10px"> | ||
| We're thrilled to have you on board for our upcoming course, | ||
| <strong>{{ enrollment.run }}</strong>. This is a significant step |
There was a problem hiding this comment.
Any specific reason, you used run instead of course?
| <strong>{{ enrollment.run }}</strong>. This is a significant step | |
| <strong>{{ enrollment.run.course }}</strong>. This is a significant step |
There was a problem hiding this comment.
I used enrollment.run here and in all other instances you mentioned because it shows the course name along with the course run that makes it more descriptive. We've used a similar approach in other templates, which might be the reason. However, if it only needs to be the course name, we can change it accordingly.
| </p> | ||
| <ul style="margin: 0 0 10px; padding-left: 20px"> | ||
| <li> | ||
| Course Name: <strong>{{ enrollment.run }}</strong> |
There was a problem hiding this comment.
| Course Name: <strong>{{ enrollment.run }}</strong> | |
| Course Name: <strong>{{ enrollment.run.course }}</strong> |
| Upon successfully completing the course, you'll receive a digital | ||
| Professional Certificate from MIT xPRO, validating your | ||
| accomplishment and expertise in | ||
| <strong>{{ enrollment.run }}</strong>. |
There was a problem hiding this comment.
| <strong>{{ enrollment.run }}</strong>. | |
| <strong>{{ enrollment.run.course }}</strong>. |
| <p style="margin: 0 0 10px">Best of luck with your studies,</p> | ||
| <p style="margin: 0 0 10px">MIT xPRO Team</p> | ||
| <p style="margin: 0 0 10px"> | ||
| P.S. Don't forget to check out the "Getting Started" section in your |
There was a problem hiding this comment.
| P.S. Don't forget to check out the "Getting Started" section in your | |
| <strong>P.S.</strong> Don't forget to check out the "Getting Started" section in your |
ecommerce/mail_api.py
Outdated
| run_start_date = run_end_date = "" | ||
| run_start_time = "" | ||
| if enrollment.run.start_date: | ||
| run_start_date = enrollment.run.start_date | ||
| run_start_time = run_start_date.astimezone(datetime.timezone.utc).strftime( | ||
| EMAIL_TIME_FORMAT | ||
| ) | ||
| if enrollment.run.end_date: | ||
| run_end_date = enrollment.run.end_date | ||
| if run_start_date and run_end_date: | ||
| date_range = ( | ||
| f"{run_start_date.strftime(EMAIL_DATE_FORMAT)} - " | ||
| f"{run_end_date.strftime(EMAIL_DATE_FORMAT)}" | ||
| ) | ||
| else: | ||
| date_range = "" | ||
| try: | ||
| user = enrollment.user | ||
| api.send_message( | ||
| api.message_for_recipient( | ||
| user.email, | ||
| api.context_for_user( | ||
| user=user, | ||
| extra_context={ | ||
| "enrollment": enrollment, | ||
| "run_start_date": run_start_date.strftime(EMAIL_DATE_FORMAT) | ||
| if run_start_date | ||
| else run_start_date, | ||
| "run_start_time": run_start_time, | ||
| "run_date_range": date_range, | ||
| }, | ||
| ), | ||
| EMAIL_WELCOME_COURSE_RUN_ENROLLMENT, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
the code can be refactored as:
| run_start_date = run_end_date = "" | |
| run_start_time = "" | |
| if enrollment.run.start_date: | |
| run_start_date = enrollment.run.start_date | |
| run_start_time = run_start_date.astimezone(datetime.timezone.utc).strftime( | |
| EMAIL_TIME_FORMAT | |
| ) | |
| if enrollment.run.end_date: | |
| run_end_date = enrollment.run.end_date | |
| if run_start_date and run_end_date: | |
| date_range = ( | |
| f"{run_start_date.strftime(EMAIL_DATE_FORMAT)} - " | |
| f"{run_end_date.strftime(EMAIL_DATE_FORMAT)}" | |
| ) | |
| else: | |
| date_range = "" | |
| try: | |
| user = enrollment.user | |
| api.send_message( | |
| api.message_for_recipient( | |
| user.email, | |
| api.context_for_user( | |
| user=user, | |
| extra_context={ | |
| "enrollment": enrollment, | |
| "run_start_date": run_start_date.strftime(EMAIL_DATE_FORMAT) | |
| if run_start_date | |
| else run_start_date, | |
| "run_start_time": run_start_time, | |
| "run_date_range": date_range, | |
| }, | |
| ), | |
| EMAIL_WELCOME_COURSE_RUN_ENROLLMENT, | |
| ) | |
| ) | |
| run_start_date = run_end_date = "" | |
| run_start_time = "" | |
| if enrollment.run.start_date: | |
| run_start_date = enrollment.run.start_date.strftime(EMAIL_DATE_FORMAT) | |
| run_start_time = enrollment.run.start_date.astimezone(datetime.timezone.utc).strftime( | |
| EMAIL_TIME_FORMAT | |
| ) | |
| if enrollment.run.end_date: | |
| run_end_date = enrollment.run.end_date.strftime(EMAIL_DATE_FORMAT) | |
| date_range = f"{run_start_date} - {run_end_date}" if run_start_date and run_end_date else "" | |
| try: | |
| user = enrollment.user | |
| api.send_message( | |
| api.message_for_recipient( | |
| user.email, | |
| api.context_for_user( | |
| user=user, | |
| extra_context={ | |
| "enrollment": enrollment, | |
| "run_start_date": run_start_date, | |
| "run_start_time": run_start_time, | |
| "run_date_range": date_range, | |
| }, | |
| ), | |
| EMAIL_WELCOME_COURSE_RUN_ENROLLMENT, | |
| ) | |
| ) |
mudassir-hafeez
force-pushed
the
mudassir/Welcome-emails-for-xpro-learners
branch
from
075b984 to
f790184
Compare
|
@arslanashraf7 would you please do an quick review before merging this PR? |
I'll take a look, Could you also get a review from @cachob on the screenshots? |
There was a problem hiding this comment.
Just requested a couple of changes. Along with that, I would recommend:
- Share the complete screenshot of the email with @cachob - You can add it to the PR description.
- The order of the enrollment and welcome emails is not specified - In my testing the order was random e.g. sometimes I got the welcome email before the enrollment email. (We should check with @cachob If that matters)
ecommerce/mail_api.py
Outdated
| @@ -191,6 +194,46 @@ def send_course_run_unenrollment_email(enrollment): | |||
| log.exception("Error sending unenrollment success email: %s", exp) # noqa: TRY401 | |||
|
|
|||
|
|
|||
| def send_welcome_course_run_enrollment_email(enrollment): | |||
There was a problem hiding this comment.
| def send_welcome_course_run_enrollment_email(enrollment): | |
| def send_course_run_enrollment_welcome_email(enrollment): |
ecommerce/mail_api.py
Outdated
| @@ -191,6 +194,46 @@ def send_course_run_unenrollment_email(enrollment): | |||
| log.exception("Error sending unenrollment success email: %s", exp) # noqa: TRY401 | |||
|
|
|||
|
|
|||
| def send_welcome_course_run_enrollment_email(enrollment): | |||
| """ | |||
| Welcome the user of successful enrollment for a course run | |||
There was a problem hiding this comment.
| Welcome the user of successful enrollment for a course run | |
| Send welcome email to the user on successful enrollment |
ecommerce/mail_api.py
Outdated
| ).strftime(EMAIL_TIME_FORMAT) | ||
| if enrollment.run.end_date: | ||
| run_end_date = enrollment.run.end_date.strftime(EMAIL_DATE_FORMAT) | ||
| date_range = ( |
There was a problem hiding this comment.
| date_range = ( | |
| run_duration = ( |
ecommerce/mail_api.py
Outdated
| ) | ||
| ) | ||
| except: # noqa: E722 | ||
| log.exception("Error sending welcome success email") |
There was a problem hiding this comment.
| log.exception("Error sending welcome success email") | |
| log.exception("Error sending welcome email") |
ecommerce/mail_api.py
Outdated
| if enrollment.run.start_date: | ||
| run_start_date = enrollment.run.start_date.strftime(EMAIL_DATE_FORMAT) | ||
| run_start_time = enrollment.run.start_date.astimezone( | ||
| datetime.timezone.utc | ||
| ).strftime(EMAIL_TIME_FORMAT) |
There was a problem hiding this comment.
nit: We can format this in one go. e.g. a format with - in it and then we can split date and time.
ecommerce/mail_api.py
Outdated
| run_end_date = enrollment.run.end_date.strftime(EMAIL_DATE_FORMAT) | ||
| date_range = ( |
There was a problem hiding this comment.
nit: This and above method for start dates could be a utility function that we can call from here.
| tuple: A tuple containing the formatted date and time strings. | ||
| """ | ||
| if run_date: | ||
| from ecommerce.mail_api import EMAIL_DATE_FORMAT |
There was a problem hiding this comment.
Use import here to avoid circular import
mudassir-hafeez
force-pushed
the
mudassir/Welcome-emails-for-xpro-learners
branch
from
58f6c3c to
af424be
Compare
Added
At my end, it seems to be in order, first enrollment email and then welcome email. Also we are sending them synchronously. Not sure if it is due to server delay or latency at your end. @cachob would you please look into the Arslan's comment #3017 (review) if that matters? |
|
@arslanashraf7 / @mudassir-hafeez - I don't think the order of which emails come first is critical. Are we able to ensure the order? I assumed that these could be subject to delays depending on the email service, user email settings, etc. The screenshots look good. Do you mind giving me a quick overview of how this feature will work? |
We can get around this with:
A question for you @cachob: Do you this we should keep this feature behind a feature flag as well? Any chance the course team might want to turn this off at some point?
I'd let @mudassir-hafeez answer this. |
@cachob, this feature works similarly to the enrollment email. There is no admin interaction required. The welcome email will be delivered to the user along with the enrollment email for each course enrollment. If we enroll a user in a course (or program), the welcome email will be automatically delivered on enrollment. This also applies to program enrollment, where the welcome email will be delivered for each course within the program. This ensures that learners receive the welcome email automatically at the moment of enrollment. CC: @arslanashraf7 |
|
Also discussed with @arslanashraf7 specifically for the case of a program. For example, if we have 5 courses in a program (in production) and 5 enrollment emails were being delivered previously. With this feature, now it will add 5 welcome emails for these course enrollments in the program, which will make a total of 10 emails. This number will vary if we have more or fewer courses in the program. We thought to check with @blarghmatey from DevOps to ensure we wouldn't encounter any issues (e.g., rate limiting, emails being marked as spam or any other potential issues) with this number of emails being sent. |
There was a problem hiding this comment.
👍 Looks good! Thanks for making all the changes. We can merge this PR once we have responses from @cachob on the above conversation and nothing new is to be incorporated.
|
@arslanashraf7 / @mudassir-hafeez - thanks for the responses. Let's set this up behind a feature flag as well. I think moving forward all enhancements like these should be behind one as well. |
|
@arslanashraf7 would you please re-review as I have made some changes to make this feature behind a feature flag? |
| @@ -200,6 +200,9 @@ def send_course_run_enrollment_welcome_email(enrollment): | |||
| Args: | |||
| enrollment (CourseRunEnrollment): the enrollment for which to send the welcome email | |||
| """ | |||
| if not settings.FEATURES.get("ENROLLMENT_WELCOME_EMAIL", False): | |||
There was a problem hiding this comment.
The feature flags are more readable when applied to the initiation of something. In this case, the flag should be placed where we are calling send_course_run_enrollment_welcome_email from. Someone looking at the code in api.py might think that the email would be sent but that would be blocked in this function. So, If we take this flag to the api.py before calling this function that would make more sense for code reader.
There was a problem hiding this comment.
It’s a good suggestion, but we will need to make this change in both places. Actually, I tried to make this change in a common place as it is being called not only through api.py but also through courses/management/utils.py for transfer enrollment.
There was a problem hiding this comment.
Alright, Let's keep it here then.
There was a problem hiding this comment.
Alright, Let's keep it here then.
Sorry I was looking into addressing your request, and I pushed commit as didn't notice we were agreed upon.
I have reverted it. Now It seems we are good and ready to go to merge.
mudassir-hafeez
force-pushed
the
mudassir/Welcome-emails-for-xpro-learners
branch
from
7ac04f8 to
560b541
Compare
mudassir-hafeez
force-pushed
the
mudassir/Welcome-emails-for-xpro-learners
branch
from
d607fc5 to
8079eb4
Compare
This reverts commit 8079eb4.
What are the relevant tickets?
#2470
Description (What does it do?)
This PR implements the welcome email feature, which sends a welcome email based on each user's course enrollment.
How can this be tested?
Checkout to this branch and make sure openedx is configured.
Try to enroll any user to any course through frontend, or you can use create enrollment command to enroll a user.
Check if you receive welcome email along with enrollment email.
Verify the content of the email with welcome letter provided in the the ticket https://github.com/mitodl/hq/issues/2470#issue-1912198810 or you can see the link here: https://docs.google.com/document/d/1gGdXSVT4U34_bIADqGzZvcEyWuulM6VK8VDbqyDF1MY/edit
Now, again try to enroll user with transfer enrollment, and follow steps 3 and 4.
Screenshots (if appropriate):
See screenshot here:
To see the downloaded email as PDF:
Arbisoft Mail - Welcome! course-v1_edX+DemoX+Demo_Course _ Demonstration Course starts on Feb 5, 2013.pdf
Note: The logo is missing from the email attachment above because the links point to a local server. However,
you can refer to the screenshot below to review the logo, which should ideally be the same once deployed:
Additional Context