diff --git a/frontend_tests/node_tests/composebox_typeahead.js b/frontend_tests/node_tests/composebox_typeahead.js index b3e95e3412..69bfde8e9f 100644 --- a/frontend_tests/node_tests/composebox_typeahead.js +++ b/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]); diff --git a/frontend_tests/node_tests/dispatch.js b/frontend_tests/node_tests/dispatch.js index 5b5be1df68..de2f2f1528 100644 --- a/frontend_tests/node_tests/dispatch.js +++ b/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"); diff --git a/frontend_tests/node_tests/example2.js b/frontend_tests/node_tests/example2.js index 691515cce8..f10f431a1d 100644 --- a/frontend_tests/node_tests/example2.js +++ b/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); diff --git a/frontend_tests/node_tests/message_events.js b/frontend_tests/node_tests/message_events.js index 2e4fbed488..9aaef1abb0 100644 --- a/frontend_tests/node_tests/message_events.js +++ b/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); diff --git a/frontend_tests/node_tests/message_fetch.js b/frontend_tests/node_tests/message_fetch.js index f555c9efa4..123e1e9b2a 100644 --- a/frontend_tests/node_tests/message_fetch.js +++ b/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); diff --git a/frontend_tests/node_tests/message_store.js b/frontend_tests/node_tests/message_store.js index c16a75cfb3..06e1794094 100644 --- a/frontend_tests/node_tests/message_store.js +++ b/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, diff --git a/frontend_tests/node_tests/narrow_unread.js b/frontend_tests/node_tests/narrow_unread.js index 7ae51e4155..e347e079a5 100644 --- a/frontend_tests/node_tests/narrow_unread.js +++ b/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); diff --git a/frontend_tests/node_tests/people.js b/frontend_tests/node_tests/people.js index c58b72ddf7..36080e0210 100644 --- a/frontend_tests/node_tests/people.js +++ b/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); diff --git a/frontend_tests/node_tests/search_suggestion.js b/frontend_tests/node_tests/search_suggestion.js index e3a32d0a9b..1487f66c06 100644 --- a/frontend_tests/node_tests/search_suggestion.js +++ b/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; diff --git a/frontend_tests/node_tests/search_suggestion_legacy.js b/frontend_tests/node_tests/search_suggestion_legacy.js index 5fa4afd22d..975cd267d6 100644 --- a/frontend_tests/node_tests/search_suggestion_legacy.js +++ b/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 = diff --git a/frontend_tests/node_tests/starred_messages.js b/frontend_tests/node_tests/starred_messages.js index 41064609e4..1673b3ae69 100644 --- a/frontend_tests/node_tests/starred_messages.js +++ b/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", diff --git a/frontend_tests/node_tests/unread.js b/frontend_tests/node_tests/unread.js index a6b779999c..8cb9d2cd24 100644 --- a/frontend_tests/node_tests/unread.js +++ b/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"); diff --git a/static/js/compose_fade.js b/static/js/compose_fade.js index 6e3ccee3ad..93d0d625f6 100644 --- a/static/js/compose_fade.js +++ b/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); diff --git a/static/js/message_events.js b/static/js/message_events.js index ac5068c6f1..09329d607b 100644 --- a/static/js/message_events.js +++ b/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); diff --git a/static/js/message_fetch.js b/static/js/message_fetch.js index f0efb3d59d..ab42017f26 100644 --- a/static/js/message_fetch.js +++ b/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 diff --git a/static/js/message_helper.js b/static/js/message_helper.js new file mode 100644 index 0000000000..91dd070b0f --- /dev/null +++ b/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; +} diff --git a/static/js/message_store.js b/static/js/message_store.js index 971ff14750..8ea96f144b 100644 --- a/static/js/message_store.js +++ b/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": diff --git a/static/js/message_user_ids.js b/static/js/message_user_ids.js new file mode 100644 index 0000000000..e4684fd99c --- /dev/null +++ b/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); +} diff --git a/static/js/people.js b/static/js/people.js index d390529561..e7f7fc6b20 100644 --- a/static/js/people.js +++ b/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);