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 <p359101898@gmail.com>

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);
+    const args = bot_stub.get_args("user_id", "bot");
-    let 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}) => {

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
+   not have side effects beyond the people object and the bots list.
-   we don't immediately update the buddy list, but that's
+   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

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);

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.role = bot_user.role;
-    info.is_active = bot_user.is_active;
+    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();
 }