From c4f74f470d14a4690e2e32b30654c56d84776512 Mon Sep 17 00:00:00 2001
From: Prakhar Pratyush <prakhar@zulip.com>
Date: Fri, 1 Nov 2024 19:53:30 +0530
Subject: [PATCH] remote_server_post_analytics: Add durable=True to outermost
 transaction.

This commit adds 'durable=True' to the outermost transaction
in 'remote_server_post_analytics'.

It also adds 'savepoint=False' to inner transaction.atomic
decorator to avoid creating savepoint.

This is as a part of our plan to explicitly mark all the
transaction.atomic decorators with either 'savepoint=False' or
'durable=True' as required.

* 'savepoint=True' is used in special cases.
---
 corporate/lib/stripe.py                 | 2 +-
 zerver/tests/test_push_notifications.py | 6 ++----
 zilencer/views.py                       | 8 ++++----
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/corporate/lib/stripe.py b/corporate/lib/stripe.py
index 381e06cb73..00ca44c0f9 100644
--- a/corporate/lib/stripe.py
+++ b/corporate/lib/stripe.py
@@ -2197,7 +2197,7 @@ class BillingSession(ABC):
 
     # event_time should roughly be timezone_now(). Not designed to handle
     # event_times in the past or future
-    @transaction.atomic
+    @transaction.atomic(savepoint=False)
     def make_end_of_cycle_updates_if_needed(
         self, plan: CustomerPlan, event_time: datetime
     ) -> tuple[CustomerPlan | None, LicenseLedger | None]:
diff --git a/zerver/tests/test_push_notifications.py b/zerver/tests/test_push_notifications.py
index ccbf4be4c4..141be5d3af 100644
--- a/zerver/tests/test_push_notifications.py
+++ b/zerver/tests/test_push_notifications.py
@@ -14,7 +14,6 @@ import orjson
 import responses
 import time_machine
 from django.conf import settings
-from django.db import transaction
 from django.db.models import F, Q
 from django.http.response import ResponseHeaders
 from django.test import override_settings
@@ -2145,9 +2144,7 @@ class AnalyticsBouncerTest(BouncerTestCase):
             plan_type=RemoteRealm.PLAN_TYPE_SELF_MANAGED,
         )
 
-        with transaction.atomic(), self.assertLogs("zulip.analytics", level="WARNING") as m:
-            # The usual atomic() wrapper to avoid IntegrityError breaking the test's
-            # transaction.
+        with self.assertLogs("zulip.analytics", level="WARNING") as m:
             send_server_data_to_push_bouncer()
         self.assertEqual(m.output, ["WARNING:zulip.analytics:Duplicate registration detected."])
 
@@ -2705,6 +2702,7 @@ class AnalyticsBouncerTest(BouncerTestCase):
         # Now we want to test the other side of this - bouncer's handling
         # of a deleted realm.
         with (
+            self.captureOnCommitCallbacks(execute=True),
             self.assertLogs(logger, level="WARNING") as analytics_logger,
             mock.patch(
                 "corporate.lib.stripe.RemoteRealmBillingSession.on_paid_plan", return_value=True
diff --git a/zilencer/views.py b/zilencer/views.py
index fb3ec2aea7..430f4bbb29 100644
--- a/zilencer/views.py
+++ b/zilencer/views.py
@@ -50,7 +50,7 @@ from zerver.lib.push_notifications import (
     send_apple_push_notification,
     send_test_push_notification_directly_to_devices,
 )
-from zerver.lib.queue import queue_json_publish
+from zerver.lib.queue import queue_event_on_commit
 from zerver.lib.remote_server import (
     InstallationCountDataForAnalytics,
     RealmAuditLogDataForAnalytics,
@@ -1012,7 +1012,7 @@ def update_remote_realm_data_for_server(
     }
     for context in new_locally_deleted_remote_realms_on_paid_plan_contexts:
         email_dict["context"] = context
-        queue_json_publish("email_senders", email_dict)
+        queue_event_on_commit("email_senders", email_dict)
 
 
 def get_human_user_realm_uuids(
@@ -1156,7 +1156,7 @@ def handle_customer_migration_from_server_to_realm(
 
 
 @typed_endpoint
-@transaction.atomic
+@transaction.atomic(durable=True)
 def remote_server_post_analytics(
     request: HttpRequest,
     server: RemoteZulipServer,
@@ -1267,7 +1267,7 @@ def remote_server_post_analytics(
         # 'last_audit_log_update' needs to be an atomic operation.
         # This helps to rely on 'last_audit_log_update' to assume
         # RemoteRealmAuditLog and LicenseLedger are up-to-date.
-        with transaction.atomic():
+        with transaction.atomic(savepoint=False):
             # Important: Do not return early if we receive 0 rows; we must
             # updated last_audit_log_update even if there are no new rows,
             # to help identify server whose ability to connect to this