From 8b438767e57d790c6a7bd0b84f78dd12e31b795d Mon Sep 17 00:00:00 2001 From: Andrei Prigorshnev Date: Thu, 20 Apr 2023 19:05:17 +0400 Subject: [PATCH] FIX: send notifications after a chat message was updated with new mentions (#21173) Steps to reproduce the bug: 1. Send a chat message 2. Edit the message and add a mention to it 3. The mentioned user won't receive a notification This PR fixes the problem. Also: 1. There's no need anymore to have a code for removing notifications in the `notify_edit` method, because a call to `@chat_message.update_mentions` in the first line of the `notify_edit` method does that job: https://github.com/discourse/discourse/blob/ff56f403a2e54db124a70cdcda5a9c1ca27f3535/plugins/chat/lib/chat/notifier.rb#L90 2. There's no need to load mention records from database, it's enough to pluck user ids --- plugins/chat/lib/chat/notifier.rb | 19 ++++++----------- .../components/chat/message_updater_spec.rb | 21 ++++++++++--------- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/plugins/chat/lib/chat/notifier.rb b/plugins/chat/lib/chat/notifier.rb index ba67d2ccc47..d06d75dc7b3 100644 --- a/plugins/chat/lib/chat/notifier.rb +++ b/plugins/chat/lib/chat/notifier.rb @@ -89,24 +89,17 @@ module Chat def notify_edit @chat_message.update_mentions(@mentions.all_mentioned_users_ids) - existing_notifications = - Chat::Mention.includes(:user, :notification).where(chat_message: @chat_message) - already_notified_user_ids = existing_notifications.map(&:user_id) + already_notified_user_ids = + Chat::Mention + .where(chat_message: @chat_message) + .where.not(notification: nil) + .pluck(:user_id) to_notify = list_users_to_notify - mentioned_user_ids = to_notify.extract!(:all_mentioned_user_ids)[:all_mentioned_user_ids] - - needs_deletion = already_notified_user_ids - mentioned_user_ids - needs_deletion.each do |user_id| - chat_mention = existing_notifications.detect { |n| n.user_id == user_id } - chat_mention.notification.destroy! if chat_mention.notification.present? - end - - needs_notification_ids = mentioned_user_ids - already_notified_user_ids + needs_notification_ids = to_notify[:all_mentioned_user_ids] - already_notified_user_ids return if needs_notification_ids.blank? notify_creator_of_inaccessible_mentions(to_notify) - notify_mentioned_users(to_notify, already_notified_user_ids: already_notified_user_ids) to_notify diff --git a/plugins/chat/spec/components/chat/message_updater_spec.rb b/plugins/chat/spec/components/chat/message_updater_spec.rb index f496b76d3ad..9017657bbdd 100644 --- a/plugins/chat/spec/components/chat/message_updater_spec.rb +++ b/plugins/chat/spec/components/chat/message_updater_spec.rb @@ -126,16 +126,17 @@ describe Chat::MessageUpdater do expect(events.map { _1[:event_name] }).to include(:chat_message_edited) end - it "creates mention notifications for unmentioned users" do - chat_message = create_chat_message(user1, "This will be changed", public_chat_channel) - expect { - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: - "this is a message with @system @mentions @#{user2.username} and @#{user3.username}", - ) - }.to change { user2.chat_mentions.count }.by(1).and change { user3.chat_mentions.count }.by(1) + it "sends notifications if a message was updated with new mentions" do + message = create_chat_message(user1, "Mentioning @#{user2.username}", public_chat_channel) + + Chat::MessageUpdater.update( + guardian: guardian, + chat_message: message, + new_content: "Mentioning @#{user2.username} and @#{user3.username}", + ) + + mention = user3.chat_mentions.where(chat_message: message).first + expect(mention.notification).to be_present end it "doesn't create mentions for already mentioned users" do