fix(notification platform): Remove hardcoded email provider #28577
Conversation
src/sentry/notifications/helpers.py
Outdated
| @@ -91,7 +91,7 @@ def _get_setting_mapping_from_mapping( | |||
|
|
|||
| return { | |||
| provider: _get_notification_setting_default(provider, type, user=user) | |||
| for provider in [ExternalProviders.EMAIL] | |||
| for provider in [ExternalProviders.EMAIL, ExternalProviders.SLACK] | |||
There was a problem hiding this comment.
I wasn't sure if I should do this or just get all ExternalProviders enum values
There was a problem hiding this comment.
You can remove the XXX(CEO) comment now.
There was a problem hiding this comment.
the comment was about if we change the front end to allow a user to set Slack notifications for project A, and email notifications for project B, which we don't allow, so I think it still stands.
There was a problem hiding this comment.
Yeah we should just do all of them here.
from sentry.notifications.notify import notification_providers
...
for provider in notification_providers()There was a problem hiding this comment.
Maybe a separate Notification Platform enum? If this list is replicated in multiple places it'd make for easier changes down the line.
src/sentry/mail/adapter.py
Outdated
| ExternalProviders.EMAIL | ||
| ] | ||
| recipients = NotificationSetting.objects.get_notification_recipients(project) | ||
| return set.union(*recipients.values()) if recipients else {} |
There was a problem hiding this comment.
get_notification_recipients() never returns None so it's safe to always return the union. I'd also rename the variable to make it clearer why we're calling .values().
recipients_by_provider = NotificationSetting.objects.get_notification_recipients(project)
return set.union(*recipients_by_provider.values())There was a problem hiding this comment.
It does in
sentry/tests/sentry/mail/test_adapter.py
Line 759 in 36cb56e
which is why I added the check
There was a problem hiding this comment.
I just checked this out:
>>> set.union(set([1]), set([1]))
{1}
>>> set.union(set([1]), [1])
{1}
>>> set.union([1], [1])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: descriptor 'union' requires a 'set' object but received a 'list'
I think because .values() returns lists you won't be able to use set.union(*...).
recipients_by_provider = NotificationSetting.objects.get_notification_recipients(project)
return set([user for users in recipients_by_provider.values() for user in users])
src/sentry/mail/adapter.py
Outdated
| ExternalProviders.EMAIL | ||
| ] | ||
| recipients = NotificationSetting.objects.get_notification_recipients(project) | ||
| return set.union(*recipients.values()) if recipients else {} |
There was a problem hiding this comment.
some cleeeaan python 🐍
i had to look up almost everything in this line
src/sentry/notifications/helpers.py
Outdated
| @@ -91,7 +91,7 @@ def _get_setting_mapping_from_mapping( | |||
|
|
|||
| return { | |||
| provider: _get_notification_setting_default(provider, type, user=user) | |||
| for provider in [ExternalProviders.EMAIL] | |||
| for provider in [ExternalProviders.EMAIL, ExternalProviders.SLACK] | |||
There was a problem hiding this comment.
Maybe a separate Notification Platform enum? If this list is replicated in multiple places it'd make for easier changes down the line.
| } | ||
| rule = Rule.objects.create( | ||
| project=self.project, | ||
| label="ja rule", |
There was a problem hiding this comment.
not enough people make tests fun 👏
Remove the hard coded email provider and just check for any provider - I believe this was leftover from a refactor before we were ready to launch. This fixes an issue where if a user's delivery method for issue alerts was set to Slack (and not Slack and email) and a rule fired which was set to send a notification to issue owners, it would not send.