-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
email: don't send emails when notifications are disabled #407
email: don't send emails when notifications are disabled #407
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
3343ce3
to
a400ea6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Could you please also add the release commit to this PR, so that we can already start releasing the new version of reana-commons?
ef8303c
to
0fa088a
Compare
Rebased and merged! |
Closes reanahub/reana#736.