FIX: Add editing user ids to ChatMessage and ChatMessageRevision (#18877)

This commit adds last_editor_id to ChatMessage for parity with Post in
core, as well as adding user_id to the ChatMessageRevision record since
we need to know who is making edits and revisions to messages, in case
in future we want to allow more than just the current user to edit chat
messages. The backfill for data here simply uses the record's creating
user ID, but in future if we allow other people to edit the messages it
will use their ID.

plugins/chat/app/controllers/chat_controller.rb

@@ -147,6 +147,7 @@ class Chat::ChatController < Chat::ChatBaseController
     guardian.ensure_can_edit_chat!(@message)
     chat_message_updater =
       Chat::ChatMessageUpdater.update(
+        guardian: guardian,
         chat_message: @message,
         new_content: params[:new_message],
         upload_ids: params[:upload_ids] || [],

plugins/chat/app/models/chat_message.rb

@@ -9,6 +9,7 @@ class ChatMessage < ActiveRecord::Base
   belongs_to :chat_channel
   belongs_to :user
   belongs_to :in_reply_to, class_name: "ChatMessage"
+  belongs_to :last_editor, class_name: "User"
   has_many :replies, class_name: "ChatMessage", foreign_key: "in_reply_to_id", dependent: :nullify
   has_many :revisions, class_name: "ChatMessageRevision", dependent: :destroy
   has_many :reactions, class_name: "ChatMessageReaction", dependent: :destroy
@@ -32,6 +33,8 @@ class ChatMessage < ActiveRecord::Base
 
   scope :created_before, ->(date) { where("chat_messages.created_at < ?", date) }
 
+  before_save { self.last_editor_id ||= self.user_id }
+
   def validate_message(has_uploads:)
     WatchedWordsValidator.new(attributes: [:message]).validate(self)
     Chat::DuplicateMessageValidator.new(self).validate
@@ -207,9 +210,11 @@ end
 #  message         :text
 #  cooked          :text
 #  cooked_version  :integer
+#  last_editor_id  :integer
 #
 # Indexes
 #
 #  idx_chat_messages_by_created_at_not_deleted            (created_at) WHERE (deleted_at IS NULL)
 #  index_chat_messages_on_chat_channel_id_and_created_at  (chat_channel_id,created_at)
+#  index_chat_messages_on_last_editor_id                  (last_editor_id)
 #

plugins/chat/app/models/chat_message_revision.rb

@@ -2,6 +2,7 @@
 
 class ChatMessageRevision < ActiveRecord::Base
   belongs_to :chat_message
+  belongs_to :user
 end
 
 # == Schema Information
@@ -14,8 +15,10 @@ end
 #  new_message     :text             not null
 #  created_at      :datetime         not null
 #  updated_at      :datetime         not null
+#  user_id         :integer
 #
 # Indexes
 #
 #  index_chat_message_revisions_on_chat_message_id  (chat_message_id)
+#  index_chat_message_revisions_on_user_id          (user_id)
 #

plugins/chat/db/migrate/20221101061319_add_last_editor_id_to_chat_messages.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class AddLastEditorIdToChatMessages < ActiveRecord::Migration[7.0]
+  def change
+    add_column :chat_messages, :last_editor_id, :integer
+    add_column :chat_message_revisions, :user_id, :integer
+
+    add_index :chat_messages, :last_editor_id
+    add_index :chat_message_revisions, :user_id
+  end
+end

plugins/chat/db/post_migrate/20221101071135_backfill_editing_user_ids_for_chat_messages_and_revisions.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class BackfillEditingUserIdsForChatMessagesAndRevisions < ActiveRecord::Migration[7.0]
+  def up
+    DB.exec("UPDATE chat_messages SET last_editor_id = user_id")
+    DB.exec(<<~SQL)
+      UPDATE chat_message_revisions cmr
+      SET user_id = cm.user_id
+      FROM chat_messages AS cm
+      WHERE cmr.chat_message_id = cm.id
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

plugins/chat/lib/chat_message_updater.rb

@@ -1,4 +1,5 @@
 # frozen_string_literal: true
+
 class Chat::ChatMessageUpdater
   attr_reader :error
 
@@ -8,7 +9,9 @@ class Chat::ChatMessageUpdater
     instance
   end
 
-  def initialize(chat_message:, new_content:, upload_ids: nil)
+  def initialize(guardian:, chat_message:, new_content:, upload_ids: nil)
+    @guardian = guardian
+    @user = guardian.user
     @chat_message = chat_message
     @old_message_content = chat_message.message
     @chat_channel = @chat_message.chat_channel
@@ -23,6 +26,7 @@ class Chat::ChatMessageUpdater
     begin
       validate_channel_status!
       @chat_message.message = @new_content
+      @chat_message.last_editor_id = @user.id
       upload_info = get_upload_info
       validate_message!(has_uploads: upload_info[:uploads].any?)
       @chat_message.cook
@@ -43,6 +47,10 @@ class Chat::ChatMessageUpdater
 
   private
 
+  # TODO (martin) Since we have guardian here now we should move
+  # guardian.ensure_can_edit_chat!(@message) from the controller into
+  # this class.
+
   def validate_channel_status!
     return if @guardian.can_modify_channel_message?(@chat_channel)
     raise StandardError.new(
@@ -86,6 +94,7 @@ class Chat::ChatMessageUpdater
     @chat_message.revisions.create!(
       old_message: @old_message_content,
       new_message: @chat_message.message,
+      user_id: @user.id,
     )
   end
 end

plugins/chat/spec/components/chat_message_creator_spec.rb

@@ -95,6 +95,16 @@ describe Chat::ChatMessageCreator do
       }.to change { ChatMessage.count }.by(1)
     end
 
+    it "sets the last_editor_id to the user who created the message" do
+      message =
+        Chat::ChatMessageCreator.create(
+          chat_channel: public_chat_channel,
+          user: user1,
+          content: "this is a message",
+        ).chat_message
+      expect(message.last_editor_id).to eq(user1.id)
+    end
+
     it "creates mention notifications for public chat" do
       expect {
         Chat::ChatMessageCreator.create(

plugins/chat/spec/components/chat_message_updater_spec.rb

@@ -3,6 +3,7 @@
 require "rails_helper"
 
 describe Chat::ChatMessageUpdater do
+  let(:guardian) { Guardian.new(user1) }
   fab!(:admin1) { Fabricate(:admin) }
   fab!(:admin2) { Fabricate(:admin) }
   fab!(:user1) { Fabricate(:user) }
@@ -30,7 +31,10 @@ describe Chat::ChatMessageUpdater do
     end
     Group.refresh_automatic_groups!
     @direct_message_channel =
-      Chat::DirectMessageChannelCreator.create!(acting_user: user1, target_users: [user1, user2])
+      Chat::DirectMessageChannelCreator.create!(
+        acting_user: user1,
+        target_users: [user1, user2],
+      )
   end
 
   def create_chat_message(user, message, channel, upload_ids: nil)
@@ -51,7 +55,12 @@ describe Chat::ChatMessageUpdater do
     chat_message = create_chat_message(user1, og_message, public_chat_channel)
     new_message = "2 short"
 
-    updater = Chat::ChatMessageUpdater.update(chat_message: chat_message, new_content: new_message)
+    updater =
+      Chat::ChatMessageUpdater.update(
+        guardian: guardian,
+        chat_message: chat_message,
+        new_content: new_message,
+      )
     expect(updater.failed?).to eq(true)
     expect(updater.error.message).to match(
       I18n.t(
@@ -66,7 +75,11 @@ describe Chat::ChatMessageUpdater do
     chat_message = create_chat_message(user1, "This will be changed", public_chat_channel)
     new_message = "Change to this!"
 
-    Chat::ChatMessageUpdater.update(chat_message: chat_message, new_content: new_message)
+    Chat::ChatMessageUpdater.update(
+      guardian: guardian,
+      chat_message: chat_message,
+      new_content: new_message,
+    )
     expect(chat_message.reload.message).to eq(new_message)
   end
 
@@ -74,6 +87,7 @@ describe Chat::ChatMessageUpdater do
     chat_message = create_chat_message(user1, "This will be changed", public_chat_channel)
     expect {
       Chat::ChatMessageUpdater.update(
+        guardian: guardian,
         chat_message: chat_message,
         new_content:
           "this is a message with @system @mentions @#{user2.username} and @#{user3.username}",
@@ -86,6 +100,7 @@ describe Chat::ChatMessageUpdater do
     chat_message = create_chat_message(user1, message, public_chat_channel)
     expect {
       Chat::ChatMessageUpdater.update(
+        guardian: guardian,
         chat_message: chat_message,
         new_content: message + " editedddd",
       )
@@ -98,6 +113,7 @@ describe Chat::ChatMessageUpdater do
 
     expect {
       Chat::ChatMessageUpdater.update(
+        guardian: guardian,
         chat_message: chat_message,
         new_content: message + " @#{user_without_memberships.username}",
       )
@@ -109,6 +125,7 @@ describe Chat::ChatMessageUpdater do
       create_chat_message(user1, "ping @#{user2.username} @#{user3.username}", public_chat_channel)
     expect {
       Chat::ChatMessageUpdater.update(
+        guardian: guardian,
         chat_message: chat_message,
         new_content: "ping @#{user3.username}",
       )
@@ -119,6 +136,7 @@ describe Chat::ChatMessageUpdater do
     chat_message =
       create_chat_message(user1, "ping @#{user2.username} @#{user3.username}", public_chat_channel)
     Chat::ChatMessageUpdater.update(
+      guardian: guardian,
       chat_message: chat_message,
       new_content: "ping @#{user3.username} @#{user4.username}",
     )
@@ -132,6 +150,7 @@ describe Chat::ChatMessageUpdater do
     chat_message = create_chat_message(user1, "ping nobody", @direct_message_channel)
     expect {
       Chat::ChatMessageUpdater.update(
+        guardian: guardian,
         chat_message: chat_message,
         new_content: "ping @#{admin1.username}",
       )
@@ -143,6 +162,7 @@ describe Chat::ChatMessageUpdater do
       chat_message = create_chat_message(user1, "ping nobody", public_chat_channel)
       expect {
         Chat::ChatMessageUpdater.update(
+          guardian: guardian,
           chat_message: chat_message,
           new_content: "ping @#{admin_group.name}",
         )
@@ -156,6 +176,7 @@ describe Chat::ChatMessageUpdater do
       chat_message = create_chat_message(user1, "ping @#{admin2.username}", public_chat_channel)
       expect {
         Chat::ChatMessageUpdater.update(
+          guardian: guardian,
           chat_message: chat_message,
           new_content: "ping @#{admin_group.name} @#{admin2.username}",
         )
@@ -166,6 +187,7 @@ describe Chat::ChatMessageUpdater do
       chat_message = create_chat_message(user1, "ping @#{admin_group.name}", public_chat_channel)
       expect {
         Chat::ChatMessageUpdater.update(
+          guardian: guardian,
           chat_message: chat_message,
           new_content: "ping nobody anymore!",
         )
@@ -176,14 +198,20 @@ describe Chat::ChatMessageUpdater do
     end
   end
 
-  it "creates a chat_message_revision record" do
+  it "creates a chat_message_revision record and sets last_editor_id for the message" do
     old_message = "It's a thrsday!"
     new_message = "It's a thursday!"
     chat_message = create_chat_message(user1, old_message, public_chat_channel)
-    Chat::ChatMessageUpdater.update(chat_message: chat_message, new_content: new_message)
+    Chat::ChatMessageUpdater.update(
+      guardian: guardian,
+      chat_message: chat_message,
+      new_content: new_message,
+    )
     revision = chat_message.revisions.last
     expect(revision.old_message).to eq(old_message)
     expect(revision.new_message).to eq(new_message)
+    expect(revision.user_id).to eq(guardian.user.id)
+    expect(chat_message.reload.last_editor_id).to eq(guardian.user.id)
   end
 
   describe "uploads" do
@@ -200,6 +228,7 @@ describe Chat::ChatMessageUpdater do
         )
       expect {
         Chat::ChatMessageUpdater.update(
+          guardian: guardian,
           chat_message: chat_message,
           new_content: "I guess this is different",
           upload_ids: [upload2.id, upload1.id],
@@ -217,6 +246,7 @@ describe Chat::ChatMessageUpdater do
         )
       expect {
         Chat::ChatMessageUpdater.update(
+          guardian: guardian,
           chat_message: chat_message,
           new_content: "I guess this is different",
           upload_ids: [upload1.id],
@@ -234,6 +264,7 @@ describe Chat::ChatMessageUpdater do
         )
       expect {
         Chat::ChatMessageUpdater.update(
+          guardian: guardian,
           chat_message: chat_message,
           new_content: "I guess this is different",
           upload_ids: [],
@@ -245,6 +276,7 @@ describe Chat::ChatMessageUpdater do
       chat_message = create_chat_message(user1, "something", public_chat_channel)
       expect {
         Chat::ChatMessageUpdater.update(
+          guardian: guardian,
           chat_message: chat_message,
           new_content: "I guess this is different",
           upload_ids: [upload1.id],
@@ -256,6 +288,7 @@ describe Chat::ChatMessageUpdater do
       chat_message = create_chat_message(user1, "something", public_chat_channel)
       expect {
         Chat::ChatMessageUpdater.update(
+          guardian: guardian,
           chat_message: chat_message,
           new_content: "I guess this is different",
           upload_ids: [upload1.id, upload2.id],
@@ -263,11 +296,12 @@ describe Chat::ChatMessageUpdater do
       }.to change { ChatUpload.where(chat_message: chat_message).count }.by(2)
     end
 
-    it "doesn't remove existing uploads when BS upload ids are passed in" do
+    it "doesn't remove existing uploads when upload ids that do not exist are passed in" do
       chat_message =
         create_chat_message(user1, "something", public_chat_channel, upload_ids: [upload1.id])
       expect {
         Chat::ChatMessageUpdater.update(
+          guardian: guardian,
           chat_message: chat_message,
           new_content: "I guess this is different",
           upload_ids: [0],
@@ -280,6 +314,7 @@ describe Chat::ChatMessageUpdater do
       chat_message = create_chat_message(user1, "something", public_chat_channel)
       expect {
         Chat::ChatMessageUpdater.update(
+          guardian: guardian,
           chat_message: chat_message,
           new_content: "I guess this is different",
           upload_ids: [upload1.id, upload2.id],
@@ -298,6 +333,7 @@ describe Chat::ChatMessageUpdater do
         )
       expect {
         Chat::ChatMessageUpdater.update(
+          guardian: guardian,
           chat_message: chat_message,
           new_content: "I guess this is different",
           upload_ids: [],
@@ -316,6 +352,7 @@ describe Chat::ChatMessageUpdater do
       SiteSetting.chat_minimum_message_length = 10
       new_message = "hi :)"
       Chat::ChatMessageUpdater.update(
+        guardian: guardian,
         chat_message: chat_message,
         new_content: new_message,
         upload_ids: [upload1.id],
@@ -348,6 +385,7 @@ describe Chat::ChatMessageUpdater do
     def update_message(user)
       message.update(user: user)
       Chat::ChatMessageUpdater.update(
+        guardian: Guardian.new(user),
         chat_message: message,
         new_content: "I guess this is different",
       )

plugins/chat/spec/lib/chat_review_queue_spec.rb

@@ -117,6 +117,7 @@ describe Chat::ChatReviewQueue do
 
       it "ignores the cooldown window when the message is edited" do
         Chat::ChatMessageUpdater.update(
+          guardian: Guardian.new(message.user),
           chat_message: message,
           new_content: "I'm editing this message. Please flag it.",
         )