Browse Source

ref: fix alternatives email typing in tests (#51708)

<!-- Describe your PR here. -->
anthony sottile 1 year ago
parent
commit
357c8ebb65

+ 0 - 7
pyproject.toml

@@ -703,7 +703,6 @@ module = [
     "sentry.migrations.0407_recreate_perf_alert_subscriptions",
     "sentry.migrations.0418_add_actor_constraints",
     "sentry.models",
-    "sentry.models.activity",
     "sentry.models.actor",
     "sentry.models.apiapplication",
     "sentry.models.artifactbundle",
@@ -1318,7 +1317,6 @@ module = [
     "tests.sentry.integrations.jira_server",
     "tests.sentry.integrations.jira_server.test_integration",
     "tests.sentry.integrations.jira_server.test_webhooks",
-    "tests.sentry.integrations.msteams.notifications.test_assigned",
     "tests.sentry.integrations.msteams.notifications.test_deploy",
     "tests.sentry.integrations.msteams.notifications.test_escalating",
     "tests.sentry.integrations.msteams.notifications.test_note",
@@ -1383,7 +1381,6 @@ module = [
     "tests.sentry.mail.activity.test_note",
     "tests.sentry.mail.activity.test_release",
     "tests.sentry.mail.test_actions",
-    "tests.sentry.mail.test_adapter",
     "tests.sentry.manager.test_group_manager",
     "tests.sentry.manager.test_projectteam_manager",
     "tests.sentry.manager.test_team_manager",
@@ -1428,9 +1425,7 @@ module = [
     "tests.sentry.monitors.test_models",
     "tests.sentry.nodestore.bigtable.test_backend",
     "tests.sentry.notifications.notifications.organization_request.test_integration_request",
-    "tests.sentry.notifications.notifications.test_assigned",
     "tests.sentry.notifications.notifications.test_digests",
-    "tests.sentry.notifications.test_notifications",
     "tests.sentry.notifications.test_utils",
     "tests.sentry.notifications.utils.test_participants",
     "tests.sentry.notifications.utils.test_tasks",
@@ -1525,7 +1520,6 @@ module = [
     "tests.sentry.tasks.test_servicehooks",
     "tests.sentry.tasks.test_store",
     "tests.sentry.tasks.test_update_code_owners_schema",
-    "tests.sentry.tasks.test_weekly_reports",
     "tests.sentry.templatetags.test_sentry_assets",
     "tests.sentry.test_constants",
     "tests.sentry.test_killswitches",
@@ -1533,7 +1527,6 @@ module = [
     "tests.sentry.testutils.helpers.test_features",
     "tests.sentry.tsdb.test_redissnuba",
     "tests.sentry.utils.email.test_list_resolver",
-    "tests.sentry.utils.email.test_message_builder",
     "tests.sentry.utils.kvstore.test_common",
     "tests.sentry.utils.locking.backends.test_redis",
     "tests.sentry.utils.suspect_resolutions.test_metric_correlation",

+ 3 - 3
src/sentry/models/activity.py

@@ -67,7 +67,7 @@ class ActivityManager(BaseManager):
         send_notification: bool = True,
     ) -> Activity:
         if user:
-            user_id = user.id
+            user_id = user.id  # type: ignore[assignment]
         activity_args = {
             "project_id": group.project_id,
             "group": group,
@@ -90,12 +90,12 @@ class Activity(Model):
     project = FlexibleForeignKey("sentry.Project")
     group = FlexibleForeignKey("sentry.Group", null=True)
     # index on (type, ident)
-    type = BoundedPositiveIntegerField(choices=CHOICES)
+    type: models.Field[int | ActivityType, int] = BoundedPositiveIntegerField(choices=CHOICES)
     ident = models.CharField(max_length=64, null=True)
     # if the user is not set, it's assumed to be the system
     user_id = HybridCloudForeignKey(settings.AUTH_USER_MODEL, null=True, on_delete="SET_NULL")
     datetime = models.DateTimeField(default=timezone.now)
-    data = GzippedDictField(null=True)
+    data: models.Field[dict[str, Any], dict[str, Any]] = GzippedDictField(null=True)
 
     objects = ActivityManager()
 

+ 3 - 4
src/sentry/notifications/notifications/activity/assigned.py

@@ -10,20 +10,19 @@ from ...utils.participants import ParticipantMap
 from .base import GroupActivityNotification
 
 
-def _get_user_option(assignee_id: int) -> User | None:
+def _get_user_option(assignee_id: int | None) -> User | None:
     try:
         return User.objects.get_from_cache(id=assignee_id)
     except User.DoesNotExist:
         return None
 
 
-def _get_team_option(assignee_id: int, organization: Organization) -> Team | None:
+def _get_team_option(assignee_id: int | None, organization: Organization) -> Team | None:
     return Team.objects.filter(id=assignee_id, organization=organization).first()
 
 
 def is_team_assignee(activity: Activity) -> bool:
-    assignee_type: str | None = activity.data.get("assigneeType")
-    return assignee_type == "team"
+    return activity.data.get("assigneeType") == "team"
 
 
 def get_assignee_str(activity: Activity, organization: Organization) -> str:

+ 1 - 1
tests/sentry/integrations/msteams/notifications/test_assigned.py

@@ -1,7 +1,7 @@
 from unittest.mock import MagicMock, Mock, patch
 
 from sentry.models import Activity
-from sentry.notifications.notifications.activity import AssignedActivityNotification
+from sentry.notifications.notifications.activity.assigned import AssignedActivityNotification
 from sentry.testutils.cases import MSTeamsActivityNotificationTest
 from sentry.types.activity import ActivityType
 

+ 32 - 8
tests/sentry/mail/test_adapter.py

@@ -8,6 +8,7 @@ from unittest import mock
 import pytz
 from django.contrib.auth.models import AnonymousUser
 from django.core import mail
+from django.core.mail.message import EmailMultiAlternatives
 from django.db.models import F
 from django.utils import timezone
 
@@ -183,7 +184,9 @@ class MailAdapterNotifyTest(BaseMailAdapterTest):
             self.adapter.notify(notification, ActionTargetType.ISSUE_OWNERS)
 
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
         assert msg.subject == "[Sentry] BAR-1 - Hello world"
+        assert isinstance(msg.alternatives[0][0], str)
         assert "my rule" in msg.alternatives[0][0]
 
     def test_simple_snooze(self):
@@ -260,13 +263,13 @@ class MailAdapterNotifyTest(BaseMailAdapterTest):
 
     def test_simple_notification_generic(self):
         """Test that an issue that is neither error nor performance type renders a generic email template"""
-        event = self.store_event(
+        orig_event = self.store_event(
             data={"message": "Hello world", "level": "error"}, project_id=self.project.id
         )
-        event = event.for_group(event.groups[0])
+        event = orig_event.for_group(orig_event.groups[0])
         occurrence = IssueOccurrence(
-            self.project.id,
             uuid.uuid4().hex,
+            self.project.id,
             uuid.uuid4().hex,
             ["some-fingerprint"],
             "something bad happened",
@@ -297,6 +300,7 @@ class MailAdapterNotifyTest(BaseMailAdapterTest):
             self.adapter.notify(notification, ActionTargetType.ISSUE_OWNERS)
 
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
         assert msg.subject == f"[Sentry] BAR-1 - {occurrence.issue_title}"
         checked_values = [
             "Issue Data",
@@ -308,16 +312,17 @@ class MailAdapterNotifyTest(BaseMailAdapterTest):
             "Value 3",
         ]
         for checked_value in checked_values:
+            assert isinstance(msg.alternatives[0][0], str)
             assert (
                 checked_value in msg.alternatives[0][0]
             ), f"{checked_value} not present in message"
 
     def test_simple_notification_generic_no_evidence(self):
         """Test that an issue with no evidence that is neither error nor performance type renders a generic email template"""
-        event = self.store_event(
+        orig_event = self.store_event(
             data={"message": "Hello world", "level": "error"}, project_id=self.project.id
         )
-        event = event.for_group(event.groups[0])
+        event = orig_event.for_group(orig_event.groups[0])
         occurrence = IssueOccurrence(
             uuid.uuid4().hex,
             self.project.id,
@@ -347,7 +352,9 @@ class MailAdapterNotifyTest(BaseMailAdapterTest):
             self.adapter.notify(notification, ActionTargetType.ISSUE_OWNERS)
 
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
         assert msg.subject == "[Sentry] BAR-1 - something bad happened"
+        assert isinstance(msg.alternatives[0][0], str)
         assert "Issue Data" not in msg.alternatives[0][0]
 
     def test_simple_notification_perf(self):
@@ -361,6 +368,7 @@ class MailAdapterNotifyTest(BaseMailAdapterTest):
             self.adapter.notify(notification, ActionTargetType.ISSUE_OWNERS)
 
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
         assert msg.subject == "[Sentry] BAR-1 - N+1 Query"
         checked_values = [
             "Transaction Name",
@@ -372,6 +380,7 @@ class MailAdapterNotifyTest(BaseMailAdapterTest):
             "db - SELECT `books_author`.`id`, `books_author`.`name` FROM `books_autho...",
         ]
         for checked_value in checked_values:
+            assert isinstance(msg.alternatives[0][0], str)
             assert (
                 checked_value in msg.alternatives[0][0]
             ), f"{checked_value} not present in message"
@@ -542,8 +551,8 @@ class MailAdapterNotifyTest(BaseMailAdapterTest):
         from django.template.defaultfilters import date
 
         timestamp = datetime.now(tz=pytz.utc)
-        local_timestamp = timezone.localtime(timestamp, pytz.timezone("Europe/Vienna"))
-        local_timestamp = date(local_timestamp, "N j, Y, g:i:s a e")
+        local_timestamp_s = timezone.localtime(timestamp, pytz.timezone("Europe/Vienna"))
+        local_timestamp = date(local_timestamp_s, "N j, Y, g:i:s a e")
 
         UserOption.objects.create(user=self.user, key="timezone", value="Europe/Vienna")
 
@@ -560,6 +569,7 @@ class MailAdapterNotifyTest(BaseMailAdapterTest):
 
         assert len(mail.outbox) == 1
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
         assert local_timestamp in str(msg.alternatives)
 
     def test_notify_with_suspect_commits(self):
@@ -610,6 +620,7 @@ class MailAdapterNotifyTest(BaseMailAdapterTest):
             },
             project_id=self.project.id,
         )
+        assert event.group is not None
         GroupRelease.objects.create(
             group_id=event.group.id, project_id=self.project.id, release_id=self.release.id
         )
@@ -638,6 +649,8 @@ class MailAdapterNotifyTest(BaseMailAdapterTest):
         assert len(mail.outbox) >= 1
 
         msg = mail.outbox[-1]
+        assert isinstance(msg, EmailMultiAlternatives)
+        assert isinstance(msg.alternatives[0][0], str)
         assert (
             f"/settings/{organization.slug}/integrations/slack/?referrer=alert_email"
             in msg.alternatives[0][0]
@@ -659,6 +672,8 @@ class MailAdapterNotifyTest(BaseMailAdapterTest):
         assert len(mail.outbox) >= 1
 
         msg = mail.outbox[-1]
+        assert isinstance(msg, EmailMultiAlternatives)
+        assert isinstance(msg.alternatives[0][0], str)
         assert (
             f"/settings/{organization.slug}/integrations/slack/?referrer=alert_email"
             not in msg.alternatives[0][0]
@@ -679,6 +694,8 @@ class MailAdapterNotifyTest(BaseMailAdapterTest):
         assert len(mail.outbox) >= 1
 
         msg = mail.outbox[-1]
+        assert isinstance(msg, EmailMultiAlternatives)
+        assert isinstance(msg.alternatives[0][0], str)
         assert (
             f"/settings/{organization.slug}/integrations/slack/?referrer=alert_email"
             not in msg.alternatives[0][0]
@@ -1107,6 +1124,7 @@ class MailAdapterNotifyIssueOwnersTest(BaseMailAdapterTest):
             data={"message": "Hello world", "level": "error"}, project_id=self.project.id
         )
         # Header is based on the group substatus
+        assert event.group is not None
         event.group.substatus = GroupSubStatus.REGRESSED
         event.group.save()
 
@@ -1119,7 +1137,9 @@ class MailAdapterNotifyIssueOwnersTest(BaseMailAdapterTest):
             self.adapter.notify(notification, ActionTargetType.ISSUE_OWNERS)
 
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
         assert msg.subject == "[Sentry] BAR-1 - Hello world"
+        assert isinstance(msg.alternatives[0][0], str)
         assert "Regressed issue" in msg.alternatives[0][0]
 
 
@@ -1536,8 +1556,10 @@ class MailAdapterHandleSignalTest(BaseMailAdapterTest):
 
         assert len(mail.outbox) == 1
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
 
         # email includes issue metadata
+        assert isinstance(msg.alternatives[0][0], str)
         assert "group-header" in msg.alternatives[0][0]
         assert "enhanced privacy" not in msg.body
 
@@ -1548,7 +1570,7 @@ class MailAdapterHandleSignalTest(BaseMailAdapterTest):
         assert msg.to == [self.user.email]
 
     def test_user_feedback__enhanced_privacy(self):
-        self.organization.update(flags=F("flags").bitor(Organization.flags.enhanced_privacy))
+        self.organization.update(flags=F("flags").bitor(Organization.flags.enhanced_privacy))  # type: ignore[attr-defined]
         assert self.organization.flags.enhanced_privacy.is_set is True
         NotificationSetting.objects.update_settings(
             ExternalProviders.EMAIL,
@@ -1567,8 +1589,10 @@ class MailAdapterHandleSignalTest(BaseMailAdapterTest):
 
         assert len(mail.outbox) == 1
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
 
         # email does not include issue metadata
+        assert isinstance(msg.alternatives[0][0], str)
         assert "group-header" not in msg.alternatives[0][0]
         assert "enhanced privacy" in msg.body
 

+ 3 - 0
tests/sentry/notifications/notifications/test_assigned.py

@@ -1,5 +1,6 @@
 import responses
 from django.core import mail
+from django.core.mail.message import EmailMultiAlternatives
 
 from sentry.models import (
     Identity,
@@ -51,9 +52,11 @@ class AssignedNotificationAPITest(APITestCase):
         assert response.status_code == 200, response.content
 
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
         # check the txt version
         assert f"assigned {self.group.qualified_short_id} to themselves" in msg.body
         # check the html version
+        assert isinstance(msg.alternatives[0][0], str)
         assert f"{self.group.qualified_short_id}</a> to themselves</p>" in msg.alternatives[0][0]
 
         attachment, text = get_attachment()

+ 15 - 0
tests/sentry/notifications/test_notifications.py

@@ -6,6 +6,7 @@ from urllib.parse import parse_qs
 
 import responses
 from django.core import mail
+from django.core.mail.message import EmailMultiAlternatives
 from django.utils import timezone
 from sentry_relay import parse_release
 
@@ -120,9 +121,11 @@ class ActivityNotificationTest(APITestCase):
         assert response.status_code == 201, response.content
 
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
         # check the txt version
         assert "blah blah" in msg.body
         # check the html version
+        assert isinstance(msg.alternatives[0][0], str)
         assert "blah blah</p></div>" in msg.alternatives[0][0]
 
         attachment, text = get_attachment()
@@ -154,9 +157,11 @@ class ActivityNotificationTest(APITestCase):
         assert response.status_code == 200, response.content
 
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
         # check the txt version
         assert f"Unassigned\n\n{self.user.username} unassigned {self.short_id}" in msg.body
         # check the html version
+        assert isinstance(msg.alternatives[0][0], str)
         assert f"{self.user.username}</strong> unassigned" in msg.alternatives[0][0]
 
         attachment, text = get_attachment()
@@ -181,9 +186,11 @@ class ActivityNotificationTest(APITestCase):
         assert response.status_code == 200, response.content
 
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
         # check the txt version
         assert f"{self.user.username} marked {self.short_id} as resolved" in msg.body
         # check the html version
+        assert isinstance(msg.alternatives[0][0], str)
         assert f"{self.short_id}</a> as resolved</p>" in msg.alternatives[0][0]
 
         attachment, text = get_attachment()
@@ -232,9 +239,11 @@ class ActivityNotificationTest(APITestCase):
         assert response.status_code == 201, response.content
 
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
         # check the txt version
         assert f"Version {version_parsed} was deployed to {self.environment.name} on" in msg.body
         # check the html version
+        assert isinstance(msg.alternatives[0][0], str)
         assert (
             f"Version {version_parsed} was deployed to {self.environment.name}\n    </h2>\n"
             in msg.alternatives[0][0]
@@ -299,9 +308,11 @@ class ActivityNotificationTest(APITestCase):
         assert not group.is_resolved()
 
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
         # check the txt version
         assert f"Sentry marked {group.qualified_short_id} as a regression" in msg.body
         # check the html version
+        assert isinstance(msg.alternatives[0][0], str)
         assert f"{group.qualified_short_id}</a> as a regression</p>" in msg.alternatives[0][0]
 
         attachment, text = get_attachment()
@@ -346,12 +357,14 @@ class ActivityNotificationTest(APITestCase):
         assert response.status_code == 200, response.content
 
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
         # check the txt version
         assert (
             f"Resolved Issue\n\n{self.user.username} marked {self.short_id} as resolved in {release.version}"
             in msg.body
         )
         # check the html version
+        assert isinstance(msg.alternatives[0][0], str)
         assert (
             f'text-decoration: none">{self.short_id}</a> as resolved in' in msg.alternatives[0][0]
         )
@@ -428,9 +441,11 @@ class ActivityNotificationTest(APITestCase):
             )
 
         msg = mail.outbox[0]
+        assert isinstance(msg, EmailMultiAlternatives)
         # check the txt version
         assert "Details\n-------\n\n" in msg.body
         # check the html version
+        assert isinstance(msg.alternatives[0][0], str)
         assert "Hello world</pre>" in msg.alternatives[0][0]
 
         attachment, text = get_attachment()

+ 4 - 1
tests/sentry/tasks/test_weekly_reports.py

@@ -5,6 +5,7 @@ from unittest import mock
 
 import pytz
 from django.core import mail
+from django.core.mail.message import EmailMultiAlternatives
 from django.db.models import F
 from django.utils import timezone
 from freezegun import freeze_time
@@ -83,8 +84,10 @@ class WeeklyReportsTest(OutcomesSnubaTest, SnubaTestCase):
             assert len(mail.outbox) == 1
 
             message = mail.outbox[0]
+            assert isinstance(message, EmailMultiAlternatives)
             assert self.organization.name in message.subject
             html = message.alternatives[0][0]
+            assert isinstance(html, str)
             assert (
                 f"http://{self.organization.slug}.testserver/issues/?referrer=weekly-email" in html
             )
@@ -115,7 +118,7 @@ class WeeklyReportsTest(OutcomesSnubaTest, SnubaTestCase):
 
         with in_test_psql_role_override("postgres"):
             OrganizationMember.objects.filter(user_id=self.user.id).update(
-                flags=F("flags").bitor(OrganizationMember.flags["member-limit:restricted"])
+                flags=F("flags").bitor(OrganizationMember.flags["member-limit:restricted"])  # type: ignore[index]
             )
 
         # disabled

+ 9 - 1
tests/sentry/utils/email/test_message_builder.py

@@ -2,6 +2,7 @@ import functools
 from unittest.mock import patch
 
 from django.core import mail
+from django.core.mail.message import EmailMultiAlternatives
 
 from sentry import options
 from sentry.models import GroupEmailThread, User, UserEmail, UserOption
@@ -24,6 +25,7 @@ class MessageBuilderTest(TestCase):
         assert len(mail.outbox) == 1
 
         out = mail.outbox[0]
+        assert isinstance(out, EmailMultiAlternatives)
         assert out.to == ["foo@example.com"]
         assert out.subject == "Test"
         assert out.extra_headers["X-Test"] == "foo"
@@ -46,6 +48,7 @@ class MessageBuilderTest(TestCase):
         assert len(mail.outbox) == 1
 
         out = mail.outbox[0]
+        assert isinstance(out, EmailMultiAlternatives)
         assert out.to == ["foo@example.com"]
         assert out.subject == "Test"
         assert out.extra_headers["X-Test"] == "foo"
@@ -68,6 +71,7 @@ class MessageBuilderTest(TestCase):
         assert len(mail.outbox) == 1
 
         out = mail.outbox[0]
+        assert isinstance(out, EmailMultiAlternatives)
         assert out.to == ["foo@example.com"]
         assert out.subject == "Test"
         assert out.extra_headers["Reply-To"] == "bar@example.com"
@@ -141,6 +145,7 @@ class MessageBuilderTest(TestCase):
         assert len(mail.outbox) == 1
 
         out = mail.outbox[0]
+        assert isinstance(out, EmailMultiAlternatives)
         assert out.to == ["foo@example.com"]
         assert out.subject == "Test"
         assert out.extra_headers["Message-Id"] == "abc123"
@@ -163,6 +168,7 @@ class MessageBuilderTest(TestCase):
         assert len(mail.outbox) == 1
 
         out = mail.outbox[0]
+        assert isinstance(out, EmailMultiAlternatives)
         assert out.to == ["foo@example.com"]
         assert out.subject == "Test", "First message should not have Re: prefix"
         assert out.extra_headers["Message-Id"] == "abc123"
@@ -197,6 +203,7 @@ class MessageBuilderTest(TestCase):
         assert len(mail.outbox) == 1
 
         out = mail.outbox[0]
+        assert isinstance(out, EmailMultiAlternatives)
         assert out.to == ["foo@example.com"]
         assert out.subject == "Re: Test"
         assert out.extra_headers["Message-Id"] == "abc123"
@@ -223,6 +230,7 @@ class MessageBuilderTest(TestCase):
         assert len(mail.outbox) == 2
 
         out = mail.outbox[1]
+        assert isinstance(out, EmailMultiAlternatives)
         assert out.to == ["foo@example.com"]
         assert out.subject == "Re: Test"
         assert out.extra_headers["Message-Id"] == "321cba"
@@ -280,7 +288,7 @@ class MessageBuilderTest(TestCase):
                 subject="Test",
                 body="hello world",
                 html_body="<b>hello world</b>",
-                reference=object(),
+                reference=self.user,
             )
             .get_built_messages(["foo@example.com"])[0]
             .message()