diff --git a/plugins/chat/app/jobs/scheduled/chat/periodical_updates.rb b/plugins/chat/app/jobs/scheduled/chat/periodical_updates.rb index 7b6c1f3318e..e2faabe637c 100644 --- a/plugins/chat/app/jobs/scheduled/chat/periodical_updates.rb +++ b/plugins/chat/app/jobs/scheduled/chat/periodical_updates.rb @@ -9,6 +9,7 @@ module Jobs # TODO: Add rebaking of old messages (baked_version < # Chat::Message::BAKED_VERSION or baked_version IS NULL) ::Chat::Channel.ensure_consistency! + ::Chat::Thread.ensure_consistency! nil end end diff --git a/plugins/chat/app/models/chat/channel.rb b/plugins/chat/app/models/chat/channel.rb index c5c67d23d95..6d9067af40a 100644 --- a/plugins/chat/app/models/chat/channel.rb +++ b/plugins/chat/app/models/chat/channel.rb @@ -165,32 +165,33 @@ end # # Table name: chat_channels # -# id :bigint not null, primary key -# chatable_id :integer not null -# deleted_at :datetime -# deleted_by_id :integer -# featured_in_category_id :integer -# delete_after_seconds :integer -# chatable_type :string not null -# created_at :datetime not null -# updated_at :datetime not null -# name :string -# description :text -# status :integer default("open"), not null -# user_count :integer default(0), not null -# last_message_sent_at :datetime not null -# auto_join_users :boolean default(FALSE), not null -# allow_channel_wide_mentions :boolean default(TRUE), not null -# user_count_stale :boolean default(FALSE), not null -# slug :string -# type :string -# threading_enabled :boolean default(FALSE), not null +# id :bigint not null, primary key +# chatable_id :integer not null +# deleted_at :datetime +# deleted_by_id :integer +# featured_in_category_id :integer +# delete_after_seconds :integer +# chatable_type :string not null +# created_at :datetime not null +# updated_at :datetime not null +# name :string +# description :text +# status :integer default("open"), not null +# user_count :integer default(0), not null +# last_message_sent_at :datetime not null +# auto_join_users :boolean default(FALSE), not null +# user_count_stale :boolean default(FALSE), not null +# slug :string +# type :string +# allow_channel_wide_mentions :boolean default(TRUE), not null +# messages_count :integer default(0), not null +# threading_enabled :boolean default(FALSE), not null # # 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_messages_count (messages_count) # index_chat_channels_on_slug (slug) UNIQUE # index_chat_channels_on_status (status) # diff --git a/plugins/chat/app/models/chat/message.rb b/plugins/chat/app/models/chat/message.rb index 18339a65be7..2a1640570d5 100644 --- a/plugins/chat/app/models/chat/message.rb +++ b/plugins/chat/app/models/chat/message.rb @@ -348,6 +348,7 @@ end # Indexes # # idx_chat_messages_by_created_at_not_deleted (created_at) WHERE (deleted_at IS NULL) +# idx_chat_messages_by_thread_id_not_deleted (thread_id) WHERE (deleted_at IS NULL) # index_chat_messages_on_chat_channel_id_and_created_at (chat_channel_id,created_at) # index_chat_messages_on_chat_channel_id_and_id (chat_channel_id,id) WHERE (deleted_at IS NULL) # index_chat_messages_on_last_editor_id (last_editor_id) diff --git a/plugins/chat/app/models/chat/thread.rb b/plugins/chat/app/models/chat/thread.rb index 25fb68f45b4..f78fc05f02e 100644 --- a/plugins/chat/app/models/chat/thread.rb +++ b/plugins/chat/app/models/chat/thread.rb @@ -18,6 +18,10 @@ module Chat enum :status, { open: 0, read_only: 1, closed: 2, archived: 3 }, scopes: false + def replies + self.chat_messages.where.not(id: self.original_message_id) + end + def url "#{channel.url}/t/#{self.id}" end @@ -29,6 +33,32 @@ module Chat def excerpt original_message.excerpt(max_length: EXCERPT_LENGTH) end + + def self.ensure_consistency! + update_counts + end + + def self.update_counts + # NOTE: Chat::Thread#replies_count is not updated every time + # a message is created or deleted in a channel, the UI will lag + # behind unless it is kept in sync with MessageBus. The count + # has 1 subtracted from it to account for the original message. + # + # It is updated eventually via Jobs::Chat::PeriodicalUpdates. In + # future we may want to update this more frequently. + DB.exec <<~SQL + UPDATE chat_threads threads + SET replies_count = subquery.replies_count + FROM ( + SELECT COUNT(*) - 1 AS replies_count, thread_id + FROM chat_messages + WHERE chat_messages.deleted_at IS NULL AND thread_id IS NOT NULL + GROUP BY thread_id + ) subquery + WHERE threads.id = subquery.thread_id + AND subquery.replies_count != threads.replies_count + SQL + end end end @@ -44,6 +74,7 @@ end # title :string # created_at :datetime not null # updated_at :datetime not null +# replies_count :integer default(0), not null # # Indexes # @@ -51,5 +82,6 @@ end # index_chat_threads_on_channel_id_and_status (channel_id,status) # index_chat_threads_on_original_message_id (original_message_id) # index_chat_threads_on_original_message_user_id (original_message_user_id) +# index_chat_threads_on_replies_count (replies_count) # index_chat_threads_on_status (status) # diff --git a/plugins/chat/db/migrate/20230411012630_add_thread_not_deleted_index_chat_messages.rb b/plugins/chat/db/migrate/20230411012630_add_thread_not_deleted_index_chat_messages.rb new file mode 100644 index 00000000000..3596ea46a09 --- /dev/null +++ b/plugins/chat/db/migrate/20230411012630_add_thread_not_deleted_index_chat_messages.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class AddThreadNotDeletedIndexChatMessages < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + + def change + execute <<~SQL + DROP INDEX IF EXISTS idx_chat_messages_by_thread_id_not_deleted + SQL + + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + idx_chat_messages_by_thread_id_not_deleted + ON chat_messages (thread_id) + WHERE deleted_at IS NULL + SQL + end + + def down + execute <<~SQL + DROP INDEX IF EXISTS idx_chat_messages_by_thread_id_not_deleted + SQL + end +end diff --git a/plugins/chat/db/migrate/20230411023246_add_chat_message_replies_count_to_chat_threads.rb b/plugins/chat/db/migrate/20230411023246_add_chat_message_replies_count_to_chat_threads.rb new file mode 100644 index 00000000000..6c2702d2876 --- /dev/null +++ b/plugins/chat/db/migrate/20230411023246_add_chat_message_replies_count_to_chat_threads.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddChatMessageRepliesCountToChatThreads < ActiveRecord::Migration[7.0] + def change + add_column :chat_threads, :replies_count, :integer, null: false, default: 0 + add_index :chat_threads, :replies_count + end +end diff --git a/plugins/chat/db/post_migrate/20230411023340_update_thread_reply_counts.rb b/plugins/chat/db/post_migrate/20230411023340_update_thread_reply_counts.rb new file mode 100644 index 00000000000..b3dc2998e8c --- /dev/null +++ b/plugins/chat/db/post_migrate/20230411023340_update_thread_reply_counts.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class UpdateThreadReplyCounts < ActiveRecord::Migration[7.0] + def up + DB.exec <<~SQL + UPDATE chat_threads threads + SET replies_count = subquery.replies_count + FROM ( + SELECT COUNT(*) - 1 AS replies_count, thread_id + FROM chat_messages + WHERE chat_messages.deleted_at IS NULL AND thread_id IS NOT NULL + GROUP BY thread_id + ) subquery + WHERE threads.id = subquery.thread_id + AND subquery.replies_count != threads.replies_count + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index e66fba77824..e2d89aaf0a2 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/plugins/chat/spec/fabricators/chat_fabricator.rb @@ -147,4 +147,6 @@ Fabricator(:chat_thread, class_name: "Chat::Thread") do original_message do |attrs| Fabricate(:chat_message, chat_channel: attrs[:channel] || Fabricate(:chat_channel)) end + + after_create { |thread| thread.original_message.update!(thread_id: thread.id) } end diff --git a/plugins/chat/spec/models/chat/thread_spec.rb b/plugins/chat/spec/models/chat/thread_spec.rb new file mode 100644 index 00000000000..dbed433a1c3 --- /dev/null +++ b/plugins/chat/spec/models/chat/thread_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +RSpec.describe Chat::Thread do + describe ".ensure_consistency!" do + fab!(:channel) { Fabricate(:category_channel) } + fab!(:thread_1) { Fabricate(:chat_thread, channel: channel) } + fab!(:thread_2) { Fabricate(:chat_thread, channel: channel) } + fab!(:thread_3) { Fabricate(:chat_thread, channel: channel) } + + before do + Fabricate(:chat_message, chat_channel: channel, thread: thread_1) + Fabricate(:chat_message, chat_channel: channel, thread: thread_1) + Fabricate(:chat_message, chat_channel: channel, thread: thread_1) + + Fabricate(:chat_message, chat_channel: channel, thread: thread_2) + Fabricate(:chat_message, chat_channel: channel, thread: thread_2) + Fabricate(:chat_message, chat_channel: channel, thread: thread_2) + Fabricate(:chat_message, chat_channel: channel, thread: thread_2) + + Fabricate(:chat_message, chat_channel: channel, thread: thread_3) + end + + describe "updating replies_count for all threads" do + it "counts correctly and does not include the original message" do + described_class.ensure_consistency! + expect(thread_1.reload.replies_count).to eq(3) + expect(thread_2.reload.replies_count).to eq(4) + expect(thread_3.reload.replies_count).to eq(1) + end + + it "does not count deleted messages" do + thread_1.chat_messages.last.trash! + described_class.ensure_consistency! + expect(thread_1.reload.replies_count).to eq(2) + end + + it "sets the replies count to 0 if all the messages but the original message are deleted" do + thread_1.replies.delete_all + + described_class.ensure_consistency! + expect(thread_1.reload.replies_count).to eq(0) + end + end + end +end