ref(notifications): split notification setting tests and remove deletion code (#60667)

The `NotificationSetting` model was split into
`NotificaitonSettingOption` and `NotificaitonSettingProvider` tables so
this PR splits up the test file.

This also modifies the deletion code to not check `NotificationSetting`
anymore and instead check new tables.

Also fixes a bug where we didn't delete the right settings when an
`ExternalActor` row was removed.

src/sentry/models/integrations/external_actor.py

@@ -73,8 +73,7 @@ class ExternalActor(ReplicatedRegionModel):
                 install = integration.get_installation(organization_id=self.organization.id)
                 team = self.team
                 install.notify_remove_external_team(external_team=self, team=team)
-                # Does the provider need to be an input?
-                notifications_service.remove_notification_settings_for_team(
+                notifications_service.remove_notification_settings_for_provider_team(
                     team_id=team.id, provider=ExternalProviders(self.provider)
                 )
 

src/sentry/services/hybrid_cloud/notifications/impl.py

@@ -4,7 +4,6 @@ from typing import List, Mapping, MutableMapping, Optional, Tuple
 
 from django.db import router, transaction
 
-from sentry.models.notificationsetting import NotificationSetting
 from sentry.models.notificationsettingoption import NotificationSettingOption
 from sentry.models.notificationsettingprovider import NotificationSettingProvider
 from sentry.models.user import User
@@ -18,7 +17,7 @@ from sentry.notifications.types import (
 from sentry.services.hybrid_cloud.actor import ActorType, RpcActor
 from sentry.services.hybrid_cloud.notifications import NotificationsService
 from sentry.services.hybrid_cloud.user.service import user_service
-from sentry.types.integrations import ExternalProviderEnum, ExternalProviders
+from sentry.types.integrations import EXTERNAL_PROVIDERS, ExternalProviderEnum, ExternalProviders
 
 
 class DatabaseBackedNotificationsService(NotificationsService):
@@ -81,34 +80,21 @@ class DatabaseBackedNotificationsService(NotificationsService):
             **kwargs,
         )
 
-    def remove_notification_settings(
-        self, *, team_id: Optional[int], user_id: Optional[int], provider: ExternalProviders
+    def remove_notification_settings_for_provider_team(
+        self, *, team_id: int, provider: ExternalProviders
     ) -> None:
         """
-        Delete notification settings based on an actor_id
-        There is no foreign key relationship so we have to manually cascade.
+        This function removes provider settings if a team has been unlinked from a provider.
         """
-        assert (team_id and not user_id) or (
-            user_id and not team_id
-        ), "Can only remove settings for team or user"
-        team_ids = [team_id] if team_id else None
-        user_ids = [user_id] if user_id else None
-        NotificationSetting.objects._filter(
-            team_ids=team_ids, user_ids=user_ids, provider=provider
+        # skip if not a supported provider with settings
+        if provider not in EXTERNAL_PROVIDERS:
+            return
+        NotificationSettingProvider.objects.filter(
+            team_id=team_id, provider=EXTERNAL_PROVIDERS[provider]
         ).delete()
-        # delete all options for team/user
-        query_args = {"team_id": team_id, "user_id": user_id}
-        NotificationSettingOption.objects.filter(**query_args).delete()
-        NotificationSettingProvider.objects.filter(**query_args).delete()
-
-    def remove_notification_settings_for_team(
-        self, *, team_id: int, provider: ExternalProviders
-    ) -> None:
-        self.remove_notification_settings(team_id=team_id, user_id=None, provider=provider)
 
     def remove_notification_settings_for_organization(self, *, organization_id: int) -> None:
         assert organization_id, "organization_id must be a positive integer"
-        NotificationSetting.objects.remove_for_organization(organization_id=organization_id)
         NotificationSettingOption.objects.filter(
             scope_type=NotificationScopeEnum.ORGANIZATION.value,
             scope_identifier=organization_id,
@@ -119,7 +105,6 @@ class DatabaseBackedNotificationsService(NotificationsService):
         ).delete()
 
     def remove_notification_settings_for_project(self, *, project_id: int) -> None:
-        NotificationSetting.objects.remove_for_project(project_id=project_id)
         NotificationSettingOption.objects.filter(
             scope_type=NotificationScopeEnum.PROJECT.value,
             scope_identifier=project_id,

src/sentry/services/hybrid_cloud/notifications/service.py

@@ -55,7 +55,7 @@ class NotificationsService(RpcService):
 
     @rpc_method
     @abstractmethod
-    def remove_notification_settings_for_team(
+    def remove_notification_settings_for_provider_team(
         self, *, team_id: int, provider: ExternalProviders
     ) -> None:
         pass

tests/sentry/models/test_external_actor.py

@@ -8,7 +8,7 @@ from sentry.testutils.silo import region_silo_test
 class ExternalActorTest(TestCase):
     def setUp(self) -> None:
         actor = Actor.objects.create(type=ACTOR_TYPES["team"])
-        org = self.create_organization()
+        org = self.create_organization(owner=self.user)
         team = self.create_team(organization=org, actor=actor)
 
         integrations = [

tests/sentry/models/test_notificationsetting.py

@@ -1,108 +0,0 @@
-from sentry.models.notificationsetting import NotificationSetting
-from sentry.models.notificationsettingoption import NotificationSettingOption
-from sentry.models.notificationsettingprovider import NotificationSettingProvider
-from sentry.models.user import User
-from sentry.notifications.types import NotificationSettingOptionValues, NotificationSettingTypes
-from sentry.silo import SiloMode
-from sentry.tasks.deletion.hybrid_cloud import schedule_hybrid_cloud_foreign_key_jobs_control
-from sentry.testutils.cases import TestCase
-from sentry.testutils.outbox import outbox_runner
-from sentry.testutils.silo import assume_test_silo_mode, control_silo_test
-from sentry.types.integrations import ExternalProviders
-
-
-def _get_kwargs(kwargs):
-    return dict(
-        provider=ExternalProviders.EMAIL, type=NotificationSettingTypes.ISSUE_ALERTS, **kwargs
-    )
-
-
-def assert_no_notification_settings():
-    assert NotificationSetting.objects.all().count() == 0
-    assert NotificationSettingOption.objects.all().count() == 0
-    assert NotificationSettingProvider.objects.all().count() == 0
-
-
-def create_setting(**kwargs):
-    NotificationSetting.objects.update_settings(
-        value=NotificationSettingOptionValues.ALWAYS,
-        **_get_kwargs(kwargs),
-    )
-
-
-@control_silo_test
-class NotificationSettingTest(TestCase):
-    def test_remove_for_user(self):
-        create_setting(user_id=self.user.id)
-
-        # Refresh user for actor
-        self.user = User.objects.get(id=self.user.id)
-
-        # Deletion is deferred and tasks aren't run in tests.
-        with outbox_runner():
-            self.user.delete()
-
-        assert_no_notification_settings()
-
-    def test_remove_for_team(self):
-        create_setting(
-            team_id=self.team.id,
-            project=self.project,
-        )
-
-        # Deletion is deferred and tasks aren't run in tests.
-        with assume_test_silo_mode(SiloMode.REGION), outbox_runner():
-            self.team.delete()
-
-        with self.tasks():
-            schedule_hybrid_cloud_foreign_key_jobs_control()
-
-        assert_no_notification_settings()
-
-    def test_remove_for_project(self):
-        create_setting(
-            user_id=self.user.id,
-            project=self.project,
-        )
-        with assume_test_silo_mode(SiloMode.REGION):
-            self.project.delete()
-        assert_no_notification_settings()
-
-    def test_remove_for_organization(self):
-        create_setting(
-            user_id=self.user.id,
-            organization=self.organization,
-        )
-        with assume_test_silo_mode(SiloMode.REGION), outbox_runner():
-            self.organization.delete()
-        assert_no_notification_settings()
-
-    def test_user_id(self):
-        NotificationSetting.objects.update_settings(
-            ExternalProviders.EMAIL,
-            NotificationSettingTypes.ISSUE_ALERTS,
-            NotificationSettingOptionValues.ALWAYS,
-            user_id=self.user.id,
-        )
-        ns = NotificationSetting.objects.find_settings(
-            provider=ExternalProviders.EMAIL,
-            type=NotificationSettingTypes.ISSUE_ALERTS,
-            user_id=self.user.id,
-        )[0]
-        assert ns.user_id == self.user.id
-        assert ns.team_id is None
-
-    def test_team_id(self):
-        NotificationSetting.objects.update_settings(
-            ExternalProviders.EMAIL,
-            NotificationSettingTypes.ISSUE_ALERTS,
-            NotificationSettingOptionValues.ALWAYS,
-            team_id=self.team.id,
-        )
-        ns = NotificationSetting.objects.find_settings(
-            provider=ExternalProviders.EMAIL,
-            type=NotificationSettingTypes.ISSUE_ALERTS,
-            team_id=self.team.id,
-        )[0]
-        assert ns.team_id == self.team.id
-        assert ns.user_id is None

tests/sentry/models/test_notificationsettingoption.py

@@ -0,0 +1,75 @@
+from sentry.models.notificationsettingoption import NotificationSettingOption
+from sentry.models.user import User
+from sentry.silo import SiloMode
+from sentry.tasks.deletion.hybrid_cloud import schedule_hybrid_cloud_foreign_key_jobs_control
+from sentry.testutils.cases import TestCase
+from sentry.testutils.outbox import outbox_runner
+from sentry.testutils.silo import assume_test_silo_mode, control_silo_test
+
+
+def assert_no_notification_settings():
+    assert NotificationSettingOption.objects.all().count() == 0
+
+
+@control_silo_test
+class NotificationSettingTest(TestCase):
+    def test_remove_for_user(self):
+        NotificationSettingOption.objects.create(
+            user_id=self.user.id,
+            scope_type="user",
+            scope_identifier=self.user.id,
+            type="alerts",
+            value="never",
+        )
+
+        # Refresh user for actor
+        self.user = User.objects.get(id=self.user.id)
+
+        # Deletion is deferred and tasks aren't run in tests.
+        with outbox_runner():
+            self.user.delete()
+
+        assert_no_notification_settings()
+
+    def test_remove_for_team(self):
+        NotificationSettingOption.objects.create(
+            team_id=self.team.id,
+            scope_type="team",
+            scope_identifier=self.team.id,
+            type="alerts",
+            value="never",
+        )
+
+        # Deletion is deferred and tasks aren't run in tests.
+        with assume_test_silo_mode(SiloMode.REGION), outbox_runner():
+            self.team.delete()
+
+        with self.tasks():
+            schedule_hybrid_cloud_foreign_key_jobs_control()
+
+        assert_no_notification_settings()
+
+    def test_remove_for_project(self):
+        NotificationSettingOption.objects.create(
+            user_id=self.user.id,
+            scope_type="project",
+            scope_identifier=self.project.id,
+            type="alerts",
+            value="never",
+        )
+
+        with assume_test_silo_mode(SiloMode.REGION):
+            self.project.delete()
+        assert_no_notification_settings()
+
+    def test_remove_for_organization(self):
+        NotificationSettingOption.objects.create(
+            user_id=self.user.id,
+            scope_type="organization",
+            scope_identifier=self.organization.id,
+            type="alerts",
+            value="never",
+        )
+        with assume_test_silo_mode(SiloMode.REGION), outbox_runner():
+            self.organization.delete()
+        assert_no_notification_settings()

tests/sentry/models/test_notificationsettingprovider.py

@@ -0,0 +1,79 @@
+from sentry.models.notificationsettingprovider import NotificationSettingProvider
+from sentry.models.user import User
+from sentry.silo import SiloMode
+from sentry.tasks.deletion.hybrid_cloud import schedule_hybrid_cloud_foreign_key_jobs_control
+from sentry.testutils.cases import TestCase
+from sentry.testutils.outbox import outbox_runner
+from sentry.testutils.silo import assume_test_silo_mode, control_silo_test
+
+
+def assert_no_notification_settings():
+    assert NotificationSettingProvider.objects.all().count() == 0
+
+
+@control_silo_test
+class NotificationSettingTest(TestCase):
+    def test_remove_for_user(self):
+        NotificationSettingProvider.objects.create(
+            user_id=self.user.id,
+            scope_type="user",
+            scope_identifier=self.user.id,
+            type="alerts",
+            value="never",
+            provider="slack",
+        )
+
+        # Refresh user for actor
+        self.user = User.objects.get(id=self.user.id)
+
+        # Deletion is deferred and tasks aren't run in tests.
+        with outbox_runner():
+            self.user.delete()
+
+        assert_no_notification_settings()
+
+    def test_remove_for_team(self):
+        NotificationSettingProvider.objects.create(
+            team_id=self.team.id,
+            scope_type="team",
+            scope_identifier=self.team.id,
+            type="alerts",
+            value="never",
+            provider="slack",
+        )
+
+        # Deletion is deferred and tasks aren't run in tests.
+        with assume_test_silo_mode(SiloMode.REGION), outbox_runner():
+            self.team.delete()
+
+        with self.tasks():
+            schedule_hybrid_cloud_foreign_key_jobs_control()
+
+        assert_no_notification_settings()
+
+    def test_remove_for_project(self):
+        NotificationSettingProvider.objects.create(
+            user_id=self.user.id,
+            scope_type="project",
+            scope_identifier=self.project.id,
+            type="alerts",
+            value="never",
+            provider="slack",
+        )
+
+        with assume_test_silo_mode(SiloMode.REGION):
+            self.project.delete()
+        assert_no_notification_settings()
+
+    def test_remove_for_organization(self):
+        NotificationSettingProvider.objects.create(
+            user_id=self.user.id,
+            scope_type="organization",
+            scope_identifier=self.organization.id,
+            type="alerts",
+            value="never",
+            provider="slack",
+        )
+        with assume_test_silo_mode(SiloMode.REGION), outbox_runner():
+            self.organization.delete()
+        assert_no_notification_settings()

tests/sentry/models/test_organization.py

@@ -15,17 +15,13 @@ from sentry.api.endpoints.organization_details import (
 from sentry.auth.authenticators.totp import TotpInterface
 from sentry.models.apikey import ApiKey
 from sentry.models.auditlogentry import AuditLogEntry
-from sentry.models.notificationsetting import NotificationSetting
+from sentry.models.notificationsettingoption import NotificationSettingOption
+from sentry.models.notificationsettingprovider import NotificationSettingProvider
 from sentry.models.options.organization_option import OrganizationOption
 from sentry.models.options.user_option import UserOption
 from sentry.models.organization import Organization
 from sentry.models.organizationmember import OrganizationMember
 from sentry.models.user import User
-from sentry.notifications.types import (
-    NotificationScopeType,
-    NotificationSettingOptionValues,
-    NotificationSettingTypes,
-)
 from sentry.silo import SiloMode
 from sentry.tasks.deletion.hybrid_cloud import (
     schedule_hybrid_cloud_foreign_key_jobs,
@@ -36,7 +32,6 @@ from sentry.testutils.helpers.features import with_feature
 from sentry.testutils.hybrid_cloud import HybridCloudTestMixin
 from sentry.testutils.outbox import outbox_runner
 from sentry.testutils.silo import assume_test_silo_mode, region_silo_test
-from sentry.types.integrations import ExternalProviders
 
 
 @region_silo_test
@@ -433,19 +428,15 @@ class Require2fa(TestCase, HybridCloudTestMixin):
 class OrganizationDeletionTest(TestCase):
     def add_org_notification_settings(self, org: Organization, user: User):
         with assume_test_silo_mode(SiloMode.CONTROL):
-            NotificationSetting.objects.create(
-                scope_type=NotificationScopeType.ORGANIZATION.value,
-                target_id=user.id,
-                provider=ExternalProviders.EMAIL.value,
-                type=NotificationSettingTypes.DEPLOY.value,
-                scope_identifier=org.id,
-                user=user,
-                value=NotificationSettingOptionValues.NEVER.value,
-            )
-
-            assert NotificationSetting.objects.filter(
-                scope_type=NotificationScopeType.ORGANIZATION.value, scope_identifier=org.id
-            ).exists()
+            args = {
+                "scope_type": "organization",
+                "scope_identifier": org.id,
+                "type": "deploy",
+                "user_id": user.id,
+                "value": "never",
+            }
+            NotificationSettingOption.objects.create(**args)
+            NotificationSettingProvider.objects.create(**args, provider="slack")
 
     def test_hybrid_cloud_deletion(self):
         org = self.create_organization()
@@ -480,11 +471,12 @@ class OrganizationDeletionTest(TestCase):
             # Ensure they are all now gone.
             assert not UserOption.objects.filter(organization_id=org_id).exists()
 
-            assert NotificationSetting.objects.filter(
-                scope_type=NotificationScopeType.ORGANIZATION.value,
+            assert NotificationSettingOption.objects.filter(
+                scope_type="organization",
                 scope_identifier=unaffected_org.id,
             ).exists()
 
-            assert not NotificationSetting.objects.filter(
-                scope_type=NotificationScopeType.ORGANIZATION.value, scope_identifier=org_id
+            assert not NotificationSettingOption.objects.filter(
+                scope_type="organization",
+                scope_identifier=org_id,
             ).exists()