ref(weekly report): Prevent resending emails (#65203)

Build up the user context per organization first, then send emails. 

Closes https://github.com/getsentry/sentry/issues/55476

src/sentry/tasks/weekly_reports.py

@@ -3,8 +3,10 @@ from __future__ import annotations
 import heapq
 import logging
 import uuid
+from collections.abc import Mapping
 from datetime import timedelta
 from functools import partial, reduce
+from typing import Any
 
 import sentry_sdk
 from django.db.models import Count, F
@@ -26,6 +28,7 @@ from sentry.models.group import Group, GroupStatus
 from sentry.models.grouphistory import GroupHistory, GroupHistoryStatus
 from sentry.models.organization import Organization, OrganizationStatus
 from sentry.models.organizationmember import OrganizationMember
+from sentry.models.user import User
 from sentry.services.hybrid_cloud.notifications import notifications_service
 from sentry.silo import SiloMode
 from sentry.snuba.dataset import Dataset
@@ -107,7 +110,7 @@ class ProjectContext:
         )
 
 
-def check_if_project_is_empty(project_ctx):
+def check_if_project_is_empty(project_ctx: ProjectContext) -> bool:
     """
     Check if this project has any content we could show in an email.
     """
@@ -124,7 +127,7 @@ def check_if_project_is_empty(project_ctx):
     )
 
 
-def check_if_ctx_is_empty(ctx):
+def check_if_ctx_is_empty(ctx: OrganizationReportContext) -> bool:
     """
     Check if the context is empty. If it is, we don't want to send an email.
     """
@@ -140,7 +143,9 @@ def check_if_ctx_is_empty(ctx):
     silo_mode=SiloMode.REGION,
 )
 @retry
-def schedule_organizations(dry_run=False, timestamp=None, duration=None):
+def schedule_organizations(
+    dry_run: bool = False, timestamp: float | None = None, duration: int | None = None
+) -> None:
     if timestamp is None:
         # The time that the report was generated
         timestamp = to_timestamp(floor_to_utc_day(timezone.now()))
