From 58463f61a1fa171cfa8c95112dac0db1a1de22c8 Mon Sep 17 00:00:00 2001 From: Stephen Cefali Date: Tue, 2 Nov 2021 10:51:29 -0700 Subject: [PATCH 1/3] merge changes --- src/sentry/notifications/manager.py | 12 +++++++----- .../notifications/organization_request.py | 14 ++++++++++---- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/sentry/notifications/manager.py b/src/sentry/notifications/manager.py index 6dcd55175e0078..645bbae414edf2 100644 --- a/src/sentry/notifications/manager.py +++ b/src/sentry/notifications/manager.py @@ -299,23 +299,25 @@ def get_for_recipient_by_parent( def filter_to_accepting_recipients( self, - project: "Project", + parent: Union["Organization", "Project"], recipients: Iterable[Union["Team", "User"]], + type: NotificationSettingTypes = NotificationSettingTypes.ISSUE_ALERTS, ) -> Mapping[ExternalProviders, Iterable[Union["Team", "User"]]]: """ Filters a list of teams or users down to the recipients by provider who are subscribed to alerts. We check both the project level settings and global default settings. """ - notification_settings = self.get_for_recipient_by_parent( - NotificationSettingTypes.ISSUE_ALERTS, parent=project, recipients=recipients - ) + from sentry.models import Organization + + notification_settings = self.get_for_recipient_by_parent(type, parent, recipients) notification_settings_by_recipient = transform_to_notification_settings_by_recipient( notification_settings, recipients ) mapping = defaultdict(set) + organization = parent if isinstance(parent, Organization) else parent.organization should_use_slack_automatic = features.has( - "organizations:notification-slack-automatic", project.organization + "organizations:notification-slack-automatic", organization ) for recipient in recipients: providers = where_should_recipient_be_notified( diff --git a/src/sentry/notifications/notifications/organization_request.py b/src/sentry/notifications/notifications/organization_request.py index 8868b0cd528379..667d30a419bc62 100644 --- a/src/sentry/notifications/notifications/organization_request.py +++ b/src/sentry/notifications/notifications/organization_request.py @@ -4,9 +4,10 @@ from sentry import analytics, features, roles from sentry.integrations.slack.utils.notifications import get_settings_url -from sentry.models import OrganizationMember, Team +from sentry.models import NotificationSetting, OrganizationMember, Team from sentry.notifications.notifications.base import BaseNotification, MessageAction from sentry.notifications.notify import notification_providers +from sentry.notifications.types import NotificationSettingTypes from sentry.types.integrations import ExternalProviders, get_provider_name if TYPE_CHECKING: @@ -68,11 +69,16 @@ def get_participants(self) -> Mapping[ExternalProviders, Iterable[Union["Team", if features.has("organizations:slack-requests", self.organization): available_providers = notification_providers() - # TODO: need to read off notification settings recipients = list(self.determine_recipients()) - output = {provider: recipients for provider in available_providers} + recipients_by_provider = NotificationSetting.objects.filter_to_accepting_recipients( + self.organization, recipients, NotificationSettingTypes.APPROVAL + ) - return output + return { + provider: recipients_of_provider + for provider, recipients_of_provider in recipients_by_provider.items() + if provider in available_providers + } def send(self) -> None: from sentry.notifications.notify import notify From 0b35ff9690f50703da40eb93aa57d1fa8be8bbe9 Mon Sep 17 00:00:00 2001 From: Stephen Cefali Date: Tue, 2 Nov 2021 12:47:01 -0700 Subject: [PATCH 2/3] adds tests --- src/sentry/notifications/helpers.py | 6 +- src/sentry/notifications/manager.py | 2 +- .../notifications/notifications/__init__.py | 0 .../test_organization_request.py | 58 +++++++++++++++++++ 4 files changed, 63 insertions(+), 3 deletions(-) create mode 100644 tests/sentry/integrations/slack/notifications/notifications/__init__.py create mode 100644 tests/sentry/integrations/slack/notifications/notifications/test_organization_request.py diff --git a/src/sentry/notifications/helpers.py b/src/sentry/notifications/helpers.py index 87a4bd73dc8a77..aa6efd356cafdd 100644 --- a/src/sentry/notifications/helpers.py +++ b/src/sentry/notifications/helpers.py @@ -41,8 +41,9 @@ def _get_notification_setting_default( """ In order to increase engagement, we automatically opt users into receiving Slack notifications if they install Slack and link their identity. + Approval notifications always default to Slack being on. """ - if should_use_slack_automatic: + if should_use_slack_automatic or type == NotificationSettingTypes.APPROVAL: return NOTIFICATION_SETTINGS_ALL_SOMETIMES[type] return NOTIFICATION_SETTING_DEFAULTS[provider][type] @@ -86,6 +87,7 @@ def where_should_recipient_be_notified( ], recipient: Team | User, should_use_slack_automatic: bool = False, + type: NotificationSettingTypes = NotificationSettingTypes.ISSUE_ALERTS, ) -> list[ExternalProviders]: """ Given a mapping of default and specific notification settings by user, @@ -94,7 +96,7 @@ def where_should_recipient_be_notified( mapping = _get_setting_mapping_from_mapping( notification_settings_by_recipient, recipient, - NotificationSettingTypes.ISSUE_ALERTS, + type, should_use_slack_automatic=should_use_slack_automatic, ) return [ diff --git a/src/sentry/notifications/manager.py b/src/sentry/notifications/manager.py index 654428ac5bdd8a..07746b125acfbf 100644 --- a/src/sentry/notifications/manager.py +++ b/src/sentry/notifications/manager.py @@ -311,7 +311,7 @@ def filter_to_accepting_recipients( ) for recipient in recipients: providers = where_should_recipient_be_notified( - notification_settings_by_recipient, recipient, should_use_slack_automatic + notification_settings_by_recipient, recipient, should_use_slack_automatic, type ) for provider in providers: mapping[provider].add(recipient) diff --git a/tests/sentry/integrations/slack/notifications/notifications/__init__.py b/tests/sentry/integrations/slack/notifications/notifications/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/tests/sentry/integrations/slack/notifications/notifications/test_organization_request.py b/tests/sentry/integrations/slack/notifications/notifications/test_organization_request.py new file mode 100644 index 00000000000000..582e2381e9591c --- /dev/null +++ b/tests/sentry/integrations/slack/notifications/notifications/test_organization_request.py @@ -0,0 +1,58 @@ +from sentry.models import NotificationSetting, OrganizationMember +from sentry.notifications.notifications.organization_request import OrganizationRequestNotification +from sentry.notifications.types import NotificationSettingOptionValues, NotificationSettingTypes +from sentry.testutils import TestCase +from sentry.testutils.helpers import with_feature +from sentry.types.integrations import ExternalProviders + + +class DummyRequestNotification(OrganizationRequestNotification): + def __init__(self, organization, requester, member_ids): + super().__init__(organization, requester) + self.member_ids = member_ids + + def determine_member_recipients(self): + return OrganizationMember.objects.filter(id__in=self.member_ids) + + +class GetParticipantsTest(TestCase): + def setUp(self): + self.user1 = self.create_user() + self.user2 = self.create_user() + self.member1 = self.create_member(user=self.user1, organization=self.organization) + self.member2 = self.create_member(user=self.user2, organization=self.organization) + + @with_feature("organizations:slack-requests") + def test_default_to_slack(self): + member_ids = [self.member1.id, self.member2.id] + notification = DummyRequestNotification(self.organization, self.user, member_ids) + + assert notification.get_participants() == { + ExternalProviders.EMAIL: {self.user1, self.user2}, + ExternalProviders.SLACK: {self.user1, self.user2}, + } + + @with_feature("organizations:slack-requests") + def test_turn_off_settings(self): + + NotificationSetting.objects.update_settings( + ExternalProviders.SLACK, + NotificationSettingTypes.APPROVAL, + NotificationSettingOptionValues.ALWAYS, + user=self.user1, + ) + + NotificationSetting.objects.update_settings( + ExternalProviders.EMAIL, + NotificationSettingTypes.APPROVAL, + NotificationSettingOptionValues.ALWAYS, + user=self.user2, + ) + + member_ids = [self.member1.id, self.member2.id] + notification = DummyRequestNotification(self.organization, self.user, member_ids) + + assert notification.get_participants() == { + ExternalProviders.EMAIL: {self.user2}, + ExternalProviders.SLACK: {self.user1}, + } From ea5bd334cf7bb6fca614cd34aba4db4fbb9360bc Mon Sep 17 00:00:00 2001 From: Stephen Cefali Date: Tue, 2 Nov 2021 14:58:10 -0700 Subject: [PATCH 3/3] move test --- .../slack => }/notifications/notifications/__init__.py | 0 .../notifications/notifications/test_organization_request.py | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename tests/sentry/{integrations/slack => }/notifications/notifications/__init__.py (100%) rename tests/sentry/{integrations/slack => }/notifications/notifications/test_organization_request.py (100%) diff --git a/tests/sentry/integrations/slack/notifications/notifications/__init__.py b/tests/sentry/notifications/notifications/__init__.py similarity index 100% rename from tests/sentry/integrations/slack/notifications/notifications/__init__.py rename to tests/sentry/notifications/notifications/__init__.py diff --git a/tests/sentry/integrations/slack/notifications/notifications/test_organization_request.py b/tests/sentry/notifications/notifications/test_organization_request.py similarity index 100% rename from tests/sentry/integrations/slack/notifications/notifications/test_organization_request.py rename to tests/sentry/notifications/notifications/test_organization_request.py