Browse Source

ref(rulesnooze): Model changes (#46907)

Follow up to https://github.com/getsentry/sentry/pull/46846 

In order to support muting rules and alert rules entirely (not just per
user) we'll make the user column nullable and consider that to mean it's
muted for all users who would have received the alert. Initially I was
going to handle this via an `until` column on the rule and alert rule
tables which is why it wasn't in the previous migration, but this is how
we're moving forward.

This also adds a `date_added` column and an `owner_id` column so we can
show who muted the alert and how long it's been muted.
Colleen O'Rourke 1 year ago
parent
commit
1649060fc0

+ 1 - 1
migrations_lockfile.txt

@@ -6,5 +6,5 @@ To resolve this, rebase against latest master and regenerate your migration. Thi
 will then be regenerated, and you should be able to merge without conflicts.
 will then be regenerated, and you should be able to merge without conflicts.
 
 
 nodestore: 0002_nodestore_no_dictfield
 nodestore: 0002_nodestore_no_dictfield
-sentry: 0404_backfill_user_avatars
+sentry: 0405_rulesnooze_user_null
 social_auth: 0001_initial
 social_auth: 0001_initial

+ 76 - 0
src/sentry/migrations/0405_rulesnooze_user_null.py

@@ -0,0 +1,76 @@
+# Generated by Django 2.2.28 on 2023-04-05 21:42
+
+import django.utils.timezone
+from django.db import migrations, models
+
+import sentry.db.models.fields.hybrid_cloud_foreign_key
+from sentry.new_migrations.migrations import CheckedMigration
+
+
+class Migration(CheckedMigration):
+    # This flag is used to mark that a migration shouldn't be automatically run in production. For
+    # the most part, this should only be used for operations where it's safe to run the migration
+    # after your code has deployed. So this should not be used for most operations that alter the
+    # schema of a table.
+    # Here are some things that make sense to mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that they can
+    #   be monitored and not block the deploy for a long period of time while they run.
+    # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to
+    #   have ops run this and not block the deploy. Note that while adding an index is a schema
+    #   change, it's completely safe to run the operation after the code has deployed.
+    is_dangerous = False
+
+    dependencies = [
+        ("sentry", "0404_backfill_user_avatars"),
+    ]
+
+    operations = [
+        migrations.SeparateDatabaseAndState(
+            database_operations=[
+                migrations.RunSQL(
+                    """
+                    ALTER TABLE "sentry_rulesnooze" ADD COLUMN "date_added" timestamp NOT NULL;
+                    """,
+                    reverse_sql="""
+                    ALTER TABLE "sentry_rulesnooze" DROP COLUMN "date_added";
+                    """,
+                    hints={"tables": ["sentry_rulesnooze"]},
+                ),
+            ],
+            state_operations=[
+                migrations.AddField(
+                    model_name="rulesnooze",
+                    name="date_added",
+                    field=models.DateTimeField(default=django.utils.timezone.now),
+                ),
+            ],
+        ),
+        migrations.AddField(
+            model_name="rulesnooze",
+            name="owner_id",
+            field=sentry.db.models.fields.hybrid_cloud_foreign_key.HybridCloudForeignKey(
+                "sentry.User", db_index=True, null=True, on_delete="SET_NULL"
+            ),
+        ),
+        migrations.AlterField(
+            model_name="rulesnooze",
+            name="user_id",
+            field=sentry.db.models.fields.hybrid_cloud_foreign_key.HybridCloudForeignKey(
+                "sentry.User", db_index=True, null=True, on_delete="CASCADE"
+            ),
+        ),
+        migrations.AddConstraint(
+            model_name="rulesnooze",
+            constraint=models.UniqueConstraint(
+                condition=models.Q(user_id__isnull=True), fields=("rule",), name="unique_rule_user"
+            ),
+        ),
+        migrations.AddConstraint(
+            model_name="rulesnooze",
+            constraint=models.UniqueConstraint(
+                condition=models.Q(user_id__isnull=True),
+                fields=("alert_rule",),
+                name="unique_alert_rule_user",
+            ),
+        ),
+    ]

+ 24 - 6
src/sentry/models/rulesnooze.py

@@ -1,5 +1,6 @@
 from django.db import models
 from django.db import models
-from django.db.models import CheckConstraint, Q
+from django.db.models import CheckConstraint, Q, UniqueConstraint
+from django.utils import timezone
 
 
 from sentry.db.models import FlexibleForeignKey, Model, region_silo_only_model, sane_repr
 from sentry.db.models import FlexibleForeignKey, Model, region_silo_only_model, sane_repr
 from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey
 from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey
@@ -8,26 +9,43 @@ from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignK
 @region_silo_only_model
 @region_silo_only_model
 class RuleSnooze(Model):
 class RuleSnooze(Model):
     """
     """
-    Duration an issue alert or metric alert is snoozed for a user. Null `until` value means snoozed forever.
+    Duration an issue alert or metric alert is snoozed for a user.
+    Null `until` value means snoozed forever.
+    Null `user_id` value means snoozed for all users.
     """
     """
 
 
     __include_in_export__ = True
     __include_in_export__ = True
 
 
-    user_id = HybridCloudForeignKey("sentry.User", on_delete="CASCADE")
+    user_id = HybridCloudForeignKey("sentry.User", on_delete="CASCADE", null=True)
+    owner_id = HybridCloudForeignKey("sentry.User", on_delete="SET_NULL", null=True)
     rule = FlexibleForeignKey("sentry.Rule", null=True)
     rule = FlexibleForeignKey("sentry.Rule", null=True)
     alert_rule = FlexibleForeignKey("sentry.AlertRule", null=True)
     alert_rule = FlexibleForeignKey("sentry.AlertRule", null=True)
     until = models.DateTimeField(null=True, db_index=True)
     until = models.DateTimeField(null=True, db_index=True)
+    date_added = models.DateTimeField(default=timezone.now)
 
 
     class Meta:
     class Meta:
         db_table = "sentry_rulesnooze"
         db_table = "sentry_rulesnooze"
         app_label = "sentry"
         app_label = "sentry"
-        unique_together = (("user_id", "rule"), ("user_id", "alert_rule"))
+        unique_together = (
+            ("user_id", "rule"),
+            ("user_id", "alert_rule"),
+        )
         constraints = [
         constraints = [
             CheckConstraint(
             CheckConstraint(
                 check=Q(rule__isnull=False, alert_rule__isnull=True)
                 check=Q(rule__isnull=False, alert_rule__isnull=True)
                 | Q(rule__isnull=True, alert_rule__isnull=False),
                 | Q(rule__isnull=True, alert_rule__isnull=False),
                 name="rule_or_alert_rule",
                 name="rule_or_alert_rule",
-            )
+            ),
+            UniqueConstraint(
+                fields=["rule"],
+                condition=Q(user_id__isnull=True),
+                name="unique_rule_user",
+            ),
+            UniqueConstraint(
+                fields=["alert_rule"],
+                condition=Q(user_id__isnull=True),
+                name="unique_alert_rule_user",
+            ),
         ]
         ]
 
 
-    __repr__ = sane_repr("user_id", "rule_id", "alert_rule_id", "until")
+    __repr__ = sane_repr("user_id", "owner_id", "rule_id", "alert_rule_id", "until", "date_added")

+ 109 - 0
tests/sentry/models/test_rulesnooze.py

@@ -0,0 +1,109 @@
+from datetime import datetime, timedelta
+
+import pytest
+from django.db import IntegrityError, transaction
+
+from sentry.models import Rule, RuleSnooze
+from sentry.testutils import APITestCase
+
+
+class RuleSnoozeTest(APITestCase):
+    def setUp(self):
+        self.issue_alert_rule = Rule.objects.create(
+            label="test rule", project=self.project, owner=self.team.actor
+        )
+        self.metric_alert_rule = self.create_alert_rule(
+            organization=self.project.organization, projects=[self.project]
+        )
+        self.user2 = self.create_user()
+
+    def test_snooze_user_and_global(self):
+        """Test that a rule can be snoozed by a user and globally"""
+        issue_alert_rule_snooze_user = RuleSnooze.objects.create(
+            user_id=self.user.id,
+            owner_id=self.user.id,
+            rule=self.issue_alert_rule,
+            until=datetime.now() + timedelta(days=10),
+        )
+        issue_alert_rule_snooze_all = RuleSnooze.objects.create(
+            owner_id=self.user2.id,
+            rule=self.issue_alert_rule,
+            until=datetime.now() + timedelta(days=1),
+        )
+        assert RuleSnooze.objects.filter(id=issue_alert_rule_snooze_user.id).exists()
+        assert RuleSnooze.objects.filter(id=issue_alert_rule_snooze_all.id).exists()
+
+    def test_issue_alert_until_and_forever(self):
+        issue_alert_rule_snooze_user_until = RuleSnooze.objects.create(
+            user_id=self.user.id,
+            owner_id=self.user.id,
+            rule=self.issue_alert_rule,
+            until=datetime.now() + timedelta(days=1),
+        )
+        assert RuleSnooze.objects.filter(id=issue_alert_rule_snooze_user_until.id).exists()
+
+        issue_alert_rule2 = Rule.objects.create(
+            label="test rule", project=self.project, owner=self.team.actor
+        )
+        issue_alert_rule_snooze_user_forever = RuleSnooze.objects.create(
+            user_id=self.user.id, owner_id=self.user.id, rule=issue_alert_rule2
+        )
+        assert RuleSnooze.objects.filter(id=issue_alert_rule_snooze_user_forever.id).exists()
+
+    def test_metric_alert_until_and_forever(self):
+        metric_alert_rule_snooze_user = RuleSnooze.objects.create(
+            user_id=self.user.id,
+            owner_id=self.user.id,
+            alert_rule=self.metric_alert_rule,
+            until=datetime.now() + timedelta(days=1),
+        )
+        assert RuleSnooze.objects.filter(id=metric_alert_rule_snooze_user.id).exists()
+
+        metric_alert_rule2 = self.create_alert_rule(
+            organization=self.project.organization, projects=[self.project]
+        )
+        metric_alert_rule_snooze_user = RuleSnooze.objects.create(
+            user_id=self.user.id, owner_id=self.user.id, alert_rule=metric_alert_rule2
+        )
+        assert RuleSnooze.objects.filter(id=metric_alert_rule_snooze_user.id).exists()
+
+    def test_constraints(self):
+        # ensure the rule can be globally ignored after it's been individually ignored
+        metric_alert_rule_snooze_all = RuleSnooze.objects.create(alert_rule=self.metric_alert_rule)
+        issue_alert_rule_snooze_all = RuleSnooze.objects.create(rule=self.issue_alert_rule)
+
+        assert RuleSnooze.objects.filter(id=metric_alert_rule_snooze_all.id).exists()
+        assert RuleSnooze.objects.filter(id=issue_alert_rule_snooze_all.id).exists()
+
+        # ensure another user can ignore the same issue alert
+        issue_alert_rule_snooze_user2 = RuleSnooze.objects.create(
+            user_id=self.user2.id, rule=self.issue_alert_rule
+        )
+        assert RuleSnooze.objects.filter(id=issue_alert_rule_snooze_user2.id).exists()
+
+    def test_errors(self):
+        # ensure no dupes
+        RuleSnooze.objects.create(owner_id=self.user.id, alert_rule=self.metric_alert_rule)
+        with pytest.raises(IntegrityError), transaction.atomic():
+            RuleSnooze.objects.create(alert_rule=self.metric_alert_rule)
+
+        RuleSnooze.objects.create(owner_id=self.user.id, rule=self.issue_alert_rule)
+        with pytest.raises(IntegrityError), transaction.atomic():
+            RuleSnooze.objects.create(rule=self.issue_alert_rule)
+
+        # ensure valid data
+        with pytest.raises(IntegrityError), transaction.atomic():
+            RuleSnooze.objects.create(
+                owner_id=self.user.id, rule=self.issue_alert_rule, alert_rule=self.metric_alert_rule
+            )
+
+        with pytest.raises(IntegrityError), transaction.atomic():
+            RuleSnooze.objects.create(
+                user_id=self.user.id,
+                owner_id=self.user.id,
+            )
+
+        with pytest.raises(IntegrityError), transaction.atomic():
+            RuleSnooze.objects.create(
+                owner_id=self.user.id, until=datetime.now() + timedelta(days=1)
+            )