feat: add SES routing for account activation emails with fallback support#207
Conversation
nthriveni-sonata-ship-it
force-pushed
the
feature/account-activation-ses-routing
branch
from
4aeff43 to
d52602d
Compare
nthriveni-sonata-ship-it
force-pushed
the
feature/account-activation-ses-routing
branch
from
d52602d to
83ef742
Compare
| ) | ||
| return | ||
|
|
||
| _remove_ses_overrides(msg) |
There was a problem hiding this comment.
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).
| msg.options.update({ | ||
| 'transactional': True, | ||
| 'override_default_channel': 'django_email', | ||
| 'from_address': configuration_helpers.get_value( |
There was a problem hiding this comment.
Looking in the management.py file, it looks like this action is already handled here
| """ | ||
| Apply SES routing to ACE message if flag is enabled. | ||
| """ | ||
| if not ENABLE_SES_FOR_ACCOUNT_ACTIVATION.is_enabled(): |
There was a problem hiding this comment.
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.
| log.warning( | ||
| "SES send failed for %s, falling back to default ACE channel", | ||
| dest_addr, | ||
| exc_info=True, | ||
| ) |
There was a problem hiding this comment.
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?
| def _remove_ses_overrides(msg): | ||
| msg.options.pop('override_default_channel', None) | ||
| msg.options.pop('transactional', None) | ||
| msg.options.pop('from_address', None) |
There was a problem hiding this comment.
assuming that we don't need to add the from_address, as it has all been the same, do we want to pop this?
| ) | ||
|
|
||
|
|
||
| def _remove_ses_overrides(msg): |
There was a problem hiding this comment.
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 = {} | ||
|
|
||
| msg.options.update({ | ||
| 'transactional': True, |
There was a problem hiding this comment.
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.
nthriveni-sonata-ship-it
force-pushed
the
feature/account-activation-ses-routing
branch
7 times, most recently
from
00851e2 to
af0e027
Compare
nthriveni-sonata-ship-it
force-pushed
the
feature/account-activation-ses-routing
branch
from
af0e027 to
3917337
Compare
| """ | ||
| import logging | ||
|
|
||
|
|
There was a problem hiding this comment.
we can probably remove this line and that means the only changes made are in tasks.py
jsnwesson
left a comment
jsnwesson
left a comment
There was a problem hiding this comment.
Aside from the one new comment i made, everything looks good!
3 participants
Summary
Adds feature-flagged SES routing for account activation emails with a safe fallback mechanism.
Changes
Behavior
Testing