DEV: Refactor STI/polymorphic associations in chat (#20789)

app/models/bookmark.rb

@@ -22,7 +22,7 @@ class Bookmark < ActiveRecord::Base
   end
 
   def self.valid_bookmarkable_types
-    Bookmark.registered_bookmarkables.map { |bm| bm.model.to_s }
+    Bookmark.registered_bookmarkables.map { |bm| bm.model.polymorphic_name }
   end
 
   belongs_to :user

app/models/reviewable.rb

@@ -129,7 +129,7 @@ class Reviewable < ActiveRecord::Base
       update_args = {
         status: statuses[:pending],
         id: target.id,
-        type: target.class.sti_name,
+        type: target.class.polymorphic_name,
         potential_spam: potential_spam == true ? true : nil,
       }
 

plugins/chat/app/jobs/regular/chat/channel_delete.rb

@@ -46,7 +46,7 @@ module Jobs
           # by the CleanUpUploads job in core
           ::UploadReference.where(
             target_id: message_ids,
-            target_type: ::Chat::Message.sti_name,
+            target_type: ::Chat::Message.polymorphic_name,
           ).delete_all
 
           # only the messages and the channel are Trashable, everything else gets

plugins/chat/app/models/chat/category_channel.rb

@@ -7,10 +7,6 @@ module Chat
     delegate :read_restricted?, to: :category
     delegate :url, to: :chatable, prefix: true
 
-    def self.polymorphic_class_for(name)
-      Chat::Chatable.polymorphic_class_for(name) || super(name)
-    end
-
     %i[category_channel? public_channel? chatable_has_custom_fields?].each do |name|
       define_method(name) { true }
     end

plugins/chat/app/models/chat/channel.rb

@@ -3,19 +3,11 @@
 module Chat
   class Channel < ActiveRecord::Base
     include Trashable
+    include TypeMappable
 
     self.table_name = "chat_channels"
 
     belongs_to :chatable, polymorphic: true
-
-    def self.sti_class_for(type_name)
-      Chat::Chatable.sti_class_for(type_name) || super(type_name)
-    end
-
-    def self.sti_name
-      Chat::Chatable.sti_name_for(self) || super
-    end
-
     belongs_to :direct_message,
                class_name: "Chat::DirectMessage",
                foreign_key: :chatable_id,
@@ -51,6 +43,14 @@ module Chat
     delegate :empty?, to: :chat_messages, prefix: true
 
     class << self
+      def sti_class_mapping =
+        {
+          "CategoryChannel" => Chat::CategoryChannel,
+          "DirectMessageChannel" => Chat::DirectMessageChannel,
+        }
+
+      def polymorphic_class_mapping = { "DirectMessage" => Chat::DirectMessage }
+
       def editable_statuses
         statuses.filter { |k, _| !%w[read_only archived].include?(k) }
       end

plugins/chat/app/models/chat/direct_message.rb

@@ -5,10 +5,7 @@ module Chat
     self.table_name = "direct_message_channels"
 
     include Chatable
-
+    include TypeMappable
-    def self.polymorphic_name
-      Chat::Chatable.polymorphic_name_for(self) || super
-    end
 
     has_many :direct_message_users,
              class_name: "Chat::DirectMessageUser",
@@ -17,11 +14,15 @@ module Chat
 
     has_one :direct_message_channel, as: :chatable, class_name: "Chat::DirectMessageChannel"
 
-    def self.for_user_ids(user_ids)
+    class << self
+      def polymorphic_class_mapping = { "DirectMessage" => Chat::DirectMessage }
+
+      def for_user_ids(user_ids)
         joins(:users)
           .group("direct_message_channels.id")
           .having("ARRAY[?] = ARRAY_AGG(users.id ORDER BY users.id)", user_ids.sort)
-        &.first
+          .first
+      end
     end
 
     def user_can_access?(user)

plugins/chat/app/models/chat/direct_message_channel.rb

@@ -4,10 +4,6 @@ module Chat
   class DirectMessageChannel < Channel
     alias_attribute :direct_message, :chatable
 
-    def self.polymorphic_class_for(name)
-      Chat::Chatable.polymorphic_class_for(name) || super(name)
-    end
-
     def direct_message_channel?
       true
     end

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

@@ -3,13 +3,14 @@
 module Chat
   class Message < ActiveRecord::Base
     include Trashable
+    include TypeMappable
 
     self.table_name = "chat_messages"
 
-    attribute :has_oneboxes, default: false
-
     BAKED_VERSION = 2
 
+    attribute :has_oneboxes, default: false
+
     belongs_to :chat_channel, class_name: "Chat::Channel"
     belongs_to :user
     belongs_to :in_reply_to, class_name: "Chat::Message"
@@ -30,36 +31,18 @@ module Chat
              foreign_key: :chat_message_id
     has_many :bookmarks,
              -> {
-               unscope(where: :bookmarkable_type).where(bookmarkable_type: Chat::Message.sti_name)
+               unscope(where: :bookmarkable_type).where(
+                 bookmarkable_type: Chat::Message.polymorphic_name,
+               )
              },
              as: :bookmarkable,
              dependent: :destroy
     has_many :upload_references,
-             -> { unscope(where: :target_type).where(target_type: Chat::Message.sti_name) },
+             -> { unscope(where: :target_type).where(target_type: Chat::Message.polymorphic_name) },
              dependent: :destroy,
              foreign_key: :target_id
     has_many :uploads, through: :upload_references, class_name: "::Upload"
 
-    CLASS_MAPPING = { "ChatMessage" => Chat::Message }
-
-    # the model used when loading type column
-    def self.sti_class_for(name)
-      CLASS_MAPPING[name] if CLASS_MAPPING.key?(name)
-    end
-    # the type column value
-    def self.sti_name
-      CLASS_MAPPING.invert.fetch(self)
-    end
-
-    # the model used when loading chatable_type column
-    def self.polymorphic_class_for(name)
-      CLASS_MAPPING[name] if CLASS_MAPPING.key?(name)
-    end
-    # the type stored in *_type column of polymorphic associations
-    def self.polymorphic_name
-      CLASS_MAPPING.invert.fetch(self) || super
-    end
-
     has_one :chat_webhook_event,
             dependent: :destroy,
             class_name: "Chat::WebhookEvent",
@@ -77,7 +60,6 @@ module Chat
               },
             )
           }
