DEV: Add messages_count to ChatChannel table (#19295)

This commit adds the messages_count column for ChatChannel messages,
which is the number of not-deleted messages in the channel.

This is not updated every time a message is created or deleted in a
channel, so it should not be displayed in the UI.
It is updated eventually via Jobs::ChatPeriodicalUpdates, which
will have additional functions in future after being introduced
here.

Also update these counts for existing channels in a post migration.

plugins/chat/app/jobs/scheduled/chat_periodical_updates.rb

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+module Jobs
+  class ChatPeriodicalUpdates < ::Jobs::Scheduled
+    every 15.minutes
+
+    def execute(args = nil)
+      # TODO: Add rebaking of old messages (baked_version <
+      # ChatMessage::BAKED_VERSION or baked_version IS NULL)
+      ChatChannel.ensure_consistency!
+      nil
+    end
+  end
+end

plugins/chat/app/jobs/scheduled/update_user_counts_for_chat_channels.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 module Jobs
+  # TODO (martin) Move into ChatChannel.ensure_consistency! so it
+  # is run with ChatPeriodicalUpdates
   class UpdateUserCountsForChatChannels < ::Jobs::Scheduled
     every 1.hour
 

plugins/chat/app/models/chat_channel.rb

@@ -83,6 +83,31 @@ class ChatChannel < ActiveRecord::Base
     "#{Discourse.base_path}/chat/channel/#{self.id}/#{self.slug || "-"}"
   end
 
+  def self.ensure_consistency!
+    update_counts
+  end
+
+  # TODO (martin) Move UpdateUserCountsForChatChannels into here
+  def self.update_counts
+
+    # NOTE: ChatChannel#messages_count is not updated every time
+    # a message is created or deleted in a channel, so it should not
+    # be displayed in the UI. It is updated eventually via Jobs::ChatPeriodicalUpdates
+    DB.exec <<~SQL
+      UPDATE chat_channels channels
+      SET messages_count = subquery.messages_count
+      FROM (
+        SELECT COUNT(*) AS messages_count, chat_channel_id
+        FROM chat_messages
+        WHERE chat_messages.deleted_at IS NULL
+        GROUP BY chat_channel_id
+      ) subquery
+      WHERE channels.id = subquery.chat_channel_id
+      AND channels.deleted_at IS NULL
+      AND subquery.messages_count != channels.messages_count
+    SQL
+  end
+
   private
 
   def change_status(acting_user, target_status)
@@ -143,6 +168,7 @@ end
 #
 # Indexes
 #
+#  index_chat_channels_on_messages_count             (messages_count)
 #  index_chat_channels_on_chatable_id                    (chatable_id)
 #  index_chat_channels_on_chatable_id_and_chatable_type  (chatable_id,chatable_type)
 #  index_chat_channels_on_slug                           (slug) UNIQUE

plugins/chat/db/migrate/20221202032006_add_chat_message_count_to_chat_channels.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class AddChatMessageCountToChatChannels < ActiveRecord::Migration[7.0]
+  def change
+    add_column :chat_channels, :messages_count, :integer, null: false, default: 0
+    add_index :chat_channels, :messages_count
+  end
+end

plugins/chat/db/post_migrate/20221202043755_update_chat_channel_message_counts.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+class UpdateChatChannelMessageCounts < ActiveRecord::Migration[7.0]
+  def up
+    DB.exec <<~SQL
+      UPDATE chat_channels channels
+      SET messages_count = subquery.messages_count
+      FROM (
+        SELECT COUNT(*) AS messages_count, chat_channel_id
+        FROM chat_messages
+        WHERE chat_messages.deleted_at IS NULL
+        GROUP BY chat_channel_id
+      ) subquery
+      WHERE channels.id = subquery.chat_channel_id
+      AND channels.deleted_at IS NULL
+      AND subquery.messages_count != channels.messages_count
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

plugins/chat/plugin.rb

@@ -201,6 +201,7 @@ after_initialize do
   load File.expand_path("../app/jobs/scheduled/update_user_counts_for_chat_channels.rb", __FILE__)
   load File.expand_path("../app/jobs/scheduled/email_chat_notifications.rb", __FILE__)
   load File.expand_path("../app/jobs/scheduled/auto_join_users.rb", __FILE__)
+  load File.expand_path("../app/jobs/scheduled/chat_periodical_updates.rb", __FILE__)
   load File.expand_path("../app/services/chat_publisher.rb", __FILE__)
   load File.expand_path("../app/services/chat_message_destroyer.rb", __FILE__)
   load File.expand_path("../app/controllers/api_controller.rb", __FILE__)

plugins/chat/spec/jobs/chat_periodical_updates_spec.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+RSpec.describe Jobs::ChatPeriodicalUpdates do
+  it "works" do
+    # does not blow up, no mocks, everything is called
+    Jobs::ChatPeriodicalUpdates.new.execute(nil)
+  end
+end

plugins/chat/spec/models/chat_channel_spec.rb

@@ -1,35 +1,84 @@
 # frozen_string_literal: true
 
 RSpec.describe ChatChannel do
-  fab!(:category_channel) { Fabricate(:category_channel) }
-  fab!(:dm_channel) { Fabricate(:direct_message_channel) }
+  fab!(:category_channel1) { Fabricate(:category_channel) }
+  fab!(:dm_channel1) { Fabricate(:direct_message_channel) }
 
   describe "#relative_url" do
     context "when the slug is nil" do
       it "uses a - instead" do
-        category_channel.slug = nil
-        expect(category_channel.relative_url).to eq("/chat/channel/#{category_channel.id}/-")
+        category_channel1.slug = nil
+        expect(category_channel1.relative_url).to eq("/chat/channel/#{category_channel1.id}/-")
       end
     end
 
     context "when the slug is not nil" do
-      before { category_channel.update!(slug: "some-cool-channel") }
+      before { category_channel1.update!(slug: "some-cool-channel") }
 
       it "includes the slug for the channel" do
-        expect(category_channel.relative_url).to eq(
-          "/chat/channel/#{category_channel.id}/some-cool-channel",
+        expect(category_channel1.relative_url).to eq(
+          "/chat/channel/#{category_channel1.id}/some-cool-channel",
         )
       end
     end
   end
 
+  describe ".ensure_consistency!" do
+    fab!(:category_channel2) { Fabricate(:category_channel) }
+    fab!(:category_channel3) { Fabricate(:category_channel) }
+    fab!(:category_channel4) { Fabricate(:category_channel) }
+    fab!(:dm_channel2) { Fabricate(:direct_message_channel) }
+
+    before do
+      Fabricate(:chat_message, chat_channel: category_channel1)
+      Fabricate(:chat_message, chat_channel: category_channel1)
+      Fabricate(:chat_message, chat_channel: category_channel1)
+
+      Fabricate(:chat_message, chat_channel: category_channel2)
+      Fabricate(:chat_message, chat_channel: category_channel2)
+      Fabricate(:chat_message, chat_channel: category_channel2)
+      Fabricate(:chat_message, chat_channel: category_channel2)
+
+      Fabricate(:chat_message, chat_channel: category_channel3)
+
+      Fabricate(:chat_message, chat_channel: dm_channel2)
+      Fabricate(:chat_message, chat_channel: dm_channel2)
+    end
+
+    describe "updating messages_count for all channels" do
+      it "counts correctly" do
+        described_class.ensure_consistency!
+        expect(category_channel1.reload.messages_count).to eq(3)
+        expect(category_channel2.reload.messages_count).to eq(4)
+        expect(category_channel3.reload.messages_count).to eq(1)
+        expect(category_channel4.reload.messages_count).to eq(0)
+        expect(dm_channel1.reload.messages_count).to eq(0)
+        expect(dm_channel2.reload.messages_count).to eq(2)
+      end
+
+      it "does not count deleted messages" do
+        category_channel3.chat_messages.last.trash!
+        described_class.ensure_consistency!
+        expect(category_channel3.reload.messages_count).to eq(0)
+      end
+
+      it "does not update deleted channels" do
+        described_class.ensure_consistency!
+        category_channel3.chat_messages.last.trash!
+        category_channel3.trash!
+        described_class.ensure_consistency!
+        expect(category_channel3.reload.messages_count).to eq(1)
+      end
+    end
+  end
+
   describe "#allow_channel_wide_mentions" do
     it "defaults to true" do
-      expect(category_channel.allow_channel_wide_mentions).to be(true)
+      expect(category_channel1.allow_channel_wide_mentions).to be(true)
     end
 
     it "cant be nullified" do
-      expect { category_channel.update!(allow_channel_wide_mentions: nil) }.to raise_error(
+      expect { category_channel1.update!(allow_channel_wide_mentions: nil) }.to raise_error(
         ActiveRecord::NotNullViolation,
       )
     end