diff --git a/web/src/activity.ts b/web/src/activity.ts index f235c521d5..023227f72f 100644 --- a/web/src/activity.ts +++ b/web/src/activity.ts @@ -10,6 +10,13 @@ import * as watchdog from "./watchdog"; const post_presence_response_schema = z.object({ msg: z.string(), result: z.string(), + // A bunch of these fields below are .optional() due to the fact + // that we have two modes of querying the presence endpoint: + // ping_only mode and a mode where we also fetch presence data + // for the realm. + // For ping_only requests, these fields are not returned in the + // response. If we're fetching presence data however, they should + // all be present, and send_presence_to_server() will validate that. server_timestamp: z.number().optional(), zephyr_mirror_active: z.boolean().optional(), presences: z @@ -21,6 +28,7 @@ const post_presence_response_schema = z.object({ }), ) .optional(), + presence_last_update_id: z.number().optional(), }); /* Keep in sync with views.py:update_active_status_backend() */ @@ -119,6 +127,7 @@ export function send_presence_to_server(redraw?: () => void): void { ping_only: !redraw, new_user_input, slim_presence: true, + last_update_id: presence.presence_last_update_id, }, success(response) { const data = post_presence_response_schema.parse(response); @@ -141,7 +150,16 @@ export function send_presence_to_server(redraw?: () => void): void { data.server_timestamp !== undefined, "Server timestamp should be present if not a ping only presence request", ); - presence.set_info(data.presences, data.server_timestamp); + assert( + data.presence_last_update_id !== undefined, + "Presence last update id should be present if not a ping only presence request", + ); + + presence.set_info( + data.presences, + data.server_timestamp, + data.presence_last_update_id, + ); redraw(); } }, diff --git a/web/src/activity_ui.ts b/web/src/activity_ui.ts index 9a5405c840..dad62269cf 100644 --- a/web/src/activity_ui.ts +++ b/web/src/activity_ui.ts @@ -78,6 +78,16 @@ export function redraw_user(user_id: number): void { }); } +export function check_should_redraw_new_user(user_id: number): boolean { + if (realm.realm_presence_disabled) { + return false; + } + + const user_is_in_presence_info = presence.presence_info.has(user_id); + const user_not_yet_known = people.maybe_get_user_by_id(user_id, true) === undefined; + return user_is_in_presence_info && user_not_yet_known; +} + export function searching(): boolean { return user_filter?.searching() ?? false; } diff --git a/web/src/buddy_data.ts b/web/src/buddy_data.ts index 572c6e8898..8f277861a4 100644 --- a/web/src/buddy_data.ts +++ b/web/src/buddy_data.ts @@ -1,4 +1,3 @@ -import * as blueslip from "./blueslip"; import * as hash_util from "./hash_util"; import {$t} from "./i18n"; import * as muted_users from "./muted_users"; @@ -312,10 +311,11 @@ function filter_user_ids(user_filter_text: string, user_ids: number[]): number[] // This first filter is for whether the user is eligible to be // displayed in the right sidebar at all. user_ids = user_ids.filter((user_id) => { - const person = people.maybe_get_user_by_id(user_id); + const person = people.maybe_get_user_by_id(user_id, true); if (!person) { - blueslip.warn("Got user_id in presence but not people: " + user_id); + // See the comments in presence.set_info for details, but this is an expected race. + // User IDs for whom we have presence but no user metadata should be skipped. return false; } diff --git a/web/src/presence.ts b/web/src/presence.ts index 35cd4c8a36..e192fdabf3 100644 --- a/web/src/presence.ts +++ b/web/src/presence.ts @@ -34,10 +34,16 @@ export type PresenceInfoFromEvent = { const raw_info = new Map(); export const presence_info = new Map(); -// We use this internally and export it for testing convenience. +// An integer that is updated whenever we get new presence data. +// TODO: Improve this comment. +export let presence_last_update_id = -1; + +// We keep and export this for testing convenience. export function clear_internal_data(): void { raw_info.clear(); presence_info.clear(); + + presence_last_update_id = -1; } const BIG_REALM_COUNT = 250; @@ -166,6 +172,7 @@ export function update_info_from_event( export function set_info( presences: Record>, server_timestamp: number, + last_update_id: number, ): void { /* Example `presences` data: @@ -177,7 +184,8 @@ export function set_info( } */ - clear_internal_data(); + presence_last_update_id = last_update_id; + for (const [user_id_str, info] of Object.entries(presences)) { const user_id = Number.parseInt(user_id_str, 10); @@ -190,19 +198,14 @@ export function set_info( // new users were created in the meantime, we may see user IDs // not yet present in people.js if server_events doesn't have // current data (or we've been offline, our event queue was - // GC'ed, and we're about to reload). Such user_ids being - // present could, in turn, create spammy downstream exceptions - // when rendering the buddy list. To address this, we check - // if the user ID is not yet present in people.js, and if it - // is, we skip storing that user (we'll see them again in the - // next presence request in 1 minute anyway). - // - // It's important to check both suspect_offline and - // reload_state.is_in_progress, because races where presence - // returns data on users not yet received via the server_events - // system are common in both situations. + // GC'ed, and we're about to reload). + // Despite that, we still add the presence data to our structures, + // and it is the job of the code using them to correctly + // ignore these until we receive the basic metadata on this user. + // We skip inaccessible users here, as we do in other places; + // presence info for them is not used. const person = people.maybe_get_user_by_id(user_id, true); - if (person === undefined || person.is_inaccessible_user) { + if (person?.is_inaccessible_user) { // There are a number of situations where it is expected // that we get presence data for a user ID that we do // not have in our user database, including when we're @@ -212,8 +215,8 @@ export function set_info( // and whenever presence wins a race with the events system // for events regarding a newly created or visible user. // - // Either way, we deal by skipping this user and - // continuing with processing everyone else. + // Either way, we still record the information unless + // we're dealing with an inaccessible user. continue; } @@ -281,6 +284,7 @@ export function last_active_date(user_id: number): Date | undefined { export function initialize(params: { presences: Record>; server_timestamp: number; + presence_last_update_id: number; }): void { - set_info(params.presences, params.server_timestamp); + set_info(params.presences, params.server_timestamp, params.presence_last_update_id); } diff --git a/web/src/server_events_dispatch.js b/web/src/server_events_dispatch.js index 1cd47b5a69..e32f02b78a 100644 --- a/web/src/server_events_dispatch.js +++ b/web/src/server_events_dispatch.js @@ -454,13 +454,24 @@ export function dispatch_normal_event(event) { case "realm_user": switch (event.op) { - case "add": + case "add": { + // There may be presence data we already received from the server + // before getting this event. Check if we need to redraw. + const should_redraw = activity_ui.check_should_redraw_new_user( + event.person.user_id, + ); + people.add_active_user(event.person); settings_account.maybe_update_deactivate_account_button(); if (event.person.is_bot) { settings_users.redraw_bots_list(); } + + if (should_redraw) { + activity_ui.redraw_user(event.person.user_id); + } break; + } case "update": user_events.update_person(event.person); settings_account.maybe_update_deactivate_account_button(); diff --git a/web/src/ui_init.js b/web/src/ui_init.js index bbf742965d..300383537f 100644 --- a/web/src/ui_init.js +++ b/web/src/ui_init.js @@ -480,7 +480,7 @@ export function initialize_everything(state_data) { const pm_conversations_params = pop_fields("recent_private_conversations"); - const presence_params = pop_fields("presences", "server_timestamp"); + const presence_params = pop_fields("presences", "server_timestamp", "presence_last_update_id"); const starred_messages_params = pop_fields("starred_messages"); const stream_data_params = pop_fields( diff --git a/web/tests/activity.test.js b/web/tests/activity.test.js index 9eb6271207..04dddf1c1f 100644 --- a/web/tests/activity.test.js +++ b/web/tests/activity.test.js @@ -805,6 +805,7 @@ test("initialize", ({override, mock_template}) => { msg: "", result: "success", server_timestamp: 0, + presence_last_update_id: -1, }); $(window).trigger("focus"); clear(); @@ -829,6 +830,7 @@ test("initialize", ({override, mock_template}) => { msg: "", result: "success", server_timestamp: 0, + presence_last_update_id: -1, }); assert.ok($("#zephyr-mirror-error").hasClass("show")); @@ -881,3 +883,20 @@ test("test_send_or_receive_no_presence_for_spectator", () => { page_params.is_spectator = true; activity.send_presence_to_server(); }); + +test("check_should_redraw_new_user", () => { + presence.presence_info.set(9999, {status: "active"}); + + // A user that wasn't yet known, but has presence info should be redrawn + // when being added. + assert.equal(activity_ui.check_should_redraw_new_user(9999), true); + + // We don't even build the user sidebar if realm_presence_disabled is true, + // so nothing to redraw. + realm.realm_presence_disabled = true; + assert.equal(activity_ui.check_should_redraw_new_user(9999), false); + + realm.realm_presence_disabled = false; + // A new user that didn't have presence info should not be redrawn. + assert.equal(activity_ui.check_should_redraw_new_user(99999), false); +}); diff --git a/web/tests/buddy_data.test.js b/web/tests/buddy_data.test.js index 8e43bb6e46..4581113413 100644 --- a/web/tests/buddy_data.test.js +++ b/web/tests/buddy_data.test.js @@ -6,7 +6,6 @@ const _ = require("lodash"); const {mock_esm, zrequire} = require("./lib/namespace"); const {run_test} = require("./lib/test"); -const blueslip = require("./lib/zblueslip"); const {current_user, realm, user_settings} = require("./lib/zpage_params"); const timerender = mock_esm("../src/timerender"); @@ -500,9 +499,16 @@ test("get_items_for_users", () => { ]); }); -test("error handling", () => { - presence.presence_info.set(42, {status: "active"}); - blueslip.expect("error", "Unknown user_id in maybe_get_user_by_id"); - blueslip.expect("warn", "Got user_id in presence but not people: 42"); - buddy_data.get_filtered_and_sorted_user_ids(); +test("unknown user id in presence", () => { + // This test is to make sure that we don't generate errors + // when we have a user_id in presence that we don't have + // information about. + // Such scenarios can happen if we receive presence info involving + // a new user before the user creation event reaches us. + presence.presence_info.set(999, {status: "active"}); + const user_ids = buddy_data.get_filtered_and_sorted_user_ids(); + + // This user id should not be present in the filtered list, + // it's meant to be ignored until we know about the user. + assert.equal(user_ids.includes(999), false); }); diff --git a/web/tests/dispatch.test.js b/web/tests/dispatch.test.js index a9c569b1be..bdc9f36338 100644 --- a/web/tests/dispatch.test.js +++ b/web/tests/dispatch.test.js @@ -125,6 +125,7 @@ const alert_words = zrequire("alert_words"); const emoji = zrequire("emoji"); const message_store = zrequire("message_store"); const people = zrequire("people"); +const presence = zrequire("presence"); const user_status = zrequire("user_status"); const onboarding_steps = zrequire("onboarding_steps"); @@ -728,8 +729,28 @@ run_test("realm_domains", ({override}) => { assert_same(realm.realm_domains, []); }); +run_test("add_realm_user_redraw_logic", ({override}) => { + presence.presence_info.set(999, {status: "active"}); + + override(settings_account, "maybe_update_deactivate_account_button", noop); + + const check_should_redraw_new_user_stub = make_stub(); + // make_stub().f returns true by default, so it's already doing what we want. + override(activity_ui, "check_should_redraw_new_user", check_should_redraw_new_user_stub.f); + const redraw_user_stub = make_stub(); + override(activity_ui, "redraw_user", redraw_user_stub.f); + + const event = event_fixtures.realm_user__add; + event.person.user_id = 999; + dispatch(event); + + assert.equal(redraw_user_stub.num_calls, 1); + assert.equal(redraw_user_stub.get_args("user_id").user_id, 999); +}); + run_test("realm_user", ({override}) => { override(settings_account, "maybe_update_deactivate_account_button", noop); + override(activity_ui, "check_should_redraw_new_user", noop); let event = event_fixtures.realm_user__add; dispatch({...event}); const added_person = people.get_by_user_id(event.person.user_id); diff --git a/web/tests/example4.test.js b/web/tests/example4.test.js index fa23f84894..067bb4cb52 100644 --- a/web/tests/example4.test.js +++ b/web/tests/example4.test.js @@ -86,6 +86,7 @@ run_test("add users with event", ({override}) => { // We need to override a stub here before dispatching the event. // Keep reading to see how overriding works! override(settings_users, "redraw_bots_list", noop); + override(activity_ui, "check_should_redraw_new_user", noop); // Let's simulate dispatching our event! server_events_dispatch.dispatch_normal_event(event); diff --git a/web/tests/presence.test.js b/web/tests/presence.test.js index b7469eb519..02e0a2e747 100644 --- a/web/tests/presence.test.js +++ b/web/tests/presence.test.js @@ -68,6 +68,11 @@ people.add_active_user(zoe); people.add_active_user(bot); people.add_active_user(john); people.add_active_user(jane); + +const inaccessible_user_id = 9999; +const inaccessible_user = people.add_inaccessible_user(inaccessible_user_id); +inaccessible_user.is_inaccessible_user = true; + people.initialize_current_user(me.user_id); function test(label, f) { @@ -83,17 +88,6 @@ test("my user", () => { assert.equal(presence.get_status(me.user_id), "active"); }); -test("unknown user", () => { - const unknown_user_id = 999; - const now = 888888; - const presences = {}; - presences[unknown_user_id.toString()] = "does-not-matter"; - - // We just skip the unknown user. - presence.set_info(presences, now); - assert.equal(presence.presence_info.get(unknown_user_id), undefined); -}); - test("status_from_raw", () => { const status_from_raw = presence.status_from_raw; @@ -137,6 +131,8 @@ test("set_presence_info", () => { const recent = now + 1 - OFFLINE_THRESHOLD_SECS; const a_while_ago = now - OFFLINE_THRESHOLD_SECS * 2; + const unknown_user_id = 999; + presences[alice.user_id.toString()] = { active_timestamp: recent, }; @@ -162,6 +158,15 @@ test("set_presence_info", () => { idle_timestamp: now, }; + // Unknown user ids can also be in the presence data. + presences[unknown_user_id.toString()] = { + idle_timestamp: now, + }; + + presences[inaccessible_user_id.toString()] = { + idle_timestamp: now, + }; + const params = {}; params.presences = presences; params.server_timestamp = now; @@ -204,6 +209,14 @@ test("set_presence_info", () => { assert.deepEqual(presence.presence_info.get(jane.user_id), {status: "idle", last_active: now}); assert.equal(presence.get_status(jane.user_id), "idle"); + + assert.deepEqual(presence.presence_info.get(unknown_user_id), { + status: "idle", + last_active: now, + }); + assert.equal(presence.get_status(unknown_user_id), "idle"); + + assert.equal(presence.presence_info.get(inaccessible_user_id), undefined); }); test("missing values", () => {