DEV: Refactor a little chat uploads

This is extracted from #22390.

This patch simplifies a little how we handle uploads in chat, relying on
ActiveRecord mechanisms instead of calling custom methods.

This also makes `Chat::Message#validate_message` a “real” AR validation,
meaning it will run automatically when `#valid?` is called.

plugins/chat/app/models/chat/message.rb

@@ -73,9 +73,11 @@ module Chat
 
     before_save { ensure_last_editor_id }
 
+    validate :validate_message
+
     def self.polymorphic_class_mapping = { "ChatMessage" => Chat::Message }
 
-    def validate_message(has_uploads:)
+    def validate_message
       self.message =
         TextCleaner.clean(self.message, strip_whitespaces: true, strip_zero_width_spaces: true)
 
@@ -85,7 +87,7 @@ module Chat
         Chat::DuplicateMessageValidator.new(self).validate
       end
 
-      if !has_uploads && message_too_short?
+      if uploads.empty? && message_too_short?
         self.errors.add(
           :base,
           I18n.t(
@@ -103,23 +105,6 @@ module Chat
       end
     end
 
-    def attach_uploads(uploads)
-      return if uploads.blank? || self.new_record?
-
-      now = Time.now
-      ref_record_attrs =
-        uploads.map do |upload|
-          {
-            upload_id: upload.id,
-            target_id: self.id,
-            target_type: self.class.polymorphic_name,
-            created_at: now,
-            updated_at: now,
-          }
-        end
-      UploadReference.insert_all!(ref_record_attrs)
-    end
-
     def excerpt(max_length: 50)
       # just show the URL if the whole message is a URL, because we cannot excerpt oneboxes
       return message if UrlHelper.relaxed_parse(message).is_a?(URI)

plugins/chat/app/services/chat/channel_view_builder.rb

@@ -184,7 +184,7 @@ module Chat
         context.threads =
           ::Chat::Thread
             .strict_loading
-            .includes(last_message: [:user], original_message_user: :user_status)
+            .includes(last_message: %i[user uploads], original_message_user: :user_status)
             .where(id: messages.map(&:thread_id).compact.uniq)
 
         # Saves us having to load the same message we already have.

plugins/chat/lib/chat/message_creator.rb

@@ -48,8 +48,8 @@ module Chat
     def create
       begin
         validate_channel_status!
-        uploads = get_uploads
-        validate_message!(has_uploads: uploads.any?)
+        @chat_message.uploads = get_uploads
+        validate_message!
         validate_reply_chain!
         validate_existing_thread!
 
@@ -60,7 +60,6 @@ module Chat
 
         create_chat_webhook_event
         create_thread
-        @chat_message.attach_uploads(uploads)
         Chat::Draft.where(user_id: @user.id, chat_channel_id: @chat_channel.id).destroy_all
         post_process_resolved_thread
         update_channel_last_message
@@ -154,11 +153,9 @@ module Chat
       end
     end
 
-    def validate_message!(has_uploads:)
-      @chat_message.validate_message(has_uploads: has_uploads)
-      if @chat_message.errors.present?
-        raise StandardError.new(@chat_message.errors.map(&:full_message).join(", "))
-      end
+    def validate_message!
+      return if @chat_message.valid?
+      raise StandardError.new(@chat_message.errors.map(&:full_message).join(", "))
     end
 
     def create_chat_webhook_event

plugins/chat/lib/chat/message_updater.rb

@@ -28,12 +28,12 @@ module Chat
         @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.uploads = upload_info[:uploads] if upload_info[:changed]
+        validate_message!
         @chat_message.cook
         @chat_message.save!
 
         @chat_message.update_mentions
-        update_uploads(upload_info)
         revision = save_revision!
 
         @chat_message.reload
@@ -63,11 +63,9 @@ module Chat
             )
     end
 
-    def validate_message!(has_uploads:)
-      @chat_message.validate_message(has_uploads: has_uploads)
-      if @chat_message.errors.present?
-        raise StandardError.new(@chat_message.errors.map(&:full_message).join(", "))
-      end
+    def validate_message!
+      return if @chat_message.valid?
+      raise StandardError.new(@chat_message.errors.map(&:full_message).join(", "))
     end
 
     def get_upload_info
@@ -85,13 +83,6 @@ module Chat
       { uploads: uploads, changed: difference.any? }
     end
 
-    def update_uploads(upload_info)
-      return unless upload_info[:changed]
-
-      UploadReference.where(target: @chat_message).destroy_all
-      @chat_message.attach_uploads(upload_info[:uploads])
-    end
-
     def save_revision!
       @chat_message.revisions.create!(
         old_message: @old_message_content,

plugins/chat/spec/lib/chat/transcript_service_spec.rb

@@ -118,7 +118,6 @@ describe Chat::TranscriptService do
 
   it "generates image / attachment / video / audio markdown inside the [chat] bbcode for upload-only messages" do
     SiteSetting.authorized_extensions = "mp4|mp3|pdf|jpg"
-    message = Fabricate(:chat_message, user: user1, chat_channel: channel, message: "")
     video = Fabricate(:upload, original_filename: "test_video.mp4", extension: "mp4")
     audio = Fabricate(:upload, original_filename: "test_audio.mp3", extension: "mp3")
     attachment = Fabricate(:upload, original_filename: "test_file.pdf", extension: "pdf")
@@ -130,10 +129,14 @@ describe Chat::TranscriptService do
         original_filename: "test_img.jpg",
         extension: "jpg",
       )
-    UploadReference.create(target: message, created_at: 10.seconds.ago, upload: video)
-    UploadReference.create(target: message, created_at: 9.seconds.ago, upload: audio)
-    UploadReference.create(target: message, created_at: 8.seconds.ago, upload: attachment)
-    UploadReference.create(target: message, created_at: 7.seconds.ago, upload: image)
+    message =
+      Fabricate(
+        :chat_message,
+        user: user1,
+        chat_channel: channel,
+        message: "",
+        uploads: [video, audio, attachment, image],
+      )
     video_markdown = UploadMarkdown.new(video).to_markdown
     audio_markdown = UploadMarkdown.new(audio).to_markdown
     attachment_markdown = UploadMarkdown.new(attachment).to_markdown

plugins/chat/spec/models/chat/message_spec.rb

@@ -293,8 +293,7 @@ describe Chat::Message do
     it "excerpts upload file name if message is empty" do
       gif =
         Fabricate(:upload, original_filename: "cat.gif", width: 400, height: 300, extension: "gif")
-      message = Fabricate(:chat_message, message: "")
-      message.attach_uploads([gif])
+      message = Fabricate(:chat_message, message: "", uploads: [gif])
 
       expect(message.excerpt).to eq "cat.gif"
     end
@@ -377,7 +376,7 @@ describe Chat::Message do
         )
       image2 =
         Fabricate(:upload, original_filename: "meme.jpg", width: 10, height: 10, extension: "jpg")
