From 89d0ad1d60d053bb19645f05ab0cbe2776a0047d Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Fri, 11 Oct 2024 08:53:32 +0530 Subject: [PATCH] user_group_edit: Add support to update subgroups of existing groups. --- tools/test-js-with-node | 1 + web/src/add_group_members_pill.ts | 61 +++++++ web/src/server_events_dispatch.js | 2 + web/src/user_group_components.ts | 62 +++++++ web/src/user_group_edit.js | 12 ++ web/src/user_group_edit_members.ts | 169 ++++++++++++++++-- web/src/user_group_popover.ts | 5 + web/styles/input_pill.css | 4 + web/styles/subscriptions.css | 6 +- .../user_group_display_only_pill.hbs | 8 + .../user_group_settings/add_members_form.hbs | 2 +- .../user_group_members_table.hbs | 2 +- .../user_group_membership_request_result.hbs | 18 ++ .../user_group_subgroup_entry.hbs | 16 ++ web/tests/dispatch.test.js | 6 + 15 files changed, 352 insertions(+), 22 deletions(-) create mode 100644 web/src/add_group_members_pill.ts create mode 100644 web/templates/user_group_display_only_pill.hbs create mode 100644 web/templates/user_group_settings/user_group_subgroup_entry.hbs diff --git a/tools/test-js-with-node b/tools/test-js-with-node index 13e57a1cb5..9e38314ffd 100755 --- a/tools/test-js-with-node +++ b/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", diff --git a/web/src/add_group_members_pill.ts b/web/src/add_group_members_pill.ts new file mode 100644 index 0000000000..78c9cd24ae --- /dev/null +++ b/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(); + }); +} diff --git a/web/src/server_events_dispatch.js b/web/src/server_events_dispatch.js index 0a865ec426..a45c015768 100644 --- a/web/src/server_events_dispatch.js +++ b/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); diff --git a/web/src/user_group_components.ts b/web/src/user_group_components.ts index f9b303e48c..d3a774c76a 100644 --- a/web/src/user_group_components.ts +++ b/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; + }; +} diff --git a/web/src/user_group_edit.js b/web/src/user_group_edit.js index 3e79358969..ba9cc7aaf0 100644 --- a/web/src/user_group_edit.js +++ b/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; diff --git a/web/src/user_group_edit_members.ts b/web/src/user_group_edit_members.ts index 67bc19fddd..9a6b12b0c0 100644 --- a/web/src/user_group_edit_members.ts +++ b/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; +let member_list_widget: ListWidgetType; 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[]; -}): ListWidgetType { - people.sort_but_pin_current_user_on_top(users); - + users: (User | UserGroup)[]; +}): ListWidgetType { 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, - id: user_sort.sort_user_id, - ...ListWidget.generic_sort_functions("alphabetic", ["full_name"]), + email: user_group_components.sort_group_member_email, + name: user_group_components.sort_group_member_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("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(); + const already_added_subgroups = new Set(); + + 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}); + }, + ); } diff --git a/web/src/user_group_popover.ts b/web/src/user_group_popover.ts index 4c5b38f647..bfa0113ff0 100644 --- a/web/src/user_group_popover.ts +++ b/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[] { diff --git a/web/styles/input_pill.css b/web/styles/input_pill.css index f1fcb332ff..ecfb66ba11 100644 --- a/web/styles/input_pill.css +++ b/web/styles/input_pill.css @@ -284,6 +284,10 @@ } } +.display_only_group_pill .zulip-icon-triple-users { + font-size: 19px; +} + @keyframes shake { 10%, 90% { diff --git a/web/styles/subscriptions.css b/web/styles/subscriptions.css index 493af4947d..521bb049c6 100644 --- a/web/styles/subscriptions.css +++ b/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; } diff --git a/web/templates/user_group_display_only_pill.hbs b/web/templates/user_group_display_only_pill.hbs new file mode 100644 index 0000000000..f0a6598bbb --- /dev/null +++ b/web/templates/user_group_display_only_pill.hbs @@ -0,0 +1,8 @@ + + + + + {{display_value}} + + + diff --git a/web/templates/user_group_settings/add_members_form.hbs b/web/templates/user_group_settings/add_members_form.hbs index 0d042263fd..910ff74112 100644 --- a/web/templates/user_group_settings/add_members_form.hbs +++ b/web/templates/user_group_settings/add_members_form.hbs @@ -1,7 +1,7 @@
+ data-placeholder="{{t 'Add users or groups. Use #channelname to add all subscribers.' }}"> {{~! Squash whitespace so that placeholder is displayed when empty. ~}}
diff --git a/web/templates/user_group_settings/user_group_members_table.hbs b/web/templates/user_group_settings/user_group_members_table.hbs index 4f32d953c5..509595fa4f 100644 --- a/web/templates/user_group_settings/user_group_members_table.hbs +++ b/web/templates/user_group_settings/user_group_members_table.hbs @@ -2,7 +2,7 @@
- + diff --git a/web/templates/user_group_settings/user_group_membership_request_result.hbs b/web/templates/user_group_settings/user_group_membership_request_result.hbs index 61edcd8de6..a535e664b4 100644 --- a/web/templates/user_group_settings/user_group_membership_request_result.hbs +++ b/web/templates/user_group_settings/user_group_membership_request_result.hbs @@ -18,4 +18,22 @@ {{/if}} {{#unless @last}},{{else}}.{{/unless}} {{/each}} +
+{{/if}} +{{#if already_added_subgroups}} + {{#each already_added_subgroups}} + {{#if @first}} + {{t "Already subgroups:" }} + {{/if}} + {{name}}{{#unless @last}},{{else}}.{{/unless}} + {{/each}} +
+{{/if}} +{{#if ignored_deactivated_groups}} + {{#each ignored_deactivated_groups}} + {{#if @first}} + {{t "Ignored deactivated groups:" }} + {{/if}} + {{name}}{{#unless @last}},{{else}}.{{/unless}} + {{/each}} {{/if}} diff --git a/web/templates/user_group_settings/user_group_subgroup_entry.hbs b/web/templates/user_group_settings/user_group_subgroup_entry.hbs new file mode 100644 index 0000000000..36e96bbc45 --- /dev/null +++ b/web/templates/user_group_settings/user_group_subgroup_entry.hbs @@ -0,0 +1,16 @@ + + + {{#if can_edit}} + + {{/if}} + diff --git a/web/tests/dispatch.test.js b/web/tests/dispatch.test.js index f66b2ee3ba..c0dca90fb5 100644 --- a/web/tests/dispatch.test.js +++ b/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);
{{t "Name" }}{{t "Name" }} {{t "Email" }} {{t "Actions" }}
+ {{> ../user_group_display_only_pill}} + +
+
+ +
+
+