diff --git a/plugins/chat/app/controllers/chat_controller.rb b/plugins/chat/app/controllers/chat_controller.rb index 287bcd8f359..f84ce9b2739 100644 --- a/plugins/chat/app/controllers/chat_controller.rb +++ b/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] || [], diff --git a/plugins/chat/app/models/chat_message.rb b/plugins/chat/app/models/chat_message.rb index b0784d4ccb6..f54c4627381 100644 --- a/plugins/chat/app/models/chat_message.rb +++ b/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) # diff --git a/plugins/chat/app/models/chat_message_revision.rb b/plugins/chat/app/models/chat_message_revision.rb index 03f19168350..a0caed17fbd 100644 --- a/plugins/chat/app/models/chat_message_revision.rb +++ b/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) # diff --git a/plugins/chat/db/migrate/20221101061319_add_last_editor_id_to_chat_messages.rb b/plugins/chat/db/migrate/20221101061319_add_last_editor_id_to_chat_messages.rb new file mode 100644 index 00000000000..feb782b127e --- /dev/null +++ b/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 diff --git a/plugins/chat/db/post_migrate/20221101071135_backfill_editing_user_ids_for_chat_messages_and_revisions.rb b/plugins/chat/db/post_migrate/20221101071135_backfill_editing_user_ids_for_chat_messages_and_revisions.rb new file mode 100644 index 00000000000..01a736844e9 --- /dev/null +++ b/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 diff --git a/plugins/chat/lib/chat_message_updater.rb b/plugins/chat/lib/chat_message_updater.rb index df67587343c..39636cbcd07 100644 --- a/plugins/chat/lib/chat_message_updater.rb +++ b/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 diff --git a/plugins/chat/spec/components/chat_message_creator_spec.rb b/plugins/chat/spec/components/chat_message_creator_spec.rb index 1128ab9cc9d..1ead27d5b20 100644 --- a/plugins/chat/spec/components/chat_message_creator_spec.rb +++ b/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( diff --git a/plugins/chat/spec/components/chat_message_updater_spec.rb b/plugins/chat/spec/components/chat_message_updater_spec.rb index 048de7aae9e..37aea837328 100644 --- a/plugins/chat/spec/components/chat_message_updater_spec.rb +++ b/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", ) diff --git a/plugins/chat/spec/lib/chat_review_queue_spec.rb b/plugins/chat/spec/lib/chat_review_queue_spec.rb index 2a1023fa158..23433dab010 100644 --- a/plugins/chat/spec/lib/chat_review_queue_spec.rb +++ b/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.", )