@@ -167,7 +172,12 @@ def schedule_organizations(dry_run=False, timestamp=None, duration=None):
 )
 @retry
 def prepare_organization_report(
-    timestamp, duration, organization_id, dry_run=False, target_user=None, email_override=None
+    timestamp: float,
+    duration: int,
+    organization_id: int,
+    dry_run: bool = False,
+    target_user: User | None = None,
+    email_override: str | None = None,
 ):
     if target_user and not hasattr(target_user, "id"):
         logger.error(
@@ -225,17 +235,21 @@ def prepare_organization_report(
 # Organization Passes
 
 
-# Find the projects associated with an user.
-# Populates context.project_ownership which is { user_id: set<project_id> }
-def user_project_ownership(ctx):
+def user_project_ownership(ctx: OrganizationReportContext) -> None:
+    """Find the projects associated with each user.
+    Populates context.project_ownership which is { user_id: set<project_id> }
+    """
     for project_id, user_id in OrganizationMember.objects.filter(
         organization_id=ctx.organization.id, teams__projectteam__project__isnull=False
     ).values_list("teams__projectteam__project_id", "user_id"):
         ctx.project_ownership.setdefault(user_id, set()).add(project_id)
 
 
-# Populates context.projects which is { project_id: ProjectContext }
-def project_event_counts_for_organization(ctx):
+def project_event_counts_for_organization(ctx: OrganizationReportContext) -> None:
+    """
+    Populates context.projects which is { project_id: ProjectContext }
+    """
+
     def zerofill_data(data):
         return zerofill(data, ctx.start, ctx.end, ONE_DAY, fill_default=0)
 
@@ -300,7 +314,7 @@ def project_event_counts_for_organization(ctx):
                 )
 
 
-def organization_project_issue_substatus_summaries(ctx: OrganizationReportContext):
+def organization_project_issue_substatus_summaries(ctx: OrganizationReportContext) -> None:
     substatus_counts = (
         Group.objects.filter(
             project__organization_id=ctx.organization.id,
@@ -324,7 +338,7 @@ def organization_project_issue_substatus_summaries(ctx: OrganizationReportContex
 
 
 # Project passes
-def project_key_errors(ctx, project):
+def project_key_errors(ctx: OrganizationReportContext, project) -> None:
     if not project.first_event:
         return
     # Take the 3 most frequently occuring events
@@ -355,7 +369,7 @@ def project_key_errors(ctx, project):
 
 
 # Organization pass. Depends on project_key_errors.
-def fetch_key_error_groups(ctx):
+def fetch_key_error_groups(ctx: OrganizationReportContext) -> None:
     all_key_error_group_ids = []
     for project_ctx in ctx.projects.values():
         all_key_error_group_ids.extend([group_id for group_id, count in project_ctx.key_errors])
@@ -520,7 +534,7 @@ def project_key_performance_issues(ctx, project):
 
 
 # Organization pass. Depends on project_key_performance_issue.
-def fetch_key_performance_issue_groups(ctx):
+def fetch_key_performance_issue_groups(ctx: OrganizationReportContext):
     all_groups = []
     for project_ctx in ctx.projects.values():
         all_groups.extend([group for group, count in project_ctx.key_performance_issues])
@@ -547,17 +561,38 @@ def fetch_key_performance_issue_groups(ctx):
         ]
 
 
-# Deliver reports
-# For all users in the organization, we generate the template context for the user, and send the email.
-
-
-def deliver_reports(ctx, dry_run=False, target_user=None, email_override=None):
+def deliver_reports(
+    ctx: OrganizationReportContext,
+    dry_run: bool = False,
+    target_user: User | None = None,
+    email_override: str | None = None,
+) -> None:
+    """
+    For all users in the organization, we generate the template context for the user, and send the email.
+    """
     # Specify a sentry user to send this email.
+    template_context: Mapping[str, Any] | None = None
+    user_id: int | None = None
+
     if email_override:
         target_user_id = (
             target_user.id if target_user else None
         )  # if None, generates report for a user with access to all projects
-        send_email(ctx, target_user_id, dry_run=dry_run, email_override=email_override)
+        user_template_context_by_user_id_list = prepare_template_context(
+            ctx=ctx, user_ids=[target_user_id]
+        )
+        if user_template_context_by_user_id_list:
+            user_template_context_by_user_id = user_template_context_by_user_id_list[0]
+            template_context = user_template_context_by_user_id.get("context")
+            user_id = user_template_context_by_user_id.get("user_id")
+            if template_context and user_id:
+                send_email(
+                    ctx=ctx,
+                    template_ctx=template_context,
+                    user_id=user_id,
+                    dry_run=dry_run,
+                    email_override=email_override,
+                )
     else:
         user_list = list(
             OrganizationMember.objects.filter(
@@ -571,8 +606,19 @@ def deliver_reports(ctx, dry_run=False, target_user=None, email_override=None):
         user_ids = notifications_service.get_users_for_weekly_reports(
             organization_id=ctx.organization.id, user_ids=user_list
         )
+        user_template_context_by_user_id_list = []
         for user_id in user_ids:
-            send_email(ctx, user_id, dry_run=dry_run)
+            user_template_context_by_user_id_list = prepare_template_context(
+                ctx=ctx, user_ids=user_ids
+            )
+        if user_template_context_by_user_id_list:
+            for user_template in user_template_context_by_user_id_list:
+                template_context = user_template.get("context")
+                user_id = user_template.get("user_id")
+                if template_context and user_id:
+                    send_email(
+                        ctx=ctx, template_ctx=template_context, user_id=user_id, dry_run=dry_run
+                    )
 
 
 project_breakdown_colors = ["#422C6E", "#895289", "#D6567F", "#F38150", "#F2B713"]
@@ -945,16 +991,30 @@ def render_template_context(ctx, user_id):
     }
 
 
-def send_email(ctx, user_id, dry_run=False, email_override=None):
-    template_ctx = render_template_context(ctx, user_id)
-    if not template_ctx:
-        logger.debug(
-            "Skipping report for %s to <User: %s>, no qualifying reports to deliver.",
-            ctx.organization.id,
-            user_id,
-        )
-        return
+def prepare_template_context(
+    ctx: OrganizationReportContext, user_ids: list[int]
+) -> list[Mapping[str, Any]] | list:
+    user_template_context_by_user_id_list = []
+    for user_id in user_ids:
+        template_ctx = render_template_context(ctx, user_id)
+        if not template_ctx:
+            logger.debug(
+                "Skipping report for %s to <User: %s>, no qualifying reports to deliver.",
+                ctx.organization.id,
+                user_id,
+            )
+            continue
+        user_template_context_by_user_id_list.append({"context": template_ctx, "user_id": user_id})
+    return user_template_context_by_user_id_list
 
+
+def send_email(
+    ctx: OrganizationReportContext,
+    template_ctx: Mapping[str, Any],
+    user_id: int,
+    dry_run: bool = False,
+    email_override: str | None = None,
+) -> None:
     message = MessageBuilder(
         subject=f"Weekly Report for {ctx.organization.name}: {date_format(ctx.start)} - {date_format(ctx.end)}",
         template="sentry/emails/reports/body.txt",
@@ -965,6 +1025,7 @@ def send_email(ctx, user_id, dry_run=False, email_override=None):
     )
     if dry_run:
         return
+
     if email_override:
         message.send(to=(email_override,))
     else:

tests/sentry/tasks/test_weekly_reports.py

@@ -24,6 +24,7 @@ from sentry.tasks.weekly_reports import (
     group_status_to_color,
     organization_project_issue_substatus_summaries,
     prepare_organization_report,
+    prepare_template_context,
     schedule_organizations,
 )
 from sentry.testutils.cases import OutcomesSnubaTest, SnubaTestCase
@@ -120,11 +121,16 @@ class WeeklyReportsTest(OutcomesSnubaTest, SnubaTestCase):
                 f"http://{self.organization.slug}.testserver/issues/?referrer=weekly_report" in html
             )
 
+    @mock.patch("sentry.tasks.weekly_reports.prepare_template_context")
     @mock.patch("sentry.tasks.weekly_reports.send_email")
-    def test_deliver_reports_respects_settings(self, mock_send_email):
+    def test_deliver_reports_respects_settings(
+        self, mock_send_email, mock_prepare_template_context
+    ):
         user = self.user
         organization = self.organization
         ctx = OrganizationReportContext(0, 0, organization)
+        template_context = prepare_template_context(ctx, [user.id])
+        mock_prepare_template_context.return_value = template_context
 
         def set_option_value(value):
             with assume_test_silo_mode(SiloMode.CONTROL):
@@ -144,7 +150,13 @@ class WeeklyReportsTest(OutcomesSnubaTest, SnubaTestCase):
         # enabled
         set_option_value("always")
         deliver_reports(ctx)
-        mock_send_email.assert_called_once_with(ctx, user.id, dry_run=False)
+        assert mock_send_email.call_count == 1
+        mock_send_email.assert_called_once_with(
+            ctx=ctx,
+            template_ctx=template_context[0].get("context"),
+            user_id=template_context[0].get("user_id"),
+            dry_run=False,
+        )
 
     @mock.patch("sentry.tasks.weekly_reports.send_email")
     def test_member_disabled(self, mock_send_email):
@@ -365,6 +377,122 @@ class WeeklyReportsTest(OutcomesSnubaTest, SnubaTestCase):
             user_project_count=1,
         )
 
+    @mock.patch("sentry.analytics.record")
+    @mock.patch("sentry.tasks.weekly_reports.MessageBuilder")
+    def test_message_builder_multiple_users_prevent_resend(self, message_builder, record):
+        now = django_timezone.now()
+
+        two_days_ago = now - timedelta(days=2)
+        three_days_ago = now - timedelta(days=3)
+
+        user = self.create_user()
+        self.create_member(teams=[self.team], user=user, organization=self.organization)
+        user2 = self.create_user()
+        self.create_member(teams=[self.team], user=user2, organization=self.organization)
+
+        event1 = self.store_event(
+            data={
+                "event_id": "a" * 32,
+                "message": "message",
+                "timestamp": iso_format(three_days_ago),
+                "stacktrace": copy.deepcopy(DEFAULT_EVENT_DATA["stacktrace"]),
+                "fingerprint": ["group-1"],
+            },
+            project_id=self.project.id,
+        )
+
+        event2 = self.store_event(
+            data={
+                "event_id": "b" * 32,
+                "message": "message",
+                "timestamp": iso_format(three_days_ago),
+                "stacktrace": copy.deepcopy(DEFAULT_EVENT_DATA["stacktrace"]),
+                "fingerprint": ["group-2"],
+            },
+            project_id=self.project.id,
+        )
+        self.store_outcomes(
+            {
+                "org_id": self.organization.id,
+                "project_id": self.project.id,
+                "outcome": Outcome.ACCEPTED,
+                "category": DataCategory.ERROR,
+                "timestamp": three_days_ago,
+                "key_id": 1,
+            },
+            num_times=2,
+        )
+
+        self.store_outcomes(
+            {
+                "org_id": self.organization.id,
+                "project_id": self.project.id,
+                "outcome": Outcome.ACCEPTED,
+                "category": DataCategory.TRANSACTION,
+                "timestamp": three_days_ago,
+                "key_id": 1,
+            },
+            num_times=10,
+        )
+
+        group1 = event1.group
+        group2 = event2.group
+
+        group1.status = GroupStatus.RESOLVED
+        group1.substatus = None
+        group1.resolved_at = two_days_ago
+        group1.save()
+
+        group2.status = GroupStatus.RESOLVED
+        group2.substatus = None
+        group2.resolved_at = two_days_ago
+        group2.save()
+
+        with mock.patch(
+            "sentry.tasks.weekly_reports.prepare_template_context", side_effect=ValueError("oh no!")
+        ), mock.patch("sentry.tasks.weekly_reports.send_email") as mock_send_email:
+            with pytest.raises(Exception):
+                prepare_organization_report(to_timestamp(now), ONE_DAY * 7, self.organization.id)
+                mock_send_email.assert_not_called()
+
+        prepare_organization_report(to_timestamp(now), ONE_DAY * 7, self.organization.id)
+        for call_args in message_builder.call_args_list:
+            message_params = call_args.kwargs
+            context = message_params["context"]
+
+            assert message_params["template"] == "sentry/emails/reports/body.txt"
+            assert message_params["html_template"] == "sentry/emails/reports/body.html"
+
+            assert context["organization"] == self.organization
+            assert context["issue_summary"] == {
+                "escalating_substatus_count": 0,
+                "new_substatus_count": 0,
+                "ongoing_substatus_count": 0,
+                "regression_substatus_count": 0,
+                "total_substatus_count": 0,
+            }
+            assert len(context["key_errors"]) == 2
+            assert context["trends"]["total_error_count"] == 2
+            assert context["trends"]["total_transaction_count"] == 10
+            assert "Weekly Report for" in message_params["subject"]
+
+            assert isinstance(context["notification_uuid"], str)
+
+        record.assert_any_call(
+            "weekly_report.sent",
+            user_id=user.id,
+            organization_id=self.organization.id,
+            notification_uuid=mock.ANY,
+            user_project_count=1,
+        )
+        record.assert_any_call(
+            "weekly_report.sent",
+            user_id=user2.id,
+            organization_id=self.organization.id,
+            notification_uuid=mock.ANY,
+            user_project_count=1,
+        )
+
     @mock.patch("sentry.tasks.weekly_reports.MessageBuilder")
     @with_feature("organizations:escalating-issues")
     def test_message_builder_substatus_simple(self, message_builder):
@@ -677,7 +805,7 @@ class WeeklyReportsTest(OutcomesSnubaTest, SnubaTestCase):
                 user_project_count=1,
             )
 
-        message_builder.return_value.send.assert_called_with(to=("jonathan@speedwagon.org",))
+            message_builder.return_value.send.assert_called_with(to=("jonathan@speedwagon.org",))
 
     @mock.patch("sentry.tasks.weekly_reports.logger")
     def test_email_override_invalid_target_user(self, logger):