diff --git a/plugins/chat/app/controllers/chat/chat_controller.rb b/plugins/chat/app/controllers/chat/chat_controller.rb index 32e2e7f703e..0a5ec268eb8 100644 --- a/plugins/chat/app/controllers/chat/chat_controller.rb +++ b/plugins/chat/app/controllers/chat/chat_controller.rb @@ -111,9 +111,11 @@ module Chat return render_json_error(chat_message_creator.error) if chat_message_creator.failed? - @user_chat_channel_membership.update!( - last_read_message_id: chat_message_creator.chat_message.id, - ) + if !chat_message_creator.chat_message.thread_id.present? + @user_chat_channel_membership.update!( + last_read_message_id: chat_message_creator.chat_message.id, + ) + end if @chat_channel.direct_message_channel? # If any of the channel users is ignoring, muting, or preventing DMs from @@ -141,7 +143,13 @@ module Chat Chat::Publisher.publish_user_tracking_state( current_user, @chat_channel.id, - chat_message_creator.chat_message.id, + ( + if chat_message_creator.chat_message.thread_id.present? + @user_chat_channel_membership.last_read_message_id + else + chat_message_creator.chat_message.id + end + ), ) render json: success_json end @@ -263,11 +271,8 @@ module Chat can_load_more_past = past_messages.count == PAST_MESSAGE_LIMIT can_load_more_future = future_messages.count == FUTURE_MESSAGE_LIMIT - messages = [ - past_messages.reverse, - (!include_thread_messages? && @message.in_thread?) ? [] : [@message], - future_messages, - ].reduce([], :concat) + looked_up_message = !include_thread_messages? && @message.thread_reply? ? [] : [@message] + messages = [past_messages.reverse, looked_up_message, future_messages].reduce([], :concat) chat_view = Chat::View.new( chat_channel: @chat_channel, diff --git a/plugins/chat/app/queries/chat/channel_unreads_query.rb b/plugins/chat/app/queries/chat/channel_unreads_query.rb index 20f046367d3..90184ad8736 100644 --- a/plugins/chat/app/queries/chat/channel_unreads_query.rb +++ b/plugins/chat/app/queries/chat/channel_unreads_query.rb @@ -1,36 +1,58 @@ # frozen_string_literal: true module Chat + ## + # Handles counting unread messages and mentions for a list of channels. + # This is used for unread indicators in the chat UI. By default only the + # channels that the user is a member of will be counted and returned in + # the result. class ChannelUnreadsQuery - def self.call(channel_ids:, user_id:) + ## + # @param channel_ids [Array] The IDs of the channels to count. + # @param user_id [Integer] The ID of the user to count for. + # @param include_no_membership_channels [Boolean] Whether to include channels + # that the user is not a member of. These counts will always be 0. + def self.call(channel_ids:, user_id:, include_no_membership_channels: false) sql = <<~SQL - SELECT ( - SELECT COUNT(*) AS unread_count - FROM chat_messages - INNER JOIN chat_channels ON chat_channels.id = chat_messages.chat_channel_id - INNER JOIN user_chat_channel_memberships ON user_chat_channel_memberships.chat_channel_id = chat_channels.id - WHERE chat_channels.id = memberships.chat_channel_id - AND chat_messages.user_id != :user_id - AND user_chat_channel_memberships.user_id = :user_id - AND chat_messages.id > COALESCE(user_chat_channel_memberships.last_read_message_id, 0) - AND chat_messages.deleted_at IS NULL - ) AS unread_count, - ( - SELECT COUNT(*) AS mention_count - FROM notifications - INNER JOIN user_chat_channel_memberships ON user_chat_channel_memberships.user_id = :user_id - WHERE NOT read - AND user_chat_channel_memberships.chat_channel_id = memberships.chat_channel_id - AND notifications.user_id = :user_id - AND notifications.notification_type = :notification_type - AND (data::json->>'chat_message_id')::bigint > COALESCE(user_chat_channel_memberships.last_read_message_id, 0) - AND (data::json->>'chat_channel_id')::bigint = memberships.chat_channel_id - ) AS mention_count, - memberships.chat_channel_id AS channel_id - FROM user_chat_channel_memberships AS memberships - WHERE memberships.user_id = :user_id AND memberships.chat_channel_id IN (:channel_ids) - GROUP BY memberships.chat_channel_id - SQL + SELECT ( + SELECT COUNT(*) AS unread_count + FROM chat_messages + INNER JOIN chat_channels ON chat_channels.id = chat_messages.chat_channel_id + INNER JOIN user_chat_channel_memberships ON user_chat_channel_memberships.chat_channel_id = chat_channels.id + LEFT JOIN chat_threads ON chat_threads.id = chat_messages.thread_id + WHERE chat_channels.id = memberships.chat_channel_id + AND chat_messages.user_id != :user_id + AND user_chat_channel_memberships.user_id = :user_id + AND chat_messages.id > COALESCE(user_chat_channel_memberships.last_read_message_id, 0) + AND chat_messages.deleted_at IS NULL + AND (chat_messages.thread_id IS NULL OR chat_messages.id = chat_threads.original_message_id) + ) AS unread_count, + ( + SELECT COUNT(*) AS mention_count + FROM notifications + INNER JOIN user_chat_channel_memberships ON user_chat_channel_memberships.user_id = :user_id + WHERE NOT read + AND user_chat_channel_memberships.chat_channel_id = memberships.chat_channel_id + AND notifications.user_id = :user_id + AND notifications.notification_type = :notification_type + AND (data::json->>'chat_message_id')::bigint > COALESCE(user_chat_channel_memberships.last_read_message_id, 0) + AND (data::json->>'chat_channel_id')::bigint = memberships.chat_channel_id + ) AS mention_count, + memberships.chat_channel_id AS channel_id + FROM user_chat_channel_memberships AS memberships + WHERE memberships.user_id = :user_id AND memberships.chat_channel_id IN (:channel_ids) + GROUP BY memberships.chat_channel_id + SQL + + sql += <<~SQL if include_no_membership_channels + UNION ALL + SELECT 0 AS unread_count, 0 AS mention_count, chat_channels.id AS channel_id + FROM chat_channels + LEFT JOIN user_chat_channel_memberships ON user_chat_channel_memberships.chat_channel_id = chat_channels.id + AND user_chat_channel_memberships.user_id = :user_id + WHERE chat_channels.id IN (:channel_ids) AND user_chat_channel_memberships.id IS NULL + GROUP BY chat_channels.id + SQL DB.query( sql, diff --git a/plugins/chat/app/services/chat/mark_all_user_channels_read.rb b/plugins/chat/app/services/chat/mark_all_user_channels_read.rb index 43bd8a62b3e..c3a67338d85 100644 --- a/plugins/chat/app/services/chat/mark_all_user_channels_read.rb +++ b/plugins/chat/app/services/chat/mark_all_user_channels_read.rb @@ -30,7 +30,9 @@ module Chat ( SELECT chat_messages.chat_channel_id, MAX(chat_messages.id) AS newest_message_id FROM chat_messages + LEFT JOIN chat_threads ON chat_threads.id = chat_messages.thread_id WHERE chat_messages.deleted_at IS NULL + AND (chat_messages.thread_id IS NULL or chat_messages.id = chat_threads.original_message_id) GROUP BY chat_messages.chat_channel_id ) AS subquery WHERE user_chat_channel_memberships.chat_channel_id = subquery.chat_channel_id AND diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-channel-thread-pane-subscriptions-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-channel-thread-pane-subscriptions-manager.js index bee7089edc2..8cd9521e6b7 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-channel-thread-pane-subscriptions-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-channel-thread-pane-subscriptions-manager.js @@ -24,7 +24,8 @@ export default class ChatChannelThreadPaneSubscriptionsManager extends ChatPaneB ); this.messagesManager.addMessages([message]); - // TODO (martin) All the scrolling and new message indicator shenanigans. + // TODO (martin) All the scrolling and new message indicator shenanigans, + // as well as handling marking the thread as read. } // NOTE: noop, there is nothing to do when a thread is created diff --git a/plugins/chat/lib/chat/channel_fetcher.rb b/plugins/chat/lib/chat/channel_fetcher.rb index 852185ef4e0..26c59117702 100644 --- a/plugins/chat/lib/chat/channel_fetcher.rb +++ b/plugins/chat/lib/chat/channel_fetcher.rb @@ -191,43 +191,25 @@ module Chat def self.decorate_memberships_with_tracking_data(guardian, channels, memberships) unread_counts_per_channel = unread_counts(channels, guardian.user.id) - mention_notifications = - Notification.unread.where( - user_id: guardian.user.id, - notification_type: Notification.types[:chat_mention], - ) - mention_notification_data = mention_notifications.map { |m| JSON.parse(m.data) } - channels.each do |channel| membership = memberships.find { |m| m.chat_channel_id == channel.id } if membership - membership.unread_mentions = - mention_notification_data.count do |data| - data["chat_channel_id"] == channel.id && - data["chat_message_id"] > (membership.last_read_message_id || 0) - end + channel_unread_counts = + unread_counts_per_channel.find { |uc| uc.channel_id == channel.id } - membership.unread_count = unread_counts_per_channel[channel.id] if !membership.muted + membership.unread_mentions = channel_unread_counts.mention_count + membership.unread_count = channel_unread_counts.unread_count if !membership.muted end end end def self.unread_counts(channels, user_id) - unread_counts = DB.query_array(<<~SQL, channel_ids: channels.map(&:id), user_id: user_id).to_h - SELECT cc.id, COUNT(*) as count - FROM chat_messages cm - JOIN chat_channels cc ON cc.id = cm.chat_channel_id - JOIN user_chat_channel_memberships uccm ON uccm.chat_channel_id = cc.id - WHERE cc.id IN (:channel_ids) - AND cm.user_id != :user_id - AND uccm.user_id = :user_id - AND cm.id > COALESCE(uccm.last_read_message_id, 0) - AND cm.deleted_at IS NULL - GROUP BY cc.id - SQL - unread_counts.default = 0 - unread_counts + Chat::ChannelUnreadsQuery.call( + channel_ids: channels.map(&:id), + user_id: user_id, + include_no_membership_channels: true, + ) end def self.find_with_access_check(channel_id_or_name, guardian) diff --git a/plugins/chat/spec/lib/chat/channel_fetcher_spec.rb b/plugins/chat/spec/lib/chat/channel_fetcher_spec.rb index d556aa43e84..9a2c32e5476 100644 --- a/plugins/chat/spec/lib/chat/channel_fetcher_spec.rb +++ b/plugins/chat/spec/lib/chat/channel_fetcher_spec.rb @@ -59,14 +59,18 @@ describe Chat::ChannelFetcher do it "returns the correct count" do unread_counts = described_class.unread_counts([category_channel], user1) - expect(unread_counts[category_channel.id]).to eq(2) + expect( + unread_counts.find { |uc| uc.channel_id == category_channel.id }.unread_count, + ).to eq(2) end end context "with no unread messages" do it "returns the correct count" do unread_counts = described_class.unread_counts([category_channel], user1) - expect(unread_counts[category_channel.id]).to eq(0) + expect( + unread_counts.find { |uc| uc.channel_id == category_channel.id }.unread_count, + ).to eq(0) end end @@ -79,7 +83,9 @@ describe Chat::ChannelFetcher do it "returns the correct count" do unread_counts = described_class.unread_counts([category_channel], user1) - expect(unread_counts[category_channel.id]).to eq(0) + expect( + unread_counts.find { |uc| uc.channel_id == category_channel.id }.unread_count, + ).to eq(0) end end end @@ -92,7 +98,9 @@ describe Chat::ChannelFetcher do it "returns the correct count" do unread_counts = described_class.unread_counts([category_channel], user1) - expect(unread_counts[category_channel.id]).to eq(0) + expect( + unread_counts.find { |uc| uc.channel_id == category_channel.id }.unread_count, + ).to eq(0) end end end diff --git a/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb b/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb index f8ac2ffb27f..a1e2f7eef6e 100644 --- a/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb +++ b/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb @@ -21,14 +21,35 @@ describe Chat::ChannelUnreadsQuery do ).to eq({ mention_count: 0, unread_count: 1, channel_id: channel_1.id }) end + context "for unread messages in a thread" do + fab!(:thread_om) { Fabricate(:chat_message, chat_channel: channel_1) } + fab!(:thread) { Fabricate(:chat_thread, channel: channel_1, original_message: thread_om) } + + it "does include the original message in the unread count" do + expect( + described_class.call(channel_ids: [channel_1.id], user_id: current_user.id).first.to_h, + ).to eq({ mention_count: 0, unread_count: 2, channel_id: channel_1.id }) + end + + it "does not include other thread messages in the unread count" do + Fabricate(:chat_message, chat_channel: channel_1, thread: thread) + Fabricate(:chat_message, chat_channel: channel_1, thread: thread) + expect( + described_class.call(channel_ids: [channel_1.id], user_id: current_user.id).first.to_h, + ).to eq({ mention_count: 0, unread_count: 2, channel_id: channel_1.id }) + end + end + context "for multiple channels" do fab!(:channel_2) { Fabricate(:category_channel) } - it "returns accurate counts" do + before do channel_2.add(current_user) Fabricate(:chat_message, chat_channel: channel_2) Fabricate(:chat_message, chat_channel: channel_2) + end + it "returns accurate counts" do expect( described_class.call( channel_ids: [channel_1.id, channel_2.id], @@ -41,6 +62,41 @@ describe Chat::ChannelUnreadsQuery do ], ) end + + context "for channels where the user has no membership" do + before do + current_user + .user_chat_channel_memberships + .where(chat_channel_id: channel_2.id) + .destroy_all + end + + it "does not return counts for the channels" do + expect( + described_class.call( + channel_ids: [channel_1.id, channel_2.id], + user_id: current_user.id, + ).map(&:to_h), + ).to match_array([{ mention_count: 0, unread_count: 1, channel_id: channel_1.id }]) + end + + context "when include_no_membership_channels is true" do + it "does return zeroed counts for the channels" do + expect( + described_class.call( + channel_ids: [channel_1.id, channel_2.id], + user_id: current_user.id, + include_no_membership_channels: true, + ).map(&:to_h), + ).to match_array( + [ + { mention_count: 0, unread_count: 1, channel_id: channel_1.id }, + { mention_count: 0, unread_count: 0, channel_id: channel_2.id }, + ], + ) + end + end + end end end diff --git a/plugins/chat/spec/requests/chat_controller_spec.rb b/plugins/chat/spec/requests/chat_controller_spec.rb index e0e46f68d3b..573474f8a5d 100644 --- a/plugins/chat/spec/requests/chat_controller_spec.rb +++ b/plugins/chat/spec/requests/chat_controller_spec.rb @@ -347,19 +347,72 @@ RSpec.describe Chat::ChatController do ) end - it "sends a message for regular user when staff-only is disabled and they are following channel" do - sign_in(user) - Chat::UserChatChannelMembership.create( - user: user, - chat_channel: chat_channel, - following: true, - ) + context "when the regular user is following the channel" do + fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel) } + fab!(:membership) do + Chat::UserChatChannelMembership.create( + user: user, + chat_channel: chat_channel, + following: true, + last_read_message_id: message_1.id, + ) + end - expect { post "/chat/#{chat_channel.id}.json", params: { message: message } }.to change { - Chat::Message.count - }.by(1) - expect(response.status).to eq(200) - expect(Chat::Message.last.message).to eq(message) + it "sends a message for regular user when staff-only is disabled and they are following channel" do + sign_in(user) + + expect { post "/chat/#{chat_channel.id}.json", params: { message: message } }.to change { + Chat::Message.count + }.by(1) + expect(response.status).to eq(200) + expect(Chat::Message.last.message).to eq(message) + end + + it "updates the last_read_message_id for the user who sent the message" do + sign_in(user) + post "/chat/#{chat_channel.id}.json", params: { message: message } + expect(response.status).to eq(200) + expect(membership.reload.last_read_message_id).to eq(Chat::Message.last.id) + end + + it "publishes user tracking state using the new chat message as the last_read_message_id" do + sign_in(user) + messages = + MessageBus.track_publish( + Chat::Publisher.user_tracking_state_message_bus_channel(user.id), + ) { post "/chat/#{chat_channel.id}.json", params: { message: message } } + expect(response.status).to eq(200) + expect(messages.first.data["last_read_message_id"]).to eq(Chat::Message.last.id) + end + + context "when sending a message in a thread" do + fab!(:thread) do + Fabricate(:chat_thread, channel: chat_channel, original_message: message_1) + end + + it "does not update the last_read_message_id for the user who sent the message" do + sign_in(user) + post "/chat/#{chat_channel.id}.json", params: { message: message, thread_id: thread.id } + expect(response.status).to eq(200) + expect(membership.reload.last_read_message_id).to eq(message_1.id) + end + + it "publishes user tracking state using the old membership last_read_message_id" do + sign_in(user) + messages = + MessageBus.track_publish( + Chat::Publisher.user_tracking_state_message_bus_channel(user.id), + ) do + post "/chat/#{chat_channel.id}.json", + params: { + message: message, + thread_id: thread.id, + } + end + expect(response.status).to eq(200) + expect(messages.first.data["last_read_message_id"]).to eq(message_1.id) + end + end end end diff --git a/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb b/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb index 1af1a5f61bd..029f0baa145 100644 --- a/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb +++ b/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb @@ -154,6 +154,23 @@ RSpec.describe Chat::MarkAllUserChannelsRead do }, ) end + + context "for threads" do + fab!(:thread) { Fabricate(:chat_thread, channel: channel_1, original_message: message_2) } + + it "does not use thread replies for last_read_message_id" do + Fabricate(:chat_message, chat_channel: channel_1, user: other_user, thread: thread) + result + expect(membership_1.reload.last_read_message_id).to eq(message_2.id) + end + + it "does use thread original messages for last_read_message_id" do + new_om = Fabricate(:chat_message, chat_channel: channel_1, user: other_user) + thread.update!(original_message: new_om, original_message_user: other_user) + result + expect(membership_1.reload.last_read_message_id).to eq(new_om.id) + end + end end end end diff --git a/plugins/chat/spec/system/channel_thread_message_echoing_spec.rb b/plugins/chat/spec/system/channel_thread_message_echoing_spec.rb index 28f00751a22..1e3f4274045 100644 --- a/plugins/chat/spec/system/channel_thread_message_echoing_spec.rb +++ b/plugins/chat/spec/system/channel_thread_message_echoing_spec.rb @@ -85,5 +85,17 @@ describe "Channel thread message echoing", type: :system, js: true do channel_page.message_by_id_selector(thread.replies.last.id), ) end + + it "does show the thread original_message if it is the last message in the channel" do + new_thread = Fabricate(:chat_thread, channel: channel) + current_user + .user_chat_channel_memberships + .find_by(chat_channel: channel) + .update!(last_read_message_id: new_thread.original_message_id) + chat_page.visit_channel(channel) + expect(channel_page).to have_css( + channel_page.message_by_id_selector(new_thread.original_message_id), + ) + end end end diff --git a/plugins/chat/spec/system/single_thread_spec.rb b/plugins/chat/spec/system/single_thread_spec.rb index 561423a0904..8abb164e8d2 100644 --- a/plugins/chat/spec/system/single_thread_spec.rb +++ b/plugins/chat/spec/system/single_thread_spec.rb @@ -143,6 +143,22 @@ describe "Single thread in side panel", type: :system, js: true do expect(open_thread).to have_message(thread.id, text: "this is a test message") end end + + it "does not mark the channel unread if another user sends a message in the thread" do + other_user = Fabricate(:user) + chat_system_user_bootstrap(user: other_user, channel: channel) + Chat::MessageCreator.create( + chat_channel: channel, + user: other_user, + content: "Hello world!", + thread_id: thread.id, + ) + sign_in(current_user) + chat_page.visit_channel(channel) + expect(page).not_to have_css( + ".sidebar-section-link.channel-#{channel.id} .sidebar-section-link-suffix.unread", + ) + end end context "when using mobile" do