Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FIX: Incorrect unread count shown in channel when message deleted (#21410) 

When we were deleting messages in chat, we would find all of
the UserChatChannelMembership records that had a matching
last_read_message_id and set that column to NULL.

This became an issue when multiple users had that deleted message
set to their last_read_message_id. When we called ChannelUnreadsQuery
to get the unread count for each of the user's channels, we were
COALESCing the last_read_message_id and returning 0 if it was NULL,
which meant that the unread count for the channel would be the total
count of the messages not sent by the user in that channel.

This was particularly noticeable for DM channels since we show
the count with the indicator in the header. This issue would disappear
as soon as the user opened the problem channel, because we would then
set the last_read_message_id to an actual ID.

To circumvent this, instead of NULLifying the last_read_message_id in
most cases, it makes more sense to just set it to the most recent
non-deleted chat message ID for the channel. The only time it will
be set to NULL now is when there are no more other messages in the
channel.
This commit is contained in:
Martin Brennan 2023-05-05 15:28:48 +02:00 committed by GitHub
parent 98ea356fec
commit ae3231e140
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 147 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

46
plugins/chat/app/services/chat/action/reset_user_last_read_channel_message.rb Normal file
View File

@ -0,0 +1,46 @@
# frozen_string_literal: true
module Chat
module Action
class ResetUserLastReadChannelMessage
# @param [Array] last_read_message_ids The message IDs to match with the
# last_read_message_ids in UserChannelMembership which will be reset
# to NULL or the most recent non-deleted message in the channel to
# update read state.
# @param [Integer] channel_ids The channel IDs of the memberships to update,
# this is used to find the latest non-deleted message in the channel.
def self.call(last_read_message_ids, channel_ids)
sql = <<~SQL
-- update the last_read_message_id to the most recent
-- non-deleted message in the channel so unread counts are correct.
-- the cte row_number is necessary to only return a single row
-- for each channel to prevent additional data being returned
WITH cte AS (
SELECT * FROM (
SELECT id, chat_channel_id, row_number() OVER (
PARTITION BY chat_channel_id ORDER BY created_at DESC, id DESC
) AS row_number
FROM chat_messages
WHERE deleted_at IS NULL AND chat_channel_id IN (:channel_ids)
) AS recent_messages
WHERE recent_messages.row_number = 1
)
UPDATE user_chat_channel_memberships
SET last_read_message_id = cte.id
FROM cte
WHERE user_chat_channel_memberships.last_read_message_id IN (:last_read_message_ids)
AND cte.chat_channel_id = user_chat_channel_memberships.chat_channel_id;
-- then reset all last_read_message_ids to null
-- for the cases where all messages in the channel were
-- already deleted
UPDATE user_chat_channel_memberships
SET last_read_message_id = NULL
WHERE last_read_message_id IN (:last_read_message_ids);
SQL
DB.exec(sql, last_read_message_ids: last_read_message_ids, channel_ids: channel_ids)
end
end
end
end

15
plugins/chat/app/services/chat/message_destroyer.rb
View File

@ -6,17 +6,20 @@ module Chat
chat_messages_query
.in_batches(of: batch_size)
.each do |relation|
destroyed_ids = relation.destroy_all.pluck(:id)
reset_last_read(destroyed_ids)
delete_flags(destroyed_ids)
destroyed_ids = relation.destroy_all.pluck(:id, :chat_channel_id)
destroyed_message_ids = destroyed_ids.map(&:first).uniq
destroyed_message_channel_ids = destroyed_ids.map(&:second).uniq
reset_last_read(destroyed_message_ids, destroyed_message_channel_ids)
delete_flags(destroyed_message_ids)
end
end
private
def reset_last_read(message_ids)
Chat::UserChatChannelMembership.where(last_read_message_id: message_ids).update_all(
last_read_message_id: nil,
def reset_last_read(destroyed_message_ids, destroyed_message_channel_ids)
::Chat::Action::ResetUserLastReadChannelMessage.call(
destroyed_message_ids,
destroyed_message_channel_ids,
)
end

4
plugins/chat/app/services/chat/trash_message.rb
View File

@ -58,9 +58,7 @@ module Chat
end
def update_tracking_state(message:, **)
Chat::UserChatChannelMembership.where(last_read_message_id: message.id).update_all(
last_read_message_id: nil,
)
::Chat::Action::ResetUserLastReadChannelMessage.call([message.id], [message.chat_channel_id])
end
def update_thread_reply_cache(message:, **)

49
plugins/chat/spec/services/actions/reset_user_last_read_channel_message_spec.rb Normal file
View File

@ -0,0 +1,49 @@
# frozen_string_literal: true
RSpec.describe Chat::Action::ResetUserLastReadChannelMessage do
fab!(:channel_1) { Fabricate(:chat_channel) }
fab!(:channel_2) { Fabricate(:chat_channel) }
fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, created_at: 1.hour.ago) }
fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel_1, created_at: 2.seconds.ago) }
fab!(:message_3) { Fabricate(:chat_message, chat_channel: channel_1, created_at: 3.minutes.ago) }
fab!(:message_4) { Fabricate(:chat_message, chat_channel: channel_2, created_at: 30.seconds.ago) }
fab!(:message_5) { Fabricate(:chat_message, chat_channel: channel_2, created_at: 3.seconds.ago) }
fab!(:message_6) { Fabricate(:chat_message, chat_channel: channel_2, created_at: 1.day.ago) }
fab!(:membership_1) do
Fabricate(
:user_chat_channel_membership,
chat_channel: channel_1,
last_read_message_id: message_3.id,
)
end
fab!(:membership_2) do
Fabricate(
:user_chat_channel_membership,
chat_channel: channel_2,
last_read_message_id: message_6.id,
)
end
context "when there are non-deleted messages left in the channel" do
before do
message_3.trash!
message_6.trash!
end
it "sets the matching membership last_read_message_ids to the most recently created message ID" do
described_class.call([message_3.id, message_6.id], [channel_1.id, channel_2.id])
expect(membership_1.reload.last_read_message_id).to eq(message_2.id)
expect(membership_2.reload.last_read_message_id).to eq(message_5.id)
end
end
context "when there are no more non-deleted messages left in the channel" do
before { [message_1, message_2, message_4, message_5].each(&:trash!) }
it "sets the matching membership last_read_message_ids to NULL" do
described_class.call([message_3.id, message_6.id], [channel_1.id, channel_2.id])
expect(membership_1.reload.last_read_message_id).to be_nil
expect(membership_2.reload.last_read_message_id).to be_nil
end
end
end

