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

feat: Ability to configure edx-ace with course emails #29900

Merged
merged 1 commit into from
Mar 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions lms/djangoapps/bulk_email/message_types.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
"""
ACE message types for bulk course emails.
"""

from openedx.core.djangoapps.ace_common.message import BaseMessageType


class BulkEmail(BaseMessageType):
"""
Course message to list of recepient by instructors.
"""
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.options['from_address'] = kwargs['context']['from_address']
84 changes: 84 additions & 0 deletions lms/djangoapps/bulk_email/messages.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
"""
Module to define email message related classes and methods
"""
from abc import ABC, abstractmethod

from django.contrib.auth import get_user_model
from django.core.mail import EmailMultiAlternatives
from edx_ace import ace
from edx_ace.recipient import Recipient

from lms.djangoapps.bulk_email.message_types import BulkEmail
from openedx.core.lib.celery.task_utils import emulate_http_request

User = get_user_model()


class CourseEmailMessage(ABC):
"""
Abstract base class for course email messages
"""

@abstractmethod
def send(self):
"""
Triggers sending of email message
"""


class DjangoEmail(CourseEmailMessage):
"""
Email message class to send email directly using django mail API.
"""
def __init__(self, connection, course_email, email_context):
"""
Construct message content using course_email model and context
"""
self.connection = connection
template_context = email_context.copy()
# use the CourseEmailTemplate that was associated with the CourseEmail
course_email_template = course_email.get_template()

plaintext_msg = course_email_template.render_plaintext(course_email.text_message, template_context)
html_msg = course_email_template.render_htmltext(course_email.html_message, template_context)

# Create email:
message = EmailMultiAlternatives(
course_email.subject,
plaintext_msg,
email_context['from_address'],
[email_context['email']]
)
message.attach_alternative(html_msg, 'text/html')
self.message = message

def send(self):
"""
send email using already opened connection
"""
self.connection.send_messages([self.message])


class ACEEmail(CourseEmailMessage):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no expert on the use of edx-ace, but I am used to seeing ACE messages inheriting from a common base message class named BaseMessageType. Example:

class VerificationApproved(BaseMessageType):
.

It's not apparent to me why we used a different (abstract) base class. Why is that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it to have a common send() function between the DjangoEmail and ACE implementations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes to achieve polymorphism or have single method to send both types of emails.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BaseMessageType is limited to defining message types of edx-ace emails whereas CourseEmailMessage is base class for two types of course emails we have

  1. Emails using Custom templates
  2. Emails using edx-ace templates

"""
Email message class to send email using edx-ace.
"""
def __init__(self, site, email_context):
"""
Construct edx-ace message using email_context
"""
self.site = site
self.user = User.objects.get(email=email_context['email'])
message = BulkEmail(context=email_context).personalize(
recipient=Recipient(email_context['user_id'], email_context['email']),
language=email_context['course_language'],
user_context={"name": email_context['name']},
)
self.message = message

def send(self):
"""
Send message by emulating request in the context of site and user
"""
with emulate_http_request(site=self.site, user=self.user):
ace.send(self.message)
48 changes: 24 additions & 24 deletions lms/djangoapps/bulk_email/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
from celery.exceptions import RetryTaskError
from celery.states import FAILURE, RETRY, SUCCESS
from django.conf import settings
from django.core.mail import EmailMultiAlternatives, get_connection
from django.contrib.sites.models import Site
from django.core.mail import get_connection
from django.core.mail.message import forbid_multi_line_headers
from django.urls import reverse
from django.utils import timezone
Expand All @@ -42,7 +43,14 @@
from common.djangoapps.util.string_utils import _has_non_ascii_characters
from lms.djangoapps.branding.api import get_logo_url_for_email
from lms.djangoapps.bulk_email.api import get_unsubscribed_link
from lms.djangoapps.bulk_email.toggles import is_email_use_course_id_from_for_bulk_enabled
from lms.djangoapps.bulk_email.messages import (
DjangoEmail,
ACEEmail,
)
from lms.djangoapps.bulk_email.toggles import (
is_bulk_email_edx_ace_enabled,
is_email_use_course_id_from_for_bulk_enabled,
)
from lms.djangoapps.bulk_email.models import CourseEmail, Optout
from lms.djangoapps.courseware.courses import get_course
from lms.djangoapps.instructor_task.models import InstructorTask
Expand All @@ -52,6 +60,7 @@
queue_subtasks_for_query,
update_subtask_status
)
from openedx.core.djangoapps.ace_common.template_context import get_base_template_context
from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
from openedx.core.lib.courses import course_image_url

