From e040721090a65aac8b49fff3d5a881697f6d90b6 Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Tue, 26 May 2020 12:25:21 +0000 Subject: [PATCH] refactor: Extract huddle_data.js. This makes it so that search_suggestion.js does not depend on activity.js. That dependency hasn't really been "elegant" for quite some time, but it will become particularly unnecessary when we go to remove the "Group PMs" section from the right sidebar. This commit introduces a temporary wart where we have these two functions with the same name in a sort of unnecessarily complicated code stack: activity.process_loaded_messages huddle_data.process_loaded_messages But we will eliminate the former function very soon, and our message-related codepaths will just call the `huddle_data` version directly. TESTING NOTES: Now that `huddle_data` is a tiny leaf module, it's super easy to just use the real implementation of what was formerly called `activity.get_huddles()` (and is now in `huddle_data`). When I first wrote this commit, introducing the real implementation of `get_huddles` exposed some bugs that I fixed in the immediately prior commits to this. When the tests were originally written, I believe `activity.js` had some annoying `jQuery` dependencies that made it hard to unit test against. We've slimmed it over time to be mostly just a "controller" module. But even in its current state it would have been a bit of a bloated dependency. The other friction for using the actual version of `get_huddles` was setting up the message data, but that's pretty minor. --- frontend_tests/node_tests/activity.js | 12 ++++---- .../node_tests/search_suggestion.js | 29 +++++++++++------- .../node_tests/search_suggestion_legacy.js | 29 +++++++++++------- static/js/activity.js | 28 ++--------------- static/js/huddle_data.js | 30 +++++++++++++++++++ static/js/search_suggestion.js | 3 +- 6 files changed, 78 insertions(+), 53 deletions(-) create mode 100644 static/js/huddle_data.js diff --git a/frontend_tests/node_tests/activity.js b/frontend_tests/node_tests/activity.js index 7a1a9dc979..c0c6f9aa33 100644 --- a/frontend_tests/node_tests/activity.js +++ b/frontend_tests/node_tests/activity.js @@ -2,6 +2,8 @@ set_global('$', global.make_zjquery()); let filter_key_handlers; +const huddle_data = zrequire('huddle_data'); + const _page_params = { realm_users: [], user_id: 999, @@ -212,7 +214,7 @@ run_test('process_loaded_messages', () => { const user_ids_string1 = people.emails_strings_to_user_ids_string(huddle1); const user_ids_string2 = people.emails_strings_to_user_ids_string(huddle2); - assert.deepEqual(activity.get_huddles(), [user_ids_string2, user_ids_string1]); + assert.deepEqual(huddle_data.get_huddles(), [user_ids_string2, user_ids_string1]); }); run_test('full_huddle_name', () => { @@ -742,18 +744,18 @@ run_test('update_huddles_and_redraw', () => { li.set_find_results('.count', count); count.set_parent(li); - const real_get_huddles = activity.get_huddles; - activity.get_huddles = () => ['1,2']; + const real_get_huddles = huddle_data.get_huddles; + huddle_data.get_huddles = () => ['1,2']; activity.update_huddles = real_update_huddles; activity.redraw(); assert.equal($('#group-pm-list').hasClass('show'), false); page_params.realm_presence_disabled = false; activity.redraw(); assert.equal($('#group-pm-list').hasClass('show'), true); - activity.get_huddles = () => []; + huddle_data.get_huddles = () => []; activity.redraw(); assert.equal($('#group-pm-list').hasClass('show'), false); - activity.get_huddles = real_get_huddles; + huddle_data.get_huddles = real_get_huddles; activity.update_huddles = function () {}; }); diff --git a/frontend_tests/node_tests/search_suggestion.js b/frontend_tests/node_tests/search_suggestion.js index 9e66c89884..143276a45a 100644 --- a/frontend_tests/node_tests/search_suggestion.js +++ b/frontend_tests/node_tests/search_suggestion.js @@ -10,6 +10,8 @@ const settings_config = zrequire('settings_config'); page_params.realm_email_address_visibility = settings_config.email_address_visibility_values.admins_only.code; +const huddle_data = zrequire('huddle_data'); + zrequire('typeahead_helper'); set_global('Handlebars', global.make_handlebars()); zrequire('Filter', 'js/filter'); @@ -281,12 +283,6 @@ run_test('group_suggestions', () => { return; }; - set_global('activity', { - get_huddles: function () { - return []; - }, - }); - // Entering a comma in a pm-with query should immediately generate // suggestions for the next person. let query = 'pm-with:bob@zulip.com,'; @@ -388,11 +384,22 @@ run_test('group_suggestions', () => { ]; assert.deepEqual(suggestions.strings, expected); - set_global('activity', { - get_huddles: function () { - return ['42,101', '42,101,103']; - }, - }); + function message(user_ids, timestamp) { + return { + type: 'private', + display_recipient: user_ids.map((id) => { + return { + id: id, + }; + }), + timestamp: timestamp, + }; + } + + huddle_data.process_loaded_messages([ + message([bob.user_id, ted.user_id], 99), + message([bob.user_id, ted.user_id, jeff.user_id], 98), + ]); // Simulate a past huddle which should now prioritize ted over alice query = 'pm-with:bob@zulip.com,'; diff --git a/frontend_tests/node_tests/search_suggestion_legacy.js b/frontend_tests/node_tests/search_suggestion_legacy.js index 57b0c845bb..288f244e74 100644 --- a/frontend_tests/node_tests/search_suggestion_legacy.js +++ b/frontend_tests/node_tests/search_suggestion_legacy.js @@ -9,6 +9,8 @@ const settings_config = zrequire('settings_config'); page_params.realm_email_address_visibility = settings_config.email_address_visibility_values.admins_only.code; +const huddle_data = zrequire('huddle_data'); + zrequire('typeahead_helper'); set_global('Handlebars', global.make_handlebars()); zrequire('Filter', 'js/filter'); @@ -278,12 +280,6 @@ run_test('group_suggestions', () => { return; }; - set_global('activity', { - get_huddles: function () { - return []; - }, - }); - // Entering a comma in a pm-with query should immediately generate // suggestions for the next person. let query = 'pm-with:bob@zulip.com,'; @@ -387,11 +383,22 @@ run_test('group_suggestions', () => { ]; assert.deepEqual(suggestions.strings, expected); - set_global('activity', { - get_huddles: function () { - return ['42,101', '42,101,103']; - }, - }); + function message(user_ids, timestamp) { + return { + type: 'private', + display_recipient: user_ids.map((id) => { + return { + id: id, + }; + }), + timestamp: timestamp, + }; + } + + huddle_data.process_loaded_messages([ + message([bob.user_id, ted.user_id], 99), + message([bob.user_id, ted.user_id, jeff.user_id], 98), + ]); // Simulate a past huddle which should now prioritize ted over alice query = 'pm-with:bob@zulip.com,'; diff --git a/static/js/activity.js b/static/js/activity.js index 87ebd1a465..41a57bfdf2 100644 --- a/static/js/activity.js +++ b/static/js/activity.js @@ -1,4 +1,5 @@ const render_group_pms = require('../templates/group_pms.hbs'); +const huddle_data = require("./huddle_data"); /* Helpers for detecting user activity and managing user idle states @@ -26,8 +27,6 @@ exports.client_is_active = document.hasFocus && document.hasFocus(); // server-initiated reload as user activity. exports.new_user_input = true; -const huddle_timestamps = new Map(); - function update_pm_count_in_dom(count_span, value_span, count) { const li = count_span.parents('li'); @@ -96,20 +95,7 @@ exports.update_dom_with_unread_counts = function (counts) { }; exports.process_loaded_messages = function (messages) { - let need_resize = false; - - for (const message of messages) { - const huddle_string = people.huddle_string(message); - - if (huddle_string) { - const old_timestamp = huddle_timestamps.get(huddle_string); - - if (!old_timestamp || old_timestamp < message.timestamp) { - huddle_timestamps.set(huddle_string, message.timestamp); - need_resize = true; - } - } - } + const need_resize = huddle_data.process_loaded_messages(messages); exports.update_huddles(); @@ -118,14 +104,6 @@ exports.process_loaded_messages = function (messages) { } }; -exports.get_huddles = function () { - let huddles = Array.from(huddle_timestamps.keys()); - huddles = _.sortBy(huddles, function (huddle) { - return huddle_timestamps.get(huddle); - }); - return huddles.reverse(); -}; - function huddle_split(huddle) { return huddle.split(',').map(s => parseInt(s, 10)); } @@ -236,7 +214,7 @@ exports.update_huddles = function () { return; } - const huddles = exports.get_huddles().slice(0, 10); + const huddles = huddle_data.get_huddles().slice(0, 10); if (huddles.length === 0) { hide_huddles(); diff --git a/static/js/huddle_data.js b/static/js/huddle_data.js new file mode 100644 index 0000000000..c71872254d --- /dev/null +++ b/static/js/huddle_data.js @@ -0,0 +1,30 @@ +const huddle_timestamps = new Map(); + +exports.process_loaded_messages = function (messages) { + let need_resize = false; + + for (const message of messages) { + const huddle_string = people.huddle_string(message); + + if (huddle_string) { + const old_timestamp = huddle_timestamps.get(huddle_string); + + if (!old_timestamp || old_timestamp < message.timestamp) { + huddle_timestamps.set(huddle_string, message.timestamp); + need_resize = true; + } + } + } + + return need_resize; +}; + +exports.get_huddles = function () { + let huddles = Array.from(huddle_timestamps.keys()); + huddles = _.sortBy(huddles, function (huddle) { + return huddle_timestamps.get(huddle); + }); + return huddles.reverse(); +}; + +window.huddle_data = exports; diff --git a/static/js/search_suggestion.js b/static/js/search_suggestion.js index 2c6f3898aa..c111578cf5 100644 --- a/static/js/search_suggestion.js +++ b/static/js/search_suggestion.js @@ -1,4 +1,5 @@ const settings_data = require("./settings_data"); +const huddle_data = require("./huddle_data"); exports.max_num_of_search_results = 12; @@ -53,7 +54,7 @@ function compare_by_huddle(huddle) { }); // Construct dict for all huddles, so we can lookup each's recency - const huddles = activity.get_huddles(); + const huddles = huddle_data.get_huddles(); const huddle_dict = new Map(); for (const [i, huddle] of huddles.entries()) { huddle_dict.set(huddle, i + 1);