From f6072ba8044fda065810917e396b1288a51435a0 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 3 Jul 2023 11:41:51 +1000 Subject: [PATCH] DEV: Move user count update for channels to ensure_consistency! (#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. --- .../chat/update_user_counts_for_channels.rb | 32 ---- plugins/chat/app/models/chat/channel.rb | 56 +++++-- .../update_user_counts_for_channels_spec.rb | 59 -------- plugins/chat/spec/models/chat/channel_spec.rb | 141 +++++++++++++++--- plugins/chat/spec/plugin_spec.rb | 2 +- 5 files changed, 164 insertions(+), 126 deletions(-) delete mode 100644 plugins/chat/app/jobs/scheduled/chat/update_user_counts_for_channels.rb delete mode 100644 plugins/chat/spec/jobs/scheduled/update_user_counts_for_channels_spec.rb diff --git a/plugins/chat/app/jobs/scheduled/chat/update_user_counts_for_channels.rb b/plugins/chat/app/jobs/scheduled/chat/update_user_counts_for_channels.rb deleted file mode 100644 index 5dff311bd63..00000000000 --- a/plugins/chat/app/jobs/scheduled/chat/update_user_counts_for_channels.rb +++ /dev/null @@ -1,32 +0,0 @@ -# frozen_string_literal: true - -module Jobs - # TODO (martin) Move into Chat::Channel.ensure_consistency! so it - # is run with Jobs::Chat::PeriodicalUpdates - module Chat - class UpdateUserCountsForChannels < ::Jobs::Scheduled - every 1.hour - - # FIXME: This could become huge as the amount of channels grows, we - # need a different approach here. Perhaps we should only bother for - # channels updated or with new messages in the past N days? Perhaps - # we could update all the counts in a single query as well? - def execute(args = {}) - return if !SiteSetting.chat_enabled - - ::Chat::Channel - .where(status: %i[open closed]) - .find_each { |chat_channel| set_user_count(chat_channel) } - end - - def set_user_count(chat_channel) - current_count = chat_channel.user_count || 0 - new_count = ::Chat::ChannelMembershipsQuery.count(chat_channel) - return if current_count == new_count - - chat_channel.update(user_count: new_count, user_count_stale: false) - ::Chat::Publisher.publish_chat_channel_metadata(chat_channel) - end - end - end -end diff --git a/plugins/chat/app/models/chat/channel.rb b/plugins/chat/app/models/chat/channel.rb index 76af00560cc..c9c4d3dffc2 100644 --- a/plugins/chat/app/models/chat/channel.rb +++ b/plugins/chat/app/models/chat/channel.rb @@ -104,27 +104,53 @@ module Chat 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) diff --git a/plugins/chat/spec/jobs/scheduled/update_user_counts_for_channels_spec.rb b/plugins/chat/spec/jobs/scheduled/update_user_counts_for_channels_spec.rb deleted file mode 100644 index 67cdbe6228e..00000000000 --- a/plugins/chat/spec/jobs/scheduled/update_user_counts_for_channels_spec.rb +++ /dev/null @@ -1,59 +0,0 @@ -# frozen_string_literal: true - -require "rails_helper" - -describe Jobs::Chat::UpdateUserCountsForChannels do - fab!(:chat_channel_1) { Fabricate(:category_channel, user_count: 0) } - fab!(:chat_channel_2) { Fabricate(:category_channel, user_count: 0) } - 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: chat_channel_1, following: true) - user_1.user_chat_channel_memberships.create!(chat_channel: chat_channel_2, following: true) - - user_2.user_chat_channel_memberships.create!(chat_channel: chat_channel_1, following: true) - user_2.user_chat_channel_memberships.create!(chat_channel: chat_channel_2, following: true) - - user_3.user_chat_channel_memberships.create!(chat_channel: chat_channel_1, following: false) - user_3.user_chat_channel_memberships.create!(chat_channel: chat_channel_2, following: true) - end - - it "sets the user_count correctly for each chat channel" do - create_memberships - - Jobs::Chat::UpdateUserCountsForChannels.new.execute - - expect(chat_channel_1.reload.user_count).to eq(2) - expect(chat_channel_2.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: chat_channel_1, following: true) - user_2.user_chat_channel_memberships.create!(chat_channel: chat_channel_2, following: true) - user_3.user_chat_channel_memberships.create!(chat_channel: chat_channel_2, following: true) - user_4.user_chat_channel_memberships.create!(chat_channel: chat_channel_2, following: true) - user_2.update(suspended_till: 3.weeks.from_now) - user_3.update(staged: true) - user_4.update(active: false) - - Jobs::Chat::UpdateUserCountsForChannels.new.execute - - expect(chat_channel_1.reload.user_count).to eq(1) - expect(chat_channel_2.reload.user_count).to eq(0) - end - - it "does not count archived, or read_only channels" do - create_memberships - - chat_channel_1.update!(status: :archived) - Jobs::Chat::UpdateUserCountsForChannels.new.execute - expect(chat_channel_1.reload.user_count).to eq(0) - - chat_channel_1.update!(status: :read_only) - Jobs::Chat::UpdateUserCountsForChannels.new.execute - expect(chat_channel_1.reload.user_count).to eq(0) - end -end diff --git a/plugins/chat/spec/models/chat/channel_spec.rb b/plugins/chat/spec/models/chat/channel_spec.rb index efd0a835df7..748f761b93b 100644 --- a/plugins/chat/spec/models/chat/channel_spec.rb +++ b/plugins/chat/spec/models/chat/channel_spec.rb @@ -25,27 +25,28 @@ RSpec.describe Chat::Channel do 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 + 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 + it "counts correctly" do described_class.ensure_consistency! expect(category_channel1.reload.messages_count).to eq(3) @@ -70,6 +71,108 @@ RSpec.describe Chat::Channel do 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 diff --git a/plugins/chat/spec/plugin_spec.rb b/plugins/chat/spec/plugin_spec.rb index 821ea4385bc..83db7860bf5 100644 --- a/plugins/chat/spec/plugin_spec.rb +++ b/plugins/chat/spec/plugin_spec.rb @@ -170,7 +170,7 @@ describe Chat do 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