diff --git a/plugins/chat/app/jobs/scheduled/chat_periodical_updates.rb b/plugins/chat/app/jobs/scheduled/chat_periodical_updates.rb new file mode 100644 index 00000000000..c7ca56fcb15 --- /dev/null +++ b/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 diff --git a/plugins/chat/app/jobs/scheduled/update_user_counts_for_chat_channels.rb b/plugins/chat/app/jobs/scheduled/update_user_counts_for_chat_channels.rb index 968982819f2..8880732b8e5 100644 --- a/plugins/chat/app/jobs/scheduled/update_user_counts_for_chat_channels.rb +++ b/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 diff --git a/plugins/chat/app/models/chat_channel.rb b/plugins/chat/app/models/chat_channel.rb index b87cf6c9709..7f537161ee4 100644 --- a/plugins/chat/app/models/chat_channel.rb +++ b/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 diff --git a/plugins/chat/db/migrate/20221202032006_add_chat_message_count_to_chat_channels.rb b/plugins/chat/db/migrate/20221202032006_add_chat_message_count_to_chat_channels.rb new file mode 100644 index 00000000000..7eb8f4c8dad --- /dev/null +++ b/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 diff --git a/plugins/chat/db/post_migrate/20221202043755_update_chat_channel_message_counts.rb b/plugins/chat/db/post_migrate/20221202043755_update_chat_channel_message_counts.rb new file mode 100644 index 00000000000..bc8c49ae317 --- /dev/null +++ b/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 diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb index c2f7c56ee44..b4fbd64ebe2 100644 --- a/plugins/chat/plugin.rb +++ b/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__) diff --git a/plugins/chat/spec/jobs/chat_periodical_updates_spec.rb b/plugins/chat/spec/jobs/chat_periodical_updates_spec.rb new file mode 100644 index 00000000000..113a58229ce --- /dev/null +++ b/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 diff --git a/plugins/chat/spec/models/chat_channel_spec.rb b/plugins/chat/spec/models/chat_channel_spec.rb index 3e1d6ec3529..06d12125106 100644 --- a/plugins/chat/spec/models/chat_channel_spec.rb +++ b/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