From d11537a8cd6a179e52090dfaf4902bad60df26fe Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Wed, 2 Apr 2025 13:27:11 +0000 Subject: [PATCH] streams: Change sorting order for add subscribers. See https://chat.zulip.org/#narrow/channel/101-design/topic/New.20channel.2Fgroup.20UX.20changes/with/2142992 for more details on the order. We decided to have a common function called `sort_setting_options` which accepts a compare function for `group` and `stream` settings each. --- web/src/add_subscribers_pill.ts | 2 + web/src/pill_typeahead.ts | 8 ++ web/src/typeahead_helper.ts | 127 ++++++++++++++++- web/tests/pill_typeahead.test.cjs | 40 +++++- web/tests/typeahead_helper.test.cjs | 205 ++++++++++++++++++++++++++-- 5 files changed, 360 insertions(+), 22 deletions(-) diff --git a/web/src/add_subscribers_pill.ts b/web/src/add_subscribers_pill.ts index 0367f63dde..9a3b16c00c 100644 --- a/web/src/add_subscribers_pill.ts +++ b/web/src/add_subscribers_pill.ts @@ -63,11 +63,13 @@ export function set_up_pill_typeahead({ user_group: boolean; user: boolean; user_group_source?: () => UserGroup[]; + is_stream_subscriber_input: boolean; } = { user_source: get_users, stream: true, user_group: true, user: true, + is_stream_subscriber_input: true, }; if (get_user_groups !== undefined) { opts.user_group_source = get_user_groups; diff --git a/web/src/pill_typeahead.ts b/web/src/pill_typeahead.ts index d78b53aad2..ac08baa8e5 100644 --- a/web/src/pill_typeahead.ts +++ b/web/src/pill_typeahead.ts @@ -264,6 +264,7 @@ export function set_up_combined( user_group_source?: () => UserGroup[]; exclude_bots?: boolean; update_func?: () => void; + is_stream_subscriber_input?: boolean; }, ): void { if (!opts.user && !opts.user_group && !opts.stream) { @@ -382,6 +383,13 @@ export function set_up_combined( } } + if (opts.is_stream_subscriber_input) { + return typeahead_helper.sort_stream_setting_options({ + users, + query, + groups, + }); + } return typeahead_helper.sort_recipients({ users, query, diff --git a/web/src/typeahead_helper.ts b/web/src/typeahead_helper.ts index 860ebe8a7c..9688461cb8 100644 --- a/web/src/typeahead_helper.ts +++ b/web/src/typeahead_helper.ts @@ -643,7 +643,7 @@ export function rewire_sort_recipients(value: typeof sort_recipients): void { sort_recipients = value; } -export function compare_setting_options( +export function compare_group_setting_options( option_a: UserPillData | UserGroupPillData, option_b: UserPillData | UserGroupPillData, target_group: UserGroup | undefined, @@ -703,18 +703,24 @@ export function compare_setting_options( return 1; } -export let sort_group_setting_options = ({ +export const sort_setting_options = ({ users, query, groups, + compare_setting_options, target_group, }: { users: UserPillData[]; query: string; groups: UserGroupPillData[]; + compare_setting_options: ( + option_a: UserPillData | UserGroupPillData, + option_b: UserPillData | UserGroupPillData, + target_group: UserGroup | undefined, + ) => number; target_group: UserGroup | undefined; }): (UserPillData | UserGroupPillData)[] => { - function sort_group_setting_items( + function sort_setting_items( objs: (UserPillData | UserGroupPillData)[], ): (UserPillData | UserGroupPillData)[] { objs.sort((option_a, option_b) => @@ -739,13 +745,13 @@ export let sort_group_setting_options = ({ return [user_groups.get_display_group_name(g.name)]; }); - const exact_matches = sort_group_setting_items([ + const exact_matches = sort_setting_items([ ...groups_results.exact_matches, ...users_name_results.exact_matches, ...email_results.exact_matches, ]); - const prefix_matches = sort_group_setting_items([ + const prefix_matches = sort_setting_items([ ...groups_results.begins_with_case_sensitive_matches, ...groups_results.begins_with_case_insensitive_matches, ...users_name_results.begins_with_case_sensitive_matches, @@ -754,13 +760,13 @@ export let sort_group_setting_options = ({ ...email_results.begins_with_case_insensitive_matches, ]); - const word_boundary_matches = sort_group_setting_items([ + const word_boundary_matches = sort_setting_items([ ...groups_results.word_boundary_matches, ...users_name_results.word_boundary_matches, ...email_results.word_boundary_matches, ]); - const no_matches = sort_group_setting_items([ + const no_matches = sort_setting_items([ ...groups_results.no_matches, ...email_results.no_matches, ]); @@ -796,10 +802,117 @@ export let sort_group_setting_options = ({ return setting_options.slice(0, MAX_ITEMS); }; +export let sort_group_setting_options = ({ + users, + query, + groups, + target_group, +}: { + users: UserPillData[]; + query: string; + groups: UserGroupPillData[]; + target_group: UserGroup | undefined; +}): (UserPillData | UserGroupPillData)[] => + sort_setting_options({ + users, + query, + groups, + compare_setting_options: compare_group_setting_options, + target_group, + }); + export function rewire_sort_group_setting_options(value: typeof sort_group_setting_options): void { sort_group_setting_options = value; } +export function compare_stream_setting_options( + option_a: UserPillData | UserGroupPillData, + option_b: UserPillData | UserGroupPillData, +): number { + if (option_a.type === "user_group") { + const user_group_a = user_groups.get_user_group_from_id(option_a.id); + if (user_group_a.name === "role:members") { + return -1; + } + } + if (option_b.type === "user_group") { + const user_group_b = user_groups.get_user_group_from_id(option_b.id); + if (user_group_b.name === "role:members") { + return 1; + } + } + + if (option_a.type === "user_group" && option_b.type === "user") { + const user_group_a = user_groups.get_user_group_from_id(option_a.id); + + if (user_group_a.is_system_group) { + return 1; + } + return -1; + } + + if (option_b.type === "user_group" && option_a.type === "user") { + const user_group_b = user_groups.get_user_group_from_id(option_b.id); + if (user_group_b.is_system_group) { + return -1; + } + return 1; + } + + if (option_a.type === "user_group" && option_b.type === "user_group") { + const user_group_a = user_groups.get_user_group_from_id(option_a.id); + const user_group_b = user_groups.get_user_group_from_id(option_b.id); + + if (user_group_a.is_system_group && !user_group_b.is_system_group) { + return 1; + } + + if (user_group_b.is_system_group && !user_group_a.is_system_group) { + return -1; + } + + if (user_group_a.name < user_group_b.name) { + return -1; + } + + return 1; + } + + assert(option_a.type === "user"); + assert(option_b.type === "user"); + + if (option_a.user.full_name < option_b.user.full_name) { + return -1; + } else if (option_a.user.full_name === option_b.user.full_name) { + return 0; + } + + return 1; +} + +export let sort_stream_setting_options = ({ + users, + query, + groups, +}: { + users: UserPillData[]; + query: string; + groups: UserGroupPillData[]; +}): (UserPillData | UserGroupPillData)[] => + sort_setting_options({ + users, + query, + groups, + compare_setting_options: compare_stream_setting_options, + target_group: undefined, + }); + +export function rewire_sort_stream_setting_options( + value: typeof sort_stream_setting_options, +): void { + sort_stream_setting_options = value; +} + type SlashCommand = { name: string; }; diff --git a/web/tests/pill_typeahead.test.cjs b/web/tests/pill_typeahead.test.cjs index 9aad919133..b168c70a0b 100644 --- a/web/tests/pill_typeahead.test.cjs +++ b/web/tests/pill_typeahead.test.cjs @@ -30,6 +30,7 @@ set_realm(realm); let sort_recipients_called = false; let sort_streams_called = false; let sort_group_setting_options_called = false; +let sort_stream_setting_options_called = false; const $fake_rendered_person = $.create("fake-rendered-person"); const $fake_rendered_stream = $.create("fake-rendered-stream"); const $fake_rendered_group = $.create("fake-rendered-group"); @@ -45,6 +46,10 @@ function override_typeahead_helper(override_rewire) { sort_recipients_called = true; return users; }); + override_rewire(typeahead_helper, "sort_stream_setting_options", ({users}) => { + sort_stream_setting_options_called = true; + return users; + }); } function user_item(user) { @@ -427,6 +432,13 @@ run_test("set_up_combined", ({mock_template, override, override_rewire}) => { update_func_called = true; } + function mock_pill_removes(widget) { + const pills = widget._get_pills_for_testing(); + for (const pill of pills) { + pill.$element.remove = noop; + } + } + let opts = {}; override(bootstrap_typeahead, "Typeahead", (input_element, config) => { assert.equal(input_element.$element, $fake_input); @@ -518,15 +530,31 @@ run_test("set_up_combined", ({mock_template, override, override_rewire}) => { config.sorter([denmark_item], stream_query); assert.ok(sort_streams_called); } - if (opts.user_group) { + if (opts.user_group && !opts.is_stream_subscriber_input) { sort_recipients_called = false; + sort_stream_setting_options_called = false; config.sorter([testers_item], group_query); assert.ok(sort_recipients_called); - } - if (opts.user) { + assert.ok(!sort_stream_setting_options_called); + } else if (opts.user_group && opts.is_stream_subscriber_input) { sort_recipients_called = false; + sort_stream_setting_options_called = false; + config.sorter([testers_item], group_query); + assert.ok(!sort_recipients_called); + assert.ok(sort_stream_setting_options_called); + } + if (opts.user && !opts.is_stream_subscriber_input) { + sort_recipients_called = false; + sort_stream_setting_options_called = false; config.sorter([me_item], person_query); assert.ok(sort_recipients_called); + assert.ok(!sort_stream_setting_options_called); + } else if (opts.user && opts.is_stream_subscriber_input) { + sort_recipients_called = false; + sort_stream_setting_options_called = false; + config.sorter([me_item], person_query); + assert.ok(!sort_recipients_called); + assert.ok(sort_stream_setting_options_called); } })(); @@ -599,6 +627,10 @@ run_test("set_up_combined", ({mock_template, override, override_rewire}) => { assert.equal(number_of_pills(), 3); assert.ok(update_func_called); + + // Clear pills for the next test. + mock_pill_removes($pill_widget); + $pill_widget.clear(); } })(); @@ -634,6 +666,8 @@ run_test("set_up_combined", ({mock_template, override, override_rewire}) => { for (const config of all_possible_opts) { opts = config; test_pill_typeahead(config); + opts = {...config, is_stream_subscriber_input: true}; + test_pill_typeahead({...config, is_stream_subscriber_input: true}); } // Special case to test coverage and to test diff --git a/web/tests/typeahead_helper.test.cjs b/web/tests/typeahead_helper.test.cjs index e7b56204a0..4c457eb3d0 100644 --- a/web/tests/typeahead_helper.test.cjs +++ b/web/tests/typeahead_helper.test.cjs @@ -178,9 +178,25 @@ const members_group = { members: new Set([]), is_system_group: true, }; +const members_group_item = user_group_item(members_group); + +const everyone_group = { + id: 6, + name: "role:everyone", + description: "", + members: new Set([]), + is_system_group: true, +}; user_groups.initialize({ - realm_user_groups: [bob_system_group, bob_group, second_bob_group, admins_group, members_group], + realm_user_groups: [ + bob_system_group, + bob_group, + second_bob_group, + admins_group, + members_group, + everyone_group, + ], }); function test(label, f) { @@ -1137,6 +1153,7 @@ test("sort_group_setting_options", ({override_rewire}) => { b_user_2.full_name, b_user_1.full_name, b_user_3.full_name, + everyone_group.name, members_group.name, admins_group.name, a_user.full_name, @@ -1150,6 +1167,7 @@ test("sort_group_setting_options", ({override_rewire}) => { b_user_2.full_name, b_user_1.full_name, b_user_3.full_name, + everyone_group.name, members_group.name, admins_group.name, a_user.full_name, @@ -1161,6 +1179,7 @@ test("sort_group_setting_options", ({override_rewire}) => { admins_group.name, a_user.full_name, bob_system_group.name, + everyone_group.name, members_group.name, bob_group.name, second_bob_group.name, @@ -1172,6 +1191,7 @@ test("sort_group_setting_options", ({override_rewire}) => { assert.deepEqual(get_group_setting_typeahead_result("me", second_bob_group), [ members_group.name, bob_system_group.name, + everyone_group.name, admins_group.name, bob_group.name, second_bob_group.name, @@ -1183,6 +1203,7 @@ test("sort_group_setting_options", ({override_rewire}) => { ]); assert.deepEqual(get_group_setting_typeahead_result("ever", second_bob_group), [ + everyone_group.name, members_group.name, bob_system_group.name, admins_group.name, @@ -1198,6 +1219,7 @@ test("sort_group_setting_options", ({override_rewire}) => { assert.deepEqual(get_group_setting_typeahead_result("translated: members", second_bob_group), [ members_group.name, bob_system_group.name, + everyone_group.name, admins_group.name, bob_group.name, second_bob_group.name, @@ -1219,30 +1241,189 @@ test("sort_group_setting_options", ({override_rewire}) => { ]); }); -test("compare_setting_options", () => { +test("compare_group_setting_options", () => { // User group has higher priority than user. - assert.equal(th.compare_setting_options(a_user_item, bob_group_item, bob_group), 1); - assert.equal(th.compare_setting_options(bob_group_item, a_user_item, bob_group), -1); + assert.equal(th.compare_group_setting_options(a_user_item, bob_group_item, bob_group), 1); + assert.equal(th.compare_group_setting_options(bob_group_item, a_user_item, bob_group), -1); // System user group has higher priority than other user groups. - assert.equal(th.compare_setting_options(bob_group_item, bob_system_group_item, bob_group), 1); - assert.equal(th.compare_setting_options(bob_system_group_item, bob_group_item, bob_group), -1); assert.equal( - th.compare_setting_options(admins_group_item, bob_system_group_item, bob_group), + th.compare_group_setting_options(bob_group_item, bob_system_group_item, bob_group), + 1, + ); + assert.equal( + th.compare_group_setting_options(bob_system_group_item, bob_group_item, bob_group), + -1, + ); + assert.equal( + th.compare_group_setting_options(admins_group_item, bob_system_group_item, bob_group), 1, ); // In case both groups are not system groups, alphabetical order is used to decide priority. - assert.equal(th.compare_setting_options(bob_group_item, admins_group_item, bob_group), 1); - assert.equal(th.compare_setting_options(admins_group_item, bob_group_item, bob_group), -1); + assert.equal(th.compare_group_setting_options(bob_group_item, admins_group_item, bob_group), 1); + assert.equal( + th.compare_group_setting_options(admins_group_item, bob_group_item, bob_group), + -1, + ); // A user who is a member of the group being changed has higher priority. // If both the users are not members of the group being changed, alphabetical order // is used to decide priority. - assert.equal(th.compare_setting_options(b_user_1_item, b_user_2_item, bob_group), -1); - assert.equal(th.compare_setting_options(b_user_1_item, b_user_2_item, second_bob_group), 1); + assert.equal(th.compare_group_setting_options(b_user_1_item, b_user_2_item, bob_group), -1); + assert.equal( + th.compare_group_setting_options(b_user_1_item, b_user_2_item, second_bob_group), + 1, + ); // Get coverage for case where two users have same names. Original order is preserved // in such cases. - assert.equal(th.compare_setting_options(b_user_1_item, b_user_1_item, bob_group), 0); + assert.equal(th.compare_group_setting_options(b_user_1_item, b_user_1_item, bob_group), 0); +}); + +test("sort_stream_setting_options", ({override_rewire}) => { + function get_stream_setting_typeahead_result(query) { + const users = people.get_realm_active_human_users().map((user) => ({type: "user", user})); + const groups = user_groups.get_all_realm_user_groups().map((group) => ({ + type: "user_group", + ...group, + })); + const result = th.sort_stream_setting_options({ + users, + query, + groups, + }); + return result.map((item) => { + if (item.type === "user") { + return item.user.full_name; + } + + return item.name; + }); + } + + assert.deepEqual(get_stream_setting_typeahead_result("Bo"), [ + bob_group.name, + second_bob_group.name, + b_user_1.full_name, + b_user_2.full_name, + b_user_3.full_name, + bob_system_group.name, + members_group.name, + admins_group.name, + a_user.full_name, + zman.full_name, + everyone_group.name, + ]); + + assert.deepEqual(get_stream_setting_typeahead_result("bo"), [ + bob_group.name, + second_bob_group.name, + b_user_1.full_name, + b_user_2.full_name, + b_user_3.full_name, + bob_system_group.name, + members_group.name, + admins_group.name, + a_user.full_name, + zman.full_name, + everyone_group.name, + ]); + + assert.deepEqual(get_stream_setting_typeahead_result("Z"), [ + zman.full_name, + admins_group.name, + a_user.full_name, + members_group.name, + bob_group.name, + second_bob_group.name, + b_user_1.full_name, + b_user_2.full_name, + b_user_3.full_name, + bob_system_group.name, + everyone_group.name, + ]); + + assert.deepEqual(get_stream_setting_typeahead_result("me"), [ + members_group.name, + admins_group.name, + bob_group.name, + second_bob_group.name, + a_user.full_name, + b_user_1.full_name, + b_user_2.full_name, + b_user_3.full_name, + zman.full_name, + bob_system_group.name, + everyone_group.name, + ]); + + assert.deepEqual(get_stream_setting_typeahead_result("ever"), [ + members_group.name, + everyone_group.name, + admins_group.name, + bob_group.name, + second_bob_group.name, + a_user.full_name, + b_user_1.full_name, + b_user_2.full_name, + b_user_3.full_name, + zman.full_name, + bob_system_group.name, + ]); + + assert.deepEqual(get_stream_setting_typeahead_result("translated: members"), [ + members_group.name, + admins_group.name, + bob_group.name, + second_bob_group.name, + a_user.full_name, + b_user_1.full_name, + b_user_2.full_name, + b_user_3.full_name, + zman.full_name, + bob_system_group.name, + everyone_group.name, + ]); + + override_rewire(bootstrap_typeahead, "MAX_ITEMS", 6); + assert.deepEqual(get_stream_setting_typeahead_result("Bo"), [ + bob_group.name, + second_bob_group.name, + b_user_1.full_name, + b_user_2.full_name, + b_user_3.full_name, + bob_system_group.name, + ]); +}); + +test("compare_stream_setting_options", () => { + // Non system user group has higher priority than user. + assert.equal(th.compare_stream_setting_options(a_user_item, bob_group_item), 1); + assert.equal(th.compare_stream_setting_options(bob_group_item, a_user_item), -1); + // User has higher priority than non `role:members` system user group. + assert.equal(th.compare_stream_setting_options(a_user_item, bob_system_group_item), -1); + assert.equal(th.compare_stream_setting_options(bob_system_group_item, a_user_item), 1); + + // System user group has lower priority than other user groups. + assert.equal(th.compare_stream_setting_options(bob_group_item, bob_system_group_item), -1); + assert.equal(th.compare_stream_setting_options(bob_system_group_item, bob_group_item), 1); + assert.equal(th.compare_stream_setting_options(admins_group_item, bob_system_group_item), -1); + // Members group always takes priority against any other group. + assert.equal(th.compare_stream_setting_options(bob_group_item, members_group_item), 1); + assert.equal(th.compare_stream_setting_options(members_group_item, bob_group_item), -1); + assert.equal(th.compare_stream_setting_options(admins_group_item, members_group_item), 1); + assert.equal(th.compare_stream_setting_options(members_group_item, admins_group_item), -1); + + // In case both groups are not system groups, alphabetical order is used to decide priority. + assert.equal(th.compare_stream_setting_options(bob_group_item, admins_group_item), 1); + assert.equal(th.compare_stream_setting_options(admins_group_item, bob_group_item), -1); + + // Use alphabetical order to compare two users. + assert.equal(th.compare_stream_setting_options(b_user_1_item, b_user_2_item), -1); + assert.equal(th.compare_stream_setting_options(b_user_2_item, b_user_1_item), 1); + + // Get coverage for case where two users have same names. Original order is preserved + // in such cases. + assert.equal(th.compare_stream_setting_options(b_user_1_item, b_user_1_item), 0); });