DEV: Drop chat_uploads table and model and remove old references (#20926)

Followup to 0924f874bd,
we migrated Chat::Upload records to UploadReference records
there and have not been making new Chat::Upload records
for some time, we can now delete the model and table.

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,

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",

plugins/chat/app/models/chat/upload.rb

@@ -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
-#

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

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

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 {
+        }.to change { UploadReference.where(upload_id: upload1.id).count }.by(1)
-                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 {
+        }.to change { UploadReference.where(upload_id: [upload1.id, upload2.id]).count }.by(2)
-                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 {
+        }.to not_change { UploadReference.where(upload_id: upload1.id).count }
-                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

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 {
+      }.to change { UploadReference.where(upload_id: upload2.id).count }.by(-1)
-              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 {
+      }.to change { UploadReference.where(target: chat_message).count }.by(-2)
-              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 {
+      }.to change { UploadReference.where(target: chat_message).count }.by(1)
-              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 {
+      }.to change { UploadReference.where(target: chat_message).count }.by(2)
-              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 {
+      }.to not_change { UploadReference.where(target: chat_message).count }
-              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 {
+      }.to not_change { UploadReference.where(target: chat_message).count }
-              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 {
+      }.to not_change { UploadReference.where(target: chat_message).count }
-              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

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" }

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)

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
+        UploadReference.count
-      }.and not_change { UploadReference.count }
+      }
     end
 
     it "does nothing for an empty uploads array" do
-      expect { chat_message.attach_uploads([]) }.to not_change {
+      expect { chat_message.attach_uploads([]) }.to not_change { UploadReference.count }
-        chat_upload_count
-      }.and 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