diff --git a/web/src/people.ts b/web/src/people.ts index 342bcabd4b..8afe876836 100644 --- a/web/src/people.ts +++ b/web/src/people.ts @@ -1,10 +1,11 @@ import md5 from "blueimp-md5"; import assert from "minimalistic-assert"; -import type {z} from "zod"; +import {z} from "zod"; import * as typeahead from "../shared/src/typeahead.ts"; import * as blueslip from "./blueslip.ts"; +import * as channel from "./channel.ts"; import {FoldDict} from "./fold_dict.ts"; import {$t} from "./i18n.ts"; import type {DisplayRecipientUser, Message, MessageWithBooleans} from "./message_store.ts"; @@ -14,8 +15,8 @@ import {page_params} from "./page_params.ts"; import * as reload_state from "./reload_state.ts"; import * as settings_config from "./settings_config.ts"; import * as settings_data from "./settings_data.ts"; -import type {CurrentUser, StateData, profile_datum_schema, user_schema} from "./state_data.ts"; -import {current_user, realm} from "./state_data.ts"; +import type {CurrentUser, StateData, profile_datum_schema} from "./state_data.ts"; +import {current_user, realm, user_schema} from "./state_data.ts"; import * as timerender from "./timerender.ts"; import {is_user_in_setting_group} from "./user_groups.ts"; import {user_settings} from "./user_settings.ts"; @@ -48,6 +49,7 @@ let cross_realm_dict: Map; let pm_recipient_count_dict: Map; let duplicate_full_name_data: FoldDict>; let my_user_id: number; +let valid_user_ids: Set; export let INACCESSIBLE_USER_NAME: string; export let WELCOME_BOT: User; @@ -71,6 +73,7 @@ export function init(): void { non_active_user_dict = new Map(); cross_realm_dict = new Map(); // keyed by user_id pm_recipient_count_dict = new Map(); + valid_user_ids = new Set(); // This maintains a set of ids of people with same full names. duplicate_full_name_data = new FoldDict(); @@ -81,6 +84,30 @@ export function init(): void { // WE INITIALIZE DATA STRUCTURES HERE! init(); +export const user_fetch_response_schema = z.object({ + members: z.array(user_schema), + result: z.string(), + msg: z.string(), +}); + +type UsersFetchResponse = z.infer; + +type FetchUserDataParams = { + user_ids: string; + client_gravatar?: boolean; + include_custom_profile_fields?: boolean; + success?: (users: UsersFetchResponse["members"]) => void; + error?: (xhr?: JQuery.jqXHR) => void; +}; + +export function add_valid_user_id(user_id: number): void { + valid_user_ids.add(user_id); +} + +export function is_valid_user_id(user_id: number): boolean { + return valid_user_ids.has(user_id); +} + export function split_to_ints(lst: string): number[] { return lst.split(",").map((s) => Number.parseInt(s, 10)); } @@ -1495,7 +1522,13 @@ export function _add_user(person: User): void { people_by_name_dict.set(person.full_name, person); } -export function add_active_user(person: User): void { +export function add_active_user(person: User, source = "inital_fetch"): void { + // To maintain the valid_user_ids data structure, we must add new + // users to that set when we learn about them. + if (source === "server_events") { + add_valid_user_id(person.user_id); + } + active_user_dict.set(person.user_id, person); _add_user(person); non_active_user_dict.delete(person.user_id); @@ -1849,19 +1882,112 @@ export function is_displayable_conversation_participant(user_id: number): boolea return !is_valid_bot_user(user_id) && is_person_active(user_id); } -export function initialize(my_user_id: number, params: StateData["people"]): void { - for (const person of params.realm_users) { - add_active_user(person); +export function populate_valid_user_ids(params: StateData["user_groups"]): void { + // Every valid user ID is guaranteed to exist in at least one + // system group, so we can us that to compute the set of valid + // user IDs in the realm. + for (const user_group of params.realm_user_groups) { + if (user_group.is_system_group) { + valid_user_ids = valid_user_ids.union(new Set(user_group.members)); + } + } +} + +export function fetch_users_from_server(opts: FetchUserDataParams): void { + const params = { + user_ids: opts.user_ids, + client_gravatar: opts.client_gravatar, + include_custom_profile_fields: opts.include_custom_profile_fields, + }; + + channel.get({ + url: "/json/users", + data: params, + success(data) { + if (opts.success) { + const members = user_fetch_response_schema.parse(data).members; + opts.success(members); + } + }, + error(xhr: JQuery.jqXHR) { + if (opts.error) { + opts.error(xhr); + } + }, + }); +} + +export async function fetch_users(user_ids: Set): Promise { + // Requested users outside the set of known valid user IDs likely + // reflect some sort of Zulip bug, so fetch and log them. + const invalid_user_ids = user_ids.difference(valid_user_ids); + if (invalid_user_ids.size > 0) { + blueslip.error("Ignored invalid user_ids: " + [...invalid_user_ids].join(", ")); } - for (const person of params.realm_non_active_users) { + const user_ids_to_fetch = valid_user_ids.intersection(user_ids); + if (user_ids_to_fetch.size === 0) { + return []; + } + return new Promise((resolve, reject) => { + fetch_users_from_server({ + user_ids: JSON.stringify([...user_ids_to_fetch]), + success(users) { + resolve(users); + }, + error(xhr) { + let error_message = "Failed to fetch users."; + if (xhr) { + const error = z.object({msg: z.string().optional()}).parse(xhr.responseJSON); + if (error.msg) { + error_message = error.msg; + } + } + blueslip.error(error_message); + reject(new Error(error_message)); + }, + }); + }); +} + +export async function initialize( + my_user_id: number, + people_params: StateData["people"], + user_group_params: StateData["user_groups"], +): Promise { + initialize_current_user(my_user_id); + populate_valid_user_ids(user_group_params); + + // Compute the set of user IDs that we know are valid in the + // organization, but do not have a copy of. + const user_ids_to_fetch = new Set(valid_user_ids); + for (const person of people_params.realm_users) { + add_active_user(person); + user_ids_to_fetch.delete(person.user_id); + } + + for (const person of people_params.realm_non_active_users) { non_active_user_dict.set(person.user_id, person); _add_user(person); + user_ids_to_fetch.delete(person.user_id); } - for (const person of params.cross_realm_bots) { + for (const person of people_params.cross_realm_bots) { add_cross_realm_user(person); + user_ids_to_fetch.delete(person.user_id); } - initialize_current_user(my_user_id); + // Fetch all the missing users. This code path is temporary: We + // plan to move to a model where the web app expects to have an + // incomplete users dataset in large organizations. + await fetch_users(user_ids_to_fetch).then((users) => { + for (const user of users) { + if (user.is_active) { + add_active_user(user); + } else { + non_active_user_dict.set(user.user_id, user); + _add_user(user); + } + } + }); } diff --git a/web/src/server_events_dispatch.js b/web/src/server_events_dispatch.js index 9c08643fd1..5a985e3384 100644 --- a/web/src/server_events_dispatch.js +++ b/web/src/server_events_dispatch.js @@ -540,7 +540,7 @@ export function dispatch_normal_event(event) { event.person.user_id, ); - people.add_active_user(event.person); + people.add_active_user(event.person, "server_events"); settings_account.maybe_update_deactivate_account_button(); if (event.person.is_bot) { settings_users.redraw_bots_list(); diff --git a/web/src/ui_init.js b/web/src/ui_init.js index 2590dd1b58..64b82ee41f 100644 --- a/web/src/ui_init.js +++ b/web/src/ui_init.js @@ -398,7 +398,7 @@ function initialize_unread_ui() { unread_ui.initialize({notify_server_messages_read: unread_ops.notify_server_messages_read}); } -export function initialize_everything(state_data) { +export async function initialize_everything(state_data) { /* When we initialize our various modules, a lot of them will consume data from the server @@ -460,7 +460,7 @@ export function initialize_everything(state_data) { compose_send_menu_popover.initialize(); realm_user_settings_defaults.initialize(state_data.realm_settings_defaults); - people.initialize(current_user.user_id, state_data.people); + await people.initialize(current_user.user_id, state_data.people, state_data.user_groups); starred_messages.initialize(state_data.starred_messages); // The emoji module must be initialized before the right sidebar diff --git a/web/tests/clipboard_handler.test.cjs b/web/tests/clipboard_handler.test.cjs index ebaea62bd9..f1a88ff6dd 100644 --- a/web/tests/clipboard_handler.test.cjs +++ b/web/tests/clipboard_handler.test.cjs @@ -7,15 +7,15 @@ const {JSDOM} = require("jsdom"); const {zrequire, mock_esm} = require("./lib/namespace.cjs"); const {run_test} = require("./lib/test.cjs"); -const clipboard_handler = zrequire("clipboard_handler"); -const stream_data = zrequire("stream_data"); -const people = zrequire("people"); const settings_config = zrequire("settings_config"); mock_esm("../src/user_settings", { user_settings: { web_channel_default_view: settings_config.web_channel_default_view_values.channel_feed.code, }, }); +const clipboard_handler = zrequire("clipboard_handler"); +const stream_data = zrequire("stream_data"); +const people = zrequire("people"); const hamlet = { user_id: 15, diff --git a/web/tests/echo.test.cjs b/web/tests/echo.test.cjs index 261db3cc61..26c6646ecd 100644 --- a/web/tests/echo.test.cjs +++ b/web/tests/echo.test.cjs @@ -216,9 +216,17 @@ run_test("build_display_recipient", ({override}) => { user_id: 21, }, ]; + const user_group_params = { + realm_user_groups: [ + { + is_system_group: true, + members: [123, 21], + }, + ], + }; params.realm_non_active_users = []; params.cross_realm_bots = []; - people.initialize(current_user.user_id, params); + people.initialize(current_user.user_id, params, user_group_params); let message = { type: "stream", @@ -232,6 +240,7 @@ run_test("build_display_recipient", ({override}) => { message = { type: "private", + to_user_ids: "21", private_message_recipient: "cordelia@zulip.com", sender_email: "iago@zulip.com", sender_full_name: "Iago", @@ -252,6 +261,7 @@ run_test("build_display_recipient", ({override}) => { message = { type: "private", + to_user_ids: "123", private_message_recipient: "iago@zulip.com", sender_email: "iago@zulip.com", sender_full_name: "Iago", @@ -342,10 +352,24 @@ run_test("insert_local_message direct message", ({override}) => { full_name: "Iago", email: "iago@zulip.com", }, + { + email: "cordelia@zulip.com", + full_name: "Cordelia", + user_id: 21, + }, ]; + const user_group_params = { + realm_user_groups: [ + { + is_system_group: true, + members: [123, 21], + }, + ], + }; params.realm_non_active_users = []; params.cross_realm_bots = []; - people.initialize(current_user.user_id, params); + people.init(); + people.initialize(current_user.user_id, params, user_group_params); let render_called = false; let insert_message_called = false; @@ -362,6 +386,7 @@ run_test("insert_local_message direct message", ({override}) => { const message_request = { private_message_recipient: "cordelia@zulip.com", + to_user_ids: "21", type: "private", sender_email: "iago@zulip.com", sender_full_name: "Iago", diff --git a/web/tests/people.test.cjs b/web/tests/people.test.cjs index 608cad5f92..ec4c6b9be4 100644 --- a/web/tests/people.test.cjs +++ b/web/tests/people.test.cjs @@ -16,6 +16,7 @@ const message_user_ids = mock_esm("../src/message_user_ids"); const settings_data = mock_esm("../src/settings_data", { user_can_access_all_other_users: () => true, }); +const channel = mock_esm("../src/channel"); const muted_users = zrequire("muted_users"); const people = zrequire("people"); @@ -1342,21 +1343,33 @@ test_people("initialize", () => { }; params.realm_non_active_users = [retiree]; + const current_user = { + email: "my_email@example.com", + user_id: 42, + full_name: "Me Myself", + }; const alice = { email: "alice@example.com", user_id: 16, full_name: "Alice", }; - params.realm_users = [alice]; + params.realm_users = [alice, current_user]; const test_bot = { email: "bot@example.com", user_id: 17, full_name: "Test Bot", }; params.cross_realm_bots = [test_bot]; + const user_group_params = { + realm_user_groups: [ + { + is_system_group: true, + members: [42, 17, 16, 15], + }, + ], + }; - const my_user_id = 42; - people.initialize(my_user_id, params); + people.initialize(current_user.user_id, params, user_group_params); assert.equal(people.is_active_user_for_popover(17), true); assert.ok(people.is_cross_realm_email("bot@example.com")); @@ -1627,3 +1640,117 @@ test_people("sort_by_username", () => { run_test("reset MockDate", () => { MockDate.reset(); }); + +test_people("fetch_users", async ({override}) => { + people.init(); + + // Valid users missing from params data sent by server. + const users_in_response = [ + { + email: "retiree@example.com", + user_id: 15, + full_name: "Retiree", + delivery_email: "", + date_joined: "", + is_active: true, + is_owner: false, + is_admin: false, + is_guest: false, + role: 1, + avatar_url: "", + avatar_version: 1, + is_bot: false, + }, + { + email: "alice@example.com", + user_id: 16, + full_name: "Alice", + delivery_email: "", + date_joined: "", + is_active: false, + is_owner: false, + is_admin: false, + is_guest: false, + role: 1, + avatar_url: "", + avatar_version: 1, + is_bot: false, + }, + ]; + + const params = {}; + params.realm_users = [ + { + email: "my_email@example.com", + user_id: 42, + full_name: "Me Myself", + }, + ]; + params.realm_non_active_users = []; + params.cross_realm_bots = [ + { + email: "bot@example.com", + user_id: 17, + full_name: "Test Bot", + }, + ]; + const user_group_params = { + realm_user_groups: [ + { + is_system_group: true, + members: [42, 17, 16, 15], + }, + ], + }; + const my_user_id = 42; + + override(channel, "get", ({url, data, success, _error}) => { + assert.equal(url, "/json/users"); + assert.ok(data.user_ids.includes("15")); + assert.ok(data.user_ids.includes("16")); + assert.ok(!data.user_ids.includes("42")); + assert.ok(!data.user_ids.includes("17")); + success({ + members: users_in_response, + result: "success", + msg: "", + }); + }); + + await people.initialize(my_user_id, params, user_group_params); + + const retiree = people.get_by_user_id(15); + const alice = people.get_by_user_id(16); + assert.equal(retiree.full_name, "Retiree"); + assert.equal(alice.full_name, "Alice"); + assert.ok(people.is_valid_user_id(15)); + assert.ok(people.is_valid_user_id(16)); + assert.ok(people.is_valid_user_id(42)); + assert.ok(people.is_valid_user_id(17)); + assert.equal(people.get_by_email("alice@example.com").user_id, 16); + assert.equal(people.get_by_email("retiree@example.com").user_id, 15); + + override(channel, "get", ({url, _data, _success, error}) => { + assert.equal(url, "/json/users"); + // Return error response. + error({responseJSON: {msg: "test error"}}); + }); + + // fetch_users should reject with an Error object + blueslip.expect("error", "test error"); + await assert.rejects( + async () => { + await people.fetch_users(new Set([15, 16])); + }, + (err) => { + // Check that the error is an instance of Error and has the correct message + assert.ok(err instanceof Error); + assert.equal(err.message, "test error"); + return true; + }, + ); + + // Just for coverage, not actually checked by blueslip. + blueslip.expect("error", "Ignored invalid user_ids: 1, 2"); + await people.fetch_users(new Set([1, 2])); +});