diff --git a/plugins/chat/app/jobs/regular/chat/channel_delete.rb b/plugins/chat/app/jobs/regular/chat/channel_delete.rb index 894fb95bdb5..8325eb30958 100644 --- a/plugins/chat/app/jobs/regular/chat/channel_delete.rb +++ b/plugins/chat/app/jobs/regular/chat/channel_delete.rb @@ -44,7 +44,6 @@ module Jobs # 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: ::Chat::Message.sti_name, diff --git a/plugins/chat/app/models/chat/message.rb b/plugins/chat/app/models/chat/message.rb index b2ca8cc0c20..18339a65be7 100644 --- a/plugins/chat/app/models/chat/message.rb +++ b/plugins/chat/app/models/chat/message.rb @@ -60,11 +60,6 @@ module Chat CLASS_MAPPING.invert.fetch(self) || super end - # TODO (martin) Remove this when we drop the ChatUpload table - has_many :chat_uploads, - dependent: :destroy, - class_name: "Chat::Upload", - foreign_key: :chat_message_id has_one :chat_webhook_event, dependent: :destroy, class_name: "Chat::WebhookEvent", diff --git a/plugins/chat/app/models/chat/upload.rb b/plugins/chat/app/models/chat/upload.rb deleted file mode 100644 index ae553d5faee..00000000000 --- a/plugins/chat/app/models/chat/upload.rb +++ /dev/null @@ -1,32 +0,0 @@ -# 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. -module Chat - class Upload < ActiveRecord::Base - self.table_name = "chat_uploads" - - belongs_to :chat_message, class_name: "Chat::Message" - belongs_to :upload - - deprecate *public_instance_methods(false) - end -end - -# == Schema Information -# -# Table name: chat_uploads -# -# id :bigint not null, primary key -# chat_message_id :integer not null -# upload_id :integer not null -# created_at :datetime not null -# updated_at :datetime not null -# -# Indexes -# -# index_chat_uploads_on_chat_message_id_and_upload_id (chat_message_id,upload_id) UNIQUE -# diff --git a/plugins/chat/db/post_migrate/20230403012844_drop_chat_uploads.rb b/plugins/chat/db/post_migrate/20230403012844_drop_chat_uploads.rb new file mode 100644 index 00000000000..d36f031e425 --- /dev/null +++ b/plugins/chat/db/post_migrate/20230403012844_drop_chat_uploads.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class DropChatUploads < ActiveRecord::Migration[7.0] + DROPPED_TABLES ||= %i[chat_uploads] + + def up + DROPPED_TABLES.each { |table| Migration::TableDropper.execute_drop(table) } + 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 13eea6cf18b..4825045f4b0 100644 --- a/plugins/chat/lib/chat/message_updater.rb +++ b/plugins/chat/lib/chat/message_updater.rb @@ -81,7 +81,6 @@ module Chat def update_uploads(upload_info) return unless upload_info[:changed] - 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/spec/components/chat/message_creator_spec.rb b/plugins/chat/spec/components/chat/message_creator_spec.rb index 703bdc9a493..f821e8b6727 100644 --- a/plugins/chat/spec/components/chat/message_creator_spec.rb +++ b/plugins/chat/spec/components/chat/message_creator_spec.rb @@ -813,9 +813,7 @@ describe Chat::MessageCreator do content: "Beep boop", upload_ids: [upload1.id], ) - }.to not_change { chat_upload_count([upload1]) }.and change { - UploadReference.where(upload_id: upload1.id).count - }.by(1) + }.to change { UploadReference.where(upload_id: upload1.id).count }.by(1) end it "can attach multiple uploads to a new message" do @@ -826,9 +824,7 @@ describe Chat::MessageCreator do content: "Beep boop", upload_ids: [upload1.id, upload2.id], ) - }.to not_change { chat_upload_count([upload1, upload2]) }.and change { - UploadReference.where(upload_id: [upload1.id, upload2.id]).count - }.by(2) + }.to 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 @@ -839,7 +835,7 @@ describe Chat::MessageCreator do content: "Beep boop", upload_ids: [private_upload.id], ) - }.not_to change { chat_upload_count([private_upload]) } + }.not_to change { UploadReference.where(upload_id: private_upload.id).count } end it "doesn't attach uploads when `chat_allow_uploads` is false" do @@ -851,9 +847,7 @@ describe Chat::MessageCreator do content: "Beep boop", upload_ids: [upload1.id], ) - }.to not_change { chat_upload_count([upload1]) }.and not_change { - UploadReference.where(upload_id: upload1.id).count - } + }.to not_change { UploadReference.where(upload_id: upload1.id).count } end end end @@ -942,11 +936,4 @@ describe Chat::MessageCreator 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 356b24076d8..f496b76d3ad 100644 --- a/plugins/chat/spec/components/chat/message_updater_spec.rb +++ b/plugins/chat/spec/components/chat/message_updater_spec.rb @@ -332,7 +332,7 @@ describe Chat::MessageUpdater do new_content: "I guess this is different", upload_ids: [upload2.id, upload1.id], ) - }.to not_change { chat_upload_count }.and not_change { UploadReference.count } + }.to not_change { UploadReference.count } end it "removes uploads that should be removed" do @@ -344,15 +344,6 @@ describe Chat::MessageUpdater do 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::MessageUpdater.update( guardian: guardian, @@ -360,9 +351,7 @@ describe Chat::MessageUpdater do new_content: "I guess this is different", upload_ids: [upload1.id], ) - }.to change { chat_upload_count([upload2]) }.by(-1).and change { - UploadReference.where(upload_id: upload2.id).count - }.by(-1) + }.to change { UploadReference.where(upload_id: upload2.id).count }.by(-1) end it "removes all uploads if they should be removed" do @@ -374,15 +363,6 @@ describe Chat::MessageUpdater do 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::MessageUpdater.update( guardian: guardian, @@ -390,9 +370,7 @@ describe Chat::MessageUpdater do new_content: "I guess this is different", upload_ids: [], ) - }.to change { chat_upload_count([upload1, upload2]) }.by(-2).and change { - UploadReference.where(target: chat_message).count - }.by(-2) + }.to change { UploadReference.where(target: chat_message).count }.by(-2) end it "adds one upload if none exist" do @@ -404,9 +382,7 @@ describe Chat::MessageUpdater do new_content: "I guess this is different", upload_ids: [upload1.id], ) - }.to not_change { chat_upload_count([upload1]) }.and change { - UploadReference.where(target: chat_message).count - }.by(1) + }.to change { UploadReference.where(target: chat_message).count }.by(1) end it "adds multiple uploads if none exist" do @@ -418,9 +394,7 @@ describe Chat::MessageUpdater do new_content: "I guess this is different", upload_ids: [upload1.id, upload2.id], ) - }.to not_change { chat_upload_count([upload1, upload2]) }.and change { - UploadReference.where(target: chat_message).count - }.by(2) + }.to 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 @@ -433,9 +407,7 @@ describe Chat::MessageUpdater do new_content: "I guess this is different", upload_ids: [0], ) - }.to not_change { chat_upload_count }.and not_change { - UploadReference.where(target: chat_message).count - } + }.to not_change { UploadReference.where(target: chat_message).count } end it "doesn't add uploads if `chat_allow_uploads` is false" do @@ -448,9 +420,7 @@ describe Chat::MessageUpdater do new_content: "I guess this is different", upload_ids: [upload1.id, upload2.id], ) - }.to not_change { chat_upload_count([upload1, upload2]) }.and not_change { - UploadReference.where(target: chat_message).count - } + }.to not_change { UploadReference.where(target: chat_message).count } end it "doesn't remove existing uploads if `chat_allow_uploads` is false" do @@ -469,9 +439,7 @@ describe Chat::MessageUpdater do new_content: "I guess this is different", upload_ids: [], ) - }.to not_change { chat_upload_count }.and not_change { - UploadReference.where(target: chat_message).count - } + }.to 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 @@ -575,12 +543,4 @@ describe Chat::MessageUpdater 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/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index d642326abfa..e66fba77824 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/plugins/chat/spec/fabricators/chat_fabricator.rb @@ -76,15 +76,6 @@ Fabricator(:chat_message_reaction, class_name: "Chat::MessageReaction") do end end -Fabricator(:chat_upload, class_name: "Chat::Upload") do - transient :user - - user { Fabricate(:user) } - - chat_message { |attrs| Fabricate(:chat_message, user: attrs[:user]) } - upload { |attrs| Fabricate(:upload, user: attrs[:user]) } -end - Fabricator(:chat_message_revision, class_name: "Chat::MessageRevision") do chat_message { Fabricate(:chat_message) } old_message { "something old" } diff --git a/plugins/chat/spec/jobs/regular/chat/channel_delete_spec.rb b/plugins/chat/spec/jobs/regular/chat/channel_delete_spec.rb index 4d9f5128faf..476df738d2f 100644 --- a/plugins/chat/spec/jobs/regular/chat/channel_delete_spec.rb +++ b/plugins/chat/spec/jobs/regular/chat/channel_delete_spec.rb @@ -20,11 +20,6 @@ describe Jobs::Chat::ChannelDelete do 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 @@ -66,10 +61,6 @@ describe Jobs::Chat::ChannelDelete do channel_memberships: Chat::UserChatChannelMembership.where(chat_channel: chat_channel).count, revisions: Chat::MessageRevision.where(chat_message_id: @message_ids).count, mentions: Chat::Mention.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: Chat::Message.sti_name).count, messages: Chat::Message.where(id: @message_ids).count, @@ -88,7 +79,6 @@ describe Jobs::Chat::ChannelDelete do 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) diff --git a/plugins/chat/spec/models/chat/message_spec.rb b/plugins/chat/spec/models/chat/message_spec.rb index e6e50052524..ef1b4b56601 100644 --- a/plugins/chat/spec/models/chat/message_spec.rb +++ b/plugins/chat/spec/models/chat/message_spec.rb @@ -466,19 +466,13 @@ describe Chat::Message do expect { webhook_1.reload }.to raise_error(ActiveRecord::RecordNotFound) end - it "destroys upload_references and chat_uploads" do + it "destroys upload_references" do message_1 = Fabricate(:chat_message) 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(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 @@ -537,7 +531,6 @@ describe Chat::Message do 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([Chat::Message.sti_name]) @@ -545,14 +538,12 @@ describe Chat::Message do it "does nothing if the message record is new" do expect { described_class.new.attach_uploads([upload_1, upload_2]) }.to not_change { - chat_upload_count - }.and not_change { UploadReference.count } + 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 } + expect { chat_message.attach_uploads([]) }.to not_change { UploadReference.count } end end @@ -617,12 +608,4 @@ describe Chat::Message do expect(message.chat_mentions.pluck(:id)).to include(*existing_mention_ids) # the mentions weren't recreated 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