Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Mark threads read when threading enabled for a channel #22458

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,16 @@
# frozen_string_literal: true

module Jobs
module Chat
class MarkAllChannelThreadsRead < Jobs::Base
sidekiq_options queue: "critical"

def execute(args = {})
return if !SiteSetting.enable_experimental_chat_threaded_discussions
channel = ::Chat::Channel.find_by(id: args[:channel_id])
return if channel.blank?
channel.mark_all_threads_as_read
end
end
end
end
25 changes: 25 additions & 0 deletions plugins/chat/app/models/chat/channel.rb
Expand Up @@ -164,6 +164,31 @@ def latest_not_deleted_message_id(anchor_message_id: nil)
SQL
end

def mark_all_threads_as_read(user: nil)
martin-brennan marked this conversation as resolved.
Show resolved Hide resolved
if !(self.threading_enabled || SiteSetting.enable_experimental_chat_threaded_discussions)
return
end

DB.exec(<<~SQL, channel_id: self.id)
UPDATE user_chat_thread_memberships
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm not familiar with the code here but when threading is enabled, do all users have user_chat_thread_membership records for all threads?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No they don't only for threads they have replied to or manually tracked (before threading is enabled this is any message they replied to), so this can potentially be a lot or only a few. Should be pretty fast though in SQL only, when I did the migration to create user_chat_thread_memberships for old records even on dev it was fast.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O so user_chat_thread_memberships records are already being created even when threading is disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and threads as well. We've been doing that in the background for months so that when people turn threading on they automatically have existing threads (especially for reply chains where multiple users replied in a chain back to an original message).

SET last_read_message_id = subquery.last_message_id
FROM (
SELECT chat_threads.id AS thread_id, MAX(chat_messages.id) AS last_message_id
FROM chat_threads
INNER JOIN chat_messages ON chat_messages.thread_id = chat_threads.id
WHERE chat_threads.channel_id = :channel_id
AND chat_messages.deleted_at IS NULL
GROUP BY chat_threads.id
) subquery
WHERE user_chat_thread_memberships.thread_id = subquery.thread_id
#{user ? "AND user_chat_thread_memberships.user_id = #{user.id}" : ""}
AND (
user_chat_thread_memberships.last_read_message_id < subquery.last_message_id OR
user_chat_thread_memberships.last_read_message_id IS NULL
)
SQL
end

private

def change_status(acting_user, target_status)
Expand Down
12 changes: 10 additions & 2 deletions plugins/chat/app/services/chat/update_channel.rb
Expand Up @@ -13,7 +13,7 @@ module Chat
# name: "SuperChannel",
# description: "This is the best channel",
# slug: "super-channel",
# threading_enaled: true,
# threading_enabled: true,
# )
#
class UpdateChannel
Expand All @@ -36,6 +36,7 @@ class UpdateChannel
policy :check_channel_permission
contract default_values_from: :channel
step :update_channel
step :mark_all_threads_as_read_if_needed
step :publish_channel_update
step :auto_join_users_if_needed

Expand Down Expand Up @@ -70,7 +71,14 @@ def check_channel_permission(guardian:, channel:, **)
end

def update_channel(channel:, contract:, **)
channel.update!(contract.attributes)
channel.assign_attributes(contract.attributes)
context.threading_enabled_changed = channel.threading_enabled_changed?
channel.save!
end

def mark_all_threads_as_read_if_needed(channel:, **)
return if !(context.threading_enabled_changed && channel.threading_enabled)
Jobs.enqueue(Jobs::Chat::MarkAllChannelThreadsRead, channel_id: channel.id)
end

def publish_channel_update(channel:, guardian:, **)
Expand Down
@@ -0,0 +1,46 @@
# frozen_string_literal: true

RSpec.describe Jobs::Chat::MarkAllChannelThreadsRead do
fab!(:channel) { Fabricate(:chat_channel, threading_enabled: true) }

context "when enable_experimental_chat_threaded_discussions is false" do
before { SiteSetting.enable_experimental_chat_threaded_discussions = false }

it "does nothing" do
Chat::Channel.any_instance.expects(:mark_all_threads_as_read).never
described_class.new.execute(channel_id: channel.id)
end
end

context "when enable_experimental_chat_threaded_discussions is true" do
fab!(:thread_1) { Fabricate(:chat_thread, channel: channel) }
fab!(:thread_2) { Fabricate(:chat_thread, channel: channel) }
fab!(:user_1) { Fabricate(:user) }
fab!(:user_2) { Fabricate(:user) }
fab!(:thread_1_message_1) { Fabricate(:chat_message, thread: thread_1) }
fab!(:thread_1_message_2) { Fabricate(:chat_message, thread: thread_1) }
fab!(:thread_1_message_3) { Fabricate(:chat_message, thread: thread_1) }
fab!(:thread_2_message_1) { Fabricate(:chat_message, thread: thread_2) }
fab!(:thread_2_message_2) { Fabricate(:chat_message, thread: thread_2) }

before do
SiteSetting.enable_experimental_chat_threaded_discussions = true
channel.add(user_1)
channel.add(user_2)
thread_1.add(user_1)
thread_2.add(user_2)
end

def unread_count(user)
Chat::ThreadUnreadsQuery.call(channel_ids: [channel.id], user_id: user.id).first.unread_count
end

it "marks all threads as read across all users in the channel" do
martin-brennan marked this conversation as resolved.
Show resolved Hide resolved
expect(unread_count(user_1)).to eq(3)
expect(unread_count(user_2)).to eq(2)
described_class.new.execute(channel_id: channel.id)
expect(unread_count(user_1)).to eq(0)
expect(unread_count(user_2)).to eq(0)
end
end
end
34 changes: 25 additions & 9 deletions plugins/chat/spec/services/chat/update_channel_spec.rb
Expand Up @@ -113,22 +113,38 @@

describe "threading_enabled" do
context "when true" do
before { params[:threading_enabled] = true }

it "changes the value to true" do
expect {
params[:threading_enabled] = true
result
}.to change { channel.reload.threading_enabled }.from(false).to(true)
expect { result }.to change { channel.reload.threading_enabled }.from(false).to(true)
end

it "enqueues a job to mark all threads in the channel as read" do
expect_enqueued_with(
job: Jobs::Chat::MarkAllChannelThreadsRead,
args: {
channel_id: channel.id,
},
) { result }
end
end

context "when false" do
it "changes the value to true" do
before { params[:threading_enabled] = false }

it "changes the value to false" do
channel.update!(threading_enabled: true)

expect {
params[:threading_enabled] = false
result
}.to change { channel.reload.threading_enabled }.from(true).to(false)
expect { result }.to change { channel.reload.threading_enabled }.from(true).to(false)
end

it "does not enqueue a job to mark all threads in the channel as read" do
expect_not_enqueued_with(
job: Jobs::Chat::MarkAllChannelThreadsRead,
args: {
channel_id: channel.id,
},
) { result }
end
end
end
Expand Down