user_group_edit: Add support to update subgroups of existing groups.

tools/test-js-with-node

@@ -46,6 +46,7 @@ EXEMPT_FILES = make_set(
     [
         "web/shared/src/poll_data.ts",
         "web/src/about_zulip.ts",
+        "web/src/add_group_members_pill.ts",
         "web/src/add_stream_options_popover.ts",
         "web/src/add_subscribers_pill.ts",
         "web/src/admin.js",

web/src/add_group_members_pill.ts

@@ -0,0 +1,61 @@
+import * as keydown_util from "./keydown_util";
+import * as stream_pill from "./stream_pill";
+import type {CombinedPillContainer} from "./typeahead_helper";
+import * as user_group_pill from "./user_group_pill";
+import * as user_pill from "./user_pill";
+
+function get_pill_user_ids(pill_widget: CombinedPillContainer): number[] {
+    const user_ids = user_pill.get_user_ids(pill_widget);
+    const stream_user_ids = stream_pill.get_user_ids(pill_widget);
+    return [...user_ids, ...stream_user_ids];
+}
+
+function get_pill_group_ids(pill_widget: CombinedPillContainer): number[] {
+    const group_user_ids = user_group_pill.get_group_ids(pill_widget);
+    return group_user_ids;
+}
+
+export function set_up_handlers({
+    get_pill_widget,
+    $parent_container,
+    pill_selector,
+    button_selector,
+    action,
+}: {
+    get_pill_widget: () => CombinedPillContainer;
+    $parent_container: JQuery;
+    pill_selector: string;
+    button_selector: string;
+    action: ({
+        pill_user_ids,
+        pill_group_ids,
+    }: {
+        pill_user_ids: number[];
+        pill_group_ids: number[];
+    }) => void;
+}): void {
+    /*
+        This is similar to add_subscribers_pill.set_up_handlers
+        with only difference that selecting a user group does
+        not add all its members to list, but instead just adds
+        the group itself.
+    */
+    function callback(): void {
+        const pill_widget = get_pill_widget();
+        const pill_user_ids = get_pill_user_ids(pill_widget);
+        const pill_group_ids = get_pill_group_ids(pill_widget);
+        action({pill_user_ids, pill_group_ids});
+    }
+
+    $parent_container.on("keyup", pill_selector, (e) => {
+        if (keydown_util.is_enter_event(e)) {
+            e.preventDefault();
+            callback();
+        }
+    });
+
+    $parent_container.on("click", button_selector, (e) => {
+        e.preventDefault();
+        callback();
+    });
+}

web/src/server_events_dispatch.js

@@ -967,9 +967,11 @@ export function dispatch_normal_event(event) {
                     break;
                 case "add_subgroups":
                     user_groups.add_subgroups(event.group_id, event.direct_subgroup_ids);
+                    user_group_edit.handle_subgroup_edit_event(event.group_id);
                     break;
                 case "remove_subgroups":
                     user_groups.remove_subgroups(event.group_id, event.direct_subgroup_ids);
+                    user_group_edit.handle_subgroup_edit_event(event.group_id);
                     break;
                 case "update":
                     user_groups.update(event);

web/src/user_group_components.ts

@@ -1,7 +1,10 @@
 import $ from "jquery";
 
 import {$t_html} from "./i18n";
+import * as people from "./people";
+import type {User} from "./people";
 import type {UserGroup} from "./user_groups";
+import * as user_sort from "./user_sort";
 
 export let active_group_id: number | undefined;
 
@@ -60,3 +63,62 @@ export function update_footer_buttons(container_name: string): void {
         $("#groups_overlay #user_group_go_to_configure_settings").hide();
     }
 }
+
+export function sort_group_member_email(a: User | UserGroup, b: User | UserGroup): number {
+    if ("user_id" in a && "user_id" in b) {
+        return user_sort.sort_email(a, b);
+    }
+
+    if ("user_id" in a) {
+        return -1;
+    }
+
+    if ("user_id" in b) {
+        return 1;
+    }
+
+    return user_sort.compare_a_b(a.name.toLowerCase(), b.name.toLowerCase());
+}
+
+export function sort_group_member_name(a: User | UserGroup, b: User | UserGroup): number {
+    let a_name;
+    if ("user_id" in a) {
+        a_name = a.full_name;
+    } else {
+        a_name = a.name;
+    }
+
+    let b_name;
+    if ("user_id" in b) {
+        b_name = b.full_name;
+    } else {
+        b_name = b.name;
+    }
+
+    return user_sort.compare_a_b(a_name.toLowerCase(), b_name.toLowerCase());
+}
+
+export function build_group_member_matcher(query: string): (member: User | UserGroup) => boolean {
+    query = query.trim();
+
+    const termlets = query.toLowerCase().split(/\s+/);
+    const termlet_matchers = termlets.map((termlet) => people.build_termlet_matcher(termlet));
+
+    return function (member: User | UserGroup): boolean {
+        if ("user_id" in member) {
+            const email = member.email.toLowerCase();
+
+            if (email.startsWith(query)) {
+                return true;
+            }
+
+            return termlet_matchers.every((matcher) => matcher(member));
+        }
+
+        const group_name = member.name;
+        if (group_name.startsWith(query)) {
+            return true;
+        }
+        return false;
+    };
+}

web/src/user_group_edit.js

@@ -285,6 +285,18 @@ function update_group_membership_button(group_id) {
     }
 }
 
+export function handle_subgroup_edit_event(group_id) {
+    if (!overlays.groups_open()) {
+        return;
+    }
+    const group = user_groups.get_user_group_from_id(group_id);
+
+    // update members list if currently rendered.
+    if (is_editing_group(group_id)) {
+        user_group_edit_members.update_member_list_widget(group);
+    }
+}
+
 export function handle_member_edit_event(group_id, user_ids) {
     if (!overlays.groups_open()) {
         return;

web/src/user_group_edit_members.ts

@@ -6,7 +6,9 @@ import render_leave_user_group_modal from "../templates/confirm_dialog/confirm_u
 import render_user_group_member_list_entry from "../templates/stream_settings/stream_member_list_entry.hbs";
 import render_user_group_members_table from "../templates/user_group_settings/user_group_members_table.hbs";
 import render_user_group_membership_request_result from "../templates/user_group_settings/user_group_membership_request_result.hbs";
+import render_user_group_subgroup_entry from "../templates/user_group_settings/user_group_subgroup_entry.hbs";
 
+import * as add_group_members_pill from "./add_group_members_pill";
 import * as add_subscribers_pill from "./add_subscribers_pill";
 import * as blueslip from "./blueslip";
 import * as channel from "./channel";
@@ -20,13 +22,13 @@ import * as scroll_util from "./scroll_util";
 import * as settings_data from "./settings_data";
 import {current_user} from "./state_data";
 import type {CombinedPillContainer} from "./typeahead_helper";
+import * as user_group_components from "./user_group_components";
 import * as user_groups from "./user_groups";
 import type {UserGroup} from "./user_groups";
-import * as user_sort from "./user_sort";
 
 export let pill_widget: CombinedPillContainer;
 let current_group_id: number;
-let member_list_widget: ListWidgetType<User, User>;
+let member_list_widget: ListWidgetType<User | UserGroup, User | UserGroup>;
 
 function get_potential_members(): User[] {
     const group = user_groups.get_user_group_from_id(current_group_id);
@@ -42,15 +44,22 @@ function get_potential_members(): User[] {
     return people.filter_all_users(is_potential_member);
 }
 
-function get_user_group_members(group: UserGroup): User[] {
+function get_user_group_members(group: UserGroup): (User | UserGroup)[] {
     const member_ids = [...group.members];
-    return people.get_users_from_ids(member_ids);
+    const member_users = people.get_users_from_ids(member_ids);
+    people.sort_but_pin_current_user_on_top(member_users);
+
+    const subgroup_ids = [...group.direct_subgroup_ids];
+    const subgroups = subgroup_ids
+        .map((group_id) => user_groups.get_user_group_from_id(group_id))
+        .sort(user_group_components.sort_group_member_name);
+
+    return [...subgroups, ...member_users];
 }
 
 export function update_member_list_widget(group: UserGroup): void {
     assert(group.id === current_group_id, "Unexpected group rerendering members list");
     const users = get_user_group_members(group);
-    people.sort_but_pin_current_user_on_top(users);
     member_list_widget.replace_list_data(users);
 }
 
@@ -66,6 +75,14 @@ function format_member_list_elem(person: User): string {
     });
 }
 
+function format_subgroup_list_elem(group: UserGroup): string {
+    return render_user_group_subgroup_entry({
+        group_id: group.id,
+        display_value: group.name,
+        can_edit: settings_data.can_manage_user_group(current_group_id),
+    });
+}
+
 function make_list_widget({
     $parent_container,
     name,
@@ -73,10 +90,8 @@ function make_list_widget({
 }: {
     $parent_container: JQuery;
     name: string;
-    users: User[];
+    users: (User | UserGroup)[];
-}): ListWidgetType<User, User> {
+}): ListWidgetType<User | UserGroup, User | UserGroup> {
-    people.sort_but_pin_current_user_on_top(users);
-
     const $list_container = $parent_container.find(".member_table");
     $list_container.empty();
 
@@ -87,17 +102,19 @@ function make_list_widget({
         get_item: ListWidget.default_get_item,
         $parent_container,
         sort_fields: {
-            email: user_sort.sort_email,
+            email: user_group_components.sort_group_member_email,
-            id: user_sort.sort_user_id,
+            name: user_group_components.sort_group_member_name,
-            ...ListWidget.generic_sort_functions("alphabetic", ["full_name"]),
         },
         modifier_html(item) {
-            return format_member_list_elem(item);
+            if ("user_id" in item) {
+                return format_member_list_elem(item);
+            }
+            return format_subgroup_list_elem(item);
         },
         filter: {
             $element: $parent_container.find<HTMLInputElement>("input.search"),
             predicate(person, value) {
-                const matcher = people.build_person_matcher(value);
+                const matcher = user_group_components.build_group_member_matcher(value);
                 const match = matcher(person);
 
                 return match;
@@ -162,12 +179,16 @@ function show_user_group_membership_request_result({
     remove_class,
     already_added_users,
     ignored_deactivated_users,
+    already_added_subgroups,
+    ignored_deactivated_groups,
 }: {
     message: string;
     add_class: string;
     remove_class: string;
     already_added_users?: User[];
     ignored_deactivated_users?: User[];
+    already_added_subgroups?: UserGroup[];
+    ignored_deactivated_groups?: UserGroup[];
 }): void {
     const $user_group_subscription_req_result_elem = $(
         ".user_group_subscription_request_result",
@@ -176,6 +197,8 @@ function show_user_group_membership_request_result({
         message,
         already_added_users,
         ignored_deactivated_users,
+        already_added_subgroups,
+        ignored_deactivated_groups,
     });
     scroll_util.get_content_element($user_group_subscription_req_result_elem).html(html);
     $user_group_subscription_req_result_elem.addClass(add_class);
@@ -186,12 +209,16 @@ export function edit_user_group_membership({
     group,
     added = [],
     removed = [],
+    added_subgroups = [],
+    removed_subgroups = [],
     success,
     error,
 }: {
     group: UserGroup;
     added?: number[];
     removed?: number[];
+    added_subgroups?: number[];
+    removed_subgroups?: number[];
     success: () => void;
     error: (xhr?: JQuery.jqXHR) => void;
 }): void {
@@ -200,13 +227,21 @@ export function edit_user_group_membership({
         data: {
             add: JSON.stringify(added),
             delete: JSON.stringify(removed),
+            add_subgroups: JSON.stringify(added_subgroups),
+            delete_subgroups: JSON.stringify(removed_subgroups),
         },
         success,
         error,
     });
 }
 
-function add_new_members({pill_user_ids}: {pill_user_ids: number[]}): void {
+function add_new_members({
+    pill_user_ids,
+    pill_group_ids,
+}: {
+    pill_user_ids: number[];
+    pill_group_ids: number[];
+}): void {
     const group = user_groups.get_user_group_from_id(current_group_id);
     if (!group) {
         return;
@@ -258,17 +293,56 @@ function add_new_members({pill_user_ids}: {pill_user_ids: number[]}): void {
         );
     }
 
-    if (user_id_set.size === 0) {
+    const deactivated_groups = new Set<number>();
+    const already_added_subgroups = new Set<number>();
+
+    const existing_subgroup_ids = new Set(group.direct_subgroup_ids);
+    const subgroup_ids_to_add = pill_group_ids.filter((group_id) => {
+        const subgroup = user_groups.get_user_group_from_id(group_id);
+        if (subgroup.deactivated) {
+            deactivated_groups.add(group_id);
+            return false;
+        }
+
+        if (existing_subgroup_ids.has(group_id)) {
+            already_added_subgroups.add(group_id);
+            return false;
+        }
+
+        return true;
+    });
+
+    let ignored_deactivated_groups: UserGroup[] = [];
+    let ignored_already_added_subgroups: UserGroup[] = [];
+    if (deactivated_groups.size > 0) {
+        const ignored_deactivated_group_ids = [...deactivated_groups];
+        ignored_deactivated_groups = ignored_deactivated_group_ids.map((group_id) =>
+            user_groups.get_user_group_from_id(group_id),
+        );
+    }
+    if (already_added_subgroups.size > 0) {
+        const ignored_already_added_subgroup_ids = [...already_added_subgroups];
+        ignored_already_added_subgroups = ignored_already_added_subgroup_ids.map((group_id) =>
+            user_groups.get_user_group_from_id(group_id),
+        );
+    }
+
+    const subgroup_id_set = new Set(subgroup_ids_to_add);
+
+    if (user_id_set.size === 0 && subgroup_id_set.size === 0) {
         show_user_group_membership_request_result({
-            message: $t({defaultMessage: "No users to add."}),
+            message: $t({defaultMessage: "No users or subgroups to add."}),
             add_class: "text-error",
             remove_class: "text-success",
             already_added_users: ignored_already_added_users,
             ignored_deactivated_users,
+            already_added_subgroups: ignored_already_added_subgroups,
+            ignored_deactivated_groups,
         });
         return;
     }
     const user_ids = [...user_id_set];
+    const subgroup_ids = [...subgroup_id_set];
 
     function invite_success(): void {
         pill_widget.clear();
@@ -278,6 +352,8 @@ function add_new_members({pill_user_ids}: {pill_user_ids: number[]}): void {
             remove_class: "text-error",
             already_added_users: ignored_already_added_users,
             ignored_deactivated_users,
+            already_added_subgroups: ignored_already_added_subgroups,
+            ignored_deactivated_groups,
         });
     }
 
@@ -304,6 +380,7 @@ function add_new_members({pill_user_ids}: {pill_user_ids: number[]}): void {
     edit_user_group_membership({
         group,
         added: user_ids,
+        added_subgroups: subgroup_ids,
         success: invite_success,
         error: invite_failure,
     });
@@ -373,8 +450,50 @@ function remove_member({
     do_remove_user_from_group();
 }
 
+function remove_subgroup({
+    group_id,
+    target_subgroup_id,
+    $list_entry,
+}: {
+    group_id: number;
+    target_subgroup_id: number;
+    $list_entry: JQuery;
+}): void {
+    const group = user_groups.get_user_group_from_id(current_group_id);
+
+    function removal_success(): void {
+        if (group_id !== current_group_id) {
+            blueslip.info("Response for subgroup removal came too late.");
+            return;
+        }
+
+        $list_entry.remove();
+        const message = $t({defaultMessage: "Removed successfully."});
+        show_user_group_membership_request_result({
+            message,
+            add_class: "text-success",
+            remove_class: "text-remove",
+        });
+    }
+
+    function removal_failure(): void {
+        show_user_group_membership_request_result({
+            message: $t({defaultMessage: "Error removing subgroup from this group."}),
+            add_class: "text-error",
+            remove_class: "text-success",
+        });
+    }
+
+    edit_user_group_membership({
+        group,
+        removed_subgroups: [target_subgroup_id],
+        success: removal_success,
+        error: removal_failure,
+    });
+}
+
 export function initialize(): void {
-    add_subscribers_pill.set_up_handlers({
+    add_group_members_pill.set_up_handlers({
         get_pill_widget: () => pill_widget,
         $parent_container: $("#groups_overlay_container"),
         pill_selector: ".edit_members_for_user_group .pill-container",
@@ -395,4 +514,18 @@ export function initialize(): void {
             remove_member({group_id, target_user_id, $list_entry});
         },
     );
+
+    $("#groups_overlay_container").on(
+        "submit",
+        ".edit_members_for_user_group .subgroup_list_remove form",
+        function (this: HTMLElement, e): void {
+            e.preventDefault();
+
+            const $list_entry = $(this).closest("tr");
+            const target_subgroup_id = Number.parseInt($list_entry.attr("data-subgroup-id")!, 10);
+            const group_id = current_group_id;
+
+            remove_subgroup({group_id, target_subgroup_id, $list_entry});
+        },
+    );
 }

web/src/user_group_popover.ts

@@ -152,6 +152,11 @@ export function register_click_handlers(): void {
         e.stopPropagation();
         toggle_user_group_info_popover(this, undefined);
     });
+
+    $("body").on("click", ".view_user_group", function (this: HTMLElement, e) {
+        e.stopPropagation();
+        toggle_user_group_info_popover(this, undefined);
+    });
 }
 
 function fetch_group_members(member_ids: number[]): PopoverGroupMember[] {

web/styles/input_pill.css

@@ -284,6 +284,10 @@
     }
 }
 
+.display_only_group_pill .zulip-icon-triple-users {
+    font-size: 19px;
+}
+
 @keyframes shake {
     10%,
     90% {

web/styles/subscriptions.css

@@ -166,7 +166,8 @@ h4.user_group_setting_subsection_title {
                 }
             }
 
-            .subscriber_list_remove {
+            .subscriber_list_remove,
+            .subgroup-list-remove {
                 padding-right: 16px;
                 display: inline-block;
             }
@@ -261,7 +262,8 @@ h4.user_group_setting_subsection_title {
     display: inline;
 }
 
-.remove-subscriber-form {
+.remove-subscriber-form,
+.remove-subgroup-form {
     margin: 0;
 }
 

web/templates/user_group_display_only_pill.hbs

@@ -0,0 +1,8 @@
+<span class="pill-container display_only_group_pill">
+    <a data-user-group-id="{{group_id}}" class="view_user_group pill" tabindex="0">
+        <i class="zulip-icon zulip-icon-triple-users no-presence-circle" aria-hidden="true"></i>
+        <span class="pill-label {{#if strikethrough}} strikethrough {{/if}}" >
+            <span class="pill-value">{{display_value}}</span>
+        </span>
+    </a>
+</span>

web/templates/user_group_settings/add_members_form.hbs

@@ -1,7 +1,7 @@
 <div class="add_members_container">
     <div class="pill-container person_picker">
         <div class="input" contenteditable="true"
-          data-placeholder="{{t 'Add members. Use usergroup or #channelname to bulk add members.' }}">
+          data-placeholder="{{t 'Add users or groups. Use #channelname to add all subscribers.' }}">
             {{~! Squash whitespace so that placeholder is displayed when empty. ~}}
         </div>
     </div>

web/templates/user_group_settings/user_group_members_table.hbs

@@ -2,7 +2,7 @@
     <div class="member_list_loading_indicator"></div>
     <table class="member-list table table-striped">
         <thead class="table-sticky-headers">
-            <th data-sort="alphabetic" data-sort-prop="full_name">{{t "Name" }}</th>
+            <th data-sort="name">{{t "Name" }}</th>
             <th class="settings-email-column" data-sort="email">{{t "Email" }}</th>
             <th class="user-remove-actions" {{#unless can_edit}}style="display:none"{{/unless}}>{{t "Actions" }}</th>
         </thead>

web/templates/user_group_settings/user_group_membership_request_result.hbs

@@ -18,4 +18,22 @@
         {{/if}}
         <a data-user-id="{{user_id}}" class="view_user_profile">{{full_name}}</a>{{#unless @last}},{{else}}.{{/unless}}
     {{/each}}
+    <br />
+{{/if}}
+{{#if already_added_subgroups}}
+    {{#each already_added_subgroups}}
+        {{#if @first}}
+        {{t "Already subgroups:" }}
+        {{/if}}
+        <a data-user-group-id="{{id}}" class="view_user_group">{{name}}</a>{{#unless @last}},{{else}}.{{/unless}}
+    {{/each}}
+    <br />
+{{/if}}
+{{#if ignored_deactivated_groups}}
+    {{#each ignored_deactivated_groups}}
+        {{#if @first}}
+        {{t "Ignored deactivated groups:" }}
+        {{/if}}
+        <a data-user-group-id="{{id}}" class="view_user_group">{{name}}</a>{{#unless @last}},{{else}}.{{/unless}}
+    {{/each}}
 {{/if}}

web/templates/user_group_settings/user_group_subgroup_entry.hbs

@@ -0,0 +1,16 @@
+<tr data-subgroup-id="{{group_id}}">
+    <td class="subgroup-name panel_user_list" colspan="2">
+        {{> ../user_group_display_only_pill}}
+    </td>
+    {{#if can_edit}}
+    <td class="remove">
+        <div class="subgroup_list_remove">
+            <form class="remove-subgroup-form">
+                <button type="submit" name="remove" class="remove-subgroup-button button small rounded btn-danger">
+                    {{t 'Remove'}}
+                </button>
+            </form>
+        </div>
+    </td>
+    {{/if}}
+</tr>

web/tests/dispatch.test.js

@@ -264,8 +264,11 @@ run_test("user groups", ({override}) => {
     {
         const stub = make_stub();
         override(user_groups, "add_subgroups", stub.f);
+        const user_group_edit_stub = make_stub();
+        override(user_group_edit, "handle_subgroup_edit_event", user_group_edit_stub.f);
         dispatch(event);
         assert.equal(stub.num_calls, 1);
+        assert.equal(user_group_edit_stub.num_calls, 1);
         const args = stub.get_args("group_id", "direct_subgroup_ids");
         assert_same(args.group_id, event.group_id);
         assert_same(args.direct_subgroup_ids, event.direct_subgroup_ids);
@@ -291,8 +294,11 @@ run_test("user groups", ({override}) => {
     {
         const stub = make_stub();
         override(user_groups, "remove_subgroups", stub.f);
+        const user_group_edit_stub = make_stub();
+        override(user_group_edit, "handle_subgroup_edit_event", user_group_edit_stub.f);
         dispatch(event);
         assert.equal(stub.num_calls, 1);
+        assert.equal(user_group_edit_stub.num_calls, 1);
         const args = stub.get_args("group_id", "direct_subgroup_ids");
         assert_same(args.group_id, event.group_id);
         assert_same(args.direct_subgroup_ids, event.direct_subgroup_ids);