-
     scope :in_dm_channel,
           -> {
             joins(:chat_channel).where(
@@ -86,11 +68,13 @@ module Chat
               },
             )
           }
-
     scope :created_before, ->(date) { where("chat_messages.created_at < ?", date) }
+    scope :uncooked, -> { where("cooked_version <> ? or cooked_version IS NULL", BAKED_VERSION) }
 
     before_save { ensure_last_editor_id }
 
+    def self.polymorphic_class_mapping = { "ChatMessage" => Chat::Message }
+
     def validate_message(has_uploads:)
       WatchedWordsValidator.new(attributes: [:message]).validate(self)
 
@@ -125,7 +109,7 @@ module Chat
           {
             upload_id: upload.id,
             target_id: self.id,
-            target_type: self.class.sti_name,
+            target_type: self.class.polymorphic_name,
             created_at: now,
             updated_at: now,
           }
@@ -193,10 +177,6 @@ module Chat
       Jobs.enqueue(Jobs::Chat::ProcessMessage, args)
     end
 
-    def self.uncooked
-      where("cooked_version <> ? or cooked_version IS NULL", BAKED_VERSION)
-    end
-
     MARKDOWN_FEATURES = %w[
       anchor
       bbcode-block

plugins/chat/app/models/chat/view.rb

@@ -56,7 +56,7 @@ module Chat
           sql,
           pending: ReviewableScore.statuses[:pending],
           message_ids: @chat_messages.map(&:id),
-          target_type: Chat::Message.sti_name,
+          target_type: Chat::Message.polymorphic_name,
         )
         .each { |row| ids[row.target_id] = row.reviewable_id }
 