Expand Down Expand Up @@ -498,16 +507,16 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas
# use the email from address in the CourseEmail, if it is present, otherwise compute it.
from_addr = course_email.from_addr or _get_source_address(course_email.course_id, course_title, course_language)

# use the CourseEmailTemplate that was associated with the CourseEmail
course_email_template = course_email.get_template()

site = Site.objects.get_current()
try:
connection = get_connection()
connection.open()

# Define context values to use in all course emails:
email_context = {'name': '', 'email': ''}
email_context = {'name': '', 'email': '', 'course_email': course_email, 'from_address': from_addr}
template_context = get_base_template_context(site)
email_context.update(global_email_context)
email_context.update(template_context)

start_time = time.time()
while to_list:
Expand All @@ -519,6 +528,8 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas
recipient_num += 1
current_recipient = to_list[-1]
email = current_recipient['email']
user_id = current_recipient['pk']
profile_name = current_recipient['profile__name']
if _has_non_ascii_characters(email):
to_list.pop()
total_recipients_failed += 1
Expand All @@ -530,26 +541,16 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas
continue

email_context['email'] = email
email_context['name'] = current_recipient['profile__name']
email_context['user_id'] = current_recipient['pk']
email_context['name'] = profile_name
email_context['user_id'] = user_id
email_context['course_id'] = course_email.course_id
email_context['unsubscribe_link'] = get_unsubscribed_link(current_recipient['username'],
str(course_email.course_id))

# Construct message content using templates and context:
plaintext_msg = course_email_template.render_plaintext(course_email.text_message, email_context)
html_msg = course_email_template.render_htmltext(course_email.html_message, email_context)

# Create email:
email_msg = EmailMultiAlternatives(
course_email.subject,
plaintext_msg,
from_addr,
[email],
connection=connection
)
email_msg.attach_alternative(html_msg, 'text/html')

if is_bulk_email_edx_ace_enabled():
message = ACEEmail(site, email_context)
else:
message = DjangoEmail(connection, course_email, email_context)
# Throttle if we have gotten the rate limiter. This is not very high-tech,
# but if a task has been retried for rate-limiting reasons, then we sleep
# for a period of time between all emails within this task. Choice of
Expand All @@ -563,8 +564,7 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas
f"BulkEmail ==> Task: {parent_task_id}, SubTask: {task_id}, EmailId: {email_id}, Recipient num: "
f"{recipient_num}/{total_recipients}, Recipient UserId: {current_recipient['pk']}"
)
connection.send_messages([email_msg])

