From df03e30d68eeb2d6952e716704bca618cd35a456 Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Thu, 26 Oct 2017 12:00:30 -0700 Subject: [PATCH] popovers: Fix is-active checks for popovers. We were incorrectly reporting active bots as non-active in popovers, and we had no test coverage for cross-realm bots. We also rename the function to is_active_user_for_popover, since the old name, realm_user_is_active_human_or_bot, suggested the wrong semantics for cross-realm bots. Last but not least, we only do a blueslip warning if a user id is not found. When lookups fail, we are pretty confident that the user is not active, so an error is overkill. We can change that as part of issue #7120. Fixes #7153 --- frontend_tests/node_tests/people.js | 21 +++++++++++++++------ static/js/people.js | 25 +++++++++++++++---------- static/js/popovers.js | 4 ++-- 3 files changed, 32 insertions(+), 18 deletions(-) diff --git a/frontend_tests/node_tests/people.js b/frontend_tests/node_tests/people.js index 295fe48b94..e38f63eaef 100644 --- a/frontend_tests/node_tests/people.js +++ b/frontend_tests/node_tests/people.js @@ -66,7 +66,7 @@ initialize(); var active_user_ids = people.get_active_user_ids(); assert.deepEqual(active_user_ids, [isaac.user_id]); - assert.equal(people.realm_user_is_active_human_or_bot(isaac.user_id), true); + assert.equal(people.is_active_user_for_popover(isaac.user_id), true); assert(people.is_valid_email_for_compose(isaac.email)); // Now deactivate isaac @@ -74,7 +74,7 @@ initialize(); person = people.get_active_user_for_email(email); assert(!person); assert.equal(people.get_realm_count(), 0); - assert.equal(people.realm_user_is_active_human_or_bot(isaac.user_id), false); + assert.equal(people.is_active_user_for_popover(isaac.user_id), false); assert.equal(people.is_valid_email_for_compose(isaac.email), false); var bot_botson = { @@ -83,11 +83,17 @@ initialize(); full_name: 'Bot Botson', is_bot: true, }; - people.add(bot_botson); - assert.equal(people.realm_user_is_active_human_or_bot(bot_botson.user_id), true); + people.add_in_realm(bot_botson); + assert.equal(people.is_active_user_for_popover(bot_botson.user_id), true); - // Invalid user ID just returns false - assert.equal(people.realm_user_is_active_human_or_bot(123412), false); + // Invalid user ID returns false and warns. + var message; + blueslip.warn = function (msg) { + message = msg; + }; + + assert.equal(people.is_active_user_for_popover(123412), false); + assert.equal(message, 'Unexpectedly invalid user_id in user popover query: 123412'); // We can still get their info for non-realm needs. person = people.get_by_email(email); @@ -176,6 +182,8 @@ initialize(); assert.equal(person.user_id, 42); }()); +initialize(); + (function test_get_rest_of_realm() { var alice1 = { email: 'alice1@example.com', @@ -655,6 +663,7 @@ initialize(); people.initialize(); assert.equal(people.get_active_user_for_email('alice@example.com').full_name, 'Alice'); + assert.equal(people.is_active_user_for_popover(17), true); assert(people.is_cross_realm_email('bot@example.com')); assert(people.is_valid_email_for_compose('bot@example.com')); assert(people.is_valid_email_for_compose('alice@example.com')); diff --git a/static/js/people.js b/static/js/people.js index 86b830c8ca..70a93615f4 100644 --- a/static/js/people.js +++ b/static/js/people.js @@ -545,19 +545,24 @@ exports.get_active_user_for_email = function (email) { return active_user_dict.get(person.user_id); }; -exports.realm_user_is_active_human_or_bot = function (id) { - if (active_user_dict.get(id) !== undefined) { +exports.is_active_user_for_popover = function (user_id) { + // For popover menus, we include cross-realm bots as active + // users. + + if (cross_realm_dict.get(user_id)) { return true; } - // TODO: Technically, we should probably treat deactivated bots - // like deactivated users here. But we don't have the data to do - // that. See #7153 for notes on fixing this. - var person = exports.get_person_from_user_id(id); - if (person === undefined) { - blueslip.error("Unexpectedly invalid user ID in user popover query " + id); - return false; + if (active_user_dict.has(user_id)) { + return true; } - return !!person.is_bot; + + // TODO: We can report errors here once we start loading + // deactivated users at page-load time. For now just warn. + if (!people_by_user_id_dict.has(user_id)) { + blueslip.warn("Unexpectedly invalid user_id in user popover query: " + user_id); + } + + return false; }; exports.get_all_persons = function () { diff --git a/static/js/popovers.js b/static/js/popovers.js index 861e1a97ba..00a327997e 100644 --- a/static/js/popovers.js +++ b/static/js/popovers.js @@ -100,7 +100,7 @@ function show_user_info_popover(element, user, message) { sent_by_uri: narrow.by_sender_uri(user.email), narrowed: narrow_state.active(), private_message_class: "respond_personal_button", - is_active: people.realm_user_is_active_human_or_bot(user.user_id), + is_active: people.is_active_user_for_popover(user.user_id), }; var ypos = elt.offset().top; @@ -448,7 +448,7 @@ exports.register_click_handlers = function () { pm_with_uri: narrow.pm_with_uri(user_email), sent_by_uri: narrow.by_sender_uri(user_email), private_message_class: "compose_private_message", - is_active: people.realm_user_is_active_human_or_bot(user_id), + is_active: people.is_active_user_for_popover(user_id), }; target.popover({