Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions common/djangoapps/student/views/management.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
from common.djangoapps.util.json_request import JsonResponse
from common.djangoapps.student.signals import USER_EMAIL_CHANGED
from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order
from openedx.core.djangoapps.ace_common.utils import apply_ses_routing_if_enabled

log = logging.getLogger("edx.student")

Expand Down Expand Up @@ -230,6 +231,7 @@ def compose_activation_email(
user_context=message_context,
)

msg = apply_ses_routing_if_enabled(msg)
return msg


Expand Down
42 changes: 42 additions & 0 deletions openedx/core/djangoapps/ace_common/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,51 @@
Utility functions for edx-ace.
"""
import logging
from django.conf import settings
from edx_toggles.toggles import WaffleFlag
from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers


Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we can probably remove this line and that means the only changes made are in tasks.py

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point — I removed the unused logging from utils.py. The file now only contains the existing firebase helper, so all SES-related changes are limited to tasks.py.

log = logging.getLogger(__name__)

# .. toggle_name: user_authn.enable_ses_for_account_activation
# .. toggle_implementation: WaffleFlag
# .. toggle_default: False
# .. toggle_description: Route account activation emails via SES using ACE.
# .. toggle_use_cases: opt_in, temporary
# .. toggle_creation_date: 2026-03-31
# .. toggle_target_removal_date: None
# .. toggle_warning: Controls SES routing for account activation emails.

ENABLE_SES_FOR_ACCOUNT_ACTIVATION = WaffleFlag(
'user_authn.enable_ses_for_account_activation',
__name__,
)


def apply_ses_routing_if_enabled(msg):
"""
Apply SES routing to ACE message if flag is enabled.
"""
if not ENABLE_SES_FOR_ACCOUNT_ACTIVATION.is_enabled():
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that this probably doesn't belong in the ace_common utils file, since if we look at Goal Reminder's implementation it lives closer to the management command.

return msg

if msg.options is None:
msg.options = {}

msg.options.update({
'transactional': True,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AccountActivation in the management.py also sets the transactional property to True, so I don't think this is necessary. If anything, this further brings up the point that most of this can be moved to the management.py file and not live in ace_common.

'override_default_channel': 'django_email',
'from_address': configuration_helpers.get_value(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking in the management.py file, it looks like this action is already handled here

'ACTIVATION_EMAIL_FROM_ADDRESS'
) or configuration_helpers.get_value(
'email_from_address',
settings.DEFAULT_FROM_EMAIL
),
})

return msg


def setup_firebase_app(firebase_credentials, app_name='fcm-app'):
"""
Expand Down
100 changes: 87 additions & 13 deletions openedx/core/djangoapps/user_authn/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
from openedx.core.djangoapps.user_authn.utils import check_pwned_password
from openedx.core.lib.celery.task_utils import emulate_http_request
from openedx.core.djangoapps.ace_common.utils import ENABLE_SES_FOR_ACCOUNT_ACTIVATION

log = logging.getLogger('edx.celery.task')

Expand Down Expand Up @@ -60,6 +61,9 @@ def send_activation_email(self, msg_string, from_address=None, site_id=None):
max_retries = settings.RETRY_ACTIVATION_EMAIL_MAX_ATTEMPTS
retries = self.request.retries

if msg.options is None:
msg.options = {}

