From 220347b8440511dbe37676f4810ad3e8a3265831 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Wed, 3 Nov 2021 15:20:53 -0700 Subject: [PATCH 1/6] Fix where_should_be_participating --- src/sentry/notifications/helpers.py | 28 +++--- tests/sentry/notifications/test_utils.py | 46 --------- .../test_get_setting_mapping_from_mapping.py | 94 +++++++++++++++++++ 3 files changed, 110 insertions(+), 58 deletions(-) create mode 100644 tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py diff --git a/src/sentry/notifications/helpers.py b/src/sentry/notifications/helpers.py index aa6efd356cafdd..542941581b9ac8 100644 --- a/src/sentry/notifications/helpers.py +++ b/src/sentry/notifications/helpers.py @@ -57,27 +57,31 @@ def _get_setting_mapping_from_mapping( type: NotificationSettingTypes, should_use_slack_automatic: bool = False, ) -> Mapping[ExternalProviders, NotificationSettingOptionValues]: - # XXX(CEO): may not respect granularity of a setting for Slack a setting for email - # but we'll worry about that later since we don't have a FE for it yet + """ + XXX(CEO): may not respect granularity of a setting for Slack a setting for + email but we'll worry about that later since we don't have a FE for it yet. + """ from sentry.notifications.notify import notification_providers - specific_scope = get_scope_type(type) + # Fill in with the fallback values. + notification_setting_option = { + provider: _get_notification_setting_default( + provider, type, should_use_slack_automatic=should_use_slack_automatic + ) + for provider in notification_providers() + } + notification_settings_mapping = notification_settings_by_recipient.get(recipient) if notification_settings_mapping: - notification_setting_option = ( + specific_scope = get_scope_type(type) + notification_setting_option.update( notification_settings_mapping.get(specific_scope) or notification_settings_mapping.get(NotificationScopeType.USER) or notification_settings_mapping.get(NotificationScopeType.TEAM) + or {} ) - if notification_setting_option: - return notification_setting_option - return { - provider: _get_notification_setting_default( - provider, type, should_use_slack_automatic=should_use_slack_automatic - ) - for provider in notification_providers() - } + return notification_setting_option def where_should_recipient_be_notified( diff --git a/tests/sentry/notifications/test_utils.py b/tests/sentry/notifications/test_utils.py index ebf31b35eefcbe..a79080ce647509 100644 --- a/tests/sentry/notifications/test_utils.py +++ b/tests/sentry/notifications/test_utils.py @@ -1,6 +1,5 @@ from sentry.models import NotificationSetting from sentry.notifications.helpers import ( - _get_setting_mapping_from_mapping, collect_groups_by_project, get_scope_type, get_settings_by_provider, @@ -42,51 +41,6 @@ def setUp(self): user=self.user, ) - def test_get_setting_mapping_from_mapping_issue_alerts(self): - notification_settings = { - self.user: { - NotificationScopeType.USER: { - ExternalProviders.EMAIL: NotificationSettingOptionValues.ALWAYS - } - } - } - mapping = _get_setting_mapping_from_mapping( - notification_settings, - self.user, - NotificationSettingTypes.ISSUE_ALERTS, - ) - assert mapping == {ExternalProviders.EMAIL: NotificationSettingOptionValues.ALWAYS} - - def test_get_setting_mapping_from_mapping_deploy(self): - notification_settings = { - self.user: { - NotificationScopeType.USER: { - ExternalProviders.EMAIL: NotificationSettingOptionValues.COMMITTED_ONLY - } - } - } - mapping = _get_setting_mapping_from_mapping( - notification_settings, - self.user, - NotificationSettingTypes.DEPLOY, - ) - assert mapping == {ExternalProviders.EMAIL: NotificationSettingOptionValues.COMMITTED_ONLY} - - def test_get_setting_mapping_from_mapping_workflow(self): - notification_settings = { - self.user: { - NotificationScopeType.USER: { - ExternalProviders.EMAIL: NotificationSettingOptionValues.SUBSCRIBE_ONLY - } - } - } - mapping = _get_setting_mapping_from_mapping( - notification_settings, - self.user, - NotificationSettingTypes.WORKFLOW, - ) - assert mapping == {ExternalProviders.EMAIL: NotificationSettingOptionValues.SUBSCRIBE_ONLY} - def test_get_deploy_values_by_provider_empty_settings(self): values_by_provider = get_values_by_provider_by_type( {}, diff --git a/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py b/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py new file mode 100644 index 00000000000000..840d5d63d83b8d --- /dev/null +++ b/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py @@ -0,0 +1,94 @@ +from __future__ import annotations + +from sentry.notifications.helpers import _get_setting_mapping_from_mapping +from sentry.notifications.types import ( + NotificationScopeType, + NotificationSettingOptionValues, + NotificationSettingTypes, +) +from sentry.testutils import TestCase +from sentry.types.integrations import ExternalProviders + + +class NotificationHelpersTest(TestCase): + def test_get_setting_mapping_from_mapping_issue_alerts(self): + notification_settings = { + self.user: { + NotificationScopeType.USER: { + ExternalProviders.EMAIL: NotificationSettingOptionValues.ALWAYS + } + } + } + mapping = _get_setting_mapping_from_mapping( + notification_settings, + self.user, + NotificationSettingTypes.ISSUE_ALERTS, + ) + assert mapping == { + ExternalProviders.EMAIL: NotificationSettingOptionValues.ALWAYS, + ExternalProviders.SLACK: NotificationSettingOptionValues.NEVER, + } + + def test_get_setting_mapping_from_mapping_deploy(self): + notification_settings = { + self.user: { + NotificationScopeType.USER: { + ExternalProviders.EMAIL: NotificationSettingOptionValues.COMMITTED_ONLY + } + } + } + mapping = _get_setting_mapping_from_mapping( + notification_settings, + self.user, + NotificationSettingTypes.DEPLOY, + ) + assert mapping == { + ExternalProviders.EMAIL: NotificationSettingOptionValues.COMMITTED_ONLY, + ExternalProviders.SLACK: NotificationSettingOptionValues.NEVER, + } + + def test_get_setting_mapping_from_mapping_workflow(self): + notification_settings = { + self.user: { + NotificationScopeType.USER: { + ExternalProviders.EMAIL: NotificationSettingOptionValues.SUBSCRIBE_ONLY + } + } + } + mapping = _get_setting_mapping_from_mapping( + notification_settings, + self.user, + NotificationSettingTypes.WORKFLOW, + ) + assert mapping == { + ExternalProviders.EMAIL: NotificationSettingOptionValues.SUBSCRIBE_ONLY, + ExternalProviders.SLACK: NotificationSettingOptionValues.NEVER, + } + + def test_get_setting_mapping_from_mapping_empty(self): + mapping = _get_setting_mapping_from_mapping( + {}, self.user, NotificationSettingTypes.ISSUE_ALERTS + ) + assert mapping == { + ExternalProviders.EMAIL: NotificationSettingOptionValues.ALWAYS, + ExternalProviders.SLACK: NotificationSettingOptionValues.NEVER, + } + + def test_get_setting_mapping_from_mapping_never(self): + notification_settings = { + self.user: { + NotificationScopeType.USER: { + ExternalProviders.SLACK: NotificationSettingOptionValues.NEVER + } + } + } + + mapping = _get_setting_mapping_from_mapping( + notification_settings, + self.user, + NotificationSettingTypes.ISSUE_ALERTS, + ) + assert mapping == { + ExternalProviders.EMAIL: NotificationSettingOptionValues.ALWAYS, + ExternalProviders.SLACK: NotificationSettingOptionValues.NEVER, + } From c1317e6c66cd7765262c5b48e710c481a88f7e36 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Wed, 3 Nov 2021 15:31:16 -0700 Subject: [PATCH 2/6] fix test name --- .../utils/test_get_setting_mapping_from_mapping.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py b/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py index 840d5d63d83b8d..2b4003fef1dc11 100644 --- a/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py +++ b/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py @@ -1,5 +1,3 @@ -from __future__ import annotations - from sentry.notifications.helpers import _get_setting_mapping_from_mapping from sentry.notifications.types import ( NotificationScopeType, @@ -10,7 +8,7 @@ from sentry.types.integrations import ExternalProviders -class NotificationHelpersTest(TestCase): +class GetSettingMappingFromMappingTest(TestCase): def test_get_setting_mapping_from_mapping_issue_alerts(self): notification_settings = { self.user: { From f553628fb3a3880c1c4046a22ccb8033d85cc5c8 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Wed, 3 Nov 2021 15:44:34 -0700 Subject: [PATCH 3/6] Speed up tests --- .../utils/test_get_setting_mapping_from_mapping.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py b/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py index 2b4003fef1dc11..6ad5ebee637f42 100644 --- a/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py +++ b/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py @@ -1,14 +1,19 @@ +from unittest import TestCase + +from sentry.models import User from sentry.notifications.helpers import _get_setting_mapping_from_mapping from sentry.notifications.types import ( NotificationScopeType, NotificationSettingOptionValues, NotificationSettingTypes, ) -from sentry.testutils import TestCase from sentry.types.integrations import ExternalProviders class GetSettingMappingFromMappingTest(TestCase): + def setUp(self): + self.user = User(id=1) + def test_get_setting_mapping_from_mapping_issue_alerts(self): notification_settings = { self.user: { From 98acc387bbe953dfd80d9ef29bda1f00fc4a362f Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Wed, 3 Nov 2021 15:44:47 -0700 Subject: [PATCH 4/6] add a test --- .../test_get_setting_mapping_from_mapping.py | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py b/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py index 6ad5ebee637f42..4ccb45e3098d2b 100644 --- a/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py +++ b/tests/sentry/notifications/utils/test_get_setting_mapping_from_mapping.py @@ -77,7 +77,7 @@ def test_get_setting_mapping_from_mapping_empty(self): ExternalProviders.SLACK: NotificationSettingOptionValues.NEVER, } - def test_get_setting_mapping_from_mapping_never(self): + def test_get_setting_mapping_from_mapping_slack_never(self): notification_settings = { self.user: { NotificationScopeType.USER: { @@ -95,3 +95,22 @@ def test_get_setting_mapping_from_mapping_never(self): ExternalProviders.EMAIL: NotificationSettingOptionValues.ALWAYS, ExternalProviders.SLACK: NotificationSettingOptionValues.NEVER, } + + def test_get_setting_mapping_from_mapping_slack_always(self): + notification_settings = { + self.user: { + NotificationScopeType.USER: { + ExternalProviders.SLACK: NotificationSettingOptionValues.ALWAYS + } + } + } + + mapping = _get_setting_mapping_from_mapping( + notification_settings, + self.user, + NotificationSettingTypes.ISSUE_ALERTS, + ) + assert mapping == { + ExternalProviders.EMAIL: NotificationSettingOptionValues.ALWAYS, + ExternalProviders.SLACK: NotificationSettingOptionValues.ALWAYS, + } From cc6025630359fc7f054acdc25120b2a429f33a5a Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Wed, 3 Nov 2021 16:15:03 -0700 Subject: [PATCH 5/6] fix tests --- .../notifications/notifications/test_organization_request.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/sentry/notifications/notifications/test_organization_request.py b/tests/sentry/notifications/notifications/test_organization_request.py index 582e2381e9591c..e57e34dbb9e337 100644 --- a/tests/sentry/notifications/notifications/test_organization_request.py +++ b/tests/sentry/notifications/notifications/test_organization_request.py @@ -34,7 +34,6 @@ def test_default_to_slack(self): @with_feature("organizations:slack-requests") def test_turn_off_settings(self): - NotificationSetting.objects.update_settings( ExternalProviders.SLACK, NotificationSettingTypes.APPROVAL, @@ -53,6 +52,6 @@ def test_turn_off_settings(self): notification = DummyRequestNotification(self.organization, self.user, member_ids) assert notification.get_participants() == { - ExternalProviders.EMAIL: {self.user2}, - ExternalProviders.SLACK: {self.user1}, + ExternalProviders.EMAIL: {self.user1, self.user2}, + ExternalProviders.SLACK: {self.user1, self.user2}, } From 4dbb0ebb2fce53b3510578812cfb2cbdddeb1b31 Mon Sep 17 00:00:00 2001 From: Marcos Gaeta Date: Thu, 4 Nov 2021 11:02:51 -0700 Subject: [PATCH 6/6] fix tests --- src/sentry/testutils/helpers/slack.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/sentry/testutils/helpers/slack.py b/src/sentry/testutils/helpers/slack.py index d40911c5171040..2ca893435e563c 100644 --- a/src/sentry/testutils/helpers/slack.py +++ b/src/sentry/testutils/helpers/slack.py @@ -92,8 +92,9 @@ def link_team(team: Team, integration: Integration, channel_name: str, channel_i def send_notification(*args): - args_list = list(args)[1:] - send_notification_as_slack(*args_list, {}) + provider, *args_list = args + if provider == ExternalProviders.SLACK: + send_notification_as_slack(*args_list, {}) def get_attachment():