settings: Send presence update event toggling presence_enabled.

Fixes #21180.

zerver/actions/presence.py

@@ -3,6 +3,7 @@ import time
 from typing import Optional
 
 from django.conf import settings
+from django.db import transaction
 
 from zerver.actions.user_activity import update_user_activity_interval
 from zerver.decorator import statsd_increment
@@ -13,7 +14,9 @@ from zerver.models import Client, UserPresence, UserProfile, UserStatus, active_
 from zerver.tornado.django_api import send_event
 
 
-def send_presence_changed(user_profile: UserProfile, presence: UserPresence) -> None:
+def send_presence_changed(
+    user_profile: UserProfile, presence: UserPresence, *, force_send_update: bool = False
+) -> None:
     # Most presence data is sent to clients in the main presence
     # endpoint in response to the user's own presence; this results
     # data that is 1-2 minutes stale for who is online.  The flaw with
@@ -24,7 +27,10 @@ def send_presence_changed(user_profile: UserProfile, presence: UserPresence) ->
     # See https://zulip.readthedocs.io/en/latest/subsystems/presence.html for
     # internals documentation on presence.
     user_ids = active_user_ids(user_profile.realm_id)
-    if len(user_ids) > settings.USER_LIMIT_FOR_SENDING_PRESENCE_UPDATE_EVENTS:
+    if (
+        len(user_ids) > settings.USER_LIMIT_FOR_SENDING_PRESENCE_UPDATE_EVENTS
+        and not force_send_update
+    ):
         # These immediate presence generate quadratic work for Tornado
         # (linear number of users in each event and the frequency of
         # users coming online grows linearly with userbase too).  In
@@ -64,7 +70,12 @@ def consolidate_client(client: Client) -> Client:
 
 @statsd_increment("user_presence")
 def do_update_user_presence(
-    user_profile: UserProfile, client: Client, log_time: datetime.datetime, status: int
+    user_profile: UserProfile,
+    client: Client,
+    log_time: datetime.datetime,
+    status: int,
+    *,
+    force_send_update: bool = False,
 ) -> None:
     client = consolidate_client(client)
 
