mirror of
https://github.com/zulip/zulip.git
synced 2025-11-05 14:35:27 +00:00
people: Defer loading user data during app load.
This should greatly reduce the symptoms of a bug where a stale realm_users cache (missing a newly created user) results in the web client missing the new user and throwing exceptions.
This commit is contained in:
Aman Agrawal
Tim Abbott
@@ -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<number, User>;
|
||||
let pm_recipient_count_dict: Map<number, number>;
|
||||
let duplicate_full_name_data: FoldDict<Set<number>>;
|
||||
let my_user_id: number;
|
||||
let valid_user_ids: Set<number>;
|
||||
|
||||
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<typeof user_fetch_response_schema>;
|
||||
|
||||
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<unknown>) {
|
||||
if (opts.error) {
|
||||
opts.error(xhr);
|
||||
}
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
export async function fetch_users(user_ids: Set<number>): Promise<UsersFetchResponse["members"]> {
|
||||
// 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<void> {
|
||||
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);
|
||||
}
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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]));
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user