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 | 135 +++++++++++++++--- plugins/chat/spec/plugin_spec.rb | 2 +- 5 files changed, 161 insertions(+), 123 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 5dff311bd63c4..0000000000000 --- 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 76af00560cca6..c9c4d3dffc2a9 100644 --- a/plugins/chat/app/models/chat/channel.rb +++ b/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) 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 67cdbe6228efe..0000000000000 --- 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 efd0a835df76b..748f761b93b15 100644 --- a/plugins/chat/spec/models/chat/channel_spec.rb +++ b/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 diff --git a/plugins/chat/spec/plugin_spec.rb b/plugins/chat/spec/plugin_spec.rb index 821ea4385bc3d..83db7860bf535 100644 --- a/plugins/chat/spec/plugin_spec.rb +++ b/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