From 0924f874bd9aca3515e17d0d084a35058e9d5757 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 24 Jan 2023 13:28:21 +1000 Subject: [PATCH] DEV: Use UploadReference instead of ChatUpload in chat (#19947) We've had the UploadReference table for some time now in core, but it was added after ChatUpload was and chat was just never moved over to this new system. This commit changes all chat code dealing with uploads to create/ update/delete/query UploadReference records instead of ChatUpload records for consistency. At a later date we will drop the ChatUpload table, but for now keeping it for data backup. The migration + post migration are the same, we need both in case any chat uploads are added/removed during deploy. --- app/models/upload.rb | 4 + app/models/upload_reference.rb | 2 + .../app/jobs/regular/chat_channel_delete.rb | 38 ++++++---- plugins/chat/app/models/chat_message.rb | 40 ++++++---- plugins/chat/app/models/chat_upload.rb | 7 ++ ..._move_chat_uploads_to_upload_references.rb | 17 +++++ ..._chat_uploads_to_upload_references_post.rb | 17 +++++ plugins/chat/lib/chat_message_updater.rb | 3 +- plugins/chat/lib/chat_transcript_service.rb | 2 +- plugins/chat/lib/message_mover.rb | 6 +- plugins/chat/plugin.rb | 8 -- .../components/chat_message_creator_spec.rb | 23 ++++-- .../components/chat_message_updater_spec.rb | 58 +++++++++++++-- .../spec/jobs/chat_channel_delete_spec.rb | 73 ++++++++++++------- .../lib/chat_channel_archive_service_spec.rb | 2 +- .../spec/lib/chat_transcript_service_spec.rb | 10 +-- plugins/chat/spec/lib/message_mover_spec.rb | 4 +- plugins/chat/spec/models/chat_message_spec.rb | 54 ++++++++++++-- plugins/chat/spec/plugin_spec.rb | 4 +- plugins/chat/spec/system/uploads_spec.rb | 12 ++- spec/fabricators/upload_fabricator.rb | 5 ++ 21 files changed, 286 insertions(+), 103 deletions(-) create mode 100644 plugins/chat/db/migrate/20230123020036_move_chat_uploads_to_upload_references.rb create mode 100644 plugins/chat/db/post_migrate/20230123025112_move_chat_uploads_to_upload_references_post.rb 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