From 73a378d23f168ed0a233f76c319e9cbfdad0567f Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Thu, 29 Dec 2022 22:19:38 +0530 Subject: [PATCH] stream_settings: Use can_remove_subscribers_group setting in webapp. This commit updates the frontend to show or hide the "Unsubscribe" button in subscribers list in stream settings as per the can_remove_subscribers_group setting for the stream. --- frontend_tests/node_tests/stream_data.js | 96 +++++++++++++++++++ .../node_tests/stream_settings_ui.js | 15 +++ static/js/stream_data.js | 31 ++++++ static/js/stream_edit_subscribers.js | 10 +- static/js/stream_settings_data.js | 1 + .../stream_member_list_entry.hbs | 2 +- .../stream_settings/stream_members.hbs | 2 +- 7 files changed, 151 insertions(+), 6 deletions(-) diff --git a/frontend_tests/node_tests/stream_data.js b/frontend_tests/node_tests/stream_data.js index b1f5a092c9..6da465c06f 100644 --- a/frontend_tests/node_tests/stream_data.js +++ b/frontend_tests/node_tests/stream_data.js @@ -21,6 +21,7 @@ const sub_store = zrequire("sub_store"); const stream_data = zrequire("stream_data"); const stream_settings_data = zrequire("stream_settings_data"); const settings_config = zrequire("settings_config"); +const user_groups = zrequire("user_groups"); const me = { email: "me@zulip.com", @@ -39,6 +40,14 @@ function contains_sub(subs, sub) { return subs.some((s) => s.name === sub.name); } +const admins_group = { + name: "Admins", + id: 1, + members: new Set([1]), + is_system_group: true, + direct_subgroup_ids: new Set([]), +}; + function test(label, f) { run_test(label, (helpers) => { page_params.is_admin = false; @@ -48,6 +57,7 @@ function test(label, f) { people.add_active_user(me); people.initialize_current_user(me.user_id); stream_data.clear_subscriptions(); + user_groups.initialize({realm_user_groups: [admins_group]}); f(helpers); }); } @@ -333,6 +343,7 @@ test("admin_options", () => { stream_id: 1, is_muted: true, invite_only: false, + can_remove_subscribers_group_id: admins_group.id, }; stream_data.add_sub(sub); return sub; @@ -385,6 +396,7 @@ test("stream_settings", () => { color: "cinnamon", subscribed: true, invite_only: false, + can_remove_subscribers_group_id: admins_group.id, }; const blue = { @@ -393,6 +405,7 @@ test("stream_settings", () => { color: "blue", subscribed: false, invite_only: false, + can_remove_subscribers_group_id: admins_group.id, }; const amber = { @@ -404,6 +417,7 @@ test("stream_settings", () => { history_public_to_subscribers: true, stream_post_policy: stream_data.stream_post_policy_values.admins.code, message_retention_days: 10, + can_remove_subscribers_group_id: admins_group.id, }; stream_data.add_sub(cinnamon); stream_data.add_sub(amber); @@ -996,3 +1010,85 @@ test("can_post_messages_in_stream", () => { page_params.is_spectator = true; assert.equal(stream_data.can_post_messages_in_stream(social), false); }); + +test("can_unsubscribe_others", () => { + const admin_user_id = 1; + const moderator_user_id = 2; + const member_user_id = 3; + + const admins = { + name: "Admins", + id: 1, + members: new Set([admin_user_id]), + is_system_group: true, + direct_subgroup_ids: new Set([]), + }; + const moderators = { + name: "Moderators", + id: 2, + members: new Set([moderator_user_id]), + is_system_group: true, + direct_subgroup_ids: new Set([1]), + }; + const all = { + name: "Everyone", + id: 3, + members: new Set([member_user_id]), + is_system_group: true, + direct_subgroup_ids: new Set([2]), + }; + const nobody = { + name: "Nobody", + id: 4, + members: new Set([]), + is_system_group: true, + direct_subgroup_ids: new Set([]), + }; + + user_groups.initialize({realm_user_groups: [admins, moderators, all, nobody]}); + + const sub = { + name: "Denmark", + subscribed: true, + color: "red", + stream_id: 1, + can_remove_subscribers_group_id: admins.id, + }; + stream_data.add_sub(sub); + + people.initialize_current_user(admin_user_id); + assert.equal(stream_data.can_unsubscribe_others(sub), true); + people.initialize_current_user(moderator_user_id); + assert.equal(stream_data.can_unsubscribe_others(sub), false); + + sub.can_remove_subscribers_group_id = moderators.id; + people.initialize_current_user(admin_user_id); + assert.equal(stream_data.can_unsubscribe_others(sub), true); + people.initialize_current_user(moderator_user_id); + assert.equal(stream_data.can_unsubscribe_others(sub), true); + people.initialize_current_user(member_user_id); + assert.equal(stream_data.can_unsubscribe_others(sub), false); + + sub.can_remove_subscribers_group_id = all.id; + people.initialize_current_user(admin_user_id); + assert.equal(stream_data.can_unsubscribe_others(sub), true); + people.initialize_current_user(moderator_user_id); + assert.equal(stream_data.can_unsubscribe_others(sub), true); + people.initialize_current_user(member_user_id); + assert.equal(stream_data.can_unsubscribe_others(sub), true); + + // Even with the nobody system group, admins can still unsubscribe others. + sub.can_remove_subscribers_group_id = nobody.id; + page_params.is_admin = true; + assert.equal(stream_data.can_unsubscribe_others(sub), true); + page_params.is_admin = false; + assert.equal(stream_data.can_unsubscribe_others(sub), false); + + // This isn't a real state, but we want coverage on !can_view_subscribers. + sub.subscribed = false; + sub.invite_only = true; + page_params.is_admin = true; + assert.equal(stream_data.can_unsubscribe_others(sub), true); + page_params.is_admin = false; + assert.equal(stream_data.can_unsubscribe_others(sub), false); +}); diff --git a/frontend_tests/node_tests/stream_settings_ui.js b/frontend_tests/node_tests/stream_settings_ui.js index 726b21878c..f6f55553bf 100644 --- a/frontend_tests/node_tests/stream_settings_ui.js +++ b/frontend_tests/node_tests/stream_settings_ui.js @@ -21,8 +21,18 @@ set_global("page_params", {}); const stream_data = zrequire("stream_data"); const stream_settings_ui = zrequire("stream_settings_ui"); +const user_groups = zrequire("user_groups"); run_test("redraw_left_panel", ({mock_template}) => { + const admins_group = { + name: "Admins", + id: 1, + members: new Set([1]), + is_system_group: true, + direct_subgroup_ids: new Set([]), + }; + user_groups.initialize({realm_user_groups: [admins_group]}); + // set-up sub rows stubs const denmark = { elem: "denmark", @@ -33,6 +43,7 @@ run_test("redraw_left_panel", ({mock_template}) => { subscribers: [1], stream_weekly_traffic: null, color: "red", + can_remove_subscribers_group_id: admins_group.id, }; const poland = { elem: "poland", @@ -43,6 +54,7 @@ run_test("redraw_left_panel", ({mock_template}) => { subscribers: [1, 2, 3], stream_weekly_traffic: 13, color: "red", + can_remove_subscribers_group_id: admins_group.id, }; const pomona = { elem: "pomona", @@ -53,6 +65,7 @@ run_test("redraw_left_panel", ({mock_template}) => { subscribers: [], stream_weekly_traffic: 0, color: "red", + can_remove_subscribers_group_id: admins_group.id, }; const cpp = { elem: "cpp", @@ -63,6 +76,7 @@ run_test("redraw_left_panel", ({mock_template}) => { subscribers: [1, 2], stream_weekly_traffic: 6, color: "red", + can_remove_subscribers_group_id: admins_group.id, }; const zzyzx = { elem: "zzyzx", @@ -73,6 +87,7 @@ run_test("redraw_left_panel", ({mock_template}) => { subscribers: [1, 2], stream_weekly_traffic: 6, color: "red", + can_remove_subscribers_group_id: admins_group.id, }; const sub_row_data = [denmark, poland, pomona, cpp, zzyzx]; diff --git a/static/js/stream_data.js b/static/js/stream_data.js index 847484a38b..66a4074d11 100644 --- a/static/js/stream_data.js +++ b/static/js/stream_data.js @@ -8,6 +8,7 @@ import * as people from "./people"; import * as settings_config from "./settings_config"; import * as stream_topic_history from "./stream_topic_history"; import * as sub_store from "./sub_store"; +import * as user_groups from "./user_groups"; import {user_settings} from "./user_settings"; import * as util from "./util"; @@ -556,6 +557,36 @@ export function can_subscribe_others(sub) { return !page_params.is_guest && (!sub.invite_only || sub.subscribed); } +export function can_unsubscribe_others(sub) { + // Whether the current user has permission to remove other users + // from the stream. Organization administrators can remove users + // from any stream; additionally, users who can access the stream + // and are in the stream's can_remove_subscribers_group can do so + // as well. + // + // TODO: The API allows the current user to remove bots that it + // administers from streams; so we might need to refactor this + // logic to accept a target_user_id parameter in order to support + // that in the UI. + + // A user must be able to view subscribers in a stream in order to + // remove them. This check may never fire in practice, since the + // UI for removing subscribers generally is a list of the stream's + // subscribers. + if (!can_view_subscribers(sub)) { + return false; + } + + if (page_params.is_admin) { + return true; + } + + return user_groups.is_user_in_group( + sub.can_remove_subscribers_group_id, + people.my_current_user_id(), + ); +} + export function can_post_messages_in_stream(stream) { if (page_params.is_spectator) { return false; diff --git a/static/js/stream_edit_subscribers.js b/static/js/stream_edit_subscribers.js index 69ec572961..d139769c04 100644 --- a/static/js/stream_edit_subscribers.js +++ b/static/js/stream_edit_subscribers.js @@ -23,13 +23,13 @@ export let pill_widget; let current_stream_id; let subscribers_list_widget; -function format_member_list_elem(person) { +function format_member_list_elem(person, user_can_remove_subscribers) { return render_stream_member_list_entry({ name: person.full_name, user_id: person.user_id, is_current_user: person.user_id === page_params.user_id, email: settings_data.email_for_user_settings(person), - can_edit_subscribers: page_params.is_admin, + can_remove_subscribers: user_can_remove_subscribers, show_email: settings_data.show_email(), }); } @@ -87,6 +87,7 @@ export function enable_subscriber_management({sub, $parent_container}) { }); const user_ids = peer_data.get_subscribers(stream_id); + const user_can_remove_subscribers = stream_data.can_unsubscribe_others(sub); // We track a single subscribers_list_widget for this module, since we // only ever have one list of subscribers visible at a time. @@ -94,10 +95,11 @@ export function enable_subscriber_management({sub, $parent_container}) { $parent_container, name: "stream_subscribers", user_ids, + user_can_remove_subscribers, }); } -function make_list_widget({$parent_container, name, user_ids}) { +function make_list_widget({$parent_container, name, user_ids, user_can_remove_subscribers}) { const users = people.get_users_from_ids(user_ids); people.sort_but_pin_current_user_on_top(users); @@ -109,7 +111,7 @@ function make_list_widget({$parent_container, name, user_ids}) { return ListWidget.create($list_container, users, { name, modifier(item) { - return format_member_list_elem(item); + return format_member_list_elem(item, user_can_remove_subscribers); }, filter: { $element: $parent_container.find(".search"), diff --git a/static/js/stream_settings_data.js b/static/js/stream_settings_data.js index e664629ade..38f6f2d2a9 100644 --- a/static/js/stream_settings_data.js +++ b/static/js/stream_settings_data.js @@ -46,6 +46,7 @@ export function add_settings_fields(sub) { sub.can_change_stream_permissions = stream_data.can_change_permissions(sub); sub.can_access_subscribers = stream_data.can_view_subscribers(sub); sub.can_add_subscribers = stream_data.can_subscribe_others(sub); + sub.can_remove_subscribers = stream_data.can_unsubscribe_others(sub); sub.preview_url = hash_util.by_stream_url(sub.stream_id); sub.is_old_stream = sub.stream_weekly_traffic !== null; diff --git a/static/templates/stream_settings/stream_member_list_entry.hbs b/static/templates/stream_settings/stream_member_list_entry.hbs index 5cee339a1a..8c7fcd2932 100644 --- a/static/templates/stream_settings/stream_member_list_entry.hbs +++ b/static/templates/stream_settings/stream_member_list_entry.hbs @@ -9,7 +9,7 @@ {{t "(hidden)"}} {{/if}} {{user_id}} - {{#if can_edit_subscribers}} + {{#if can_remove_subscribers}}
diff --git a/static/templates/stream_settings/stream_members.hbs b/static/templates/stream_settings/stream_members.hbs index 608835081d..5f27282c87 100644 --- a/static/templates/stream_settings/stream_members.hbs +++ b/static/templates/stream_settings/stream_members.hbs @@ -24,7 +24,7 @@ {{t "Name" }} {{t "Email" }} {{t "User ID" }} - {{#if is_realm_admin}} + {{#if can_remove_subscribers}} {{t "Actions" }} {{/if}}