Browse Source

fixes bug where we pass in projects for deploy notifications (#58280)

Fixes SENTRY-16VX

Note that if we ever made project level release settings, we'll need to
re-architect this to check project settings.
Stephen Cefali 1 year ago
parent
commit
bfe139eee9

+ 2 - 3
src/sentry/notifications/notificationcontroller.py

@@ -171,9 +171,8 @@ class NotificationController:
         Args:
             setting_type: If specified, only return settings of this type.
         """
-
-        if self.project_ids and len(list(self.project_ids)) > 2 and not project_id:
-            raise Exception("Must specify project_id if controller has more than 2 projects")
+        if self.project_ids and len(list(self.project_ids)) > 1 and not project_id:
+            raise Exception("Must specify project_id if controller has more than 1 projects")
 
         most_specific_setting_options: MutableMapping[
             Recipient, MutableMapping[NotificationSettingEnum, NotificationSettingsOptionEnum]

+ 2 - 2
src/sentry/notifications/utils/participants.py

@@ -214,11 +214,11 @@ def get_participants_for_release(
 
     actors = RpcActor.many_from_object(RpcUser(id=user_id) for user_id in user_ids)
     if should_use_notifications_v2(organization):
+        # don't pass in projects since the settings are scoped to the organization only for now
         providers_by_recipient = notifications_service.get_participants(
+            type=NotificationSettingEnum.DEPLOY,
             recipients=actors,
-            project_ids=[project.id for project in projects],
             organization_id=organization.id,
-            type=NotificationSettingEnum.DEPLOY,
         )
 
         users_to_reasons_by_provider = ParticipantMap()

+ 2 - 2
src/sentry/services/hybrid_cloud/notifications/impl.py

@@ -247,9 +247,9 @@ class DatabaseBackedNotificationsService(NotificationsService):
         self,
         *,
         recipients: List[RpcActor],
-        project_ids: Optional[List[int]],
-        organization_id: Optional[int],
         type: NotificationSettingEnum,
+        project_ids: Optional[List[int]] = None,
+        organization_id: Optional[int] = None,
     ) -> MutableMapping[
         int, MutableMapping[int, str]
     ]:  # { actor_id : { provider_str: value_str } }

+ 2 - 2
src/sentry/services/hybrid_cloud/notifications/service.py

@@ -151,9 +151,9 @@ class NotificationsService(RpcService):
         self,
         *,
         recipients: List[RpcActor],
-        project_ids: Optional[List[int]],
-        organization_id: Optional[int],
         type: NotificationSettingEnum,
+        project_ids: Optional[List[int]] = None,
+        organization_id: Optional[int] = None,
     ) -> MutableMapping[int, MutableMapping[int, str]]:
         pass
 

+ 63 - 2
tests/sentry/integrations/slack/notifications/test_deploy.py

@@ -1,4 +1,4 @@
-from unittest import mock, skip
+from unittest import mock
 
 import responses
 from django.utils import timezone
@@ -8,6 +8,7 @@ from sentry.models.deploy import Deploy
 from sentry.models.release import Release
 from sentry.notifications.notifications.activity.release import ReleaseActivityNotification
 from sentry.testutils.cases import SlackActivityNotificationTest
+from sentry.testutils.helpers.features import with_feature
 from sentry.testutils.helpers.slack import get_attachment, send_notification
 from sentry.testutils.silo import region_silo_test
 from sentry.types.activity import ActivityType
@@ -17,7 +18,6 @@ from sentry.types.activity import ActivityType
 class SlackDeployNotificationTest(SlackActivityNotificationTest):
     @responses.activate
     @mock.patch("sentry.notifications.notify.notify", side_effect=send_notification)
-    @skip("Test is flaky")
     def test_deploy(self, mock_func):
         """
         Test that a Slack message is sent with the expected payload when a deploy happens.
@@ -75,3 +75,64 @@ class SlackDeployNotificationTest(SlackActivityNotificationTest):
             == f"{first_project.slug} | <http://testserver/settings/account/notifications/"
             f"deploy/?referrer=release_activity-slack-user&notification_uuid={notification_uuid}|Notification Settings>"
         )
+
+    @responses.activate
+    @mock.patch("sentry.notifications.notify.notify", side_effect=send_notification)
+    @with_feature("organizations:notification-settings-v2")
+    def test_deploy_v2(self, mock_func):
+        """
+        Test that a Slack message is sent with the expected payload when a deploy happens.
+        """
+        release = Release.objects.create(
+            version="meow" * 10,
+            organization_id=self.project.organization_id,
+            date_released=timezone.now(),
+        )
+
+        # The projects can appear out of order.
+        projects = (self.project, self.create_project(name="battlesnake"))
+        SLUGS_TO_PROJECT = {project.slug: project for project in projects}
+
+        for project in projects:
+            release.add_project(project)
+
+        deploy = Deploy.objects.create(
+            release=release,
+            organization_id=self.organization.id,
+            environment_id=self.environment.id,
+        )
+        notification = ReleaseActivityNotification(
+            Activity(
+                project=self.project,
+                user_id=self.user.id,
+                type=ActivityType.RELEASE.value,
+                data={"version": release.version, "deploy_id": deploy.id},
+            )
+        )
+        with self.tasks():
+            notification.send()
+
+        attachment, text = get_attachment()
+        assert (
+            text
+            == f"Release {release.version} was deployed to {self.environment.name} for these projects"
+        )
+
+        first_project = None
+        notification_uuid = self.get_notification_uuid(attachment["actions"][0]["url"])
+        for i in range(len(projects)):
+            project = SLUGS_TO_PROJECT[attachment["actions"][i]["text"]]
+            if not first_project:
+                first_project = project
+            assert (
+                attachment["actions"][i]["url"]
+                == f"http://testserver/organizations/{self.organization.slug}/releases/"
+                f"{release.version}/?project={project.id}&unselectedSeries=Healthy&referrer=release_activity&notification_uuid={notification_uuid}"
+            )
+        assert first_project is not None
+
+        assert (
+            attachment["footer"]
+            == f"{first_project.slug} | <http://testserver/settings/account/notifications/"
+            f"deploy/?referrer=release_activity-slack-user&notification_uuid={notification_uuid}|Notification Settings>"
+        )