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`:

d7a09fb08d/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.

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)

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
 

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
+#

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|

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:, **)

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

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

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

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

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

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)

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
 

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, {})

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!
 

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

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

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

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,
         )

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

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

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,
           )

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),
           )