message.send()
except SMTPDataError as exc:
# According to SMTP spec, we'll retry error codes in the 4xx range. 5xx range indicates hard failure.
total_recipients_failed += 1
Expand Down
55 changes: 44 additions & 11 deletions lms/djangoapps/bulk_email/tests/test_email.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory
from common.djangoapps.student.tests.factories import InstructorFactory
from common.djangoapps.student.tests.factories import StaffFactory
from lms.djangoapps.bulk_email.messages import ACEEmail
from lms.djangoapps.bulk_email.tasks import _get_course_email_context, _get_source_address
from lms.djangoapps.instructor_task.subtasks import update_subtask_status
from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort
Expand Down Expand Up @@ -176,25 +177,57 @@ class LocalizedFromAddressPlatformLangTestCase(SendEmailWithMockedUgettextMixin,
"""
Tests to ensure that the bulk email has the "From" address localized according to LANGUAGE_CODE.
"""
@override_settings(LANGUAGE_CODE='en', EMAIL_USE_COURSE_ID_FROM_FOR_BULK=True)
def test_english_platform(self):
@ddt.data(
('en', True, False),
('eo', True, False),
('en', True, True),
('eo', True, True),
)
@ddt.unpack
def test_english_platform(self, language_code, enable_use_corse_id_in_from, ace_enabled):
"""
Ensures that the source-code language (English) works well.
"""
assert self.course.language is None
# Sanity check
message = self.send_email()
self.assertRegex(message.from_email, '.*Course Staff.*')
with override_settings(
LANGUAGE_CODE=language_code,
EMAIL_USE_COURSE_ID_FROM_FOR_BULK=enable_use_corse_id_in_from,
BULK_EMAIL_SEND_USING_EDX_ACE=ace_enabled
):
message = self.send_email()
self.assertRegex(message.from_email, f'{language_code.upper()} .* Course Staff')

@override_settings(LANGUAGE_CODE='eo', EMAIL_USE_COURSE_ID_FROM_FOR_BULK=True)
def test_esperanto_platform(self):

@patch.dict(settings.FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': False})
@ddt.ddt
class AceEmailTestCase(SendEmailWithMockedUgettextMixin, EmailSendFromDashboardTestCase):
"""
Tests to ensure that the bulk email is sent using edx-ace when BULK_EMAIL_SEND_USING_EDX_ACE toggle is enabled.
"""
@ddt.data(
(True, True),
(False, False),
)
@ddt.unpack
@patch.object(ACEEmail, 'send')
def test_ace_eanbled_toggle(self, ace_enabled, email_sent_with_ace, mock_ace_email_send):
"""
Tests the fake Esperanto language to ensure proper gettext calls.
Ensures that the email message is sent via edx-ace when BULK_EMAIL_SEND_USING_EDX_ACE toggle is enabled.
"""
assert self.course.language is None
# Sanity check
message = self.send_email()
self.assertRegex(message.from_email, 'EO .* Course Staff')
mock_ace_email_send.return_value = None
test_email = {
'action': 'Send email',
'send_to': '["myself"]',
'subject': 'test subject for myself',
'message': 'test message for myself'
}

with override_settings(
BULK_EMAIL_SEND_USING_EDX_ACE=ace_enabled
):
response = self.client.post(self.send_mail_url, test_email)
self.assertEqual(email_sent_with_ace, mock_ace_email_send.called)


@patch.dict(settings.FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': False})
Expand Down
12 changes: 12 additions & 0 deletions lms/djangoapps/bulk_email/toggles.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,15 @@

def is_email_use_course_id_from_for_bulk_enabled():
return SettingToggle("EMAIL_USE_COURSE_ID_FROM_FOR_BULK", default=False).is_enabled()

# .. toggle_name: BULK_EMAIL_SEND_USING_EDX_ACE
# .. toggle_implementation: DjangoSetting
# .. toggle_default: False
# .. toggle_description: If True, use edx-ace to send bulk email messages
# .. toggle_use_cases: open_edx
# .. toggle_creation_date: 2022-02-10
# .. toggle_tickets: https://github.com/openedx/build-test-release-wg/issues/100


def is_bulk_email_edx_ace_enabled():
return SettingToggle("BULK_EMAIL_SEND_USING_EDX_ACE", default=False).is_enabled()
39 changes: 39 additions & 0 deletions lms/templates/bulk_email/edx_ace/bulkemail/email/body.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{% extends 'ace_common/edx_ace/common/base_body.html' %}

{% load i18n %}
{% load static %}
{% block content %}
<table width="100%" align="left" border="0" cellpadding="0" cellspacing="0" role="presentation">
<tr>
<td>
<p style="color: rgba(0,0,0,.75);">
{% autoescape off %}
{{ course_email.html_message }}
{% endautoescape %}
<br/>
</p>

<p style="font-size: 11px;">
{% filter force_escape %}
{% blocktrans %}
This email was automatically sent from {{ platform_name }}.
{% endblocktrans %}
{% endfilter %}
<br>
{% filter force_escape %}
{% blocktrans %}
You are receiving this email at address {{ email }} because you are enrolled in
{% endblocktrans %}
{% endfilter %} <a href='{{ course_url }}'>{{ course_title }}</a>.
<br>
{% filter force_escape %}
{% blocktrans %}
To stop receiving email like this, update your course email settings
{% endblocktrans %}
{% endfilter %} <a href='{{ email_settings_url }}'>{% trans "here" as transhere %}{{transhere|force_escape}}</a>.
<br><a href='{{ unsubscribe_link }}'>{% trans "unsubscribe" as transunsub %}{{transunsub|force_escape}}</a>
</p>
</td>
</tr>
</table>
{% endblock %}
10 changes: 10 additions & 0 deletions lms/templates/bulk_email/edx_ace/bulkemail/email/body.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{% load i18n %}
{% autoescape off %}
{{ course_email.text_message }}
{% endautoescape %}


{% blocktrans %}This email was automatically sent from {{ platform_name }}.{% endblocktrans %}
{% blocktrans %}You are receiving this email at address {{ email }} because you are enrolled in {{course_title}}.{% endblocktrans %}
{% blocktrans %}To stop receiving email like this, update your course email settings here {{ email_settings_url }}{% endblocktrans %}
{% blocktrans %}To unsubscribe click here {{ unsubscribe_link }}{% endblocktrans %}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{{ from_address }}
1 change: 1 addition & 0 deletions lms/templates/bulk_email/edx_ace/bulkemail/email/head.html
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{% extends 'ace_common/edx_ace/common/base_head.html' %}
4 changes: 4 additions & 0 deletions lms/templates/bulk_email/edx_ace/bulkemail/email/subject.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{% load i18n %}
{% autoescape off %}
{{ course_email.subject }}
{% endautoescape %}