From fbd24fa6aedc4d65dbb258da04fcdaa74da1c83a Mon Sep 17 00:00:00 2001 From: Andrei Prigorshnev Date: Tue, 19 Dec 2023 18:53:00 +0400 Subject: [PATCH] DEV: Allow chat mentions to have several notifications (#24874) This PR is a reworked version of https://github.com/discourse/discourse/pull/24670. In chat, we need the ability to have several notifications per `chat_mention`. Currently, we have one_to_one relationship between `chat_mentions` and `notifications`: https://github.com/discourse/discourse/blob/d7a09fb08de31dbeed55428b84e0a660dbd5cf7a/plugins/chat/app/models/chat/mention.rb#L9 We want to have one_to_many relationship. This PR implements that by introducing a join table between `chat_mentions` and `notifications`. The main motivation for this is that we want to solve some performance problems with mentions that we're having now. Let's say a user sends a message with @ all in a channel with 50 members, we do two things in this case at the moment: - create 50 chat_mentions - create 50 notifications We don't want to change how notifications work in core, but we want to be more efficient in chat, and create only 1 `chat_mention` which would link to 50 notifications. Also note, that on the side of notifications, having a lot of notifications is not so big problem, because notifications processing can be queued. Apart from improving performance, this change will make the code design better. Note that I've marked the old `chat_mention.notification_id` column as ignored, but I'm not deleting it in this PR. We'll delete it later in https://github.com/discourse/discourse/pull/24800. --- .../app/jobs/regular/chat/notify_mentioned.rb | 2 +- plugins/chat/app/models/chat/mention.rb | 6 ++++- .../app/models/chat/mention_notification.rb | 23 ++++++++++++++++++ .../chat/action/mark_mentions_read.rb | 7 +++++- .../chat/app/services/chat/trash_message.rb | 10 +++++--- ...14180000_add_chat_mention_notifications.rb | 13 ++++++++++ ...between_chat_mentions_and_notifications.rb | 22 +++++++++++++++++ ...mentions_and_notifications_post_migrate.rb | 24 +++++++++++++++++++ plugins/chat/lib/chat/notifier.rb | 3 ++- .../lib/chat/user_notifications_extension.rb | 7 +++--- .../jobs/regular/chat/channel_delete_spec.rb | 2 +- .../regular/chat/notify_mentioned_spec.rb | 4 ---- .../spec/mailers/user_notifications_spec.rb | 23 +++++++++++------- plugins/chat/spec/models/chat/message_spec.rb | 4 ++-- .../chat/channel_unreads_query_spec.rb | 6 ++++- .../chat/api/reads_controller_spec.rb | 2 +- .../chat/api/thread_reads_controller_spec.rb | 2 +- .../chat/mark_all_user_channels_read_spec.rb | 4 ++-- .../spec/services/chat/trash_message_spec.rb | 4 ++-- .../spec/services/chat/update_message_spec.rb | 8 +++---- .../chat/update_user_last_read_spec.rb | 2 +- .../chat/update_user_thread_last_read_spec.rb | 4 ++-- 22 files changed, 141 insertions(+), 41 deletions(-) create mode 100644 plugins/chat/app/models/chat/mention_notification.rb create mode 100644 plugins/chat/db/migrate/20231214180000_add_chat_mention_notifications.rb create mode 100644 plugins/chat/db/migrate/20231214180001_update_relationship_between_chat_mentions_and_notifications.rb create mode 100644 plugins/chat/db/post_migrate/20231214180002_update_relationship_between_chat_mentions_and_notifications_post_migrate.rb diff --git a/plugins/chat/app/jobs/regular/chat/notify_mentioned.rb b/plugins/chat/app/jobs/regular/chat/notify_mentioned.rb index 3388d17973a..bfbd925ff5d 100644 --- a/plugins/chat/app/jobs/regular/chat/notify_mentioned.rb +++ b/plugins/chat/app/jobs/regular/chat/notify_mentioned.rb @@ -122,7 +122,7 @@ module Jobs read: is_read, ) - mention.update!(notification: notification) + mention.notifications << notification end def send_notifications(membership, mention_type) diff --git a/plugins/chat/app/models/chat/mention.rb b/plugins/chat/app/models/chat/mention.rb index ab3bbee9925..9e89dd23b3e 100644 --- a/plugins/chat/app/models/chat/mention.rb +++ b/plugins/chat/app/models/chat/mention.rb @@ -3,10 +3,14 @@ module Chat class Mention < ActiveRecord::Base self.table_name = "chat_mentions" + self.ignored_columns = ["notification_id"] belongs_to :user belongs_to :chat_message, class_name: "Chat::Message" - belongs_to :notification, dependent: :destroy + has_many :mention_notifications, + class_name: "Chat::MentionNotification", + foreign_key: :chat_mention_id + has_many :notifications, through: :mention_notifications, dependent: :destroy end end diff --git a/plugins/chat/app/models/chat/mention_notification.rb b/plugins/chat/app/models/chat/mention_notification.rb new file mode 100644 index 00000000000..62c3a254341 --- /dev/null +++ b/plugins/chat/app/models/chat/mention_notification.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Chat + class MentionNotification < ActiveRecord::Base + self.table_name = "chat_mention_notifications" + + belongs_to :chat_mention + belongs_to :notification, dependent: :destroy + end +end + +# == Schema Information +# +# Table name: chat_mention_notifications +# +# chat_mention_id :integer not null +# notification_id :integer not null +# +# Indexes +# +# index_chat_mention_notifications_on_chat_mention_id (chat_mention_id) +# index_chat_mention_notifications_on_notification_id (notification_id) UNIQUE +# diff --git a/plugins/chat/app/services/chat/action/mark_mentions_read.rb b/plugins/chat/app/services/chat/action/mark_mentions_read.rb index 8d0207d63bb..f6c3f1c2a4d 100644 --- a/plugins/chat/app/services/chat/action/mark_mentions_read.rb +++ b/plugins/chat/app/services/chat/action/mark_mentions_read.rb @@ -17,7 +17,12 @@ module Chat .where(notification_type: Notification.types[:chat_mention]) .where(user: user) .where(read: false) - .joins("INNER JOIN chat_mentions ON chat_mentions.notification_id = notifications.id") + .joins( + "INNER JOIN chat_mention_notifications ON chat_mention_notifications.notification_id = notifications.id", + ) + .joins( + "INNER JOIN chat_mentions ON chat_mentions.id = chat_mention_notifications.chat_mention_id", + ) .joins("INNER JOIN chat_messages ON chat_mentions.chat_message_id = chat_messages.id") .where("chat_messages.chat_channel_id IN (?)", channel_ids) .then do |notifications| diff --git a/plugins/chat/app/services/chat/trash_message.rb b/plugins/chat/app/services/chat/trash_message.rb index 2d3b6d7a6c5..fa06c1e68ae 100644 --- a/plugins/chat/app/services/chat/trash_message.rb +++ b/plugins/chat/app/services/chat/trash_message.rb @@ -55,9 +55,13 @@ module Chat end def destroy_notifications(message:, **) - ids = Chat::Mention.where(chat_message: message).pluck(:notification_id) - Notification.where(id: ids).destroy_all - Chat::Mention.where(chat_message: message).update_all(notification_id: nil) + Notification.where( + id: + Chat::Mention + .where(chat_message: message) + .joins(:notifications) + .select("notifications.id"), + ).destroy_all end def update_tracking_state(message:, **) diff --git a/plugins/chat/db/migrate/20231214180000_add_chat_mention_notifications.rb b/plugins/chat/db/migrate/20231214180000_add_chat_mention_notifications.rb new file mode 100644 index 00000000000..6683a0441cb --- /dev/null +++ b/plugins/chat/db/migrate/20231214180000_add_chat_mention_notifications.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class AddChatMentionNotifications < ActiveRecord::Migration[7.0] + def change + create_table :chat_mention_notifications, id: false do |t| + t.integer :chat_mention_id, null: false + t.integer :notification_id, null: false + end + + add_index :chat_mention_notifications, %i[chat_mention_id] + add_index :chat_mention_notifications, %i[notification_id], unique: true + end +end diff --git a/plugins/chat/db/migrate/20231214180001_update_relationship_between_chat_mentions_and_notifications.rb b/plugins/chat/db/migrate/20231214180001_update_relationship_between_chat_mentions_and_notifications.rb new file mode 100644 index 00000000000..c90359201fa --- /dev/null +++ b/plugins/chat/db/migrate/20231214180001_update_relationship_between_chat_mentions_and_notifications.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class UpdateRelationshipBetweenChatMentionsAndNotifications < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + BATCH_SIZE = 5000 + + def up + begin + inserted_count = DB.exec(<<~SQL, batch_size: BATCH_SIZE) + INSERT INTO chat_mention_notifications(chat_mention_id, notification_id) + SELECT cm.id, cm.notification_id + FROM chat_mentions cm + LEFT JOIN chat_mention_notifications cmn ON cmn.chat_mention_id = cm.id + WHERE cm.notification_id IS NOT NULL and cmn.chat_mention_id IS NULL + LIMIT :batch_size; + SQL + end while inserted_count > 0 + end + + def down + end +end diff --git a/plugins/chat/db/post_migrate/20231214180002_update_relationship_between_chat_mentions_and_notifications_post_migrate.rb b/plugins/chat/db/post_migrate/20231214180002_update_relationship_between_chat_mentions_and_notifications_post_migrate.rb new file mode 100644 index 00000000000..858f51557e2 --- /dev/null +++ b/plugins/chat/db/post_migrate/20231214180002_update_relationship_between_chat_mentions_and_notifications_post_migrate.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class UpdateRelationshipBetweenChatMentionsAndNotificationsPostMigrate < ActiveRecord::Migration[ + 7.0 +] + disable_ddl_transaction! + BATCH_SIZE = 5000 + + def up + begin + inserted_count = DB.exec(<<~SQL, batch_size: BATCH_SIZE) + INSERT INTO chat_mention_notifications(chat_mention_id, notification_id) + SELECT cm.id, cm.notification_id + FROM chat_mentions cm + LEFT JOIN chat_mention_notifications cmn ON cmn.chat_mention_id = cm.id + WHERE cm.notification_id IS NOT NULL and cmn.chat_mention_id IS NULL + LIMIT :batch_size; + SQL + end while inserted_count > 0 + end + + def down + end +end diff --git a/plugins/chat/lib/chat/notifier.rb b/plugins/chat/lib/chat/notifier.rb index 45cfbcc1535..fae9241230c 100644 --- a/plugins/chat/lib/chat/notifier.rb +++ b/plugins/chat/lib/chat/notifier.rb @@ -85,7 +85,8 @@ module Chat already_notified_user_ids = Chat::Mention .where(chat_message: @chat_message) - .where.not(notification: nil) + .left_outer_joins(:notifications) + .where.not(notifications: { id: nil }) .pluck(:user_id) to_notify, inaccessible, all_mentioned_user_ids = list_users_to_notify diff --git a/plugins/chat/lib/chat/user_notifications_extension.rb b/plugins/chat/lib/chat/user_notifications_extension.rb index 383997277c1..ec1907feec4 100644 --- a/plugins/chat/lib/chat/user_notifications_extension.rb +++ b/plugins/chat/lib/chat/user_notifications_extension.rb @@ -11,9 +11,8 @@ module Chat .joins(:user, :chat_channel) .where.not(user: user) .where("chat_messages.created_at > ?", 1.week.ago) - .joins( - "LEFT OUTER JOIN chat_mentions cm ON cm.chat_message_id = chat_messages.id AND cm.notification_id IS NOT NULL", - ) + .joins("LEFT OUTER JOIN chat_mentions cm ON cm.chat_message_id = chat_messages.id") + .joins("LEFT OUTER JOIN chat_mention_notifications cmn ON cmn.chat_mention_id = cm.id") .joins( "INNER JOIN user_chat_channel_memberships uccm ON uccm.chat_channel_id = chat_channels.id", ) @@ -22,7 +21,7 @@ module Chat (uccm.last_read_message_id IS NULL OR chat_messages.id > uccm.last_read_message_id) AND (uccm.last_unread_mention_when_emailed_id IS NULL OR chat_messages.id > uccm.last_unread_mention_when_emailed_id) AND ( - (cm.user_id = :user_id AND uccm.following IS true AND chat_channels.chatable_type = 'Category') OR + (cm.user_id = :user_id AND cmn.notification_id IS NOT NULL AND uccm.following IS true AND chat_channels.chatable_type = 'Category') OR (chat_channels.chatable_type = 'DirectMessage') ) SQL diff --git a/plugins/chat/spec/jobs/regular/chat/channel_delete_spec.rb b/plugins/chat/spec/jobs/regular/chat/channel_delete_spec.rb index e8ea19cd1c9..6e48a4e0f92 100644 --- a/plugins/chat/spec/jobs/regular/chat/channel_delete_spec.rb +++ b/plugins/chat/spec/jobs/regular/chat/channel_delete_spec.rb @@ -26,7 +26,7 @@ describe Jobs::Chat::ChannelDelete do Chat::Mention.create( user: user2, chat_message: messages.sample, - notification: Fabricate(:notification), + notifications: [Fabricate(:notification)], ) @incoming_chat_webhook_id = Fabricate(:incoming_chat_webhook, chat_channel: chat_channel) diff --git a/plugins/chat/spec/jobs/regular/chat/notify_mentioned_spec.rb b/plugins/chat/spec/jobs/regular/chat/notify_mentioned_spec.rb index 2d02fe176d4..3cde7483515 100644 --- a/plugins/chat/spec/jobs/regular/chat/notify_mentioned_spec.rb +++ b/plugins/chat/spec/jobs/regular/chat/notify_mentioned_spec.rb @@ -282,10 +282,6 @@ describe Jobs::Chat::NotifyMentioned do expect(data_hash[:is_direct_message_channel]).to eq(false) expect(data_hash[:chat_channel_title]).to eq(expected_channel_title) expect(data_hash[:chat_channel_slug]).to eq(public_channel.slug) - - chat_mention = - Chat::Mention.where(notification: created_notification, user: user_2, chat_message: message) - expect(chat_mention).to be_present end end diff --git a/plugins/chat/spec/mailers/user_notifications_spec.rb b/plugins/chat/spec/mailers/user_notifications_spec.rb index 236d15b7d47..183e1618f00 100644 --- a/plugins/chat/spec/mailers/user_notifications_spec.rb +++ b/plugins/chat/spec/mailers/user_notifications_spec.rb @@ -257,7 +257,7 @@ describe UserNotifications do :chat_mention, user: user, chat_message: chat_message, - notification: notification, + notifications: [notification], ) end @@ -310,7 +310,7 @@ describe UserNotifications do :chat_mention, user: user, chat_message: another_chat_message, - notification: notification, + notifications: [notification], ) another_chat_channel.update!(last_message: another_chat_message) @@ -350,7 +350,7 @@ describe UserNotifications do :chat_mention, user: user, chat_message: another_chat_message, - notification: notification, + notifications: [notification], ) another_chat_channel.update!(last_message: another_chat_message) end @@ -379,7 +379,7 @@ describe UserNotifications do :chat_mention, user: user, chat_message: chat_message, - notification: notification, + notifications: [notification], ) end @@ -406,7 +406,7 @@ describe UserNotifications do :chat_mention, user: user, chat_message: chat_message, - notification: notification, + notifications: [notification], ) end @@ -513,7 +513,7 @@ describe UserNotifications do :chat_mention, user: user, chat_message: new_message, - notification: notification, + notifications: [notification], ) email = described_class.chat_summary(user, {}) @@ -642,7 +642,12 @@ describe UserNotifications do 2.times do msg = Fabricate(:chat_message, user: sender, chat_channel: channel) notification = Fabricate(:notification) - Fabricate(:chat_mention, user: user, chat_message: msg, notification: notification) + Fabricate( + :chat_mention, + user: user, + chat_message: msg, + notifications: [notification], + ) end email = described_class.chat_summary(user, {}) @@ -668,7 +673,7 @@ describe UserNotifications do :chat_mention, user: user, chat_message: msg, - notification: notification, + notifications: [notification], ) end @@ -695,7 +700,7 @@ describe UserNotifications do :chat_mention, user: user, chat_message: new_message, - notification: notification, + notifications: [notification], ) email = described_class.chat_summary(user, {}) diff --git a/plugins/chat/spec/models/chat/message_spec.rb b/plugins/chat/spec/models/chat/message_spec.rb index 1b88d55a9e2..1601529ec8b 100644 --- a/plugins/chat/spec/models/chat/message_spec.rb +++ b/plugins/chat/spec/models/chat/message_spec.rb @@ -449,8 +449,8 @@ describe Chat::Message do it "destroys chat_mention" do message_1 = Fabricate(:chat_message) - notification = Fabricate(:notification) - mention_1 = Fabricate(:chat_mention, chat_message: message_1, notification: notification) + notification = Fabricate(:notification, notification_type: Notification.types[:chat_mention]) + mention_1 = Fabricate(:chat_mention, chat_message: message_1, notifications: [notification]) message_1.reload.destroy! diff --git a/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb b/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb index 112b255c3b7..be57404be42 100644 --- a/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb +++ b/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb @@ -128,7 +128,11 @@ describe Chat::ChannelUnreadsQuery do user_id: current_user.id, data: { chat_message_id: message.id, chat_channel_id: channel.id }.to_json, ) - Chat::Mention.create!(notification: notification, user: current_user, chat_message: message) + Chat::Mention.create!( + notifications: [notification], + user: current_user, + chat_message: message, + ) end it "returns a correct unread mention" do diff --git a/plugins/chat/spec/requests/chat/api/reads_controller_spec.rb b/plugins/chat/spec/requests/chat/api/reads_controller_spec.rb index 84190706430..67b3b099b8b 100644 --- a/plugins/chat/spec/requests/chat/api/reads_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/reads_controller_spec.rb @@ -199,7 +199,7 @@ RSpec.describe Chat::Api::ReadsController do }.to_json, ) .tap do |notification| - Chat::Mention.create!(user: user, chat_message: msg, notification: notification) + Chat::Mention.create!(user: user, chat_message: msg, notifications: [notification]) end end end diff --git a/plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb b/plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb index 4f1c0808c33..fe4bc4a7257 100644 --- a/plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb @@ -66,7 +66,7 @@ RSpec.describe Chat::Api::ThreadReadsController do }.to_json, ) .tap do |notification| - Chat::Mention.create!(user: user, chat_message: msg, notification: notification) + Chat::Mention.create!(user: user, chat_message: msg, notifications: [notification]) end end end diff --git a/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb b/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb index 0ba3fb698bd..549e489c177 100644 --- a/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb +++ b/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb @@ -85,12 +85,12 @@ RSpec.describe Chat::MarkAllUserChannelsRead do before do Chat::Mention.create!( - notification: notification_1, + notifications: [notification_1], user: current_user, chat_message: message_1, ) Chat::Mention.create!( - notification: notification_2, + notifications: [notification_2], user: current_user, chat_message: message_3, ) diff --git a/plugins/chat/spec/services/chat/trash_message_spec.rb b/plugins/chat/spec/services/chat/trash_message_spec.rb index 821662433ca..cb03dea8783 100644 --- a/plugins/chat/spec/services/chat/trash_message_spec.rb +++ b/plugins/chat/spec/services/chat/trash_message_spec.rb @@ -49,13 +49,13 @@ RSpec.describe Chat::TrashMessage do it "destroys notifications for mentions" do notification = Fabricate(:notification) - mention = Fabricate(:chat_mention, chat_message: message, notification: notification) + mention = Fabricate(:chat_mention, chat_message: message, notifications: [notification]) result mention = Chat::Mention.find_by(id: mention.id) expect(mention).to be_present - expect(mention.notification_id).to be_nil + expect(mention.notifications).to be_empty end it "publishes associated Discourse and MessageBus events" do diff --git a/plugins/chat/spec/services/chat/update_message_spec.rb b/plugins/chat/spec/services/chat/update_message_spec.rb index 69979936b9a..347675c8fd1 100644 --- a/plugins/chat/spec/services/chat/update_message_spec.rb +++ b/plugins/chat/spec/services/chat/update_message_spec.rb @@ -189,7 +189,7 @@ RSpec.describe Chat::UpdateMessage do ) mention = user3.chat_mentions.where(chat_message: message.id).first - expect(mention.notification).to be_present + expect(mention.notifications.length).to be(1) end it "doesn't create mentions for already mentioned users" do @@ -215,7 +215,7 @@ RSpec.describe Chat::UpdateMessage do ) mention = user_without_memberships.chat_mentions.where(chat_message: chat_message).first - expect(mention.notification).to be_nil + expect(mention.notifications).to be_empty end it "destroys mentions that should be removed" do @@ -271,7 +271,7 @@ RSpec.describe Chat::UpdateMessage do ) mention = admin1.chat_mentions.where(chat_message_id: message.id).first - expect(mention.notification).to be_nil + expect(mention.notifications).to be_empty end it "creates a chat_mention record without notification when self mentioning" do @@ -282,7 +282,7 @@ RSpec.describe Chat::UpdateMessage do mention = user1.chat_mentions.where(chat_message: chat_message).first expect(mention).to be_present - expect(mention.notification).to be_nil + expect(mention.notifications).to be_empty end it "adds mentioned user and their status to the message bus message" do diff --git a/plugins/chat/spec/services/chat/update_user_last_read_spec.rb b/plugins/chat/spec/services/chat/update_user_last_read_spec.rb index 9db2ee92fff..bf3195b40bb 100644 --- a/plugins/chat/spec/services/chat/update_user_last_read_spec.rb +++ b/plugins/chat/spec/services/chat/update_user_last_read_spec.rb @@ -85,7 +85,7 @@ RSpec.describe Chat::UpdateUserLastRead do before do Jobs.run_immediately! Chat::Mention.create!( - notification: notification, + notifications: [notification], user: current_user, chat_message: message_1, ) diff --git a/plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb b/plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb index c1bd66fcb03..c2bc3bf6a1f 100644 --- a/plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb +++ b/plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb @@ -62,12 +62,12 @@ RSpec.describe Chat::UpdateUserThreadLastRead do before do Jobs.run_immediately! Chat::Mention.create!( - notification: notification_1, + notifications: [notification_1], user: current_user, chat_message: Fabricate(:chat_message, chat_channel: channel, thread: thread), ) Chat::Mention.create!( - notification: notification_2, + notifications: [notification_2], user: current_user, chat_message: Fabricate(:chat_message, chat_channel: channel, thread: thread), )