email: don't send emails when notifications are disabled #407
Conversation
giuseppe-steduto
force-pushed
the
not-send-emails-notifications-disabled
branch
from
2c8e733
to
3343ce3
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #407 +/- ##
==========================================
+ Coverage 35.70% 35.80% +0.10%
==========================================
Files 26 26
Lines 1560 1564 +4
==========================================
+ Hits 557 560 +3
- Misses 1003 1004 +1
|
reana_commons/email.py
Outdated
| message = EmailMessage() | ||
| message["From"] = f"REANA platform <{sender_email}>" | ||
| message["To"] = receiver_email | ||
| message["Subject"] = subject | ||
| message.set_content(body) | ||
|
|
||
| if not REANA_NOTIFICATIONS_ENABLED: | ||
| logging.info( |
There was a problem hiding this comment.
We can keep this like this for now, but for the future we should improve this as we are mixing the logic to send emails (e.g. server connection, message encoding, etc.) with REANA's notification logic (e.g. do not send emails if notifications are disabled). Not only that, the behaviour of this function depends a lot on global state, thus making this function hard to test and hard to use properly.
On another note, what do you think about throwing an exception instead of just logging this? We already have REANAEmailNotificationError, and we are already catching it in most places, except for _send_confirmation_email. In this way, we also avoid having to check REANA_NOTIFICATIONS_ENABLED to understand if the email was actually sent or not
giuseppe-steduto
force-pushed
the
not-send-emails-notifications-disabled
branch
from
3343ce3
to
a400ea6
Compare
mdonadoni
force-pushed
the
not-send-emails-notifications-disabled
branch
from
ef8303c
to
0fa088a
Compare
|
Rebased and merged! |
Closes reanahub/reana#736.