From b21f533af38411f39a11eaa9f7e7daf8e131d025 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Sat, 28 Jan 2023 21:00:46 -0500 Subject: [PATCH] settings_users: Show all bots in organization settings. We intended to show all the bots in the bots organization settings for non-admin users as well. This switches from bot_data.all_user_ids() to people.get_bot_ids() to get a full set of ids for all the bots in the organization. Because the source of data changes, "realm_user" instead of "realm_bot" triggers the update of the bots list. The code example (example4) is updated since we incorporate a side effect into "realm_user"'s "add" op. Note that while "realm_user" does not have a "delete" op, we still stop redrawing bots on "relam_bot"'s "delete" op, because "delete" was only triggered when the bot owner changes, the bot does not disappear from the list of all bots. Signed-off-by: Zixuan James Li --- frontend_tests/node_tests/dispatch.js | 31 +++++++++++---------------- frontend_tests/node_tests/example4.js | 12 +++++++---- static/js/server_events_dispatch.js | 7 +++--- static/js/settings_users.js | 12 +++++------ 4 files changed, 30 insertions(+), 32 deletions(-) diff --git a/frontend_tests/node_tests/dispatch.js b/frontend_tests/node_tests/dispatch.js index 82f3d276fc..47b472ee03 100644 --- a/frontend_tests/node_tests/dispatch.js +++ b/frontend_tests/node_tests/dispatch.js @@ -505,14 +505,11 @@ run_test("realm settings", ({override}) => { run_test("realm_bot add", ({override}) => { const event = event_fixtures.realm_bot__add; const bot_stub = make_stub(); - const admin_stub = make_stub(); override(bot_data, "add", bot_stub.f); override(settings_bots, "render_bots", () => {}); - override(settings_users, "redraw_bots_list", admin_stub.f); dispatch(event); assert.equal(bot_stub.num_calls, 1); - assert.equal(admin_stub.num_calls, 1); const args = bot_stub.get_args("bot"); assert_same(args.bot, event.bot); }); @@ -520,14 +517,11 @@ run_test("realm_bot add", ({override}) => { run_test("realm_bot remove", ({override}) => { const event = event_fixtures.realm_bot__remove; const bot_stub = make_stub(); - const admin_stub = make_stub(); override(bot_data, "deactivate", bot_stub.f); override(settings_bots, "render_bots", () => {}); - override(settings_users, "update_bot_data", admin_stub.f); dispatch(event); assert.equal(bot_stub.num_calls, 1); - assert.equal(admin_stub.num_calls, 1); const args = bot_stub.get_args("user_id"); assert_same(args.user_id, event.bot.user_id); }); @@ -535,37 +529,27 @@ run_test("realm_bot remove", ({override}) => { run_test("realm_bot delete", ({override}) => { const event = event_fixtures.realm_bot__delete; const bot_stub = make_stub(); - const admin_stub = make_stub(); override(bot_data, "del", bot_stub.f); override(settings_bots, "render_bots", () => {}); - override(settings_users, "redraw_bots_list", admin_stub.f); dispatch(event); assert.equal(bot_stub.num_calls, 1); const args = bot_stub.get_args("user_id"); assert_same(args.user_id, event.bot.user_id); - - assert.equal(admin_stub.num_calls, 1); }); run_test("realm_bot update", ({override}) => { const event = event_fixtures.realm_bot__update; const bot_stub = make_stub(); - const admin_stub = make_stub(); override(bot_data, "update", bot_stub.f); override(settings_bots, "render_bots", () => {}); - override(settings_users, "update_bot_data", admin_stub.f); dispatch(event); assert.equal(bot_stub.num_calls, 1); - assert.equal(admin_stub.num_calls, 1); - let args = bot_stub.get_args("user_id", "bot"); + const args = bot_stub.get_args("user_id", "bot"); assert_same(args.user_id, event.bot.user_id); assert_same(args.bot, event.bot); - - args = admin_stub.get_args("update_user_id", "update_bot_data"); - assert_same(args.update_user_id, event.bot.user_id); }); run_test("realm_emoji", ({override}) => { @@ -643,11 +627,14 @@ run_test("realm_domains", ({override}) => { run_test("realm_user", ({override}) => { override(settings_account, "maybe_update_deactivate_account_button", noop); let event = event_fixtures.realm_user__add; + const add_admin_stub = make_stub(); + override(settings_users, "redraw_bots_list", add_admin_stub.f); dispatch({...event}); const added_person = people.get_by_user_id(event.person.user_id); // sanity check a few individual fields assert.equal(added_person.full_name, "Test User"); assert.equal(added_person.timezone, "America/New_York"); + assert.equal(add_admin_stub.num_calls, 1); // ...but really the whole struct gets copied without any // manipulation @@ -655,23 +642,31 @@ run_test("realm_user", ({override}) => { assert.ok(people.is_active_user_for_popover(event.person.user_id)); + const remove_admin_stub = make_stub(); event = event_fixtures.realm_user__remove; override(stream_events, "remove_deactivated_user_from_all_streams", noop); override(settings_users, "update_view_on_deactivate", noop); + override(settings_users, "update_bot_data", remove_admin_stub.f); dispatch(event); // We don't actually remove the person, we just deactivate them. const removed_person = people.get_by_user_id(event.person.user_id); assert.equal(removed_person.full_name, "Test User"); assert.ok(!people.is_active_user_for_popover(event.person.user_id)); + assert.equal(remove_admin_stub.num_calls, 1); event = event_fixtures.realm_user__update; const stub = make_stub(); + const update_admin_stub = make_stub(); override(user_events, "update_person", stub.f); + override(settings_users, "update_bot_data", update_admin_stub.f); dispatch(event); assert.equal(stub.num_calls, 1); - const args = stub.get_args("person"); + assert.equal(update_admin_stub.num_calls, 1); + let args = stub.get_args("person"); assert_same(args.person, event.person); + args = update_admin_stub.get_args("update_user_id", "update_bot_data"); + assert_same(args.update_user_id, event.person.user_id); }); run_test("restart", ({override}) => { diff --git a/frontend_tests/node_tests/example4.js b/frontend_tests/node_tests/example4.js index b4adf20d82..f80fafe022 100644 --- a/frontend_tests/node_tests/example4.js +++ b/frontend_tests/node_tests/example4.js @@ -70,7 +70,7 @@ const bob = { full_name: "Bob Roberts", }; -run_test("add users with event", () => { +run_test("add users with event", ({override}) => { people.init(); const event = { @@ -81,6 +81,9 @@ run_test("add users with event", () => { assert.ok(!people.is_known_user_id(bob.user_id)); + // We need to override a stub here before dispatching the event. + // Keep reading to see how overriding works! + override(settings_users, "redraw_bots_list", () => {}); // Let's simulate dispatching our event! server_events_dispatch.dispatch_normal_event(event); @@ -91,13 +94,13 @@ run_test("add users with event", () => { /* It's actually a little surprising that adding a user does - not have side effects beyond the people object. I guess - we don't immediately update the buddy list, but that's + not have side effects beyond the people object and the bots list. + I guess we don't immediately update the buddy list, but that's because the buddy list gets updated on the next server fetch. Let's try an update next. To make this work, we will want - to override some of our stubs. + to override some more of our stubs. This is where we see a little extra benefit from the run_test wrapper. It passes us in an object that we @@ -131,6 +134,7 @@ run_test("update user with event", ({override}) => { override(message_live_update, "update_user_full_name", () => {}); override(pm_list, "update_private_messages", () => {}); override(settings_users, "update_user_data", () => {}); + override(settings_users, "update_bot_data", () => {}); // Dispatch the realm_user/update event, which will update // data structures and have other side effects that are diff --git a/static/js/server_events_dispatch.js b/static/js/server_events_dispatch.js index 91439118bc..89403901a3 100644 --- a/static/js/server_events_dispatch.js +++ b/static/js/server_events_dispatch.js @@ -324,23 +324,19 @@ export function dispatch_normal_event(event) { case "add": bot_data.add(event.bot); settings_bots.render_bots(); - settings_users.redraw_bots_list(); break; case "remove": bot_data.deactivate(event.bot.user_id); event.bot.is_active = false; settings_bots.render_bots(); - settings_users.update_bot_data(event.bot.user_id); break; case "delete": bot_data.del(event.bot.user_id); settings_bots.render_bots(); - settings_users.redraw_bots_list(); break; case "update": bot_data.update(event.bot.user_id, event.bot); settings_bots.render_bots(); - settings_users.update_bot_data(event.bot.user_id); break; default: blueslip.error("Unexpected event type realm_bot/" + event.op); @@ -430,6 +426,7 @@ export function dispatch_normal_event(event) { case "add": people.add_active_user(event.person); settings_account.maybe_update_deactivate_account_button(); + settings_users.redraw_bots_list(); break; case "remove": people.deactivate(event.person); @@ -437,10 +434,12 @@ export function dispatch_normal_event(event) { settings_users.update_view_on_deactivate(event.person.user_id); buddy_list.maybe_remove_key({key: event.person.user_id}); settings_account.maybe_update_deactivate_account_button(); + settings_users.update_bot_data(event.person.user_id); break; case "update": user_events.update_person(event.person); settings_account.maybe_update_deactivate_account_button(); + settings_users.update_bot_data(event.person.user_id); break; default: blueslip.error("Unexpected event type realm_user/" + event.op); diff --git a/static/js/settings_users.js b/static/js/settings_users.js index c36494620d..047bd49a97 100644 --- a/static/js/settings_users.js +++ b/static/js/settings_users.js @@ -177,19 +177,19 @@ function bot_owner_full_name(owner_id) { } function bot_info(bot_user_id) { - const bot_user = bot_data.get(bot_user_id); + const bot_user = people.get_by_user_id(bot_user_id); if (!bot_user) { return undefined; } - const owner_id = bot_user.owner_id; + const owner_id = bot_user.bot_owner_id; const info = {}; info.is_bot = true; - info.role = people.get_by_user_id(bot_user_id).role; - info.is_active = bot_user.is_active; + info.role = bot_user.role; + info.is_active = people.is_person_active(bot_user.user_id); info.user_id = bot_user.user_id; info.full_name = bot_user.full_name; info.bot_owner_id = owner_id; @@ -254,7 +254,7 @@ section.bots.create_table = () => { loading.make_indicator($("#admin_page_bots_loading_indicator"), {text: "Loading..."}); const $bots_table = $("#admin_bots_table"); $bots_table.hide(); - const bot_user_ids = bot_data.all_user_ids(); + const bot_user_ids = people.get_bot_ids(); bot_list_widget = ListWidget.create($bots_table, bot_user_ids, { name: "admin_bot_list", @@ -378,7 +378,7 @@ export function redraw_bots_list() { // In order to properly redraw after a user may have been added, // we need to update the bot_list_widget with the new set of bot // user IDs to display. - const bot_user_ids = bot_data.all_user_ids(); + const bot_user_ids = people.get_bot_ids(); bot_list_widget.replace_list_data(bot_user_ids); bot_list_widget.hard_redraw(); }