diff --git a/app/models/upload.rb b/app/models/upload.rb index 86bf760b8da..249c50f4be4 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -100,6 +100,10 @@ class Upload < ActiveRecord::Base self.url end + def to_markdown + UploadMarkdown.new(self).to_markdown + end + def thumbnail(width = self.thumbnail_width, height = self.thumbnail_height) optimized_images.find_by(width: width, height: height) end diff --git a/app/models/upload_reference.rb b/app/models/upload_reference.rb index 73de830f545..ea5401e1233 100644 --- a/app/models/upload_reference.rb +++ b/app/models/upload_reference.rb @@ -4,6 +4,8 @@ class UploadReference < ActiveRecord::Base belongs_to :upload belongs_to :target, polymorphic: true + delegate :to_markdown, to: :upload + def self.ensure_exist!(upload_ids: [], target: nil, target_type: nil, target_id: nil) if !target && !(target_type && target_id) raise "target OR target_type and target_id are required" diff --git a/plugins/chat/app/jobs/regular/chat_channel_delete.rb b/plugins/chat/app/jobs/regular/chat_channel_delete.rb index ac89be4db99..3d407caf9cd 100644 --- a/plugins/chat/app/jobs/regular/chat_channel_delete.rb +++ b/plugins/chat/app/jobs/regular/chat_channel_delete.rb @@ -28,24 +28,30 @@ module Jobs Rails.logger.debug( "Deleting chat messages, mentions, revisions, and uploads for channel #{chat_channel.id}", ) - ChatMessage.transaction do - chat_messages = ChatMessage.where(chat_channel: chat_channel) - message_ids = chat_messages.select(:id) - ChatMention.where(chat_message_id: message_ids).delete_all - ChatMessageRevision.where(chat_message_id: message_ids).delete_all - ChatMessageReaction.where(chat_message_id: message_ids).delete_all + chat_messages = ChatMessage.where(chat_channel: chat_channel) + delete_messages_and_related_records(chat_channel, chat_messages) if chat_messages.any? + end + end - # if the uploads are not used anywhere else they will be deleted - # by the CleanUpUploads job in core - ChatUpload.where(chat_message_id: message_ids).delete_all + def delete_messages_and_related_records(chat_channel, chat_messages) + message_ids = chat_messages.pluck(:id) - # only the messages and the channel are Trashable, everything else gets - # permanently destroyed - chat_messages.update_all( - deleted_by_id: chat_channel.deleted_by_id, - deleted_at: Time.zone.now, - ) - end + ChatMessage.transaction do + ChatMention.where(chat_message_id: message_ids).delete_all + ChatMessageRevision.where(chat_message_id: message_ids).delete_all + ChatMessageReaction.where(chat_message_id: message_ids).delete_all + + # if the uploads are not used anywhere else they will be deleted + # by the CleanUpUploads job in core + DB.exec("DELETE FROM chat_uploads WHERE chat_message_id IN (#{message_ids.join(",")})") + UploadReference.where(target_id: message_ids, target_type: "ChatMessage").delete_all + + # only the messages and the channel are Trashable, everything else gets + # permanently destroyed + chat_messages.update_all( + deleted_by_id: chat_channel.deleted_by_id, + deleted_at: Time.zone.now, + ) end end end diff --git a/plugins/chat/app/models/chat_message.rb b/plugins/chat/app/models/chat_message.rb index f7688a8a13d..79e938b55b0 100644 --- a/plugins/chat/app/models/chat_message.rb +++ b/plugins/chat/app/models/chat_message.rb @@ -14,8 +14,11 @@ class ChatMessage < ActiveRecord::Base has_many :revisions, class_name: "ChatMessageRevision", dependent: :destroy has_many :reactions, class_name: "ChatMessageReaction", dependent: :destroy has_many :bookmarks, as: :bookmarkable, dependent: :destroy + has_many :upload_references, as: :target, dependent: :destroy + has_many :uploads, through: :upload_references + + # TODO (martin) Remove this when we drop the ChatUpload table has_many :chat_uploads, dependent: :destroy - has_many :uploads, through: :chat_uploads has_one :chat_webhook_event, dependent: :destroy has_one :chat_mention, dependent: :destroy @@ -61,14 +64,20 @@ class ChatMessage < ActiveRecord::Base end def attach_uploads(uploads) - return if uploads.blank? + return if uploads.blank? || self.new_record? now = Time.now - record_attrs = + ref_record_attrs = uploads.map do |upload| - { upload_id: upload.id, chat_message_id: self.id, created_at: now, updated_at: now } + { + upload_id: upload.id, + target_id: self.id, + target_type: "ChatMessage", + created_at: now, + updated_at: now, + } end - ChatUpload.insert_all!(record_attrs) + UploadReference.insert_all!(ref_record_attrs) end def excerpt @@ -91,20 +100,19 @@ class ChatMessage < ActiveRecord::Base end def to_markdown - markdown = [] + upload_markdown = + self + .upload_references + .includes(:upload) + .order(:created_at) + .map(&:to_markdown) + .reject(&:empty?) - if self.message.present? - msg = self.message + return self.message if upload_markdown.empty? - self.chat_uploads.any? ? markdown << msg + "\n" : markdown << msg - end + return ["#{self.message}\n"].concat(upload_markdown).join("\n") if self.message.present? - self - .chat_uploads - .order(:created_at) - .each { |chat_upload| markdown << UploadMarkdown.new(chat_upload.upload).to_markdown } - - markdown.reject(&:empty?).join("\n") + upload_markdown.join("\n") end def cook diff --git a/plugins/chat/app/models/chat_upload.rb b/plugins/chat/app/models/chat_upload.rb index 3382328bfd7..f9d969c40af 100644 --- a/plugins/chat/app/models/chat_upload.rb +++ b/plugins/chat/app/models/chat_upload.rb @@ -1,8 +1,15 @@ # frozen_string_literal: true +# TODO (martin) DEPRECATED: Remove this model once UploadReference has been +# in place for a couple of months, 2023-04-01 +# +# NOTE: Do not use this model anymore, chat messages are linked to uploads via +# the UploadReference table now, just like everything else. class ChatUpload < ActiveRecord::Base belongs_to :chat_message belongs_to :upload + + deprecate *public_instance_methods(false) end # == Schema Information diff --git a/plugins/chat/db/migrate/20230123020036_move_chat_uploads_to_upload_references.rb b/plugins/chat/db/migrate/20230123020036_move_chat_uploads_to_upload_references.rb new file mode 100644 index 00000000000..22dafbcff20 --- /dev/null +++ b/plugins/chat/db/migrate/20230123020036_move_chat_uploads_to_upload_references.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class MoveChatUploadsToUploadReferences < ActiveRecord::Migration[7.0] + def up + execute <<~SQL + INSERT INTO upload_references(upload_id, target_type, target_id, created_at, updated_at) + SELECT chat_uploads.upload_id, 'ChatMessage', chat_uploads.chat_message_id, chat_uploads.created_at, chat_uploads.updated_at + FROM chat_uploads + INNER JOIN uploads ON uploads.id = chat_uploads.upload_id + ON CONFLICT DO NOTHING + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/plugins/chat/db/post_migrate/20230123025112_move_chat_uploads_to_upload_references_post.rb b/plugins/chat/db/post_migrate/20230123025112_move_chat_uploads_to_upload_references_post.rb new file mode 100644 index 00000000000..02ec2dfbffe --- /dev/null +++ b/plugins/chat/db/post_migrate/20230123025112_move_chat_uploads_to_upload_references_post.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class MoveChatUploadsToUploadReferencesPost < ActiveRecord::Migration[7.0] + def up + execute <<~SQL + INSERT INTO upload_references(upload_id, target_type, target_id, created_at, updated_at) + SELECT chat_uploads.upload_id, 'ChatMessage', chat_uploads.chat_message_id, chat_uploads.created_at, chat_uploads.updated_at + FROM chat_uploads + INNER JOIN uploads ON uploads.id = chat_uploads.upload_id + ON CONFLICT DO NOTHING + 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 04e8ae9372b..79ecfed4e2c 100644 --- a/plugins/chat/lib/chat_message_updater.rb +++ b/plugins/chat/lib/chat_message_updater.rb @@ -83,7 +83,8 @@ class Chat::ChatMessageUpdater def update_uploads(upload_info) return unless upload_info[:changed] - ChatUpload.where(chat_message: @chat_message).destroy_all + DB.exec("DELETE FROM chat_uploads WHERE chat_message_id = #{@chat_message.id}") + UploadReference.where(target: @chat_message).destroy_all @chat_message.attach_uploads(upload_info[:uploads]) end diff --git a/plugins/chat/lib/chat_transcript_service.rb b/plugins/chat/lib/chat_transcript_service.rb index 6326494cdde..f61421d35f6 100644 --- a/plugins/chat/lib/chat_transcript_service.rb +++ b/plugins/chat/lib/chat_transcript_service.rb @@ -147,7 +147,7 @@ class ChatTranscriptService def messages @messages ||= ChatMessage - .includes(:user, chat_uploads: :upload) + .includes(:user, upload_references: :upload) .where(id: @message_ids, chat_channel_id: @channel.id) .order(:created_at) end diff --git a/plugins/chat/lib/message_mover.rb b/plugins/chat/lib/message_mover.rb index fc290f757ca..11cd84ac33d 100644 --- a/plugins/chat/lib/message_mover.rb +++ b/plugins/chat/lib/message_mover.rb @@ -125,10 +125,10 @@ class Chat::MessageMover SQL DB.exec(<<~SQL) - UPDATE chat_uploads cu - SET chat_message_id = mm.new_chat_message_id + UPDATE upload_references uref + SET target_id = mm.new_chat_message_id FROM moved_chat_messages mm - WHERE cu.chat_message_id = mm.old_chat_message_id + WHERE uref.target_id = mm.old_chat_message_id AND uref.target_type = 'ChatMessage' SQL DB.exec(<<~SQL) diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb index 0766b82cc93..04a5f83a137 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -372,14 +372,6 @@ after_initialize do end end - if respond_to?(:register_upload_unused) - register_upload_unused do |uploads| - uploads.joins("LEFT JOIN chat_uploads cu ON cu.upload_id = uploads.id").where( - "cu.upload_id IS NULL", - ) - end - end - if respond_to?(:register_upload_in_use) register_upload_in_use do |upload| ChatMessage.where( diff --git a/plugins/chat/spec/components/chat_message_creator_spec.rb b/plugins/chat/spec/components/chat_message_creator_spec.rb index 45f3a4ad2e3..0f6f56a8b97 100644 --- a/plugins/chat/spec/components/chat_message_creator_spec.rb +++ b/plugins/chat/spec/components/chat_message_creator_spec.rb @@ -461,7 +461,9 @@ describe Chat::ChatMessageCreator do content: "Beep boop", upload_ids: [upload1.id], ) - }.to change { ChatUpload.where(upload_id: upload1.id).count }.by(1) + }.to not_change { chat_upload_count([upload1]) }.and change { + UploadReference.where(upload_id: upload1.id).count + }.by(1) end it "can attach multiple uploads to a new message" do @@ -472,9 +474,9 @@ describe Chat::ChatMessageCreator do content: "Beep boop", upload_ids: [upload1.id, upload2.id], ) - }.to change { ChatUpload.where(upload_id: upload1.id).count }.by(1).and change { - ChatUpload.where(upload_id: upload2.id).count - }.by(1) + }.to not_change { chat_upload_count([upload1, upload2]) }.and change { + UploadReference.where(upload_id: [upload1.id, upload2.id]).count + }.by(2) end it "filters out uploads that weren't uploaded by the user" do @@ -485,7 +487,7 @@ describe Chat::ChatMessageCreator do content: "Beep boop", upload_ids: [private_upload.id], ) - }.not_to change { ChatUpload.where(upload_id: private_upload.id).count } + }.not_to change { chat_upload_count([private_upload]) } end it "doesn't attach uploads when `chat_allow_uploads` is false" do @@ -497,7 +499,9 @@ describe Chat::ChatMessageCreator do content: "Beep boop", upload_ids: [upload1.id], ) - }.not_to change { ChatUpload.where(upload_id: upload1.id).count } + }.to not_change { chat_upload_count([upload1]) }.and not_change { + UploadReference.where(upload_id: upload1.id).count + } end end end @@ -605,4 +609,11 @@ describe Chat::ChatMessageCreator do end end end + + # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01 + def chat_upload_count(uploads) + DB.query_single( + "SELECT COUNT(*) FROM chat_uploads WHERE upload_id IN (#{uploads.map(&:id).join(",")})", + ).first + end end diff --git a/plugins/chat/spec/components/chat_message_updater_spec.rb b/plugins/chat/spec/components/chat_message_updater_spec.rb index f32f979faaf..39c00726bc6 100644 --- a/plugins/chat/spec/components/chat_message_updater_spec.rb +++ b/plugins/chat/spec/components/chat_message_updater_spec.rb @@ -329,7 +329,7 @@ describe Chat::ChatMessageUpdater do new_content: "I guess this is different", upload_ids: [upload2.id, upload1.id], ) - }.not_to change { ChatUpload.count } + }.to not_change { chat_upload_count }.and not_change { UploadReference.count } end it "removes uploads that should be removed" do @@ -340,6 +340,16 @@ describe Chat::ChatMessageUpdater do public_chat_channel, upload_ids: [upload1.id, upload2.id], ) + + # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01 + DB.exec(<<~SQL) + INSERT INTO chat_uploads(upload_id, chat_message_id, created_at, updated_at) + VALUES(#{upload1.id}, #{chat_message.id}, NOW(), NOW()) + SQL + DB.exec(<<~SQL) + INSERT INTO chat_uploads(upload_id, chat_message_id, created_at, updated_at) + VALUES(#{upload2.id}, #{chat_message.id}, NOW(), NOW()) + SQL expect { Chat::ChatMessageUpdater.update( guardian: guardian, @@ -347,7 +357,9 @@ describe Chat::ChatMessageUpdater do new_content: "I guess this is different", upload_ids: [upload1.id], ) - }.to change { ChatUpload.where(upload_id: upload2.id).count }.by(-1) + }.to change { chat_upload_count([upload2]) }.by(-1).and change { + UploadReference.where(upload_id: upload2.id).count + }.by(-1) end it "removes all uploads if they should be removed" do @@ -358,6 +370,16 @@ describe Chat::ChatMessageUpdater do public_chat_channel, upload_ids: [upload1.id, upload2.id], ) + + # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01 + DB.exec(<<~SQL) + INSERT INTO chat_uploads(upload_id, chat_message_id, created_at, updated_at) + VALUES(#{upload1.id}, #{chat_message.id}, NOW(), NOW()) + SQL + DB.exec(<<~SQL) + INSERT INTO chat_uploads(upload_id, chat_message_id, created_at, updated_at) + VALUES(#{upload2.id}, #{chat_message.id}, NOW(), NOW()) + SQL expect { Chat::ChatMessageUpdater.update( guardian: guardian, @@ -365,7 +387,9 @@ describe Chat::ChatMessageUpdater do new_content: "I guess this is different", upload_ids: [], ) - }.to change { ChatUpload.where(chat_message: chat_message).count }.by(-2) + }.to change { chat_upload_count([upload1, upload2]) }.by(-2).and change { + UploadReference.where(target: chat_message).count + }.by(-2) end it "adds one upload if none exist" do @@ -377,7 +401,9 @@ describe Chat::ChatMessageUpdater do new_content: "I guess this is different", upload_ids: [upload1.id], ) - }.to change { ChatUpload.where(chat_message: chat_message).count }.by(1) + }.to not_change { chat_upload_count([upload1]) }.and change { + UploadReference.where(target: chat_message).count + }.by(1) end it "adds multiple uploads if none exist" do @@ -389,7 +415,9 @@ describe Chat::ChatMessageUpdater do new_content: "I guess this is different", upload_ids: [upload1.id, upload2.id], ) - }.to change { ChatUpload.where(chat_message: chat_message).count }.by(2) + }.to not_change { chat_upload_count([upload1, upload2]) }.and change { + UploadReference.where(target: chat_message).count + }.by(2) end it "doesn't remove existing uploads when upload ids that do not exist are passed in" do @@ -402,7 +430,9 @@ describe Chat::ChatMessageUpdater do new_content: "I guess this is different", upload_ids: [0], ) - }.not_to change { ChatUpload.where(chat_message: chat_message).count } + }.to not_change { chat_upload_count }.and not_change { + UploadReference.where(target: chat_message).count + } end it "doesn't add uploads if `chat_allow_uploads` is false" do @@ -415,7 +445,9 @@ describe Chat::ChatMessageUpdater do new_content: "I guess this is different", upload_ids: [upload1.id, upload2.id], ) - }.not_to change { ChatUpload.where(chat_message: chat_message).count } + }.to not_change { chat_upload_count([upload1, upload2]) }.and not_change { + UploadReference.where(target: chat_message).count + } end it "doesn't remove existing uploads if `chat_allow_uploads` is false" do @@ -434,7 +466,9 @@ describe Chat::ChatMessageUpdater do new_content: "I guess this is different", upload_ids: [], ) - }.not_to change { ChatUpload.where(chat_message: chat_message).count } + }.to not_change { chat_upload_count }.and not_change { + UploadReference.where(target: chat_message).count + } end it "updates if upload is present even if length is less than `chat_minimum_message_length`" do @@ -553,4 +587,12 @@ describe Chat::ChatMessageUpdater do end end end + + # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01 + def chat_upload_count(uploads = nil) + return DB.query_single("SELECT COUNT(*) FROM chat_uploads").first if !uploads + DB.query_single( + "SELECT COUNT(*) FROM chat_uploads WHERE upload_id IN (#{uploads.map(&:id).join(",")})", + ).first + end end diff --git a/plugins/chat/spec/jobs/chat_channel_delete_spec.rb b/plugins/chat/spec/jobs/chat_channel_delete_spec.rb index 033274f04c0..3ab19ae6f3b 100644 --- a/plugins/chat/spec/jobs/chat_channel_delete_spec.rb +++ b/plugins/chat/spec/jobs/chat_channel_delete_spec.rb @@ -17,10 +17,15 @@ describe Jobs::ChatChannelDelete do 10.times { ChatMessageReaction.create(chat_message: messages.sample, user: users.sample) } 10.times do - ChatUpload.create( - upload: Fabricate(:upload, user: users.sample), - chat_message: messages.sample, - ) + upload = Fabricate(:upload, user: users.sample) + message = messages.sample + + # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01 + DB.exec(<<~SQL) + INSERT INTO chat_uploads(upload_id, chat_message_id, created_at, updated_at) + VALUES(#{upload.id}, #{message.id}, NOW(), NOW()) + SQL + UploadReference.create(target: message, upload: upload) end ChatMention.create( @@ -52,27 +57,45 @@ describe Jobs::ChatChannelDelete do chat_channel.trash! end + def counts + { + incoming_webhooks: IncomingChatWebhook.where(chat_channel_id: chat_channel.id).count, + webhook_events: + ChatWebhookEvent.where(incoming_chat_webhook_id: @incoming_chat_webhook_id).count, + drafts: ChatDraft.where(chat_channel: chat_channel).count, + channel_memberships: UserChatChannelMembership.where(chat_channel: chat_channel).count, + revisions: ChatMessageRevision.where(chat_message_id: @message_ids).count, + mentions: ChatMention.where(chat_message_id: @message_ids).count, + chat_uploads: + DB.query_single( + "SELECT COUNT(*) FROM chat_uploads WHERE chat_message_id IN (#{@message_ids.join(",")})", + ).first, + upload_references: + UploadReference.where(target_id: @message_ids, target_type: "ChatMessage").count, + messages: ChatMessage.where(id: @message_ids).count, + reactions: ChatMessageReaction.where(chat_message_id: @message_ids).count, + } + end + it "deletes all of the messages and related records completely" do - expect { described_class.new.execute(chat_channel_id: chat_channel.id) }.to change { - IncomingChatWebhook.where(chat_channel_id: chat_channel.id).count - }.by(-1).and change { - ChatWebhookEvent.where(incoming_chat_webhook_id: @incoming_chat_webhook_id).count - }.by(-1).and change { ChatDraft.where(chat_channel: chat_channel).count }.by( - -1, - ).and change { - UserChatChannelMembership.where(chat_channel: chat_channel).count - }.by(-3).and change { - ChatMessageRevision.where(chat_message_id: @message_ids).count - }.by(-1).and change { - ChatMention.where(chat_message_id: @message_ids).count - }.by(-1).and change { - ChatUpload.where(chat_message_id: @message_ids).count - }.by(-10).and change { - ChatMessage.where(id: @message_ids).count - }.by(-20).and change { - ChatMessageReaction.where( - chat_message_id: @message_ids, - ).count - }.by(-10) + initial_counts = counts + described_class.new.execute(chat_channel_id: chat_channel.id) + new_counts = counts + + expect(new_counts[:incoming_webhooks]).to eq(initial_counts[:incoming_webhooks] - 1) + expect(new_counts[:webhook_events]).to eq(initial_counts[:webhook_events] - 1) + expect(new_counts[:drafts]).to eq(initial_counts[:drafts] - 1) + expect(new_counts[:channel_memberships]).to eq(initial_counts[:channel_memberships] - 3) + expect(new_counts[:revisions]).to eq(initial_counts[:revisions] - 1) + expect(new_counts[:mentions]).to eq(initial_counts[:mentions] - 1) + expect(new_counts[:chat_uploads]).to eq(initial_counts[:chat_uploads] - 10) + expect(new_counts[:upload_references]).to eq(initial_counts[:upload_references] - 10) + expect(new_counts[:messages]).to eq(initial_counts[:messages] - 20) + expect(new_counts[:reactions]).to eq(initial_counts[:reactions] - 10) + end + + it "does not error if there are no messages in the channel" do + other_channel = Fabricate(:chat_channel) + expect { described_class.new.execute(chat_channel_id: other_channel.id) }.not_to raise_error end end diff --git a/plugins/chat/spec/lib/chat_channel_archive_service_spec.rb b/plugins/chat/spec/lib/chat_channel_archive_service_spec.rb index d42c0f3d1a9..144b1032257 100644 --- a/plugins/chat/spec/lib/chat_channel_archive_service_spec.rb +++ b/plugins/chat/spec/lib/chat_channel_archive_service_spec.rb @@ -152,7 +152,7 @@ describe Chat::ChatChannelArchiveService do it "successfully links uploads from messages to the post" do create_messages(3) && start_archive - ChatUpload.create(chat_message: ChatMessage.last, upload: Fabricate(:upload)) + UploadReference.create(target: ChatMessage.last, upload: Fabricate(:upload)) subject.new(@channel_archive).execute expect(@channel_archive.reload.complete?).to eq(true) expect(@channel_archive.destination_topic.posts.last.upload_references.count).to eq(1) diff --git a/plugins/chat/spec/lib/chat_transcript_service_spec.rb b/plugins/chat/spec/lib/chat_transcript_service_spec.rb index 0f14d92f016..c9bf1b832e1 100644 --- a/plugins/chat/spec/lib/chat_transcript_service_spec.rb +++ b/plugins/chat/spec/lib/chat_transcript_service_spec.rb @@ -130,10 +130,10 @@ describe ChatTranscriptService do original_filename: "test_img.jpg", extension: "jpg", ) - cu1 = ChatUpload.create(chat_message: message, created_at: 10.seconds.ago, upload: video) - cu2 = ChatUpload.create(chat_message: message, created_at: 9.seconds.ago, upload: audio) - cu3 = ChatUpload.create(chat_message: message, created_at: 8.seconds.ago, upload: attachment) - cu4 = ChatUpload.create(chat_message: message, created_at: 7.seconds.ago, upload: image) + 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) video_markdown = UploadMarkdown.new(video).to_markdown audio_markdown = UploadMarkdown.new(audio).to_markdown attachment_markdown = UploadMarkdown.new(attachment).to_markdown @@ -166,7 +166,7 @@ describe ChatTranscriptService do original_filename: "test_img.jpg", extension: "jpg", ) - cu = ChatUpload.create(chat_message: message, created_at: 7.seconds.ago, upload: image) + UploadReference.create(target: message, created_at: 7.seconds.ago, upload: image) image_markdown = UploadMarkdown.new(image).to_markdown expect(service(message.id).generate_markdown).to eq(<<~MARKDOWN) diff --git a/plugins/chat/spec/lib/message_mover_spec.rb b/plugins/chat/spec/lib/message_mover_spec.rb index 43182c64c1f..6fd4b6079bd 100644 --- a/plugins/chat/spec/lib/message_mover_spec.rb +++ b/plugins/chat/spec/lib/message_mover_spec.rb @@ -112,7 +112,7 @@ describe Chat::MessageMover do it "updates references for reactions, uploads, revisions, mentions, etc." do reaction = Fabricate(:chat_message_reaction, chat_message: message1) - upload = Fabricate(:chat_upload, chat_message: message1) + upload = Fabricate(:upload_reference, target: message1) mention = Fabricate(:chat_mention, chat_message: message2, user: acting_user) revision = Fabricate(:chat_message_revision, chat_message: message3) webhook_event = Fabricate(:chat_webhook_event, chat_message: message3) @@ -121,7 +121,7 @@ describe Chat::MessageMover do moved_messages = ChatMessage.where(chat_channel: destination_channel).order("created_at ASC, id ASC").last(3) expect(reaction.reload.chat_message_id).to eq(moved_messages.first.id) - expect(upload.reload.chat_message_id).to eq(moved_messages.first.id) + expect(upload.reload.target_id).to eq(moved_messages.first.id) expect(mention.reload.chat_message_id).to eq(moved_messages.second.id) expect(revision.reload.chat_message_id).to eq(moved_messages.third.id) expect(webhook_event.reload.chat_message_id).to eq(moved_messages.third.id) diff --git a/plugins/chat/spec/models/chat_message_spec.rb b/plugins/chat/spec/models/chat_message_spec.rb index 9e305f98f25..c73b1a24315 100644 --- a/plugins/chat/spec/models/chat_message_spec.rb +++ b/plugins/chat/spec/models/chat_message_spec.rb @@ -309,7 +309,7 @@ describe ChatMessage do gif = Fabricate(:upload, original_filename: "cat.gif", width: 400, height: 300, extension: "gif") message = Fabricate(:chat_message, message: "") - ChatUpload.create(chat_message: message, upload: gif) + UploadReference.create(target: message, upload: gif) expect(message.excerpt).to eq "cat.gif" end @@ -391,8 +391,8 @@ describe ChatMessage do ) image2 = Fabricate(:upload, original_filename: "meme.jpg", width: 10, height: 10, extension: "jpg") - ChatUpload.create(chat_message: message, upload: image) - ChatUpload.create(chat_message: message, upload: image2) + UploadReference.create!(target: message, upload: image) + UploadReference.create!(target: message, upload: image2) expect(message.to_markdown).to eq(<<~MSG.chomp) hey friend, what's up?! @@ -479,13 +479,20 @@ describe ChatMessage do expect { webhook_1.reload }.to raise_error(ActiveRecord::RecordNotFound) end - it "destroys chat_uploads" do + it "destroys upload_references and chat_uploads" do message_1 = Fabricate(:chat_message) - chat_upload_1 = Fabricate(:chat_upload, chat_message: message_1) + upload_reference_1 = Fabricate(:upload_reference, target: message_1) + upload_1 = Fabricate(:upload) + # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01 + DB.exec(<<~SQL) + INSERT INTO chat_uploads(upload_id, chat_message_id, created_at, updated_at) + VALUES(#{upload_1.id}, #{message_1.id}, NOW(), NOW()) + SQL message_1.destroy! - expect { chat_upload_1.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect(DB.query("SELECT * FROM chat_uploads WHERE upload_id = #{upload_1.id}")).to eq([]) + expect { upload_reference_1.reload }.to raise_error(ActiveRecord::RecordNotFound) end describe "bookmarks" do @@ -532,4 +539,39 @@ describe ChatMessage do end 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(chat_upload_count([upload_1, upload_2])).to eq(0) + 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(["ChatMessage"]) + end + + it "does nothing if the message record is new" do + expect { ChatMessage.new.attach_uploads([upload_1, upload_2]) }.to not_change { + chat_upload_count + }.and not_change { UploadReference.count } + end + + it "does nothing for an empty uploads array" do + expect { chat_message.attach_uploads([]) }.to not_change { + chat_upload_count + }.and not_change { UploadReference.count } + end + end + + # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01 + def chat_upload_count(uploads = nil) + return DB.query_single("SELECT COUNT(*) FROM chat_uploads").first if !uploads + DB.query_single( + "SELECT COUNT(*) FROM chat_uploads WHERE upload_id IN (#{uploads.map(&:id).join(",")})", + ).first + end end diff --git a/plugins/chat/spec/plugin_spec.rb b/plugins/chat/spec/plugin_spec.rb index 68cdda55f97..431d1ccc616 100644 --- a/plugins/chat/spec/plugin_spec.rb +++ b/plugins/chat/spec/plugin_spec.rb @@ -26,7 +26,7 @@ describe Chat do ) end - it "marks uploads with ChatUpload in use" do + it "marks uploads with reference to ChatMessage via UploadReference in use" do unused_upload expect { Jobs::CleanUpUploads.new.execute({}) }.to change { Upload.count }.by(-1) @@ -61,7 +61,7 @@ describe Chat do ) end - it "marks uploads with ChatUpload in use" do + it "marks uploads with reference to ChatMessage via UploadReference in use" do draft_upload unused_upload diff --git a/plugins/chat/spec/system/uploads_spec.rb b/plugins/chat/spec/system/uploads_spec.rb index 6e49290023c..a012d29b86c 100644 --- a/plugins/chat/spec/system/uploads_spec.rb +++ b/plugins/chat/spec/system/uploads_spec.rb @@ -79,15 +79,21 @@ describe "Uploading files in chat messages", type: :system, js: true do context "when editing a message with uploads" do fab!(:message_2) { Fabricate(:chat_message, user: current_user, chat_channel: channel_1) } - fab!(:chat_upload) { Fabricate(:chat_upload, chat_message: message_2, user: current_user) } + fab!(:upload_reference) do + Fabricate( + :upload_reference, + target: message_2, + upload: Fabricate(:upload, user: current_user), + ) + end before do channel_1.add(current_user) sign_in(current_user) file = file_from_fixtures("logo-dev.png", "images") - url = Discourse.store.store_upload(file, chat_upload.upload) - chat_upload.upload.update!(url: url, sha1: Upload.generate_digest(file)) + url = Discourse.store.store_upload(file, upload_reference.upload) + upload_reference.upload.update!(url: url, sha1: Upload.generate_digest(file)) end it "allows deleting uploads" do diff --git a/spec/fabricators/upload_fabricator.rb b/spec/fabricators/upload_fabricator.rb index aec9a20721d..3101a110b27 100644 --- a/spec/fabricators/upload_fabricator.rb +++ b/spec/fabricators/upload_fabricator.rb @@ -97,3 +97,8 @@ Fabricator(:secure_upload_s3, from: :upload_s3) do sha1 { SecureRandom.hex(20) } original_sha1 { sequence(:sha1) { |n| Digest::SHA1.hexdigest(n.to_s) } } end + +Fabricator(:upload_reference) do + target + upload +end