From b77ecca1a3422f1ed1d642b985bdb3094855a44a Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Thu, 12 Jun 2025 10:45:10 +0530 Subject: [PATCH] user_pill: Use user_id to get user info instead of email. --- web/src/add_group_members_pill.ts | 2 +- web/src/add_subscribers_pill.ts | 4 +-- web/src/compose_pm_pill.ts | 12 ++++++-- web/src/group_setting_pill.ts | 6 ++-- web/src/people.ts | 20 +++++++------- web/src/user_pill.ts | 16 +++++------ web/tests/compose_pm_pill.test.cjs | 44 ++++-------------------------- web/tests/user_pill.test.cjs | 38 ++++++++++++++------------ 8 files changed, 58 insertions(+), 84 deletions(-) diff --git a/web/src/add_group_members_pill.ts b/web/src/add_group_members_pill.ts index bb146d54b2..348798500c 100644 --- a/web/src/add_group_members_pill.ts +++ b/web/src/add_group_members_pill.ts @@ -79,7 +79,7 @@ export function create_item_from_text( return undefined; } - return user_pill.create_item_from_email(text, current_items); + return user_pill.create_item_from_user_id(text, current_items); } export function create({ diff --git a/web/src/add_subscribers_pill.ts b/web/src/add_subscribers_pill.ts index 73f3555ddd..7d1dc676e5 100644 --- a/web/src/add_subscribers_pill.ts +++ b/web/src/add_subscribers_pill.ts @@ -19,7 +19,7 @@ export function create_item_from_text( const funcs = [ stream_pill.create_item_from_stream_name, user_group_pill.create_item_from_group_name, - user_pill.create_item_from_email, + user_pill.create_item_from_user_id, ]; for (const func of funcs) { const item = func(text, current_items); @@ -40,7 +40,7 @@ export function get_text_from_item(item: CombinedPill): string { text = user_group_pill.get_group_name_from_item(item); break; case "user": - text = user_pill.get_email_from_item(item); + text = user_pill.get_unique_full_name_from_item(item); break; } return text; diff --git a/web/src/compose_pm_pill.ts b/web/src/compose_pm_pill.ts index 165a9f001c..97d4f342c0 100644 --- a/web/src/compose_pm_pill.ts +++ b/web/src/compose_pm_pill.ts @@ -20,8 +20,8 @@ export function initialize_pill(): UserPillWidget { const pill = input_pill.create({ $container, pill_config, - create_item_from_text: user_pill.create_item_from_email, - get_text_from_item: user_pill.get_email_from_item, + create_item_from_text: user_pill.create_item_from_user_id, + get_text_from_item: user_pill.get_unique_full_name_from_item, get_display_value_from_item: user_pill.get_display_value_from_item, generate_pill_html: (item: UserPill) => user_pill.generate_pill_html(item, true), }); @@ -66,7 +66,13 @@ export function set_from_typeahead(person: User): void { export function set_from_emails(value: string): void { // value is something like "alice@example.com,bob@example.com" clear(); - widget.appendValue(value); + if (value === "") { + return; + } + const user_ids_string = people.emails_strings_to_user_ids_string(value); + if (user_ids_string) { + widget.appendValue(user_ids_string); + } } export function set_from_user_ids(value: number[]): void { diff --git a/web/src/group_setting_pill.ts b/web/src/group_setting_pill.ts index ee78c315b5..82359a784f 100644 --- a/web/src/group_setting_pill.ts +++ b/web/src/group_setting_pill.ts @@ -56,7 +56,7 @@ function check_user_allowed_for_setting( return true; } - const user = people.get_by_email(user_item.email); + const user = people.maybe_get_user_by_id(user_item.user_id, true); return user !== undefined && !user.is_guest; } @@ -79,7 +79,7 @@ export function create_item_from_text( return undefined; } - const user_item = user_pill.create_item_from_email(text, current_items); + const user_item = user_pill.create_item_from_user_id(text, current_items); if (user_item) { if (check_user_allowed_for_setting(user_item, setting_name, setting_type)) { return user_item; @@ -97,7 +97,7 @@ export function get_text_from_item(item: GroupSettingPill): string { text = user_group_pill.get_group_name_from_item(item); break; case "user": - text = user_pill.get_email_from_item(item); + text = user_pill.get_unique_full_name_from_item(item); break; } return text; diff --git a/web/src/people.ts b/web/src/people.ts index c88837e124..cb30fb1c5c 100644 --- a/web/src/people.ts +++ b/web/src/people.ts @@ -117,14 +117,10 @@ export function get_users_from_ids(user_ids: number[]): User[] { } // Use this function only when you are sure that user_id is valid. -export let get_by_user_id = (user_id: number): User => { +export function get_by_user_id(user_id: number): User { const person = people_by_user_id_dict.get(user_id); assert(person, `Unknown user_id in get_by_user_id: ${user_id}`); return person; -}; - -export function rewire_get_by_user_id(value: typeof get_by_user_id): void { - get_by_user_id = value; } // This is type unsafe version of get_by_user_id for the callers that expects undefined values. @@ -155,7 +151,7 @@ export function validate_user_ids(user_ids: number[]): number[] { return good_ids; } -export let get_by_email = (email: string): User | undefined => { +export function get_by_email(email: string): User | undefined { const person = people_dict.get(email); if (!person) { @@ -169,10 +165,6 @@ export let get_by_email = (email: string): User | undefined => { } return person; -}; - -export function rewire_get_by_email(value: typeof get_by_email): void { - get_by_email = value; } export function get_bot_owner_user(user: User & {is_bot: true}): User | undefined { @@ -1492,6 +1484,14 @@ export function get_from_unique_full_name(query: string): User | undefined { return undefined; } +export function get_unique_full_name(full_name: string, user_id: number): string { + let unique_full_name = full_name; + if (is_duplicate_full_name(full_name)) { + unique_full_name += `|${user_id}`; + } + return unique_full_name; +} + export function get_mention_syntax(full_name: string, user_id?: number, silent = false): string { let mention = ""; if (silent) { diff --git a/web/src/user_pill.ts b/web/src/user_pill.ts index e1ea9fb533..890c830afd 100644 --- a/web/src/user_pill.ts +++ b/web/src/user_pill.ts @@ -35,15 +35,13 @@ export type UserPillWidget = InputPillContainer; export type UserPillData = {type: "user"; user: User}; -export function create_item_from_email( - email: string, +export function create_item_from_user_id( + user_id: string, current_items: CombinedPill[], pill_config?: InputPillConfig, ): UserPill | undefined { - const user = people.get_by_email(email); - + const user = people.maybe_get_user_by_id(Number(user_id), true); if (!user) { - // The email is not allowed, so return. return undefined; } @@ -84,8 +82,8 @@ export function create_item_from_email( return item; } -export function get_email_from_item(item: UserPill): string { - return item.email; +export function get_unique_full_name_from_item(item: UserPill): string { + return people.get_unique_full_name(item.full_name, item.user_id); } export function append_person( @@ -216,8 +214,8 @@ export function create_pills( const pills = input_pill.create({ $container: $pill_container, pill_config, - create_item_from_text: create_item_from_email, - get_text_from_item: get_email_from_item, + create_item_from_text: create_item_from_user_id, + get_text_from_item: get_unique_full_name_from_item, get_display_value_from_item, generate_pill_html, }); diff --git a/web/tests/compose_pm_pill.test.cjs b/web/tests/compose_pm_pill.test.cjs index 8d216c5648..5f57c0eeb3 100644 --- a/web/tests/compose_pm_pill.test.cjs +++ b/web/tests/compose_pm_pill.test.cjs @@ -18,7 +18,7 @@ let pills = { pill: {}, }; -run_test("pills", ({override, override_rewire}) => { +run_test("pills", ({override}) => { const me = { email: "me@example.com", user_id: 30, @@ -81,49 +81,18 @@ run_test("pills", ({override, override_rewire}) => { let appendValue_called; pills.appendValue = function (value) { appendValue_called = true; - assert.equal(value, "othello@example.com"); + assert.equal(value, othello.user_id.toString()); this.appendValidatedData(othello); }; - let get_by_email_called = false; - override_rewire(people, "get_by_email", (user_email) => { - get_by_email_called = true; - switch (user_email) { - case iago.email: - return iago; - case othello.email: - return othello; - /* istanbul ignore next */ - default: - throw new Error(`Unknown user email ${user_email}`); - } - }); - - let get_by_user_id_called = false; - override_rewire(people, "get_by_user_id", (id) => { - get_by_user_id_called = true; - switch (id) { - case othello.user_id: - return othello; - case hamlet.user_id: - return hamlet; - /* istanbul ignore next */ - default: - throw new Error(`Unknown user ID ${id}`); - } - }); - function test_create_item(handler) { (function test_rejection_path() { - const item = handler(othello.email, pills.items()); - assert.ok(get_by_email_called); + const item = handler(othello.user_id, pills.items()); assert.equal(item, undefined); })(); (function test_success_path() { - get_by_email_called = false; - const res = handler(iago.email, pills.items()); - assert.ok(get_by_email_called); + const res = handler(iago.user_id, pills.items()); assert.equal(typeof res, "object"); assert.equal(res.user_id, iago.user_id); assert.equal(res.full_name, iago.full_name); @@ -131,9 +100,7 @@ run_test("pills", ({override, override_rewire}) => { (function test_deactivated_pill() { people.deactivate(iago); - get_by_email_called = false; - const res = handler(iago.email, pills.items()); - assert.ok(get_by_email_called); + const res = handler(iago.user_id, pills.items()); assert.equal(typeof res, "object"); assert.equal(res.user_id, iago.user_id); assert.equal(res.full_name, iago.full_name); @@ -193,7 +160,6 @@ run_test("pills", ({override, override_rewire}) => { compose_pm_pill.set_from_emails("othello@example.com"); assert.ok(compose_pm_pill.widget); - assert.ok(get_by_user_id_called); assert.ok(pills_cleared); assert.ok(appendValue_called); assert.ok(text_cleared); diff --git a/web/tests/user_pill.test.cjs b/web/tests/user_pill.test.cjs index 94d61bca7e..563ac87970 100644 --- a/web/tests/user_pill.test.cjs +++ b/web/tests/user_pill.test.cjs @@ -27,11 +27,10 @@ const isaac = { full_name: "Isaac Newton", }; -const bogus_item = { - email: "bogus@example.com", - type: "user", - full_name: undefined, - // status_emoji_info: undefined, +const isaac_duplicate = { + email: "isaac_duplicate@example.com", + user_id: 102102, + full_name: "Isaac Newton", }; const isaac_item = { @@ -73,29 +72,34 @@ function test(label, f) { } test("create_item", ({override}) => { - function test_create_item(email, current_items, expected_item, pill_config) { - const item = user_pill.create_item_from_email(email, current_items, pill_config); + function test_create_item(user_id, current_items, expected_item, pill_config) { + const item = user_pill.create_item_from_user_id(user_id, current_items, pill_config); assert.deepEqual(item, expected_item); } settings_data.user_can_access_all_other_users = () => false; - test_create_item("bogus@example.com", [bogus_item], undefined); - test_create_item("bogus@example.com", [], undefined); - test_create_item("isaac@example.com", [], isaac_item); - test_create_item("isaac@example.com", [isaac_item], undefined); + test_create_item(isaac_item.user_id.toString(), [], isaac_item); + test_create_item(isaac_item.user_id.toString(), [isaac_item], undefined); override(realm, "realm_bot_domain", "example.com"); people.add_inaccessible_user(inaccessible_user_id); - test_create_item("user103@example.com", [], undefined, {exclude_inaccessible_users: true}); - test_create_item("user103@example.com", [], inaccessible_user_item, { + test_create_item(inaccessible_user_id.toString(), [], undefined, { + exclude_inaccessible_users: true, + }); + test_create_item(inaccessible_user_id.toString(), [], inaccessible_user_item, { exclude_inaccessible_users: false, }); }); -test("get_email", () => { - assert.equal(user_pill.get_email_from_item({email: "foo@example.com"}), "foo@example.com"); +test("get_unique_full_name_from_item", () => { + people.add_active_user(isaac); + people.add_active_user(isaac_duplicate); + assert.equal( + user_pill.get_unique_full_name_from_item({user_id: 1, full_name: isaac.full_name}), + "Isaac Newton|1", + ); }); test("append", () => { @@ -130,14 +134,14 @@ test("append", () => { }); test("get_items", () => { - const items = [isaac_item, bogus_item]; + const items = [isaac_item]; pill_widget.items = () => items; assert.deepEqual(user_pill.get_user_ids(pill_widget), [isaac.user_id]); }); test("typeahead", () => { - const items = [isaac_item, bogus_item]; + const items = [isaac_item]; pill_widget.items = () => items; // Both alice and isaac are in our realm, but isaac will be