20
plugins/chat/spec/services/chat/message_destroyer_spec.rb
View File

@ -21,6 +21,26 @@ RSpec.describe Chat::MessageDestroyer do
expect(membership.reload.last_read_message_id).to be_nil
end
it "resets last_read_message_id to the most recent non-deleted message if there is one from memberships" do
message_2 = Fabricate(:chat_message, chat_channel: message_1.chat_channel)
message_3 = Fabricate(:chat_message, chat_channel: message_1.chat_channel)
message_4 = Fabricate(:chat_message, chat_channel: message_1.chat_channel)
message_3.trash!
membership =
Chat::UserChatChannelMembership.create!(
user: user_1,
chat_channel: message_1.chat_channel,
last_read_message: message_4,
following: true,
desktop_notification_level: 2,
mobile_notification_level: 2,
)
described_class.new.destroy_in_batches(Chat::Message.where(id: message_4.id))
expect(membership.reload.last_read_message_id).to eq(message_2.id)
end
it "deletes flags associated to deleted chat messages" do
guardian = Guardian.new(Discourse.system_user)
Chat::ReviewQueue.new.flag_message(message_1, guardian, ReviewableScore.types[:off_topic])

25
plugins/chat/spec/services/chat/trash_message_spec.rb
View File

@ -65,7 +65,8 @@ RSpec.describe Chat::TrashMessage do
)
end
it "updates the tracking for users whose last_read_message_id was the trashed message" do
it "updates the tracking to the last non-deleted channel message for users whose last_read_message_id was the trashed message" do
other_message = Fabricate(:chat_message, chat_channel: message.chat_channel)
membership_1 =
Fabricate(
:user_chat_channel_membership,
@ -82,12 +83,30 @@ RSpec.describe Chat::TrashMessage do
Fabricate(
:user_chat_channel_membership,
chat_channel: message.chat_channel,
last_read_message: Fabricate(:chat_message, chat_channel: message.chat_channel),
last_read_message: other_message,
)
result
expect(membership_1.reload.last_read_message_id).to eq(other_message.id)
expect(membership_2.reload.last_read_message_id).to eq(other_message.id)
expect(membership_3.reload.last_read_message_id).to eq(other_message.id)
end
it "updates the tracking to nil when there are no other messages left in the channnel" do
membership_1 =
Fabricate(
:user_chat_channel_membership,
chat_channel: message.chat_channel,
last_read_message: message,
)
membership_2 =
Fabricate(
:user_chat_channel_membership,
chat_channel: message.chat_channel,
last_read_message: message,
)
result
expect(membership_1.reload.last_read_message_id).to be_nil
expect(membership_2.reload.last_read_message_id).to be_nil
expect(membership_3.reload.last_read_message_id).not_to be_nil
end
context "when the message has a thread" do