refactor: Extract message_helper.process_new_message.

We move the message_store.add_message_metadata function
(and all its dependencies) into a new module called
message_helper and rename the function to process_new_message.
(It does a bit more than adding message metadata, such
as updating our message store.)

We also have a "protected" interface now in message_store,
so that message_helper can access the message store:

    update_message_cache
    get_cached_message

Because update_message_cache is identical to
the former create_mock_message, we just renamed it
in the tests.

Most callers should use these functions:

    message_helper.process_new_message (setting)
    message_store.get (getting)

It's slightly annoying that the setter interface
is in a different module than the getter interface,
but that's how you break a bunch of dependencies.

We also extract the tiny message_user_ids class:

    user_ids()
    add_user_ids()

All the code moves here are pretty trivial, and
the code that was moved maintains 100% line
coverage.

The module name `message_helper` is not ideal, but it's a single
function and it'll save time to just do the topology change now and
leave thinking through the right name to later.

frontend_tests/node_tests/composebox_typeahead.js

@@ -14,7 +14,7 @@ const channel = mock_esm("../../static/js/channel");
 const compose = mock_esm("../../static/js/compose", {
     finish: noop,
 });
-const message_store = mock_esm("../../static/js/message_store", {
+const message_user_ids = mock_esm("../../static/js/message_user_ids", {
     user_ids: () => [],
 });
 const stream_topic_history = mock_esm("../../static/js/stream_topic_history");
@@ -1511,7 +1511,8 @@ test("message people", (override) => {
         the sorting step.
     */
 
-    override(message_store, "user_ids", () => [hal.user_id, harry.user_id]);
+    let user_ids = [hal.user_id, harry.user_id];
+    override(message_user_ids, "user_ids", () => user_ids);
 
     const opts = {
         want_broadcast: false,
@@ -1529,12 +1530,12 @@ test("message people", (override) => {
     assert.deepEqual(results, [harry, hamletcharacters]);
 
     // Now let's exclude Hal.
-    message_store.user_ids = () => [hamlet.user_id, harry.user_id];
+    user_ids = [hamlet.user_id, harry.user_id];
 
     results = get_results("Ha");
     assert.deepEqual(results, [harry, hamletcharacters]);
 
-    message_store.user_ids = () => [hamlet.user_id, harry.user_id, hal.user_id];
+    user_ids = [hamlet.user_id, harry.user_id, hal.user_id];
 
     results = get_results("Ha");
     assert.deepEqual(results, [harry, hamletcharacters]);

frontend_tests/node_tests/dispatch.js

@@ -78,6 +78,7 @@ page_params.realm_description = "already set description";
 const alert_words = zrequire("alert_words");
 const stream_topic_history = zrequire("stream_topic_history");
 const stream_list = zrequire("stream_list");
+const message_helper = zrequire("message_helper");
 const message_store = zrequire("message_store");
 const people = zrequire("people");
 const starred_messages = zrequire("starred_messages");
@@ -94,7 +95,7 @@ function dispatch(ev) {
 people.init();
 people.add_active_user(test_user);
 
-message_store.add_message_metadata(test_message);
+message_helper.process_new_message(test_message);
 
 const realm_emoji = {};
 const emoji_codes = zrequire("../generated/emoji/emoji_codes.json");

frontend_tests/node_tests/example2.js

@@ -11,6 +11,7 @@ const {run_test} = require("../zjsunit/test");
 // data objects.  Also, we start testing some objects that have
 // deeper dependencies.
 
+const message_helper = zrequire("message_helper");
 const message_store = zrequire("message_store");
 const people = zrequire("people");
 const stream_data = zrequire("stream_data");
@@ -67,9 +68,9 @@ run_test("message_store", () => {
     assert.equal(in_message.alerted, true);
 
     // Let's add a message into our message_store via
-    // add_message_metadata.
+    // message_helper.process_new_message.
     assert.equal(message_store.get(in_message.id), undefined);
-    message_store.add_message_metadata(in_message);
+    message_helper.process_new_message(in_message);
     const message = message_store.get(in_message.id);
     assert.equal(message, in_message);
 

frontend_tests/node_tests/message_events.js

@@ -19,6 +19,7 @@ set_global("current_msg_list", {});
 
 const people = zrequire("people");
 const message_events = zrequire("message_events");
+const message_helper = zrequire("message_helper");
 const message_store = zrequire("message_store");
 const stream_data = zrequire("stream_data");
 const stream_topic_history = zrequire("stream_topic_history");
@@ -68,7 +69,7 @@ run_test("update_messages", () => {
         type: "stream",
     };
 
-    message_store.add_message_metadata(original_message);
+    message_helper.process_new_message(original_message);
     message_store.set_message_booleans(original_message);
 
     assert.equal(original_message.mentioned, true);

frontend_tests/node_tests/message_fetch.js

@@ -29,6 +29,7 @@ mock_esm("../../static/js/ui_report", {
 });
 
 const channel = mock_esm("../../static/js/channel");
+const message_helper = mock_esm("../../static/js/message_helper");
 const message_store = mock_esm("../../static/js/message_store");
 const message_util = mock_esm("../../static/js/message_util");
 const pm_list = mock_esm("../../static/js/pm_list");
@@ -127,7 +128,7 @@ function config_process_results(messages) {
         messages_processed_for_bools.push(message);
     };
 
-    message_store.add_message_metadata = (message) => message;
+    message_helper.process_new_message = (message) => message;
 
     message_util.do_unread_count_updates = (arg) => {
         assert.deepEqual(arg, messages);

frontend_tests/node_tests/message_store.js

@@ -26,7 +26,9 @@ page_params.is_admin = true;
 const util = zrequire("util");
 const people = zrequire("people");
 const pm_conversations = zrequire("pm_conversations");
+const message_helper = zrequire("message_helper");
 const message_store = zrequire("message_store");
+const message_user_ids = zrequire("message_user_ids");
 
 const denmark = {
     subscribed: false,
@@ -94,7 +96,7 @@ function test(label, f) {
     });
 }
 
-test("add_message_metadata", () => {
+test("process_new_message", () => {
     let message = {
         sender_email: "me@example.com",
         sender_id: me.user_id,
@@ -105,9 +107,9 @@ test("add_message_metadata", () => {
         id: 2067,
     };
     message_store.set_message_booleans(message);
-    message_store.add_message_metadata(message);
+    message_helper.process_new_message(message);
 
-    assert.deepEqual(message_store.user_ids().sort(), [me.user_id, bob.user_id, cindy.user_id]);
+    assert.deepEqual(message_user_ids.user_ids().sort(), [me.user_id, bob.user_id, cindy.user_id]);
 
     assert.equal(message.is_private, true);
     assert.equal(message.reply_to, "bob@example.com,cindy@example.com");
@@ -129,7 +131,7 @@ test("add_message_metadata", () => {
         match_subject: "topic foo",
         match_content: "bar content",
     };
-    message = message_store.add_message_metadata(message);
+    message = message_helper.process_new_message(message);
 
     assert.equal(message.reply_to, "bob@example.com,cindy@example.com");
     assert.equal(message.to_user_ids, "103,104");
@@ -148,13 +150,13 @@ test("add_message_metadata", () => {
     };
 
     message_store.set_message_booleans(message);
-    message_store.add_message_metadata(message);
+    message_helper.process_new_message(message);
     assert.deepEqual(message.stream, message.display_recipient);
     assert.equal(message.reply_to, "denise@example.com");
     assert.deepEqual(message.flags, undefined);
     assert.equal(message.alerted, false);
 
-    assert.deepEqual(message_store.user_ids().sort(), [
+    assert.deepEqual(message_user_ids.user_ids().sort(), [
         me.user_id,
         bob.user_id,
         cindy.user_id,
@@ -296,7 +298,7 @@ test("update_property", () => {
     };
     for (const message of [message1, message2]) {
         message_store.set_message_booleans(message);
-        message_store.add_message_metadata(message);
+        message_helper.process_new_message(message);
     }
 
     assert.equal(message1.sender_full_name, alice.full_name);
@@ -331,7 +333,7 @@ test("message_id_change", () => {
         flags: ["has_alert_word"],
         id: 401,
     };
-    message_store.add_message_metadata(message);
+    message_helper.process_new_message(message);
 
     const opts = {
         old_id: 401,

frontend_tests/node_tests/narrow_unread.js

@@ -70,8 +70,8 @@ run_test("get_unread_ids", () => {
         display_recipient: [{id: alice.user_id}],
     };
 
-    message_store.create_mock_message(stream_msg);
-    message_store.create_mock_message(private_msg);
+    message_store.update_message_cache(stream_msg);
+    message_store.update_message_cache(private_msg);
 
     stream_data.add_sub(sub);
 

frontend_tests/node_tests/people.js

@@ -12,7 +12,7 @@ const {run_test} = require("../zjsunit/test");
 const blueslip = require("../zjsunit/zblueslip");
 const {page_params} = require("../zjsunit/zpage_params");
 
-const message_store = mock_esm("../../static/js/message_store");
+const message_user_ids = mock_esm("../../static/js/message_user_ids");
 
 const people = zrequire("people");
 const settings_config = zrequire("settings_config");
@@ -814,9 +814,9 @@ test_people("slugs", () => {
 });
 
 test_people("get_people_for_search_bar", (override) => {
-    let message_user_ids;
+    let user_ids;
 
-    override(message_store, "user_ids", () => message_user_ids);
+    override(message_user_ids, "user_ids", () => user_ids);
 
     for (const i of _.range(20)) {
         const person = {
@@ -827,16 +827,16 @@ test_people("get_people_for_search_bar", (override) => {
         people.add_active_user(person);
     }
 
-    message_user_ids = [];
+    user_ids = [];
     const big_results = people.get_people_for_search_bar("James");
 
     assert.equal(big_results.length, 20);
 
-    message_user_ids = [1001, 1002, 1003, 1004, 1005, 1006];
+    user_ids = [1001, 1002, 1003, 1004, 1005, 1006];
     const small_results = people.get_people_for_search_bar("Jones");
 
     // As long as there are 5+ results among the user_ids
-    // in message_store, we will get a small result and not
+    // in message_user_ids, we will get a small result and not
     // search all people.
     assert.equal(small_results.length, 6);
 });
@@ -1076,7 +1076,7 @@ test_people("get_visible_email", () => {
 });
 
 test_people("get_active_message_people", () => {
-    message_store.user_ids = () => [steven.user_id, maria.user_id, alice1.user_id];
+    message_user_ids.user_ids = () => [steven.user_id, maria.user_id, alice1.user_id];
 
     people.add_active_user(steven);
     people.add_active_user(maria);

frontend_tests/node_tests/search_suggestion.js

@@ -2,16 +2,12 @@
 
 const {strict: assert} = require("assert");
 
-const {mock_esm, with_field, zrequire} = require("../zjsunit/namespace");
+const {with_field, zrequire} = require("../zjsunit/namespace");
 const {run_test} = require("../zjsunit/test");
 const {page_params} = require("../zjsunit/zpage_params");
 
 page_params.search_pills_enabled = true;
 
-mock_esm("../../static/js/message_store", {
-    user_ids: () => [],
-});
-
 const settings_config = zrequire("settings_config");
 page_params.realm_email_address_visibility =
     settings_config.email_address_visibility_values.admins_only.code;

frontend_tests/node_tests/search_suggestion_legacy.js

@@ -2,14 +2,11 @@
 
 const {strict: assert} = require("assert");
 
-const {mock_esm, zrequire} = require("../zjsunit/namespace");
+const {zrequire} = require("../zjsunit/namespace");
 const {run_test} = require("../zjsunit/test");
 const {page_params} = require("../zjsunit/zpage_params");
 
 page_params.search_pills_enabled = false;
-mock_esm("../../static/js/message_store", {
-    user_ids: () => [],
-});
 
 const settings_config = zrequire("settings_config");
 page_params.realm_email_address_visibility =

frontend_tests/node_tests/starred_messages.js

@@ -51,25 +51,25 @@ run_test("get starred ids in topic", () => {
 
     // id: 1 isn't inserted, to test handling the case
     // when message_store.get() returns undefined
-    message_store.create_mock_message({
+    message_store.update_message_cache({
         id: 2,
         type: "private",
     });
-    message_store.create_mock_message({
+    message_store.update_message_cache({
         // Different stream
         id: 3,
         type: "stream",
         stream_id: 19,
         topic: "topic",
     });
-    message_store.create_mock_message({
+    message_store.update_message_cache({
         // Different topic
         id: 4,
         type: "stream",
         stream_id: 20,
         topic: "some other topic",
     });
-    message_store.create_mock_message({
+    message_store.update_message_cache({
         // Correct match
         id: 5,
         type: "stream",

frontend_tests/node_tests/unread.js

@@ -176,9 +176,9 @@ test("changing_topics", () => {
         unread: true,
     };
 
-    message_store.create_mock_message(message);
-    message_store.create_mock_message(other_message);
-    message_store.create_mock_message(sticky_message);
+    message_store.update_message_cache(message);
+    message_store.update_message_cache(other_message);
+    message_store.update_message_cache(sticky_message);
 
     unread.process_loaded_messages([sticky_message]);
     count = unread.num_unread_for_topic(stream_id, "sticky");

static/js/compose_fade.js

@@ -35,7 +35,7 @@ export function set_focused_recipient(msg_type) {
         }
     } else {
         // Normalize the recipient list so it matches the one used when
-        // adding the message (see message_store.add_message_metadata()).
+        // adding the message (see message_helper.process_new_message()).
         const reply_to = util.normalize_recipients(compose_state.private_message_recipient());
         focused_recipient.reply_to = reply_to;
         focused_recipient.to_user_ids = people.reply_to_to_user_ids_string(reply_to);

static/js/message_events.js

@@ -8,6 +8,7 @@ import * as condense from "./condense";
 import * as huddle_data from "./huddle_data";
 import * as message_edit from "./message_edit";
 import * as message_edit_history from "./message_edit_history";
+import * as message_helper from "./message_helper";
 import * as message_list from "./message_list";
 import * as message_store from "./message_store";
 import * as message_util from "./message_util";
@@ -59,14 +60,14 @@ function maybe_add_narrowed_messages(messages, msg_list) {
                 }
             }
 
-            // This second call to add_message_metadata in the
+            // This second call to process_new_message in the
             // insert_new_messages code path helps in very rare race
             // conditions, where e.g. the current user's name was
             // edited in between when they sent the message and when
             // we hear back from the server and can echo the new
             // message.  Arguably, it's counterproductive complexity.
             new_messages = new_messages.map((message) =>
-                message_store.add_message_metadata(message),
+                message_helper.process_new_message(message),
             );
 
             message_util.add_new_messages(new_messages, msg_list);
@@ -87,7 +88,7 @@ function maybe_add_narrowed_messages(messages, msg_list) {
 }
 
 export function insert_new_messages(messages, sent_by_this_client) {
-    messages = messages.map((message) => message_store.add_message_metadata(message));
+    messages = messages.map((message) => message_helper.process_new_message(message));
 
     unread.process_loaded_messages(messages);
     huddle_data.process_loaded_messages(messages);

static/js/message_fetch.js

@@ -3,6 +3,7 @@ import $ from "jquery";
 import * as channel from "./channel";
 import {Filter} from "./filter";
 import * as huddle_data from "./huddle_data";
+import * as message_helper from "./message_helper";
 import * as message_list from "./message_list";
 import * as message_scroll from "./message_scroll";
 import * as message_store from "./message_store";
@@ -49,7 +50,7 @@ function process_result(data, opts) {
 
     messages = messages.map((message) => {
         message_store.set_message_booleans(message);
-        return message_store.add_message_metadata(message);
+        return message_helper.process_new_message(message);
     });
 
     // In case any of the newly fetched messages are new, add them to

static/js/message_helper.js

@@ -0,0 +1,78 @@
+import * as alert_words from "./alert_words";
+import * as message_store from "./message_store";
+import * as message_user_ids from "./message_user_ids";
+import * as people from "./people";
+import * as recent_senders from "./recent_senders";
+import * as stream_topic_history from "./stream_topic_history";
+import * as util from "./util";
+
+export function process_new_message(message) {
+    // Call this function when processing a new message.  After
+    // a message is processed and inserted into the message store
+    // cache, most modules use message_store.get to look at
+    // messages.
+    const cached_msg = message_store.get_cached_message(message.id);
+    if (cached_msg !== undefined) {
+        // Copy the match topic and content over if they exist on
+        // the new message
+        if (util.get_match_topic(message) !== undefined) {
+            util.set_match_data(cached_msg, message);
+        }
+        return cached_msg;
+    }
+
+    message.sent_by_me = people.is_current_user(message.sender_email);
+
+    people.extract_people_from_message(message);
+    people.maybe_incr_recipient_count(message);
+
+    const sender = people.get_by_user_id(message.sender_id);
+    if (sender) {
+        message.sender_full_name = sender.full_name;
+        message.sender_email = sender.email;
+    }
+
+    // Convert topic even for PMs, as legacy code
+    // wants the empty field.
+    util.convert_message_topic(message);
+
+    switch (message.type) {
+        case "stream":
+            message.is_stream = true;
+            message.stream = message.display_recipient;
+            message.reply_to = message.sender_email;
+
+            stream_topic_history.add_message({
+                stream_id: message.stream_id,
+                topic_name: message.topic,
+                message_id: message.id,
+            });
+
+            recent_senders.process_message_for_senders(message);
+            message_user_ids.add_user_id(message.sender_id);
+            break;
+
+        case "private":
+            message.is_private = true;
+            message.reply_to = util.normalize_recipients(message_store.get_pm_emails(message));
+            message.display_reply_to = message_store.get_pm_full_names(message);
+            message.pm_with_url = people.pm_with_url(message);
+            message.to_user_ids = people.pm_reply_user_string(message);
+
+            message_store.process_message_for_recent_private_messages(message);
+
+            if (people.is_my_user_id(message.sender_id)) {
+                for (const recip of message.display_recipient) {
+                    message_user_ids.add_user_id(recip.id);
+                }
+            }
+            break;
+    }
+
+    alert_words.process_message(message);
+    if (!message.reactions) {
+        message.reactions = [];
+    }
+    message_store.update_message_cache(message);
+    return message;
+}

static/js/message_store.js

@@ -1,42 +1,23 @@
-import * as alert_words from "./alert_words";
 import * as blueslip from "./blueslip";
 import * as message_list from "./message_list";
 import * as people from "./people";
 import * as pm_conversations from "./pm_conversations";
-import * as recent_senders from "./recent_senders";
-import * as stream_topic_history from "./stream_topic_history";
-import * as util from "./util";
 
 const stored_messages = new Map();
 
-/*
-    We keep a set of user_ids for all people
-    who have sent stream messages or who have
-    been on PMs sent by the user.
+export function update_message_cache(message) {
+    // You should only call this from message_helper (or in tests).
+    stored_messages.set(message.id, message);
+}
 
-    We will use this in search to prevent really
-    large result sets for realms that have lots
-    of users who haven't sent messages recently.
-
-    We'll likely eventually want to replace this with
-    accessing some combination of data from recent_senders
-    and pm_conversations for better accuracy.
-*/
-const message_user_ids = new Set();
+export function get_cached_message(message_id) {
+    // You should only call this from message_helper.
+    // Use the get() wrapper below for most other use cases.
+    return stored_messages.get(message_id);
+}
 
 export function clear_for_testing() {
     stored_messages.clear();
-    message_user_ids.clear();
-}
-
-export function user_ids() {
-    return Array.from(message_user_ids);
-}
-
-export function create_mock_message(message) {
-    // For use in tests only. `id` is a required field,
-    // everything else is optional, as required in the test.
-    stored_messages.set(message.id, message);
 }
 
 export function get(message_id) {
@@ -149,73 +130,6 @@ export function update_booleans(message, flags) {
     message.alerted = convert_flag("has_alert_word");
 }
 
-export function add_message_metadata(message) {
-    const cached_msg = stored_messages.get(message.id);
-    if (cached_msg !== undefined) {
-        // Copy the match topic and content over if they exist on
-        // the new message
-        if (util.get_match_topic(message) !== undefined) {
-            util.set_match_data(cached_msg, message);
-        }
-        return cached_msg;
-    }
-
-    message.sent_by_me = people.is_current_user(message.sender_email);
-
-    people.extract_people_from_message(message);
-    people.maybe_incr_recipient_count(message);
-
-    const sender = people.get_by_user_id(message.sender_id);
-    if (sender) {
-        message.sender_full_name = sender.full_name;
-        message.sender_email = sender.email;
-    }
-
-    // Convert topic even for PMs, as legacy code
-    // wants the empty field.
-    util.convert_message_topic(message);
-
-    switch (message.type) {
-        case "stream":
-            message.is_stream = true;
-            message.stream = message.display_recipient;
-            message.reply_to = message.sender_email;
-
-            stream_topic_history.add_message({
-                stream_id: message.stream_id,
-                topic_name: message.topic,
-                message_id: message.id,
-            });
-
-            recent_senders.process_message_for_senders(message);
-            message_user_ids.add(message.sender_id);
-            break;
-
-        case "private":
-            message.is_private = true;
-            message.reply_to = util.normalize_recipients(get_pm_emails(message));
-            message.display_reply_to = get_pm_full_names(message);
-            message.pm_with_url = people.pm_with_url(message);
-            message.to_user_ids = people.pm_reply_user_string(message);
-
-            process_message_for_recent_private_messages(message);
-
-            if (people.is_my_user_id(message.sender_id)) {
-                for (const recip of message.display_recipient) {
-                    message_user_ids.add(recip.id);
-                }
-            }
-            break;
-    }
-
-    alert_words.process_message(message);
-    if (!message.reactions) {
-        message.reactions = [];
-    }
-    stored_messages.set(message.id, message);
-    return message;
-}
-
 export function update_property(property, value, info) {
     switch (property) {
         case "sender_full_name":

static/js/message_user_ids.js

@@ -0,0 +1,22 @@
+/*
+    We keep a set of user_ids for all people
+    who have sent stream messages or who have
+    been on PMs sent by the user.
+
+    We will use this in search to prevent really
+    large result sets for realms that have lots
+    of users who haven't sent messages recently.
+
+    We'll likely eventually want to replace this with
+    accessing some combination of data from recent_senders
+    and pm_conversations for better accuracy.
+*/
+const user_set = new Set();
+
+export function user_ids() {
+    return Array.from(user_set);
+}
+
+export function add_user_id(user_id) {
+    user_set.add(user_id);
+}

static/js/people.js

@@ -7,7 +7,7 @@ import * as typeahead from "../shared/js/typeahead";
 import * as blueslip from "./blueslip";
 import {FoldDict} from "./fold_dict";
 import {i18n} from "./i18n";
-import * as message_store from "./message_store";
+import * as message_user_ids from "./message_user_ids";
 import * as reload_state from "./reload_state";
 import * as settings_data from "./settings_data";
 import * as util from "./util";
@@ -882,7 +882,7 @@ export function get_message_people() {
         at the message_store code to see the precise
         semantics
     */
-    const message_people = message_store
+    const message_people = message_user_ids
         .user_ids()
         .map((user_id) => people_by_user_id_dict.get(user_id))
         .filter(Boolean);