From a9f40a64fd6d5b77bcb509b1aec7ff0aaa5fb8e4 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Sat, 8 Apr 2023 15:52:48 +0200 Subject: [PATCH] presence: Support null values in UserPresence. --- zerver/actions/presence.py | 59 +++++++++-------- zerver/lib/presence.py | 36 ++++++++++- .../0443_userpresence_new_table_schema.py | 8 ++- zerver/models.py | 4 +- zerver/tests/test_events.py | 17 +++++ zerver/tests/test_presence.py | 64 ++++++++++++++++++- 6 files changed, 151 insertions(+), 37 deletions(-) diff --git a/zerver/actions/presence.py b/zerver/actions/presence.py index f3593b2b71..006148f11b 100644 --- a/zerver/actions/presence.py +++ b/zerver/actions/presence.py @@ -5,7 +5,10 @@ from django.conf import settings from django.db import transaction from zerver.actions.user_activity import update_user_activity_interval -from zerver.lib.presence import format_legacy_presence_dict +from zerver.lib.presence import ( + format_legacy_presence_dict, + user_presence_datetime_with_date_joined_default, +) from zerver.lib.queue import queue_json_publish from zerver.lib.timestamp import datetime_to_timestamp from zerver.models import Client, UserPresence, UserProfile, active_user_ids, get_client @@ -42,12 +45,17 @@ def send_presence_changed( # stop sending them at all. return + last_active_time = user_presence_datetime_with_date_joined_default( + presence.last_active_time, user_profile.date_joined + ) + last_connected_time = user_presence_datetime_with_date_joined_default( + presence.last_connected_time, user_profile.date_joined + ) + # The mobile app handles these events so we need to use the old format. # The format of the event should also account for the slim_presence # API parameter when this becomes possible in the future. - presence_dict = format_legacy_presence_dict( - presence.last_active_time, presence.last_connected_time - ) + presence_dict = format_legacy_presence_dict(last_active_time, last_connected_time) event = dict( type="presence", email=user_profile.email, @@ -81,37 +89,34 @@ def do_update_user_presence( ) -> None: client = consolidate_client(client) - # TODO: While we probably DO want creating an account to - # automatically create a first `UserPresence` object with - # last_connected_time and last_active_time as the current time, - # our presence tests don't understand this, and it'd be perhaps - # wrong for some cases of account creation via the API. So we may - # want a "never" value here as the default. + # If the user doesn't have a UserPresence row yet, we create one with + # sensible defaults. If we're getting a presence update, clearly the user + # at least connected, so last_connected_time should be set. last_active_time + # will depend on whether the status sent is idle or active. defaults = dict( - # Given that these are defaults for creation of a UserPresence row - # if one doesn't yet exist, the most sensible way to do this - # is to set both last_active_time and last_connected_time - # to log_time. - last_active_time=log_time, + last_active_time=None, last_connected_time=log_time, realm_id=user_profile.realm_id, ) - if status == UserPresence.LEGACY_STATUS_IDLE_INT: - # If the presence entry for the user is just to be created, and - # we want it to be created as idle, then there needs to be an appropriate - # offset between last_active_time and last_connected_time, since that's - # what the event-sending system calculates the status based on, via - # format_legacy_presence_dict. - defaults["last_active_time"] = log_time - datetime.timedelta( - seconds=settings.PRESENCE_LEGACY_EVENT_OFFSET_FOR_ACTIVITY_SECONDS + 1 - ) + if status == UserPresence.LEGACY_STATUS_ACTIVE_INT: + defaults["last_active_time"] = log_time (presence, created) = UserPresence.objects.get_or_create( user_profile=user_profile, defaults=defaults, ) - time_since_last_active = log_time - presence.last_active_time + if presence.last_active_time is not None: + time_since_last_active = log_time - presence.last_active_time + else: + # The user was never active, so let's consider this large to go over any thresholds + # we may have. + time_since_last_active = datetime.timedelta(days=1) + if presence.last_connected_time is not None: + time_since_last_connected = log_time - presence.last_connected_time + else: + # Same approach as above. + time_since_last_connected = datetime.timedelta(days=1) assert (3 * settings.PRESENCE_PING_INTERVAL_SECS + 20) <= settings.OFFLINE_THRESHOLD_SECS now_online = time_since_last_active > datetime.timedelta( @@ -136,7 +141,7 @@ def do_update_user_presence( # times per minute with multiple connected browser windows. # We also need to be careful not to wrongly "update" the timestamp if we actually already # have newer presence than the reported log_time. - if not created and log_time - presence.last_connected_time > datetime.timedelta( + if not created and time_since_last_connected > datetime.timedelta( seconds=settings.PRESENCE_UPDATE_MIN_FREQ_SECONDS ): presence.last_connected_time = log_time @@ -149,7 +154,7 @@ def do_update_user_presence( ): presence.last_active_time = log_time update_fields.append("last_active_time") - if log_time > presence.last_connected_time: + if presence.last_connected_time is None or log_time > presence.last_connected_time: # Update last_connected_time as well to ensure # last_connected_time >= last_active_time. presence.last_connected_time = log_time diff --git a/zerver/lib/presence.py b/zerver/lib/presence.py index 49e1a1c6a3..c3f01b884a 100644 --- a/zerver/lib/presence.py +++ b/zerver/lib/presence.py @@ -1,7 +1,7 @@ import datetime import time from collections import defaultdict -from typing import Any, Dict, Mapping, Sequence, Set +from typing import Any, Dict, Mapping, Optional, Sequence, Set from django.conf import settings from django.utils.timezone import now as timezone_now @@ -26,15 +26,43 @@ def get_presence_dicts_for_rows( for presence_row in all_rows: user_key = get_user_key(presence_row) + + last_active_time = user_presence_datetime_with_date_joined_default( + presence_row["last_active_time"], presence_row["user_profile__date_joined"] + ) + last_connected_time = user_presence_datetime_with_date_joined_default( + presence_row["last_connected_time"], presence_row["user_profile__date_joined"] + ) + info = get_user_presence_info( - presence_row["last_active_time"], - presence_row["last_connected_time"], + last_active_time, + last_connected_time, ) user_statuses[user_key] = info return user_statuses +def user_presence_datetime_with_date_joined_default( + dt: Optional[datetime.datetime], date_joined: datetime.datetime +) -> datetime.datetime: + """ + Our data models support UserPresence objects not having None + values for last_active_time/last_connected_time. The legacy API + however has always sent timestamps, so for backward + compatibility we cannot send such values through the API and need + to default to a sane datetime. + + This helper functions expects to take a last_active_time or + last_connected_time value and the date_joined of the user, which + will serve as the default value if the first argument is None. + """ + if dt is None: + return date_joined + + return dt + + def get_modern_user_presence_info( last_active_time: datetime.datetime, last_connected_time: datetime.datetime ) -> Dict[str, Any]: @@ -110,6 +138,7 @@ def get_presence_for_user( "user_profile__email", "user_profile_id", "user_profile__enable_offline_push_notifications", + "user_profile__date_joined", ) presence_rows = list(query) @@ -136,6 +165,7 @@ def get_presence_dict_by_realm( "user_profile__email", "user_profile_id", "user_profile__enable_offline_push_notifications", + "user_profile__date_joined", ) presence_rows = list(query) diff --git a/zerver/migrations/0443_userpresence_new_table_schema.py b/zerver/migrations/0443_userpresence_new_table_schema.py index 7bbe4dd016..e4a7d3ca10 100644 --- a/zerver/migrations/0443_userpresence_new_table_schema.py +++ b/zerver/migrations/0443_userpresence_new_table_schema.py @@ -113,11 +113,15 @@ class Migration(migrations.Migration): ), ( "last_connected_time", - models.DateTimeField(db_index=True, default=django.utils.timezone.now), + models.DateTimeField( + db_index=True, default=django.utils.timezone.now, null=True + ), ), ( "last_active_time", - models.DateTimeField(db_index=True, default=django.utils.timezone.now), + models.DateTimeField( + db_index=True, default=django.utils.timezone.now, null=True + ), ), ( "realm", diff --git a/zerver/models.py b/zerver/models.py index 218c0a1218..0eb9193dbf 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -4120,11 +4120,11 @@ class UserPresence(models.Model): # The last time the user had a client connected to Zulip, # including idle clients where the user hasn't interacted with the # system recently (and thus might be AFK). - last_connected_time = models.DateTimeField(default=timezone_now, db_index=True) + last_connected_time = models.DateTimeField(default=timezone_now, db_index=True, null=True) # The last time a client connected to Zulip reported that the user # was actually present (E.g. via focusing a browser window or # interacting with a computer running the desktop app) - last_active_time = models.DateTimeField(default=timezone_now, db_index=True) + last_active_time = models.DateTimeField(default=timezone_now, db_index=True, null=True) # The following constants are used in the presence API for # communicating whether a user is active (last_active_time recent) diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index e5bcb6802a..b2c526df09 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -1216,6 +1216,14 @@ class NormalActionsTest(BaseAction): def test_away_events(self) -> None: client = get_client("website") + # Updating user status to away activates the codepath of disabling + # the presence_enabled user setting. Correctly simulating the presence + # event status for a typical user requires settings the user's date_joined + # further into the past. See test_change_presence_enabled for more details, + # since it tests that codepath directly. + self.user_profile.date_joined = timezone_now() - datetime.timedelta(days=15) + self.user_profile.save() + # Set all away_val = True events = self.verify_action( @@ -1920,6 +1928,15 @@ class NormalActionsTest(BaseAction): def test_change_presence_enabled(self) -> None: presence_enabled_setting = "presence_enabled" + # Disabling presence will lead to the creation of a UserPresence object for the user + # with a last_connected_time slightly preceding the moment of flipping the setting + # and last_active_time set to None. The presence API defaults to user_profile.date_joined + # for backwards compatibility when dealing with a None value. Thus for this test to properly + # check that the presence event emitted will have "idle" status, we need to simulate + # the (more realistic) scenario where date_joined is further in the past and not super recent. + self.user_profile.date_joined = timezone_now() - datetime.timedelta(days=15) + self.user_profile.save() + for val in [True, False]: events = self.verify_action( lambda: do_change_user_setting( diff --git a/zerver/tests/test_presence.py b/zerver/tests/test_presence.py index 3bc4fb3d5f..d3767d4a83 100644 --- a/zerver/tests/test_presence.py +++ b/zerver/tests/test_presence.py @@ -71,6 +71,13 @@ class UserPresenceModelTests(ZulipTestCase): presence_dct = get_presence_dict_by_realm(user_profile.realm_id) self.assert_length(presence_dct, 0) + # If the values are set to "never", ignore it just like for sufficiently old presence rows. + UserPresence.objects.filter(id=user_profile.id).update( + last_active_time=None, last_connected_time=None + ) + presence_dct = get_presence_dict_by_realm(user_profile.realm_id) + self.assert_length(presence_dct, 0) + def test_pushable_always_false(self) -> None: # This field was never used by clients of the legacy API, so we # just want to have it always set to False for API format compatibility. @@ -284,10 +291,16 @@ class UserPresenceTests(ZulipTestCase): UserPresence.objects.all().delete() self.assertEqual(filter_presence_idle_user_ids({user_profile.id}), [user_profile.id]) - # Create a first presence for the user. It's the first one, so it'll initialize both last_active_time - # and last_connected time with the current time. Thus the user will be considered active and won't - # get filtered. + # Create a first presence for the user. It's the first one and has status idle, + # so it'll initialize just last_connected time with the current time and last_active_time with None. + # Thus the user will be considered idle. self.client_post("/json/users/me/presence", {"status": "idle"}) + self.assertEqual(filter_presence_idle_user_ids({user_profile.id}), [user_profile.id]) + + # Now create a first presence with active status and check that the user is not filtered. + # This initializes the presence with both last_connected_time and last_active_time set to + # current time. + self.client_post("/json/users/me/presence", {"status": "active"}) self.assertEqual(filter_presence_idle_user_ids({user_profile.id}), []) # Make last_active_time be older than OFFLINE_THRESHOLD_SECS. That should @@ -363,6 +376,48 @@ class UserPresenceTests(ZulipTestCase): {hamlet.email}, ) + def test_null_timestamps_handling(self) -> None: + """ + Checks that the API handles presences with null presence timestamps correctly. + The data model now supports them being null, but the API should still return + valid timestamps for backwards compatibility - using date_joined as the default + to fall back on. Also it should correctly filter out users with null presence + just like it would have filtered them out if they had very old presence. + """ + self.login("hamlet") + + othello = self.example_user("othello") + # Set a predictable value for date_joined + othello.date_joined = timezone_now() - timedelta(days=1) + othello.save() + UserPresence.objects.filter(user_profile=othello).update( + last_active_time=None, last_connected_time=None + ) + + result = self.client_get(f"/json/users/{othello.id}/presence") + result_dict = self.assert_json_success(result) + + # Ensure date_joined was used as the fallback. + self.assertEqual( + result_dict["presence"]["website"]["timestamp"], + datetime_to_timestamp(othello.date_joined), + ) + + # Othello has null presence values, so should not appear in the /realm/presence response + # just like a user with over two weeks old presence. + result = self.client_get("/json/realm/presence") + result_dict = self.assert_json_success(result) + self.assertEqual(result_dict["presences"], {}) + + # If othello's presence is fresh however, it should appear in the response. + now = timezone_now() + UserPresence.objects.filter(user_profile=othello).update( + last_active_time=now, last_connected_time=now + ) + result = self.client_get("/json/realm/presence") + result_dict = self.assert_json_success(result) + self.assertEqual(set(result_dict["presences"].keys()), {othello.email}) + class SingleUserPresenceTests(ZulipTestCase): def test_email_access(self) -> None: @@ -678,6 +733,7 @@ class FormatLegacyPresenceDictTest(ZulipTestCase): presence = UserPresence( user_profile=hamlet, realm=hamlet.realm, last_active_time=now, last_connected_time=now ) + assert presence.last_active_time is not None and presence.last_connected_time is not None self.assertEqual( format_legacy_presence_dict(presence.last_active_time, presence.last_connected_time), dict( @@ -694,6 +750,7 @@ class FormatLegacyPresenceDictTest(ZulipTestCase): last_active_time=recently, last_connected_time=now, ) + assert presence.last_active_time is not None and presence.last_connected_time is not None self.assertEqual( format_legacy_presence_dict(presence.last_active_time, presence.last_connected_time), dict( @@ -710,6 +767,7 @@ class FormatLegacyPresenceDictTest(ZulipTestCase): last_active_time=a_while_ago, last_connected_time=now, ) + assert presence.last_active_time is not None and presence.last_connected_time is not None self.assertEqual( format_legacy_presence_dict(presence.last_active_time, presence.last_connected_time), dict(