DEV: Move user count update for channels to ensure_consistency! (disc…

…ourse#22321)

This fixes a longstanding TODO to move the contents of the
UpdateUserCountsForChannels job to the ensure_consistency!
method of Chat::Channel, which runs every 15 mins as part of
periodical updates.

This commit also addresses the performance issue of the original,
where we would fetch all channels and do an individual query to
get the count and update the count of each one. Now we do it all
in one query, and only publish the changed channels to the UI.

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

@@ -104,27 +104,53 @@ def relative_url
     end
 
     def self.ensure_consistency!
-      update_counts
+      update_message_counts
+      update_user_counts
     end
 
-    # TODO (martin) Move Jobs::Chat::UpdateUserCountsForChannels into here
-    def self.update_counts
+    def self.update_message_counts
       # NOTE: Chat::Channel#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::Chat::PeriodicalUpdates
       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
+        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 self.update_user_counts
+      updated_channel_ids = DB.query_single(<<~SQL, statuses: [statuses[:open], statuses[:closed]])
+        UPDATE chat_channels channels
+        SET user_count = subquery.user_count, user_count_stale = false
+        FROM (
+          SELECT COUNT(DISTINCT user_chat_channel_memberships.id) AS user_count, user_chat_channel_memberships.chat_channel_id
+          FROM user_chat_channel_memberships
+          INNER JOIN users ON users.id = user_chat_channel_memberships.user_id
+          WHERE users.active
+            AND (users.suspended_till IS NULL OR users.suspended_till <= CURRENT_TIMESTAMP)
+            AND NOT users.staged
+            AND user_chat_channel_memberships.following
+          GROUP BY user_chat_channel_memberships.chat_channel_id
+        ) subquery
+        WHERE channels.id = subquery.chat_channel_id
+        AND channels.deleted_at IS NULL
+        AND subquery.user_count != channels.user_count
+        AND channels.status IN (:statuses)
+        RETURNING channels.id;
+      SQL
+
+      Chat::Channel
+        .where(id: updated_channel_ids)
+        .find_each { |channel| ::Chat::Publisher.publish_chat_channel_metadata(channel) }
     end
 
     def latest_not_deleted_message_id(anchor_message_id: nil)

plugins/chat/spec/models/chat/channel_spec.rb

@@ -25,27 +25,28 @@
 
   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)
+    describe "updating messages_count for all channels" do
+      fab!(:category_channel3) { Fabricate(:category_channel) }
+      fab!(:category_channel4) { Fabricate(:category_channel) }
+      fab!(:dm_channel2) { Fabricate(:direct_message_channel) }
 
-      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)
+      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_channel3)
+        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: dm_channel2)
-      Fabricate(:chat_message, chat_channel: dm_channel2)
-    end
+        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)
@@ -70,6 +71,108 @@
         expect(category_channel3.reload.messages_count).to eq(1)
       end
     end
+
+    describe "updating user_count for all channels" do
+      fab!(:user_1) { Fabricate(:user) }
+      fab!(:user_2) { Fabricate(:user) }
+      fab!(:user_3) { Fabricate(:user) }
+      fab!(:user_4) { Fabricate(:user) }
+
+      def create_memberships
+        user_1.user_chat_channel_memberships.create!(
+          chat_channel: category_channel1,
+          following: true,
+        )
+        user_1.user_chat_channel_memberships.create!(
+          chat_channel: category_channel2,
+          following: true,
+        )
+
+        user_2.user_chat_channel_memberships.create!(
+          chat_channel: category_channel1,
+          following: true,
+        )
+        user_2.user_chat_channel_memberships.create!(
+          chat_channel: category_channel2,
+          following: true,
+        )
+
+        user_3.user_chat_channel_memberships.create!(
+          chat_channel: category_channel1,
+          following: false,
+        )
+        user_3.user_chat_channel_memberships.create!(
+          chat_channel: category_channel2,
+          following: true,
+        )
+      end
+
+      it "sets the user_count correctly for each chat channel" do
+        create_memberships
+
+        described_class.ensure_consistency!
+
+        expect(category_channel1.reload.user_count).to eq(2)
+        expect(category_channel2.reload.user_count).to eq(3)
+      end
+
+      it "does not count suspended, non-activated, nor staged users" do
+        user_1.user_chat_channel_memberships.create!(
+          chat_channel: category_channel1,
+          following: true,
+        )
+        user_2.user_chat_channel_memberships.create!(
+          chat_channel: category_channel2,
+          following: true,
+        )
+        user_3.user_chat_channel_memberships.create!(
+          chat_channel: category_channel2,
+          following: true,
+        )
+        user_4.user_chat_channel_memberships.create!(
+          chat_channel: category_channel2,
+          following: true,
+        )
+        user_2.update(suspended_till: 3.weeks.from_now)
+        user_3.update(staged: true)
+        user_4.update(active: false)
+
+        described_class.ensure_consistency!
+
+        expect(category_channel1.reload.user_count).to eq(1)
+        expect(category_channel2.reload.user_count).to eq(0)
+      end
+
+      it "does not count archived, or read_only channels" do
+        create_memberships
+
+        category_channel1.update!(status: :archived)
+        described_class.ensure_consistency!
+        expect(category_channel1.reload.user_count).to eq(0)
+
+        category_channel1.update!(status: :read_only)
+        described_class.ensure_consistency!
+        expect(category_channel1.reload.user_count).to eq(0)
+      end
+
+      it "publishes all the updated channels" do
+        create_memberships
+
+        messages = MessageBus.track_publish { described_class.ensure_consistency! }
+
+        expect(messages.length).to eq(3)
+        expect(messages.map(&:data)).to match_array(
+          [
+            { chat_channel_id: category_channel1.id, memberships_count: 2 },
+            { chat_channel_id: category_channel2.id, memberships_count: 3 },
+            { chat_channel_id: dm_channel1.id, memberships_count: 2 },
+          ],
+        )
+
+        messages = MessageBus.track_publish { described_class.ensure_consistency! }
+        expect(messages.length).to eq(0)
+      end
+    end
   end
 
   describe "#allow_channel_wide_mentions" do

plugins/chat/spec/plugin_spec.rb

@@ -170,7 +170,7 @@
         user_2.user_chat_channel_memberships.create!(chat_channel: chat_channel, following: true)
         user_3.user_chat_channel_memberships.create!(chat_channel: chat_channel, following: true)
         user_4.user_chat_channel_memberships.create!(chat_channel: chat_channel, following: true)
-        Jobs::Chat::UpdateUserCountsForChannels.new.execute({})
+        Chat::Channel.ensure_consistency!
 
         expect(Oneboxer.preview(chat_url)).to match_html <<~HTML
           <aside class="onebox chat-onebox">