user_groups: Do not show invalid subgroups in typeahead.

We do not show groups that will break the DAG constraint
on being added to a group as subgroups in the typeahead
shown in the members edit UI.

Fixes #32087.

web/src/add_subscribers_pill.ts

@@ -11,6 +11,7 @@ import * as stream_pill from "./stream_pill";
 import type {CombinedPill, CombinedPillContainer} from "./typeahead_helper";
 import * as user_group_pill from "./user_group_pill";
 import * as user_groups from "./user_groups";
+import type {UserGroup} from "./user_groups";
 import * as user_pill from "./user_pill";
 
 export function create_item_from_text(
@@ -51,17 +52,28 @@ export function set_up_pill_typeahead({
     pill_widget,
     $pill_container,
     get_users,
+    get_user_groups,
 }: {
     pill_widget: CombinedPillContainer;
     $pill_container: JQuery;
     get_users: () => User[];
+    get_user_groups?: () => UserGroup[];
 }): void {
-    const opts = {
+    const opts: {
+        user_source: () => User[];
+        stream: boolean;
+        user_group: boolean;
+        user: boolean;
+        user_group_source?: () => UserGroup[];
+    } = {
         user_source: get_users,
         stream: true,
         user_group: true,
         user: true,
     };
+    if (get_user_groups !== undefined) {
+        opts.user_group_source = get_user_groups;
+    }
     pill_typeahead.set_up_combined($pill_container.find(".input"), pill_widget, opts);
 }
 
@@ -91,9 +103,11 @@ export function generate_pill_html(item: CombinedPill): string {
 export function create({
     $pill_container,
     get_potential_subscribers,
+    get_potential_groups,
 }: {
     $pill_container: JQuery;
     get_potential_subscribers: () => User[];
+    get_potential_groups?: () => UserGroup[];
 }): CombinedPillContainer {
     const pill_widget = input_pill.create<CombinedPill>({
         $container: $pill_container,
@@ -106,8 +120,26 @@ export function create({
         const potential_subscribers = get_potential_subscribers();
         return user_pill.filter_taken_users(potential_subscribers, pill_widget);
     }
+    const opts: {
+        pill_widget: CombinedPillContainer;
+        $pill_container: JQuery;
+        get_users: () => User[];
+        get_user_groups?: () => UserGroup[];
+    } = {
+        pill_widget,
+        $pill_container,
+        get_users,
+    };
+    if (get_potential_groups !== undefined) {
+        function get_user_groups(): UserGroup[] {
+            assert(get_potential_groups !== undefined);
+            const potential_groups = get_potential_groups();
+            return user_group_pill.filter_taken_groups(potential_groups, pill_widget);
+        }
+        opts.get_user_groups = get_user_groups;
+    }
 
-    set_up_pill_typeahead({pill_widget, $pill_container, get_users});
+    set_up_pill_typeahead(opts);
 
     const $pill_widget_input = $pill_container.find(".input");
     const $pill_widget_button = $pill_container.parent().find(".add-users-button");

web/src/pill_typeahead.ts

@@ -222,6 +222,7 @@ export function set_up_combined(
         user_group?: boolean;
         stream?: boolean;
         user_source?: () => User[];
+        user_group_source?: () => UserGroup[];
         exclude_bots?: boolean;
         update_func?: () => void;
     },
@@ -252,7 +253,14 @@ export function set_up_combined(
             }
 
             if (include_user_groups) {
-                source = [...source, ...user_group_pill.typeahead_source(pills)];
+                if (opts.user_group_source !== undefined) {
+                    const groups: UserGroupPillData[] = opts
+                        .user_group_source()
+                        .map((user_group) => ({type: "user_group", ...user_group}));
+                    source = [...source, ...groups];
+                } else {
+                    source = [...source, ...user_group_pill.typeahead_source(pills)];
+                }
             }
 
             if (include_users) {

web/src/user_group_edit_members.ts

@@ -44,6 +44,10 @@ function get_potential_members(): User[] {
     return people.filter_all_users(is_potential_member);
 }
 
+function get_potential_subgroups(): UserGroup[] {
+    return user_groups.get_potential_subgroups(current_group_id);
+}
+
 function get_user_group_members(group: UserGroup): (User | UserGroup)[] {
     const member_ids = [...group.members];
     const member_users = people.get_users_from_ids(member_ids);
@@ -141,6 +145,7 @@ export function enable_member_management({
     pill_widget = add_subscribers_pill.create({
         $pill_container,
         get_potential_subscribers: get_potential_members,
+        get_potential_groups: get_potential_subgroups,
     });
 
     $pill_container.find(".input").on("input", () => {

web/src/user_groups.ts

@@ -311,6 +311,30 @@ export function get_recursive_group_members(target_user_group: UserGroup): Set<n
     return members;
 }
 
+export function get_potential_subgroups(target_user_group_id: number): UserGroup[] {
+    // This logic could be optimized if we maintained a reverse map
+    // from each group to the groups containing it, which might be a
+    // useful data structure for other code paths as well.
+    const target_user_group = get_user_group_from_id(target_user_group_id);
+    const already_subgroup_ids = target_user_group.direct_subgroup_ids;
+    return get_all_realm_user_groups().filter((user_group) => {
+        if (user_group.id === target_user_group.id) {
+            return false;
+        }
+
+        if (already_subgroup_ids.has(user_group.id)) {
+            return false;
+        }
+
+        const recursive_subgroup_ids = get_recursive_subgroups(user_group);
+        assert(recursive_subgroup_ids !== undefined);
+        if (recursive_subgroup_ids.has(target_user_group.id)) {
+            return false;
+        }
+        return true;
+    });
+}
+
 export function is_user_in_group(
     user_group_id: number,
     user_id: number,

web/tests/pill_typeahead.test.js

@@ -465,7 +465,11 @@ run_test("set_up_combined", ({mock_template, override, override_rewire}) => {
                 })
                 .filter(Boolean);
             if (opts.user_group) {
-                expected_result = [...expected_result, ...group_items];
+                if (opts.user_group_source) {
+                    expected_result = [...expected_result, ...opts.user_group_source()];
+                } else {
+                    expected_result = [...expected_result, ...group_items];
+                }
             }
             if (opts.user) {
                 if (opts.user_source) {
@@ -530,6 +534,8 @@ run_test("set_up_combined", ({mock_template, override, override_rewire}) => {
         {user: true, user_source: () => [fred_item, mark_item]},
         {stream: true},
         {user_group: true},
+        // user and custom user group source.
+        {user_group: true, user_group_source: () => [admins_item]},
         {user_group: true, stream: true},
         {user_group: true, user: true},
         {user: true, stream: true},

web/tests/user_groups.test.js

@@ -583,3 +583,74 @@ run_test("get_display_group_name", () => {
     assert.equal(user_groups.get_display_group_name(all.name), "translated: Everyone");
     assert.equal(user_groups.get_display_group_name(students.name), "Students");
 });
+
+run_test("get_potential_subgroups", () => {
+    // Remove existing groups.
+    user_groups.init();
+
+    const admins = {
+        name: "Administrators",
+        id: 1,
+        members: new Set([1]),
+        is_system_group: false,
+        direct_subgroup_ids: new Set([4]),
+    };
+    const all = {
+        name: "Everyone",
+        id: 2,
+        members: new Set([2, 3]),
+        is_system_group: false,
+        direct_subgroup_ids: new Set([1, 3]),
+    };
+    const students = {
+        name: "Students",
+        id: 3,
+        members: new Set([4, 5]),
+        is_system_group: false,
+        direct_subgroup_ids: new Set([]),
+    };
+    const teachers = {
+        name: "Teachers",
+        id: 4,
+        members: new Set([6, 7, 8]),
+        is_system_group: false,
+        direct_subgroup_ids: new Set([]),
+    };
+    const science = {
+        name: "Science",
+        id: 5,
+        members: new Set([9]),
+        is_system_group: false,
+        direct_subgroup_ids: new Set([]),
+    };
+
+    user_groups.initialize({
+        realm_user_groups: [admins, all, students, teachers, science],
+    });
+
+    function get_potential_subgroup_ids(group_id) {
+        return user_groups
+            .get_potential_subgroups(group_id)
+            .map((subgroup) => subgroup.id)
+            .sort();
+    }
+
+    assert.deepEqual(get_potential_subgroup_ids(all.id), [teachers.id, science.id]);
+    assert.deepEqual(get_potential_subgroup_ids(admins.id), [students.id, science.id]);
+    assert.deepEqual(get_potential_subgroup_ids(teachers.id), [students.id, science.id]);
+    assert.deepEqual(get_potential_subgroup_ids(students.id), [admins.id, teachers.id, science.id]);
+    assert.deepEqual(get_potential_subgroup_ids(science.id), [
+        admins.id,
+        all.id,
+        students.id,
+        teachers.id,
+    ]);
+
+    user_groups.add_subgroups(all.id, [teachers.id]);
+    user_groups.add_subgroups(teachers.id, [science.id]);
+    assert.deepEqual(get_potential_subgroup_ids(all.id), [science.id]);
+    assert.deepEqual(get_potential_subgroup_ids(admins.id), [students.id, science.id]);
+    assert.deepEqual(get_potential_subgroup_ids(teachers.id), [students.id]);
+    assert.deepEqual(get_potential_subgroup_ids(students.id), [admins.id, teachers.id, science.id]);
+    assert.deepEqual(get_potential_subgroup_ids(science.id), [students.id]);
+});