-      message.attach_uploads([image, image2])
+      message.uploads = [image, image2]
       expect(message.to_markdown).to eq(<<~MSG.chomp)
       hey friend, what's up?!
 
@@ -410,7 +409,7 @@ describe Chat::Message do
       Fabricate(:chat_message, message: "this is duplicate", chat_channel: channel, user: user1)
       message =
         described_class.new(message: "this is duplicate", chat_channel: channel, user: user2)
-      message.validate_message(has_uploads: false)
+      message.valid?
       expect(message.errors.full_messages).to include(I18n.t("chat.errors.duplicate_message"))
     end
   end
@@ -526,30 +525,6 @@ describe Chat::Message do
     end
   end
 
-  describe "#attach_uploads" do
-    fab!(:chat_message) { Fabricate(:chat_message) }
-    fab!(:upload_1) { Fabricate(:upload) }
-    fab!(:upload_2) { Fabricate(:upload) }
-
-    it "creates an UploadReference record for the provided uploads" do
-      chat_message.attach_uploads([upload_1, upload_2])
-      upload_references = UploadReference.where(upload_id: [upload_1, upload_2])
-      expect(upload_references.count).to eq(2)
-      expect(upload_references.map(&:target_id).uniq).to eq([chat_message.id])
-      expect(upload_references.map(&:target_type).uniq).to eq([described_class.polymorphic_name])
-    end
-
-    it "does nothing if the message record is new" do
-      expect { described_class.new.attach_uploads([upload_1, upload_2]) }.to not_change {
-        UploadReference.count
-      }
-    end
-
-    it "does nothing for an empty uploads array" do
-      expect { chat_message.attach_uploads([]) }.to not_change { UploadReference.count }
-    end
-  end
-
   describe "#create_mentions" do
     fab!(:message) { Fabricate(:chat_message) }
     fab!(:user1) { Fabricate(:user) }

plugins/chat/spec/system/channel_message_upload_spec.rb

@@ -21,7 +21,7 @@ RSpec.describe "Channel message selection", type: :system do
     chat_system_bootstrap
     channel_1.add(current_user)
     sign_in(current_user)
-    message_1.attach_uploads([image])
+    message_1.uploads = [image]
   end
 
   it "can collapse/expand an image and still have lightbox working" do