Skip to content
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.

Sign up for GitHub

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

Notifications: Don't send mail if recipient has no email address. #680

Merged
merged 2 commits into from
Dec 6, 2023
Merged

Notifications: Don't send mail if recipient has no email address. #680

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e346dab
Notifications: Don't send mail if recipient has no email address.
thet Dec 5, 2023
e35f1c8
Notifications: Don't send mail if recipient has no email address /pt.2.
thet Dec 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/euphorie/client/notifications/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,15 @@ def index(self):
)
)

def send_email(self):
if not self.account.email:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work for DTMB because the email address is not in the account there. On the other hand in plain euphorie this is unnecessary because the user ID is an email address, so every account has an email.

You probably need to use webhelpers.get_user_email().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO you should not even reach this point.
Results should be filter out earlier.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reinhardt tnx for the pointer. fixed.

@ale-rt that would involve two things:
a) Implementing this logic and the logging message in all of the different notification classes.
b) or restructuring the code so that the notification base class does that.

I think we can't or shouldn't filter out on the SQL query level - the ws_user_cache reponse JSON‌ field wouldn't allow that easily (not considering Postgres' JSON capabilities, which I‌ don't have experience with) and we would loose the logging feature.

I'm 👎 for a) and :+0: for b) - we're over budget and we would need a full test circle again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Alex is going to be furious if we do more than this quick fix.

logger.warning(
"Account %r has no email address. Not sending notification email.",
self.account.id,
)
return
super().send_email()


class BaseNotification:
id = None
Expand Down
Loading