From 38a4751b7bb8535416569bb5f41b246f83ac64c2 Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Sat, 19 Nov 2022 06:39:00 +0000 Subject: [PATCH] recent: Sort PM recipients by their last sent message id. Fixes #23563 --- frontend_tests/node_tests/message_store.js | 1 + frontend_tests/node_tests/recent_senders.js | 38 ++++++++++++++++++++ static/js/message_helper.js | 1 + static/js/recent_senders.js | 39 +++++++++++++++++++++ static/js/recent_topics_ui.js | 15 +++++--- 5 files changed, 90 insertions(+), 4 deletions(-) diff --git a/frontend_tests/node_tests/message_store.js b/frontend_tests/node_tests/message_store.js index aab7cdd734..52c7b0ba20 100644 --- a/frontend_tests/node_tests/message_store.js +++ b/frontend_tests/node_tests/message_store.js @@ -15,6 +15,7 @@ mock_esm("../../static/js/stream_topic_history", { mock_esm("../../static/js/recent_senders", { process_stream_message: noop, + process_private_message: noop, }); set_global("document", "document-stub"); diff --git a/frontend_tests/node_tests/recent_senders.js b/frontend_tests/node_tests/recent_senders.js index af9f88f8f5..4b3b7b8aad 100644 --- a/frontend_tests/node_tests/recent_senders.js +++ b/frontend_tests/node_tests/recent_senders.js @@ -26,6 +26,9 @@ function make_stream_message({stream_id, topic, sender_id}) { mock_esm("../../static/js/message_store", { get: (message_id) => messages.get(message_id), }); +mock_esm("../../static/js/people", { + my_current_user_id: () => 1, +}); const rs = zrequire("recent_senders"); zrequire("message_util.js"); @@ -310,3 +313,38 @@ test("process_stream_message", () => { true, ); }); + +test("process_pms", () => { + const sender1 = 1; // Current user id + const sender2 = 2; + const sender3 = 3; + + const user_ids_string = "2,3,4"; + rs.process_private_message({ + to_user_ids: user_ids_string, + sender_id: sender2, + id: 1, + }); + rs.process_private_message({ + to_user_ids: user_ids_string, + sender_id: sender3, + id: 2, + }); + rs.process_private_message({ + to_user_ids: user_ids_string, + sender_id: sender1, + id: 3, + }); + + // Recent topics displays avatars in the opposite order to this since + // that was simpler to implement in HTML. + assert.deepEqual(rs.get_pm_recent_senders(user_ids_string), { + participants: [1, 3, 2], + non_participants: [4], + }); + // PM doesn't exist. + assert.deepEqual(rs.get_pm_recent_senders("1000,2000"), { + participants: [], + non_participants: [], + }); +}); diff --git a/static/js/message_helper.js b/static/js/message_helper.js index 90e5d47102..0d827e3431 100644 --- a/static/js/message_helper.js +++ b/static/js/message_helper.js @@ -65,6 +65,7 @@ export function process_new_message(message) { pm_conversations.process_message(message); + recent_senders.process_private_message(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); diff --git a/static/js/recent_senders.js b/static/js/recent_senders.js index 35e7ba7700..88d22a41ad 100644 --- a/static/js/recent_senders.js +++ b/static/js/recent_senders.js @@ -2,6 +2,7 @@ import _ from "lodash"; import {FoldDict} from "./fold_dict"; import * as message_store from "./message_store"; +import * as people from "./people"; // This class is only exported for unit testing purposes. // If we find reuse opportunities, we should just put it into @@ -46,6 +47,9 @@ const stream_senders = new Map(); // topic_senders[stream_id][topic_id][sender_id] = IdTracker const topic_senders = new Map(); +// pm_senders[user_ids_string][user_id] = IdTracker +const pm_senders = new Map(); + export function clear_for_testing() { stream_senders.clear(); topic_senders.clear(); @@ -211,3 +215,38 @@ export function get_topic_recent_senders(stream_id, topic) { } return recent_senders; } + +export function process_private_message({to_user_ids, sender_id, id}) { + const sender_dict = pm_senders.get(to_user_ids) || new Map(); + const id_tracker = sender_dict.get(sender_id) || new IdTracker(); + pm_senders.set(to_user_ids, sender_dict); + sender_dict.set(sender_id, id_tracker); + id_tracker.add(id); +} + +export function get_pm_recent_senders(user_ids_string) { + const user_ids = user_ids_string.split(",").map((id) => Number.parseInt(id, 10)); + const sender_dict = pm_senders.get(user_ids_string); + const pm_senders_info = {participants: [], non_participants: []}; + if (!sender_dict) { + return pm_senders_info; + } + + function compare_pm_user_ids_by_recency(user_id1, user_id2) { + const max_id1 = sender_dict.get(user_id1)?.max_id() || -1; + const max_id2 = sender_dict.get(user_id2)?.max_id() || -1; + return max_id2 - max_id1; + } + + // Add current user to user_ids. + user_ids.push(people.my_current_user_id()); + pm_senders_info.non_participants = user_ids.filter((user_id) => { + if (sender_dict.get(user_id)) { + pm_senders_info.participants.push(user_id); + return false; + } + return true; + }); + pm_senders_info.participants.sort(compare_pm_user_ids_by_recency); + return pm_senders_info; +} diff --git a/static/js/recent_topics_ui.js b/static/js/recent_topics_ui.js index 9fc536f3e9..9924d416f2 100644 --- a/static/js/recent_topics_ui.js +++ b/static/js/recent_topics_ui.js @@ -430,10 +430,6 @@ function format_conversation(conversation_data) { context.pm_url = last_msg.pm_with_url; context.is_group = last_msg.display_recipient.length > 2; - // Display in most recent sender first order - all_senders = last_msg.display_recipient; - senders = all_senders.slice(-MAX_AVATAR).map((sender) => sender.id); - if (!context.is_group) { const user_id = Number.parseInt(last_msg.to_user_ids, 10); const user = people.get_by_user_id(user_id); @@ -446,6 +442,17 @@ function format_conversation(conversation_data) { } } + // Since the css for displaying senders in reverse order is much simpler, + // we provide our handlebars with senders in opposite order. + // Display in most recent sender first order. + // To match the behavior for streams, we display the set of users who've actually + // participated, with the most recent participants first. It could make sense to + // display the other recipients on the PM conversation with different styling, + // but it's important to not destroy the information of "who's actually talked". + all_senders = recent_senders + .get_pm_recent_senders(context.user_ids_string) + .participants.reverse(); + senders = all_senders.slice(-MAX_AVATAR); // Collect extra senders fullname for tooltip. extra_sender_ids = all_senders.slice(0, -MAX_AVATAR); displayed_other_senders = extra_sender_ids