feat: added email sent signal to edx ace #292
Conversation
edx_ace/ace.py
Outdated
| @@ -51,6 +51,7 @@ def send(msg): | |||
| try: | |||
| rendered_message = presentation.render(channel, msg) | |||
| delivery.deliver(channel, rendered_message, msg) | |||
|
|
|||
There was a problem hiding this comment.
Please remove this line to keep the commit more focused.
edx_ace/delivery.py
Outdated
| @@ -10,6 +10,7 @@ | |||
| from django.conf import settings | |||
|
|
|||
| from edx_ace.errors import RecoverableChannelDeliveryError | |||
| from edx_ace.signals import ACE_EMAIL_SENT | |||
There was a problem hiding this comment.
@AhtishamShahid Should we use a Django signal here or a https://github.com/openedx/openedx-events and https://github.com/openedx/openedx-filters event?
This is more of discussion than a request. Please let me know what do you think?
There was a problem hiding this comment.
Hi,
We are not fundamentally changing the application behavior or flow , so using openedx-filters does not seem appropriate. However, openedx-events can work similarly to our current implementation. The use of global signals would be meaningful if we intended to handle that signal in other packages as well, but that is not the case at this time. Therefore, directly using Django signals makes more sense to me.
edx_ace/signals.py
Outdated
| # signal to indicate that an email has been sent using ace | ||
| ACE_EMAIL_SENT = Signal() |
There was a problem hiding this comment.
| # signal to indicate that an email has been sent using ace | |
| ACE_EMAIL_SENT = Signal() | |
| # signal to indicate that a message has been sent using ace | |
| ACE_MESSAGE_SENT = Signal() |
We support both Push notifications and email as message types among other types.
AhtishamShahid
force-pushed
the
ahtisham/INF-1456
branch
from
dc89a61
to
26d8b5e
Compare
edx_ace/signals.py
Outdated
| from django.dispatch import Signal | ||
|
|
||
| # signal to indicate that a message has been sent using ace | ||
| ACE_EMAIL_SENT = Signal() |
There was a problem hiding this comment.
| ACE_EMAIL_SENT = Signal() | |
| ACE_MESSAGE_SENT = Signal() |
There was a problem hiding this comment.
@AhtishamShahid I've sumitted a review already, but it seems that a bad internet connection got it lost.
I was surprised that it's been merged, but since my review was lost by GitHub, it's fine.
Would you mind checking the comment? It seems that we're sending the signal in cases where the message isn't sent.
Anyway, please ensure tests are added for the following cases:
- Email is sent: ensure only one signal is sent.
- Email sending fails: Ensure no signal is sent.
- Email and Push notifications: Two signals are sent with appropriate
channelparameter
ACE_MESSAGE_SENT.send(sender=channel, message=message)
| return | ||
|
|
||
| delivery_expired_report = f'{channel_type}_delivery_expired' | ||
| logger.debug(delivery_expired_report) | ||
| message.report(delivery_expired_report, get_current_time() - start_time) | ||
| ACE_MESSAGE_SENT.send(sender=channel, message=message) |
There was a problem hiding this comment.
Would you mind explaining the logic behind this line?
It seems to be added by mistake.
There was a problem hiding this comment.
@muhammadadeeltajamul please check the review above and let me know when the notes can be addressed.
Create a PR with these changes #296 |
Description
Added signal to track if email is sent for analytics and tracking.
Ticket
https://2u-internal.atlassian.net/browse/INF-1456