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