Comment thread
jsnwesson marked this conversation as resolved.
if from_address is None:
from_address = configuration_helpers.get_value('ACTIVATION_EMAIL_FROM_ADDRESS') or (
configuration_helpers.get_value('email_from_address', settings.DEFAULT_FROM_EMAIL)
Expand All @@ -71,28 +75,98 @@ def send_activation_email(self, msg_string, from_address=None, site_id=None):
site = Site.objects.get(id=site_id) if site_id else Site.objects.get_current()
user = User.objects.get(id=msg.recipient.lms_user_id)

route_via_ses = ENABLE_SES_FOR_ACCOUNT_ACTIVATION.is_enabled()
sent_via_ses = False

if route_via_ses:
Comment thread
jsnwesson marked this conversation as resolved.
msg.options.update({
'override_default_channel': 'django_email',
'transactional': True,
'from_address': configuration_helpers.get_value(
'ACTIVATION_EMAIL_FROM_ADDRESS'
) or configuration_helpers.get_value(
'email_from_address',
settings.DEFAULT_FROM_EMAIL
),
})

Comment thread
jsnwesson marked this conversation as resolved.
try:
with emulate_http_request(site=site, user=user):
ace.send(msg)
sent_via_ses = route_via_ses

except RecoverableChannelDeliveryError:
log.info('Retrying sending email to user {dest_addr}, attempt # {attempt} of {max_attempts}'.format(
dest_addr=dest_addr,
attempt=retries,
max_attempts=max_retries
))
log.warning(
"SES send failed for %s, falling back to default ACE channel",
dest_addr,
exc_info=True,
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a weird one, because we'd have to remove this log eventually if we either transition fully to AWS SES, right? I think we can maybe wrap this in an if route_via_ses condition?


if not route_via_ses:
log.info(
'Retrying sending email to user {dest_addr}, attempt # {attempt} of {max_attempts}'.format(
dest_addr=dest_addr,
attempt=retries,
max_attempts=max_retries
)
)
try:
self.retry(
countdown=settings.RETRY_ACTIVATION_EMAIL_TIMEOUT,
max_retries=max_retries
)
except MaxRetriesExceededError:
log.error(
'Unable to send activation email to user from "%s" to "%s"',
from_address,
dest_addr,
exc_info=True
)
return

_remove_ses_overrides(msg)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fallback happens in the except RecoverableChannelDeliveryError block.

Specifically:

  • We remove SES overrides via _remove_ses_overrides(msg)
  • Then call ace.send(msg) again

That second ace.send(msg) routes via the default ACE channel (Braze).


try:
self.retry(countdown=settings.RETRY_ACTIVATION_EMAIL_TIMEOUT, max_retries=max_retries)
except MaxRetriesExceededError:
log.error(
'Unable to send activation email to user from "%s" to "%s"',
from_address,
dest_addr,
exc_info=True
with emulate_http_request(site=site, user=user):
ace.send(msg)
sent_via_ses = False

except RecoverableChannelDeliveryError:
log.info(
'Retrying sending email to user {dest_addr}, attempt # {attempt} of {max_attempts}'.format(
dest_addr=dest_addr,
attempt=retries,
max_attempts=max_retries
)
)
try:
self.retry(
countdown=settings.RETRY_ACTIVATION_EMAIL_TIMEOUT,
max_retries=max_retries
)
except MaxRetriesExceededError:
log.error(
'Unable to send activation email to user from "%s" to "%s"',
from_address,
dest_addr,
exc_info=True
)
except Exception:
log.exception(
'Unable to send activation email to user from "%s" to "%s"',
from_address,
dest_addr,
)
raise Exception # lint-amnesty, pylint: disable=raise-missing-from
raise

log.info(
'Activation email for %s sent via %s',
dest_addr,
'SES' if sent_via_ses else 'default ACE channel',
)


def _remove_ses_overrides(msg):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Last comment: I feel like the entire block of the first RecoverableChannelDeliveryError doesn't need to be changed much and that you don't need to add the same logic for RecoverableChannelDeliveryError again.
To simplify the logic here, I think this method and the log.warning that announces the ACE fallback will be used can both be wrapped in an if routing_via_ses condition at the top of the RecoverableChannelDeliveryError block and everything after can remain untouched (aside from the log.info you added about whether SES or "default ACE channel" was used.

msg.options.pop('override_default_channel', None)
msg.options.pop('transactional', None)
msg.options.pop('from_address', None)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

assuming that we don't need to add the from_address, as it has all been the same, do we want to pop this?

Loading