From a59f5451365f643c975d68462b2ce70d04cb0566 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Bodas Date: Sat, 8 May 2021 02:04:38 +0530 Subject: [PATCH] message lists: Exclude 1:1 PMs with muted users. * We hide 1:1 PMs from and to muted users throughout the UI, because doing so will not lead to loss of conversational context. The "to" part is also important, because the last few messages sent to a user before muting them would probably be asking them to stop spamming. * After this change, we will need to do filtering for either user or topic muting in pretty much all narrows, so we need to keep the `_all_items` list in MessageListData always up-to-date. * A further commit will relax this and make it possible to view these messages only when in a `pm-with/muted_user` narrow. --- .../node_tests/message_list_data.js | 80 +++++++++++++++---- frontend_tests/node_tests/narrow_local.js | 12 +-- static/js/message_list.js | 3 + static/js/message_list_data.js | 54 ++++++------- static/js/muting_ui.js | 4 +- 5 files changed, 103 insertions(+), 50 deletions(-) diff --git a/frontend_tests/node_tests/message_list_data.js b/frontend_tests/node_tests/message_list_data.js index 200ea2f191..07985861f9 100644 --- a/frontend_tests/node_tests/message_list_data.js +++ b/frontend_tests/node_tests/message_list_data.js @@ -114,6 +114,14 @@ run_test("muting", () => { {id: 1, type: "stream", stream_id: 1, topic: "muted"}, {id: 2, type: "stream", stream_id: 1, topic: "whatever"}, {id: 3, type: "stream", stream_id: 1, topic: "muted", mentioned: true}, // mentions override muting + + // 10 = muted user, 9 = non-muted user, 11 = you + {id: 4, type: "private", to_user_ids: "9,10,11", sender_id: 10}, // muted to huddle + {id: 5, type: "private", to_user_ids: "9,10,11", sender_id: 9}, // non-muted to huddle + {id: 6, type: "private", to_user_ids: "11", sender_id: 10}, // muted to 1:1 PM + {id: 7, type: "private", to_user_ids: "11", sender_id: 9}, // non-muted to 1:1 PM + {id: 8, type: "private", to_user_ids: "10", sender_id: 11}, // 1:1 PM to muted + {id: 9, type: "private", to_user_ids: "9", sender_id: 11}, // 1:1 PM to non-muted ]; // `messages_filtered_for_topic_mutes` should skip filtering @@ -132,42 +140,84 @@ run_test("muting", () => { }, ); - // Test actual behaviour of `messages_filtered_for_topic_mutes` + // Test actual behaviour of `messages_filtered_for_*` methods. mld.excludes_muted_topics = true; muting.add_muted_topic(1, "muted"); const res = mld.messages_filtered_for_topic_mutes(msgs); assert.deepEqual(res, [ {id: 2, type: "stream", stream_id: 1, topic: "whatever"}, {id: 3, type: "stream", stream_id: 1, topic: "muted", mentioned: true}, // mentions override muting + + // `messages_filtered_for_topic_mutes` does not affect private messages + {id: 4, type: "private", to_user_ids: "9,10,11", sender_id: 10}, + {id: 5, type: "private", to_user_ids: "9,10,11", sender_id: 9}, + {id: 6, type: "private", to_user_ids: "11", sender_id: 10}, + {id: 7, type: "private", to_user_ids: "11", sender_id: 9}, + {id: 8, type: "private", to_user_ids: "10", sender_id: 11}, + {id: 9, type: "private", to_user_ids: "9", sender_id: 11}, ]); + muting.add_muted_user(10); + const res_user = mld.messages_filtered_for_user_mutes(msgs); + assert.deepEqual(res_user, [ + // `messages_filtered_for_user_mutes` does not affect stream messages + {id: 1, type: "stream", stream_id: 1, topic: "muted"}, + {id: 2, type: "stream", stream_id: 1, topic: "whatever"}, + {id: 3, type: "stream", stream_id: 1, topic: "muted", mentioned: true}, + + {id: 4, type: "private", to_user_ids: "9,10,11", sender_id: 10}, // muted to huddle + {id: 5, type: "private", to_user_ids: "9,10,11", sender_id: 9}, // non-muted to huddle + {id: 7, type: "private", to_user_ids: "11", sender_id: 9}, // non-muted to 1:1 PM + {id: 9, type: "private", to_user_ids: "9", sender_id: 11}, // 1:1 PM to non-muted + ]); + + // Output filtered based on both topic and user muting. + mld._all_items = msgs; + const filtered_messages = mld.unmuted_messages(mld._all_items); + assert.deepEqual(filtered_messages, [ + {id: 2, type: "stream", stream_id: 1, topic: "whatever"}, + {id: 3, type: "stream", stream_id: 1, topic: "muted", mentioned: true}, + {id: 4, type: "private", to_user_ids: "9,10,11", sender_id: 10}, + {id: 5, type: "private", to_user_ids: "9,10,11", sender_id: 9}, + {id: 7, type: "private", to_user_ids: "11", sender_id: 9}, + {id: 9, type: "private", to_user_ids: "9", sender_id: 11}, + ]); + + // Also verify that, the correct set of messages is stored in `_items` + // once we update the list for muting. + mld.update_items_for_muting(); + assert.deepEqual(filtered_messages, mld._items); + // MessageListData methods should always attempt to filter messages, - // and update `_all_items` when `excludes_muted_topics` is true. + // and keep `_all_items` up-to-date. mld = new MessageListData({ excludes_muted_topics: true, }); assert.deepEqual(mld._all_items, []); - let messages_filtered_for_topic_mutes_calls = 0; - mld.messages_filtered_for_topic_mutes = (messages) => { - messages_filtered_for_topic_mutes_calls = messages_filtered_for_topic_mutes_calls + 1; + let unmuted_messages_calls = 0; + mld.unmuted_messages = (messages) => { + unmuted_messages_calls = unmuted_messages_calls + 1; return messages; }; - mld.add_anywhere([{id: 10}]); - assert.equal(messages_filtered_for_topic_mutes_calls, 1); - assert_msg_ids(mld._all_items, [10]); + mld.add_anywhere([{id: 10}, {id: 20}]); + assert.equal(unmuted_messages_calls, 1); + assert_msg_ids(mld._all_items, [10, 20]); - mld.prepend([{id: 9}]); - assert.equal(messages_filtered_for_topic_mutes_calls, 2); - assert_msg_ids(mld._all_items, [9, 10]); + mld.prepend([{id: 9}, {id: 19}]); + assert.equal(unmuted_messages_calls, 2); + assert_msg_ids(mld._all_items, [9, 19, 10, 20]); - mld.append([{id: 11}]); - assert.equal(messages_filtered_for_topic_mutes_calls, 3); - assert_msg_ids(mld._all_items, [9, 10, 11]); + mld.append([{id: 11}, {id: 21}]); + assert.equal(unmuted_messages_calls, 3); + assert_msg_ids(mld._all_items, [9, 19, 10, 20, 11, 21]); mld.remove([9]); - assert_msg_ids(mld._all_items, [10, 11]); + assert_msg_ids(mld._all_items, [19, 10, 20, 11, 21]); + + mld.reorder_messages(20); + assert_msg_ids(mld._all_items, [10, 11, 19, 20, 21]); mld.clear(); assert_msg_ids(mld._all_items, []); diff --git a/frontend_tests/node_tests/narrow_local.js b/frontend_tests/node_tests/narrow_local.js index a64a55a54f..a5a135d839 100644 --- a/frontend_tests/node_tests/narrow_local.js +++ b/frontend_tests/node_tests/narrow_local.js @@ -179,9 +179,9 @@ run_test("is private with no target", () => { }, has_found_newest: true, all_messages: [ - {id: 450, type: "private"}, - {id: 500, type: "private"}, - {id: 550, type: "private"}, + {id: 450, type: "private", to_user_ids: "1,2"}, + {id: 500, type: "private", to_user_ids: "1,2"}, + {id: 550, type: "private", to_user_ids: "1,2"}, ], expected_id_info: { target_id: undefined, @@ -244,9 +244,9 @@ run_test("is:private with target and no unreads", () => { empty: false, all_messages: [ {id: 350}, - {id: 400, type: "private"}, - {id: 450, type: "private"}, - {id: 500, type: "private"}, + {id: 400, type: "private", to_user_ids: "1,2"}, + {id: 450, type: "private", to_user_ids: "1,2"}, + {id: 500, type: "private", to_user_ids: "1,2"}, ], expected_id_info: { target_id: 450, diff --git a/static/js/message_list.js b/static/js/message_list.js index 594729ed56..2fc99d9f21 100644 --- a/static/js/message_list.js +++ b/static/js/message_list.js @@ -395,6 +395,9 @@ export class MessageList { // to do this is in the message_events.js code path for // processing topic edits, since that's the only place we'll // call this frequently anyway. + // + // But in any case, we need to rerender the list for user muting, + // to make sure only the right messages are hidden. this.rerender(); } diff --git a/static/js/message_list_data.js b/static/js/message_list_data.js index 58fce13be4..38717daa63 100644 --- a/static/js/message_list_data.js +++ b/static/js/message_list_data.js @@ -11,9 +11,7 @@ import * as util from "./util"; export class MessageListData { constructor({excludes_muted_topics, filter = new Filter()}) { this.excludes_muted_topics = excludes_muted_topics; - if (this.excludes_muted_topics) { - this._all_items = []; - } + this._all_items = []; this._items = []; this._hash = new Map(); this._local_only = new Set(); @@ -109,10 +107,7 @@ export class MessageListData { } clear() { - if (this.excludes_muted_topics) { - this._all_items = []; - } - + this._all_items = []; this._items = []; this._hash.clear(); } @@ -182,14 +177,29 @@ export class MessageListData { }); } + messages_filtered_for_user_mutes(messages) { + return messages.filter((message) => { + if (message.type !== "private") { + return true; + } + const recipients = util.extract_pm_recipients(message.to_user_ids); + if (recipients.length > 1) { + // Huddle message + return true; + } + + const recipient_id = Number.parseInt(recipients[0], 10); + return !muting.is_user_muted(recipient_id) && !muting.is_user_muted(message.sender_id); + }); + } + unmuted_messages(messages) { - return this.messages_filtered_for_topic_mutes(messages); + return this.messages_filtered_for_topic_mutes( + this.messages_filtered_for_user_mutes(messages), + ); } update_items_for_muting() { - if (!this.excludes_muted_topics) { - return; - } this._items = this.unmuted_messages(this._all_items); } @@ -264,10 +274,8 @@ export class MessageListData { const viewable_messages = this.unmuted_messages(messages); - if (this.excludes_muted_topics) { - this._all_items = messages.concat(this._all_items); - this._all_items.sort((a, b) => a.id - b.id); - } + this._all_items = messages.concat(this._all_items); + this._all_items.sort((a, b) => a.id - b.id); this._items = viewable_messages.concat(this._items); this._items.sort((a, b) => a.id - b.id); @@ -280,9 +288,7 @@ export class MessageListData { // Caller should have already filtered const viewable_messages = this.unmuted_messages(messages); - if (this.excludes_muted_topics) { - this._all_items = this._all_items.concat(messages); - } + this._all_items = this._all_items.concat(messages); this._items = this._items.concat(viewable_messages); this._add_to_hash(messages); @@ -293,9 +299,7 @@ export class MessageListData { // Caller should have already filtered const viewable_messages = this.unmuted_messages(messages); - if (this.excludes_muted_topics) { - this._all_items = messages.concat(this._all_items); - } + this._all_items = messages.concat(this._all_items); this._items = viewable_messages.concat(this._items); this._add_to_hash(messages); @@ -310,9 +314,7 @@ export class MessageListData { } this._items = this._items.filter((msg) => !msg_ids_to_remove.has(msg.id)); - if (this.excludes_muted_topics) { - this._all_items = this._all_items.filter((msg) => !msg_ids_to_remove.has(msg.id)); - } + this._all_items = this._all_items.filter((msg) => !msg_ids_to_remove.has(msg.id)); } // Returns messages from the given message list in the specified range, inclusive @@ -501,9 +503,7 @@ export class MessageListData { ) { blueslip.debug("Changed message ID from server caused out-of-order list, reordering"); this._items.sort(message_sort_func); - if (this.excludes_muted_topics) { - this._all_items.sort(message_sort_func); - } + this._all_items.sort(message_sort_func); return true; } diff --git a/static/js/muting_ui.js b/static/js/muting_ui.js index 5edc156a0b..b283555411 100644 --- a/static/js/muting_ui.js +++ b/static/js/muting_ui.js @@ -157,9 +157,9 @@ export function unmute_user(user_id) { } export function rerender_for_muted_user() { - message_lists.current.rerender(); + message_lists.current.update_muting_and_rerender(); if (message_lists.current !== message_lists.home) { - message_lists.home.rerender(); + message_lists.home.update_muting_and_rerender(); } if (overlays.settings_open() && settings_muted_users.loaded) {