@@ -85,7 +85,7 @@ module Chat
           sql,
           message_ids: @chat_messages.map(&:id),
           user_id: @user.id,
-          target_type: Chat::Message.sti_name,
+          target_type: Chat::Message.polymorphic_name,
         )
         .each { |row| statuses[row.target_id] = row.status }
 

plugins/chat/app/models/concerns/chat/chatable.rb

@@ -4,33 +4,6 @@ module Chat
   module Chatable
     extend ActiveSupport::Concern
 
-    STI_CLASS_MAPPING = {
-      "CategoryChannel" => Chat::CategoryChannel,
-      "DirectMessageChannel" => Chat::DirectMessageChannel,
-    }
-
-    # the model used when loading type column
-    def self.sti_class_for(name)
-      STI_CLASS_MAPPING[name] if STI_CLASS_MAPPING.key?(name)
-    end
-
-    # the type column value
-    def self.sti_name_for(klass)
-      STI_CLASS_MAPPING.invert.fetch(klass)
-    end
-
-    POLYMORPHIC_CLASS_MAPPING = { "DirectMessage" => Chat::DirectMessage }
-
-    # the model used when loading chatable_type column
-    def self.polymorphic_class_for(name)
-      POLYMORPHIC_CLASS_MAPPING[name] if POLYMORPHIC_CLASS_MAPPING.key?(name)
-    end
-
-    # the chatable_type column value
-    def self.polymorphic_name_for(klass)
-      POLYMORPHIC_CLASS_MAPPING.invert.fetch(klass)
-    end
-
     def chat_channel
       channel_class.new(chatable: self)
     end

plugins/chat/app/models/concerns/chat/type_mappable.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+module Chat
+  module TypeMappable
+    extend ActiveSupport::Concern
+
+    class_methods do
+      def sti_class_mapping = {}
+      def polymorphic_class_mapping = {}
+
+      # the model used when loading type column
+      def sti_class_for(name)
+        sti_class_mapping[name] || super
+      end
+
+      # the type column value
+      def sti_name
+        sti_class_mapping.invert[self] || super
+      end
+
+      # the model used when loading *_type column (e.g. 'chatable_type')
+      def polymorphic_class_for(name)
+        polymorphic_class_mapping[name] || super
+      end
+
+      # the *_type column value (e.g. 'chatable_type')
+      def polymorphic_name
+        polymorphic_class_mapping.invert[self] || super
+      end
+    end
+  end
+end

plugins/chat/lib/chat/bookmark_extension.rb

@@ -4,19 +4,8 @@ module Chat
   module BookmarkExtension
     extend ActiveSupport::Concern
 
-    prepended do
+    prepended { include TypeMappable }
-      def valid_bookmarkable_type
-        return true if self.bookmarkable_type == Chat::Message.sti_name
-        super if defined?(super)
-      end
 
-      CLASS_MAPPING = { "ChatMessage" => Chat::Message }
+    class_methods { def polymorphic_class_mapping = { "ChatMessage" => Chat::Message } }
-
-      # the model used when loading chatable_type column
-      def self.polymorphic_class_for(name)
-        return CLASS_MAPPING[name] if CLASS_MAPPING.key?(name)
-        super if defined?(super)
-      end
-    end
   end
 end

plugins/chat/lib/chat/category_extension.rb

@@ -6,10 +6,6 @@ module Chat
 
     include Chat::Chatable
 
-    def self.polymorphic_name
-      Chat::Chatable.polymorphic_name_for(self) || super
-    end
-
     prepended do
       has_one :category_channel,
               as: :chatable,

plugins/chat/lib/chat/message_bookmarkable.rb

@@ -23,12 +23,12 @@ module Chat
           :sanitize_sql_array,
           [
             "INNER JOIN chat_messages ON chat_messages.id = bookmarks.bookmarkable_id AND chat_messages.deleted_at IS NULL AND bookmarks.bookmarkable_type = ?",
-            Chat::Message.sti_name,
+            Chat::Message.polymorphic_name,
           ],
         )
 
       user
