feat(mail): Implement migration for moving all projects to use issue alert targeting. (#18330)

This implements the migration for converting all projects to use issue alert targeting. We copy
the function directly into the migration, and it is exactly the same other than how we import
models.

While testing this locally I ran into a few migration related bugs, and realised some other things
that need to change.  These are:
 - Stop importing plugins - This would have broken once we entirely remove the mail plugin, which
   would cause problems for on-prem and maybe single tenant. Instead, just fetch the option
   directly from `ProjectOption`.
 - Update user options directly rather than relying on manager, since manager is not available.
 - Have the `mail:alert` user option be an int rather than a string. Love our consistency
 - Update flags on the field directly rather than using `Model.update`, which isn't available in
   migrations.

The migration is idempotent - if it stops part way through it's safe to run again, since each
project is migrated entirely in a transaction, and once finished we set a flag to True. So running
again will mean we just skip already migrated projects.

If any errors occur the migration will continue to run and just log exceptions to sentry. We can
grab this log output and investigate if anything does go wrong.

migrations_lockfile.txt

@@ -10,7 +10,7 @@ auth: 0008_alter_user_username_max_length
 contenttypes: 0002_remove_content_type_name
 jira_ac: 0001_initial
 nodestore: 0001_initial
-sentry: 0066_alertrule_manager
+sentry: 0067_migrate_rules_alert_targeting
 sessions: 0001_initial
 sites: 0002_alter_domain_unique
 social_auth: 0001_initial

src/sentry/mail/utils.py

@@ -1,13 +1,11 @@
 from __future__ import absolute_import
 
 from django.db import transaction
-from django.db.models import F
 
-from sentry.models.project import Project
+from sentry.models.projectoption import ProjectOption
 from sentry.models.rule import Rule, RuleStatus
 from sentry.models.user import User
 from sentry.models.useroption import UserOption
-from sentry.plugins.base import plugins
 
 mail_action = {
     "id": "sentry.mail.actions.NotifyEmailAction",
@@ -16,15 +14,24 @@ mail_action = {
 }
 
 
+def set_user_option(UserOption, user, key, value, project):
+    inst, created = UserOption.objects.get_or_create(
+        user=user, project=project, key=key, defaults={"value": value}
+    )
+    if not created and inst.value != value:
+        inst.update(value=value)
+
+
 def migrate_project_to_issue_alert_targeting(project):
     if project.flags.has_issue_alerts_targeting:
         # Migration has already been run.
         return
     with transaction.atomic():
         # Determine whether this project actually has mail enabled
-        mail_enabled = any(
-            plugin for plugin in plugins.for_project(project, version=None) if plugin.slug == "mail"
-        )
+        try:
+            mail_enabled = ProjectOption.objects.get(project=project, key="mail:enabled").value
+        except ProjectOption.DoesNotExist:
+            mail_enabled = True
         for rule in Rule.objects.filter(project=project, status=RuleStatus.ACTIVE):
             migrate_legacy_rule(rule, mail_enabled)
 
@@ -35,11 +42,12 @@ def migrate_project_to_issue_alert_targeting(project):
             for user in User.objects.filter(
                 sentry_orgmember_set__teams__in=project.teams.all(), is_active=True
             ):
-                UserOption.objects.set_value(user, "mail:alert", "0", project=project)
-                UserOption.objects.set_value(user, "workflow:notifications", "0", project=project)
+                set_user_option(UserOption, user, "mail:alert", 0, project)
+                set_user_option(UserOption, user, "workflow:notifications", "0", project=project)
 
         # This marks the migration finished and shows the new UI
-        project.update(flags=F("flags").bitor(Project.flags.has_issue_alerts_targeting))
+        project.flags.has_issue_alerts_targeting = True
+        project.save()
 
 
 def migrate_legacy_rule(rule, mail_enabled):

src/sentry/migrations/0067_migrate_rules_alert_targeting.py

@@ -0,0 +1,137 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.29 on 2020-04-20 20:53
+from __future__ import unicode_literals
+
+import logging
+
+from django.db import migrations, transaction
+
+from sentry.utils.query import RangeQuerySetWrapperWithProgressBar
+
+mail_action = {
+    "id": "sentry.mail.actions.NotifyEmailAction",
+    "targetType": "IssueOwners",
+    "targetIdentifier": "None",
+}
+
+
+def set_user_option(UserOption, user, key, value, project):
+    inst, created = UserOption.objects.get_or_create(
+        user=user, project=project, key=key, defaults={"value": value}
+    )
+    if not created and inst.value != value:
+        inst.update(value=value)
+
+
+def migrate_project_to_issue_alert_targeting(project, ProjectOption, Rule, User, UserOption):
+    if project.flags.has_issue_alerts_targeting:
+        # Migration has already been run.
+        return
+    with transaction.atomic():
+        # Determine whether this project actually has mail enabled
+        try:
+            mail_enabled = ProjectOption.objects.get(project=project, key="mail:enabled").value
+        except ProjectOption.DoesNotExist:
+            mail_enabled = True
+        for rule in Rule.objects.filter(project=project, status=0):
+            migrate_legacy_rule(rule, mail_enabled)
+
+        if not mail_enabled:
+            # If mail disabled, then we want to disable mail options for all
+            # users associated with this project so that they don't suddenly start
+            # getting mail via the `MailAdapter`, since it will always be enabled.
+            for user in User.objects.filter(
+                sentry_orgmember_set__teams__in=project.teams.all(), is_active=True
+            ):
+                set_user_option(UserOption, user, "mail:alert", 0, project)
+                set_user_option(UserOption, user, "workflow:notifications", "0", project=project)
+
+        # This marks the migration finished and shows the new UI
+        project.flags.has_issue_alerts_targeting = True
+        project.save()
+
+
+def migrate_legacy_rule(rule, mail_enabled):
+    actions = rule.data.get("actions", [])
+    new_actions = []
+    has_mail_action = False
+    for action in actions:
+        action_id = action.get("id")
+        if action_id == "sentry.rules.actions.notify_event.NotifyEventAction":
+            # This is the "Send a notification (for all legacy integrations)" action.
+            # When this action exists, we want to add the new `NotifyEmailAction` action
+            # to the rule. We'll still leave `NotifyEventAction` in place, since it will
+            # only notify non-mail plugins once we've migrated.
+            new_actions.append(action)
+            has_mail_action = True
+        elif (
+            action_id == "sentry.rules.actions.notify_event_service.NotifyEventServiceAction"
+            and action.get("service") == "mail"
+        ):
+            # This is the "Send a notification via mail" action. When this action
+            # exists, we want to add the new `NotifyEmailAction` action to the rule.
+            # We'll drop this action from the rule, since all it does it send mail and
+            # we don't want to double up.
+            has_mail_action = True
+        else:
+            new_actions.append(action)
+
+    # We only add the new action if the mail plugin is actually enabled, and there's an
+    # action that sends by mail. We do this outside the loop to ensure we don't add it
+    # more than once.
+    if mail_enabled and has_mail_action:
+        new_actions.append(mail_action)
+
+    if actions != new_actions:
+        rule.data["actions"] = new_actions
+        rule.save()
+
+
+def migrate_to_issue_alert_targeting(apps, schema_editor):
+    Project = apps.get_model("sentry", "Project")
+    ProjectOption = apps.get_model("sentry", "ProjectOption")
+    Organization = apps.get_model("sentry", "Organization")
+    Rule = apps.get_model("sentry", "Rule")
+    User = apps.get_model("sentry", "User")
+    UserOption = apps.get_model("sentry", "UserOption")
+
+    for org in RangeQuerySetWrapperWithProgressBar(Organization.objects.filter(status=0)):
+        # We migrate a project at a time, but we prefer to group by org so that for the
+        # most part an org will see the changes all at once.
+        for project in Project.objects.filter(organization=org, status=0):
+            try:
+                migrate_project_to_issue_alert_targeting(
+                    project, ProjectOption, Rule, User, UserOption
+                )
+            except Exception:
+                # If a project fails we'll just log and continue. We shouldn't see any
+                # failures, but if we do we can analyze them and re-run this migration,
+                # since it is idempotent.
+                logging.exception("Error migrating project {}".format(project.id))
+
+
+class Migration(migrations.Migration):
+    # This flag is used to mark that a migration shouldn't be automatically run in
+    # production. We set this to True for operations that we think are risky and want
+    # someone from ops to run manually and monitor.
+    # General advice is that if in doubt, mark your migration as `is_dangerous`.
+    # Some things you should always mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that
+    #   they can be monitored. Since data migrations will now hold a transaction open
+    #   this is even more important.
+    # - Adding columns to highly active tables, even ones that are NULL.
+    is_dangerous = True
+
+    # This flag is used to decide whether to run this migration in a transaction or not.
+    # By default we prefer to run in a transaction, but for migrations where you want
+    # to `CREATE INDEX CONCURRENTLY` this needs to be set to False. Typically you'll
+    # want to create an index concurrently when adding one to an existing table.
+    atomic = False
+
+    dependencies = [("sentry", "0066_alertrule_manager")]
+
+    operations = [
+        migrations.RunPython(
+            migrate_to_issue_alert_targeting, reverse_code=migrations.RunPython.noop
+        )
+    ]

tests/sentry/mail/test_utils.py

@@ -86,7 +86,7 @@ class MigrateProjectToIssueAlertTargetingTest(TestCase):
         migrate_project_to_issue_alert_targeting(self.project)
         assert not Rule.objects.filter(project=self.project).exists()
         for user in (self.user, self.user_2):
-            assert UserOption.objects.get_value(user, "mail:alert", project=self.project) == "0"
+            assert UserOption.objects.get_value(user, "mail:alert", project=self.project) == 0
             assert (
                 UserOption.objects.get_value(user, "workflow:notifications", project=self.project)
                 == "0"
@@ -121,7 +121,7 @@ class MigrateProjectToIssueAlertTargetingTest(TestCase):
             ],
         )
         for user in (self.user, self.user_2):
-            assert UserOption.objects.get_value(user, "mail:alert", project=self.project) == "0"
+            assert UserOption.objects.get_value(user, "mail:alert", project=self.project) == 0
             assert (
                 UserOption.objects.get_value(user, "workflow:notifications", project=self.project)
                 == "0"