feat: add SES delivery support for course update emails #101
Conversation
|
Unit tests are failing. Pls check. |
ajayakkalasetti
force-pushed
the
feature/course-update-ses-delivery
branch
2 times, most recently
from
d1aa330 to
45a97f3
Compare
| with emulate_http_request(site=site, user=user): | ||
| _annonate_send_task_for_monitoring(msg) | ||
| LOG.debug('%s: Sending message = %s', log_prefix, msg_str) | ||
| is_ses_enabled = ( |
There was a problem hiding this comment.
The logic for determining is_ses_enabled is starting to grow. To keep _schedule_send clean and ensure we can reuse this logic in other tasks (like send_activation_email), I suggest pulling this into a helper function like should_route_to_ses(msg, course_key=None) placed in openedx/core/djangoapps/ace_common/utils.py
There was a problem hiding this comment.
Thanks for the suggestion. I’ve refactored the SES routing logic into a reusable helper should_route_to_ses(...) and moved it to openedx/core/djangoapps/ace_common/utils.py as suggested. _schedule_send is now cleaner and uses this helper.
| msg = Message.from_string(msg_str) | ||
| msg.options['skip_disable_user_policy'] = True | ||
| course_ids = msg.context.get('course_ids', []) | ||
| course_key = ( |
There was a problem hiding this comment.
Are we trying to choose SES route based on a course?
There was a problem hiding this comment.
SES routing is not course-based. The delivery path is determined by the environment-driven delivery_config_var (deliver_course_update) and applies uniformly to course update emails.
| 'override_default_channel': 'django_email', | ||
| 'transactional': True, | ||
| }) | ||
| ace.send(msg) |
There was a problem hiding this comment.
To ensure the account activation flow is resilient, we should implement a safe fallback pattern. If the Waffle flag is enabled but the SES fails due to a configuration or network issue, the current code will throw an exception and the email will never be sent. By wrapping the SES attempt in a try/except and reverting the options on failure, we ensure the user still receives their email via the default ACE path
There was a problem hiding this comment.
Implemented a safe fallback pattern around the SES send. If SES fails with a recoverable error, the exception is raised for retry.
asharma4-sonata
left a comment
asharma4-sonata
left a comment
There was a problem hiding this comment.
Hi @ajayakkalasetti Please find review details in inline comments.
ajayakkalasetti
force-pushed
the
feature/course-update-ses-delivery
branch
from
45a97f3 to
95368b5
Compare
ajayakkalasetti
force-pushed
the
feature/course-update-ses-delivery
branch
from
95368b5 to
524938a
Compare
|
|
||
| try: | ||
| ace.send(msg) | ||
| except RecoverableChannelDeliveryError: |
There was a problem hiding this comment.
Crazy software might raise a standard Exception rather than a specific ACE error. If we don't catch that, the email won't fall back—it will just fail the task.
| log_prefix, | ||
| user.id, | ||
| ) | ||
| raise |
There was a problem hiding this comment.
There should be a fallback flag that allows emails to be sent through the ACE default route if SES fails. The ultimate goal is to ensure the user always receives the email. We can disable the Braze route once we fully transition away from Braze
Summary
Introduces an opt-in Course Waffle Flag to enable sending Course Update emails via AWS SES.
Details
course_experience.enable_ses_for_courseupdatewaffle flag (default: disabled).Testing
Rollout
@asharma4-sonata Please review my PR and let me know if any changes need to be done.