From 766bcbc6840c9d665055441bcd77616b3a96e10e Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Mon, 7 Nov 2022 09:04:47 +1000
Subject: [PATCH] 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.
---
.../chat/app/controllers/chat_controller.rb | 1 +
plugins/chat/app/models/chat_message.rb | 5 ++
.../chat/app/models/chat_message_revision.rb | 3 ++
...319_add_last_editor_id_to_chat_messages.rb | 11 ++++
...ser_ids_for_chat_messages_and_revisions.rb | 17 +++++++
plugins/chat/lib/chat_message_updater.rb | 11 +++-
.../components/chat_message_creator_spec.rb | 10 ++++
.../components/chat_message_updater_spec.rb | 50 ++++++++++++++++---
.../chat/spec/lib/chat_review_queue_spec.rb | 1 +
9 files changed, 102 insertions(+), 7 deletions(-)
create mode 100644 plugins/chat/db/migrate/20221101061319_add_last_editor_id_to_chat_messages.rb
create mode 100644 plugins/chat/db/post_migrate/20221101071135_backfill_editing_user_ids_for_chat_messages_and_revisions.rb
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.",
)