@@ -103,8 +114,18 @@ def do_update_user_presence(
             update_fields.append("status")
         presence.save(update_fields=update_fields)
 
-    if not user_profile.realm.presence_disabled and (created or became_online):
+    if force_send_update or (
-        send_presence_changed(user_profile, presence)
+        not user_profile.realm.presence_disabled and (created or became_online)
+    ):
+        # We do a the transaction.on_commit here, rather than inside
+        # send_presence_changed, to help keep presence transactions
+        # brief; the active_user_ids call there is more expensive than
+        # this whole function.
+        transaction.on_commit(
+            lambda: send_presence_changed(
+                user_profile, presence, force_send_update=force_send_update
+            )
+        )
 
 
 def update_user_presence(

zerver/actions/user_settings.py

@@ -2,11 +2,13 @@ import datetime
 from typing import List, Optional, Union
 
 import orjson
+from django.conf import settings
 from django.db import transaction
 from django.db.models import F
 from django.utils.timezone import now as timezone_now
 
 from confirmation.models import Confirmation, create_confirmation_link
+from zerver.actions.presence import do_update_user_presence
 from zerver.lib.avatar import avatar_url
 from zerver.lib.cache import (
     cache_delete,
@@ -26,9 +28,11 @@ from zerver.models import (
     RealmAuditLog,
     ScheduledEmail,
     ScheduledMessageNotificationEmail,
+    UserPresence,
     UserProfile,
     active_user_ids,
     bot_owner_user_ids,
+    get_client,
     get_user_profile_by_id,
 )
 from zerver.tornado.django_api import send_event
@@ -454,3 +458,43 @@ def do_change_user_setting(
         # not deleted every previously synced draft - to do that use the DELETE
         # endpoint.
         Draft.objects.filter(user_profile=user_profile).delete()
+
+    if setting_name == "presence_enabled":
+        # The presence_enabled setting's primary function is to stop
+        # doing presence updates for the user altogether.
+        #
+        # When a user toggles the presence_enabled setting, we
+        # immediately trigger a presence update, so all users see the
+        # user's current presence state as consistent with the new
+        # setting; not doing so can make it look like the settings
+        # change didn't have any effect.
+        if setting_value:
+            status = UserPresence.ACTIVE
+            presence_time = timezone_now()
+        else:
+            # HACK: Remove existing presence data for the current user
+            # when disabling presence. This hack will go away when we
+            # replace our presence data structure with a simpler model
+            # that doesn't separate individual clients.
+            UserPresence.objects.filter(user_profile_id=user_profile.id).delete()
+
+            # We create a single presence entry for the user, old
+            # enough to be guaranteed to be treated as offline by
+            # correct clients, such that the user will, for as long as
+            # presence remains disabled, appear to have been last
+            # online a few minutes before they disabled presence.
+            #
+            # We add a small additional offset as a fudge factor in
+            # case of clock skew.
+            status = UserPresence.IDLE
+            presence_time = timezone_now() - datetime.timedelta(
+                seconds=settings.OFFLINE_THRESHOLD_SECS + 120
+            )
+
+        do_update_user_presence(
+            user_profile,
+            get_client("website"),
+            presence_time,
+            status,
+            force_send_update=True,
+        )

zerver/tests/test_event_system.py

@@ -5,6 +5,7 @@ from unittest import mock
 import orjson
 from django.conf import settings
 from django.http import HttpRequest, HttpResponse
+from django.test import override_settings
 from django.utils.timezone import now as timezone_now
 
 from version import API_FEATURE_LEVEL, ZULIP_MERGE_BASE, ZULIP_VERSION
@@ -1199,8 +1200,12 @@ class TestGetRawUserDataSystemBotRealm(ZulipTestCase):
 
 
 class TestUserPresenceUpdatesDisabled(ZulipTestCase):
+    # For this test, we verify do_update_user_presence doesn't send
+    # events for organizations with more than
+    # USER_LIMIT_FOR_SENDING_PRESENCE_UPDATE_EVENTS users, unless
+    # force_send_update is passed.
+    @override_settings(USER_LIMIT_FOR_SENDING_PRESENCE_UPDATE_EVENTS=3)
     def test_presence_events_disabled_on_larger_realm(self) -> None:
-        # First check that normally the mocked function gets called.
         events: List[Mapping[str, Any]] = []
         with self.tornado_redirected_to_list(events, expected_num_events=1):
             do_update_user_presence(
@@ -1208,15 +1213,14 @@ class TestUserPresenceUpdatesDisabled(ZulipTestCase):
                 get_client("website"),
                 timezone_now(),
                 UserPresence.ACTIVE,
+                force_send_update=True,
             )
 
-        # Now check that if the realm has more than the USER_LIMIT_FOR_SENDING_PRESENCE_UPDATE_EVENTS
-        # amount of active users, send_event doesn't get called.
         with self.tornado_redirected_to_list(events, expected_num_events=0):
-            with self.settings(USER_LIMIT_FOR_SENDING_PRESENCE_UPDATE_EVENTS=1):
             do_update_user_presence(
                 self.example_user("hamlet"),
                 get_client("website"),
                 timezone_now(),
                 UserPresence.ACTIVE,
+                force_send_update=False,
             )

zerver/tests/test_events.py

@@ -1733,7 +1733,11 @@ class NormalActionsTest(BaseAction):
 
     def test_change_notification_settings(self) -> None:
         for notification_setting, v in self.user_profile.notification_setting_types.items():
-            if notification_setting in ["notification_sound", "desktop_icon_count_display"]:
+            if notification_setting in [
+                "notification_sound",
+                "desktop_icon_count_display",
+                "presence_enabled",
+            ]:
                 # These settings are tested in their own tests.
                 continue
 
@@ -1769,6 +1773,26 @@ class NormalActionsTest(BaseAction):
                 check_user_settings_update("events[0]", events[0])
                 check_update_global_notifications("events[1]", events[1], setting_value)
 
+    def test_change_presence_enabled(self) -> None:
+        presence_enabled_setting = "presence_enabled"
+
+        for val in [True, False]:
+            events = self.verify_action(
+                lambda: do_change_user_setting(
+                    self.user_profile, presence_enabled_setting, val, acting_user=self.user_profile
+                ),
+                num_events=3,
+            )
+            check_user_settings_update("events[0]", events[0])
+            check_update_global_notifications("events[1]", events[1], val)
+            check_presence(
+                "events[2]",
+                events[2],
+                has_email=True,
+                presence_key="website",
+                status="active" if val else "idle",
+            )
+
     def test_change_notification_sound(self) -> None:
         notification_setting = "notification_sound"