Repository: getsentry/sentry
Pull request: ref(weekly report): Prevent resending emails
Build up the user context per organization first, then send emails. 

Closes https://github.com/getsentry/sentry/issues/55476

Problem statement:
Prevent resending weekly report emails
We recently had an error in the weekly report task. Since it has retries enabled it would get halfway through sending them and re-attempt the entire org.

Some possible solutions:
- Build the user context as a step and send them in a followup step so no emails are sent until all the contexts have been generated
- Limit the task to only retry on specific errors

ACs:
- Add a metric alert for the weekly_report task
- Limit retries to only if no emails have been sent

Hints from issue discussion:

Commit message: nit these were bugging me

Patch to review:
diff --git a/src/sentry/tasks/weekly_reports.py b/src/sentry/tasks/weekly_reports.py
index 6b9555bf131cbf..388e721a571607 100644
--- a/src/sentry/tasks/weekly_reports.py
+++ b/src/sentry/tasks/weekly_reports.py
@@ -225,17 +225,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):
+    """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):
+    """
+    Populates context.projects which is { project_id: ProjectContext }
+    """
+
     def zerofill_data(data):
         return zerofill(data, ctx.start, ctx.end, ONE_DAY, fill_default=0)
 
@@ -547,17 +551,23 @@ 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):
+    """
+    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.
     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)
+        template_by_user_id = prepare_template_context(ctx, [target_user_id])
+        send_email(
+            ctx,
+            template_by_user_id[0].get("context"),
+            template_by_user_id[0].get("user_id"),
+            dry_run=dry_run,
+            email_override=email_override,
+        )
     else:
         user_list = list(
             OrganizationMember.objects.filter(
@@ -571,8 +581,13 @@ 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
         )
+        template_by_user_id = []
         for user_id in user_ids:
-            send_email(ctx, user_id, dry_run=dry_run)
+            template_by_user_id = prepare_template_context(ctx, user_ids)
+        for user_template in template_by_user_id:
+            send_email(
+                ctx, user_template.get("context"), user_template.get("user_id"), dry_run=dry_run
+            )
 
 
 project_breakdown_colors = ["#422C6E", "#895289", "#D6567F", "#F38150", "#F2B713"]
@@ -945,16 +960,21 @@ def issue_summary():
     }
 
 
-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, user_ids):
+    template_by_user_id = []
+    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,
+            )
+        template_by_user_id.append({"context": template_ctx, "user_id": user_id})
+    return template_by_user_id
+
 
+def send_email(ctx, template_ctx, user_id, dry_run=False, email_override=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 +985,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:
diff --git a/tests/sentry/tasks/test_weekly_reports.py b/tests/sentry/tasks/test_weekly_reports.py
index 38f30fcbab9b4f..930b0e049b4a4d 100644
--- a/tests/sentry/tasks/test_weekly_reports.py
+++ b/tests/sentry/tasks/test_weekly_reports.py
@@ -24,6 +24,7 @@
     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 @@ def test_message_links_customer_domains(self):
                 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 @@ def set_option_value(value):
         # 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,
+            template_context[0].get("context"),
+            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 @@ def test_message_builder_simple(self, message_builder, record):
             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):


Code review comments:
diff_hunk: @@ -945,16 +960,21 @@ def issue_summary():
     }
 
 
-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, user_ids):
path: src/sentry/tasks/weekly_reports.py
text: @user1:
mind adding typing here?
both to the arguments and return typing

diff_hunk: @@ -571,8 +581,13 @@ 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
         )
+        template_by_user_id = []
         for user_id in user_ids:
-            send_email(ctx, user_id, dry_run=dry_run)
+            template_by_user_id = prepare_template_context(ctx, user_ids)
+        for user_template in template_by_user_id:
+            send_email(
path: src/sentry/tasks/weekly_reports.py
text: @user1:
default to using kwargs for clarity

diff_hunk: @@ -547,17 +551,23 @@ 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):
+    """
+    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.
     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)
+        template_by_user_id = prepare_template_context(ctx, [target_user_id])
+        send_email(
+            ctx,
path: src/sentry/tasks/weekly_reports.py
text: @user1:
can we make all of these kwargs?

diff_hunk: @@ -547,17 +551,23 @@ 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):
+    """
+    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.
     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)
+        template_by_user_id = prepare_template_context(ctx, [target_user_id])
path: src/sentry/tasks/weekly_reports.py
text: @user1:
sounds like this is a list of templates?
can we rename this to better represent the return value? Maybe something like `user_template_context_list`

@author:
Yeah it's a list of dicts like `[{"context": ..., "user_id": 1}]`