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

@@ -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;

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,

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;
 };

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);
-            }
+                assert.ok(!sort_stream_setting_options_called);
-            if (opts.user) {
+            } 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

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_group_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(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_group_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(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_group_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, 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);
 });