-        .bookmarks_of_type(Chat::Message.sti_name)
+        .bookmarks_of_type(Chat::Message.polymorphic_name)
         .joins(joins)
         .where("chat_messages.chat_channel_id IN (?)", accessible_channel_ids)
     end
@@ -66,7 +66,7 @@ module Chat
     end
 
     def self.cleanup_deleted
-      DB.query(<<~SQL, grace_time: 3.days.ago, bookmarkable_type: Chat::Message.sti_name)
+      DB.query(<<~SQL, grace_time: 3.days.ago, bookmarkable_type: Chat::Message.polymorphic_name)
       DELETE FROM bookmarks b
       USING chat_messages cm
       WHERE b.bookmarkable_id = cm.id

plugins/chat/lib/chat/message_mover.rb

@@ -143,7 +143,7 @@ module Chat
       WHERE cmr.chat_message_id = mm.old_chat_message_id
     SQL
 
-      DB.exec(<<~SQL, target_type: Chat::Message.sti_name)
+      DB.exec(<<~SQL, target_type: Chat::Message.polymorphic_name)
       UPDATE upload_references uref
       SET target_id = mm.new_chat_message_id
       FROM moved_chat_messages mm

plugins/chat/lib/chat/reviewable_extension.rb

@@ -4,24 +4,11 @@ module Chat
   module ReviewableExtension
     extend ActiveSupport::Concern
 
-    prepended do
+    prepended { include TypeMappable }
-      # the model used when loading type column
-      def self.sti_class_for(name)
-        return Chat::ReviewableMessage if name == "ReviewableChatMessage"
-        super(name)
-      end
 
-      # the model used when loading target_type column
+    class_methods do
-      def self.polymorphic_class_for(name)
+      def sti_class_mapping = { "ReviewableChatMessage" => Chat::ReviewableMessage }
-        return Chat::Message if name == Chat::Message.sti_name
+      def polymorphic_class_mapping = { "ChatMessage" => Chat::Message }
-        super(name)
-      end
-
-      # the type column value when saving a Chat::ReviewableMessage
-      def self.sti_name
-        return "ReviewableChatMessage" if self.to_s == "Chat::ReviewableMessage"
-        super
-      end
     end
   end
 end

plugins/chat/spec/fabricators/chat_fabricator.rb

@@ -87,7 +87,6 @@ Fabricator(:chat_reviewable_message, class_name: "Chat::ReviewableMessage") do
   reviewable_by_moderator true
   type "ReviewableChatMessage"
   created_by { Fabricate(:user) }
-  target_type Chat::Message.sti_name
   target { Fabricate(:chat_message) }
   reviewable_scores { |p| [Fabricate.build(:reviewable_score, reviewable_id: p[:id])] }
 end

plugins/chat/spec/jobs/regular/chat/channel_delete_spec.rb

@@ -62,7 +62,10 @@ describe Jobs::Chat::ChannelDelete do
       revisions: Chat::MessageRevision.where(chat_message_id: @message_ids).count,
       mentions: Chat::Mention.where(chat_message_id: @message_ids).count,
       upload_references:
-        UploadReference.where(target_id: @message_ids, target_type: Chat::Message.sti_name).count,
+        UploadReference.where(
+          target_id: @message_ids,
+          target_type: Chat::Message.polymorphic_name,
+        ).count,
       messages: Chat::Message.where(id: @message_ids).count,
       reactions: Chat::MessageReaction.where(chat_message_id: @message_ids).count,
     }

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

@@ -533,7 +533,7 @@ describe Chat::Message do
       upload_references = UploadReference.where(upload_id: [upload_1, upload_2])
       expect(upload_references.count).to eq(2)
       expect(upload_references.map(&:target_id).uniq).to eq([chat_message.id])
-      expect(upload_references.map(&:target_type).uniq).to eq([Chat::Message.sti_name])
+      expect(upload_references.map(&:target_type).uniq).to eq([described_class.polymorphic_name])
     end
 
     it "does nothing if the message record is new" do