diff --git a/plugins/chat/app/models/chat/channel.rb b/plugins/chat/app/models/chat/channel.rb index fc52dce5123..1b6f69f7c99 100644 --- a/plugins/chat/app/models/chat/channel.rb +++ b/plugins/chat/app/models/chat/channel.rb @@ -5,6 +5,9 @@ module Chat include Trashable include TypeMappable + # TODO (martin) Remove once we are using last_message instead, + # should be around August 2023. + self.ignored_columns = %w[last_message_sent_at] self.table_name = "chat_channels" belongs_to :chatable, polymorphic: true @@ -20,6 +23,10 @@ module Chat foreign_key: :chat_channel_id has_many :threads, class_name: "Chat::Thread", foreign_key: :channel_id has_one :chat_channel_archive, class_name: "Chat::ChannelArchive", foreign_key: :chat_channel_id + belongs_to :last_message, + class_name: "Chat::Message", + foreign_key: :last_message_id, + optional: true enum :status, { open: 0, read_only: 1, closed: 2, archived: 3 }, scopes: false @@ -103,6 +110,10 @@ module Chat "#{Discourse.base_path}/chat/c/#{self.slug || "-"}/#{self.id}" end + def update_last_message_id! + self.update!(last_message_id: self.latest_not_deleted_message_id) + end + def self.ensure_consistency! update_message_counts update_user_counts @@ -155,11 +166,15 @@ module Chat def latest_not_deleted_message_id(anchor_message_id: nil) DB.query_single(<<~SQL, channel_id: self.id, anchor_message_id: anchor_message_id).first - SELECT id FROM chat_messages + SELECT chat_messages.id + FROM chat_messages + LEFT JOIN chat_threads original_message_threads ON original_message_threads.original_message_id = chat_messages.id WHERE chat_channel_id = :channel_id AND deleted_at IS NULL - #{anchor_message_id ? "AND id < :anchor_message_id" : ""} - ORDER BY created_at DESC, id DESC + -- this is so only the original message of a thread is counted not all thread replies + AND (chat_messages.thread_id IS NULL OR original_message_threads.id IS NOT NULL) + #{anchor_message_id ? "AND chat_messages.id < :anchor_message_id" : ""} + ORDER BY chat_messages.created_at DESC, chat_messages.id DESC LIMIT 1 SQL end @@ -171,19 +186,12 @@ module Chat DB.exec(<<~SQL, channel_id: self.id) UPDATE user_chat_thread_memberships - 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 + SET last_read_message_id = chat_threads.last_message_id + FROM chat_threads + WHERE user_chat_thread_memberships.thread_id = chat_threads.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 < chat_threads.last_message_id OR user_chat_thread_memberships.last_read_message_id IS NULL ) SQL @@ -249,11 +257,13 @@ end # allow_channel_wide_mentions :boolean default(TRUE), not null # messages_count :integer default(0), not null # threading_enabled :boolean default(FALSE), not null +# last_message_id :bigint # # Indexes # # 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_last_message_id (last_message_id) # 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/thread.rb b/plugins/chat/app/models/chat/thread.rb index a880f17508e..5372ef880c9 100644 --- a/plugins/chat/app/models/chat/thread.rb +++ b/plugins/chat/app/models/chat/thread.rb @@ -23,6 +23,10 @@ module Chat primary_key: :id, class_name: "Chat::Message" has_many :user_chat_thread_memberships + belongs_to :last_message, + class_name: "Chat::Message", + foreign_key: :last_message_id, + optional: true enum :status, { open: 0, read_only: 1, closed: 2, archived: 3 }, scopes: false @@ -30,18 +34,8 @@ module Chat # Since the `replies` for the thread can all be deleted, to avoid errors # in lists and previews of the thread, we can consider the original message - # as the last "reply" in this case, so we don't exclude that here. - # - # This is a manual getter/setter so we can avoid N1 queries. This used to be - # a has_one relationship on the model, but that has some awkward behaviour - # and still caused N1s, and ordering was not applied in complex AR queries. - def last_reply - @last_reply ||= self.chat_messages.reorder("created_at DESC, id DESC").first - end - - def last_reply=(message) - @last_reply = message - end + # as the last message in this case as a fallback. + before_create { self.last_message_id = self.original_message_id } def add(user) Chat::UserChatThreadMembership.find_or_create_by!(user: user, thread: self) @@ -71,6 +65,10 @@ module Chat original_message.excerpt(max_length: EXCERPT_LENGTH) end + def update_last_message_id! + self.update!(last_message_id: self.latest_not_deleted_message_id) + end + def latest_not_deleted_message_id(anchor_message_id: nil) DB.query_single( <<~SQL, @@ -148,11 +146,13 @@ end # created_at :datetime not null # updated_at :datetime not null # replies_count :integer default(0), not null +# last_message_id :bigint # # Indexes # # index_chat_threads_on_channel_id (channel_id) # index_chat_threads_on_channel_id_and_status (channel_id,status) +# index_chat_threads_on_last_message_id (last_message_id) # 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) diff --git a/plugins/chat/app/serializers/chat/channel_serializer.rb b/plugins/chat/app/serializers/chat/channel_serializer.rb index cdde98af2d9..6664410ec85 100644 --- a/plugins/chat/app/serializers/chat/channel_serializer.rb +++ b/plugins/chat/app/serializers/chat/channel_serializer.rb @@ -12,7 +12,6 @@ module Chat :description, :title, :slug, - :last_message_sent_at, :status, :archive_failed, :archive_completed, @@ -24,6 +23,8 @@ module Chat :meta, :threading_enabled + has_one :last_message, serializer: Chat::LastMessageSerializer, embed: :objects + def threading_enabled SiteSetting.enable_experimental_chat_threaded_discussions && object.threading_enabled end diff --git a/plugins/chat/app/serializers/chat/last_message_serializer.rb b/plugins/chat/app/serializers/chat/last_message_serializer.rb new file mode 100644 index 00000000000..3412b766e94 --- /dev/null +++ b/plugins/chat/app/serializers/chat/last_message_serializer.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Chat + class LastMessageSerializer < ::ApplicationSerializer + # NOTE: The channel last message does not need to serialize relations + # etc. at this point in time, since the only thing we are using is + # created_at. In future we may want to serialize more for this, at which + # point we need to check existing code so we don't introduce N1s. + attributes *Chat::MessageSerializer::BASIC_ATTRIBUTES + end +end diff --git a/plugins/chat/app/serializers/chat/message_serializer.rb b/plugins/chat/app/serializers/chat/message_serializer.rb index 0d9b5d544df..111bb10f249 100644 --- a/plugins/chat/app/serializers/chat/message_serializer.rb +++ b/plugins/chat/app/serializers/chat/message_serializer.rb @@ -2,22 +2,31 @@ module Chat class MessageSerializer < ::ApplicationSerializer - attributes :id, - :message, - :cooked, - :created_at, - :excerpt, - :deleted_at, - :deleted_by_id, - :reviewable_id, - :user_flag_status, - :edited, - :reactions, - :bookmark, - :available_flags, - :thread_id, - :chat_channel_id, - :mentioned_users + BASIC_ATTRIBUTES = %i[ + id + message + cooked + created_at + excerpt + deleted_at + deleted_by_id + thread_id + chat_channel_id + ] + attributes( + *( + BASIC_ATTRIBUTES + + %i[ + mentioned_users + reactions + bookmark + available_flags + user_flag_status + reviewable_id + edited + ] + ), + ) has_one :user, serializer: Chat::MessageUserSerializer, embed: :objects has_one :chat_webhook_event, serializer: Chat::WebhookEventSerializer, embed: :objects diff --git a/plugins/chat/app/serializers/chat/thread_preview_serializer.rb b/plugins/chat/app/serializers/chat/thread_preview_serializer.rb index 53355c0d3f6..7ac1b31a590 100644 --- a/plugins/chat/app/serializers/chat/thread_preview_serializer.rb +++ b/plugins/chat/app/serializers/chat/thread_preview_serializer.rb @@ -20,19 +20,19 @@ module Chat end def last_reply_created_at - object.last_reply.created_at + object.last_message.created_at end def last_reply_id - object.last_reply.id + object.last_message.id end def last_reply_excerpt - object.last_reply.excerpt(max_length: Chat::Thread::EXCERPT_LENGTH) + object.last_message.excerpt(max_length: Chat::Thread::EXCERPT_LENGTH) end def last_reply_user - object.last_reply.user + object.last_message.user end def include_participant_data? diff --git a/plugins/chat/app/services/chat/action/reset_channels_last_message_ids.rb b/plugins/chat/app/services/chat/action/reset_channels_last_message_ids.rb new file mode 100644 index 00000000000..ff3ea6d843b --- /dev/null +++ b/plugins/chat/app/services/chat/action/reset_channels_last_message_ids.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module Chat + module Action + class ResetChannelsLastMessageIds + # @param [Array] last_message_ids The message IDs to match with the + # last_message_id in Chat::Channel 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 to update. This is used + # to scope the queries better. + def self.call(last_message_ids, channel_ids) + Chat::Channel + .where(id: channel_ids) + .where("last_message_id IN (?)", last_message_ids) + .find_in_batches { |channels| channels.each(&:update_last_message_id!) } + end + end + end +end diff --git a/plugins/chat/app/services/chat/action/reset_user_last_read_channel_message.rb b/plugins/chat/app/services/chat/action/reset_user_last_read_channel_message.rb index 50701a472cb..f15a390d0c7 100644 --- a/plugins/chat/app/services/chat/action/reset_user_last_read_channel_message.rb +++ b/plugins/chat/app/services/chat/action/reset_user_last_read_channel_message.rb @@ -16,17 +16,12 @@ module Chat -- 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 + SELECT chat_channels.id AS chat_channel_id, last_message_id + FROM chat_channels + WHERE chat_channels.id IN (:channel_ids) ) UPDATE user_chat_channel_memberships - SET last_read_message_id = cte.id + SET last_read_message_id = cte.last_message_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; diff --git a/plugins/chat/app/services/chat/channel_view_builder.rb b/plugins/chat/app/services/chat/channel_view_builder.rb index 0c3ba813b63..e6b4f72b7e5 100644 --- a/plugins/chat/app/services/chat/channel_view_builder.rb +++ b/plugins/chat/app/services/chat/channel_view_builder.rb @@ -77,7 +77,7 @@ module Chat private def fetch_channel(contract:, **) - Chat::Channel.includes(:chatable).find_by(id: contract.channel_id) + Chat::Channel.includes(:chatable, :last_message).find_by(id: contract.channel_id) end def can_view_channel(guardian:, channel:, **) @@ -181,9 +181,10 @@ module Chat context.threads = [] else context.threads = - ::Chat::Thread.includes(original_message_user: :user_status).where( - id: messages.map(&:thread_id).compact.uniq, - ) + ::Chat::Thread + .strict_loading + .includes(last_message: [:user], original_message_user: :user_status) + .where(id: messages.map(&:thread_id).compact.uniq) # Saves us having to load the same message we already have. context.threads.each do |thread| diff --git a/plugins/chat/app/services/chat/lookup_channel_threads.rb b/plugins/chat/app/services/chat/lookup_channel_threads.rb index 93ec6a87486..6ab7e896eca 100644 --- a/plugins/chat/app/services/chat/lookup_channel_threads.rb +++ b/plugins/chat/app/services/chat/lookup_channel_threads.rb @@ -102,29 +102,7 @@ module Chat .to_a end - threads = unread_threads + read_threads - - if threads.present? - last_replies = - ::Chat::Message - .strict_loading - .includes(:user, :uploads) - .from(<<~SQL) - ( - SELECT thread_id, MAX(created_at) AS latest_created_at, MAX(id) AS latest_message_id - FROM chat_messages - WHERE thread_id IN (#{threads.map(&:id).join(",")}) - GROUP BY thread_id - ) AS last_replies_subquery - SQL - .joins( - "INNER JOIN chat_messages ON chat_messages.id = last_replies_subquery.latest_message_id", - ) - .index_by(&:thread_id) - threads.each { |thread| thread.last_reply = last_replies[thread.id] } - end - - threads + unread_threads + read_threads end def fetch_tracking(guardian:, threads:, **) @@ -151,6 +129,14 @@ module Chat :channel, :user_chat_thread_memberships, original_message_user: :user_status, + last_message: [ + :chat_webhook_event, + :chat_channel, + chat_mentions: { + user: :user_status, + }, + user: :user_status, + ], original_message: [ :uploads, :chat_webhook_event, @@ -180,6 +166,9 @@ module Chat .joins( "LEFT JOIN chat_messages original_messages ON chat_threads.original_message_id = original_messages.id", ) + .joins( + "LEFT JOIN chat_messages last_message ON chat_threads.last_message_id = last_message.id", + ) .where(user_chat_thread_memberships: { user_id: guardian.user.id }) .where( "chat_threads.channel_id = :channel_id AND chat_messages.chat_channel_id = :channel_id", @@ -193,11 +182,10 @@ module Chat ], ) .where( - "original_messages.deleted_at IS NULL AND chat_messages.deleted_at IS NULL AND original_messages.id IS NOT NULL", + "original_messages.deleted_at IS NULL AND chat_messages.deleted_at IS NULL AND original_messages.id IS NOT NULL AND last_message.deleted_at IS NULL", ) - .group("chat_threads.id") .select( - "chat_threads.id AS thread_id, MAX(chat_messages.created_at) AS latest_message_created_at, MAX(chat_messages.id) AS latest_message_id", + "chat_threads.id AS thread_id, last_message.created_at AS latest_message_created_at, last_message.id AS latest_message_id", ) .to_sql end 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 e59e1d651e9..0f88cdc1101 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 @@ -24,25 +24,17 @@ module Chat def update_last_read_message_ids(guardian:, **) updated_memberships = DB.query(<<~SQL, user_id: guardian.user.id) - UPDATE user_chat_channel_memberships - SET last_read_message_id = subquery.newest_message_id - FROM - ( - 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 - subquery.newest_message_id > COALESCE(user_chat_channel_memberships.last_read_message_id, 0) AND - user_chat_channel_memberships.user_id = :user_id AND - user_chat_channel_memberships.following - RETURNING user_chat_channel_memberships.id AS membership_id, - user_chat_channel_memberships.chat_channel_id AS channel_id, - user_chat_channel_memberships.last_read_message_id; - SQL + UPDATE user_chat_channel_memberships + SET last_read_message_id = chat_channels.last_message_id + FROM chat_channels + WHERE user_chat_channel_memberships.chat_channel_id = chat_channels.id AND + chat_channels.last_message_id > COALESCE(user_chat_channel_memberships.last_read_message_id, 0) AND + user_chat_channel_memberships.user_id = :user_id AND + user_chat_channel_memberships.following + RETURNING user_chat_channel_memberships.id AS membership_id, + user_chat_channel_memberships.chat_channel_id AS channel_id, + user_chat_channel_memberships.last_read_message_id; + SQL context[:updated_memberships] = updated_memberships end diff --git a/plugins/chat/app/services/chat/message_destroyer.rb b/plugins/chat/app/services/chat/message_destroyer.rb index da7200d602b..6262a0df9bb 100644 --- a/plugins/chat/app/services/chat/message_destroyer.rb +++ b/plugins/chat/app/services/chat/message_destroyer.rb @@ -9,6 +9,11 @@ module Chat 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 + + # This needs to be done before reset_last_read so we can lean on the last_message_id + # there. + reset_last_message_ids(destroyed_message_ids, destroyed_message_channel_ids) + reset_last_read(destroyed_message_ids, destroyed_message_channel_ids) delete_flags(destroyed_message_ids) end @@ -16,6 +21,13 @@ module Chat private + def reset_last_message_ids(destroyed_message_ids, destroyed_message_channel_ids) + ::Chat::Action::ResetChannelsLastMessageIds.call( + destroyed_message_ids, + destroyed_message_channel_ids, + ) + end + def reset_last_read(destroyed_message_ids, destroyed_message_channel_ids) ::Chat::Action::ResetUserLastReadChannelMessage.call( destroyed_message_ids, diff --git a/plugins/chat/app/services/chat/publisher.rb b/plugins/chat/app/services/chat/publisher.rb index 33da0ba24ed..b80b0fc03c4 100644 --- a/plugins/chat/app/services/chat/publisher.rb +++ b/plugins/chat/app/services/chat/publisher.rb @@ -53,12 +53,13 @@ module Chat { type: "channel", channel_id: chat_channel.id, - message_id: chat_message.id, - user_id: chat_message.user.id, - username: chat_message.user.username, thread_id: chat_message.thread_id, + message: + Chat::MessageSerializer.new( + chat_message, + { scope: anonymous_guardian, root: false }, + ).as_json, }, - permissions(chat_channel), ) end @@ -68,10 +69,12 @@ module Chat { type: "thread", channel_id: chat_channel.id, - message_id: chat_message.id, - user_id: chat_message.user.id, - username: chat_message.user.username, thread_id: chat_message.thread_id, + message: + Chat::MessageSerializer.new( + chat_message, + { scope: anonymous_guardian, root: false }, + ).as_json, }, permissions(chat_channel), ) diff --git a/plugins/chat/app/services/chat/restore_message.rb b/plugins/chat/app/services/chat/restore_message.rb index 210a9ee817a..e388b687fa8 100644 --- a/plugins/chat/app/services/chat/restore_message.rb +++ b/plugins/chat/app/services/chat/restore_message.rb @@ -22,6 +22,7 @@ module Chat policy :invalid_access transaction do step :restore_message + step :update_last_message_ids step :update_thread_reply_cache end step :publish_events @@ -55,6 +56,11 @@ module Chat message.thread&.increment_replies_count_cache end + def update_last_message_ids(message:, **) + message.thread&.update_last_message_id! + message.chat_channel.update_last_message_id! + end + def publish_events(guardian:, message:, **) DiscourseEvent.trigger(:chat_message_restored, message, message.chat_channel, guardian.user) Chat::Publisher.publish_restore!(message.chat_channel, message) diff --git a/plugins/chat/app/services/chat/trash_message.rb b/plugins/chat/app/services/chat/trash_message.rb index cbfecdcc037..2a83706abb7 100644 --- a/plugins/chat/app/services/chat/trash_message.rb +++ b/plugins/chat/app/services/chat/trash_message.rb @@ -23,6 +23,7 @@ module Chat transaction do step :trash_message step :destroy_notifications + step :update_last_message_ids step :update_tracking_state step :update_thread_reply_cache end @@ -70,6 +71,11 @@ module Chat message.thread&.decrement_replies_count_cache end + def update_last_message_ids(message:, **) + message.thread&.update_last_message_id! + message.chat_channel.update_last_message_id! + end + def publish_events(guardian:, message:, **) DiscourseEvent.trigger(:chat_message_trashed, message, message.chat_channel, guardian.user) Chat::Publisher.publish_delete!(message.chat_channel, message) diff --git a/plugins/chat/app/services/chat/update_thread_notification_settings.rb b/plugins/chat/app/services/chat/update_thread_notification_settings.rb index ddb333730ec..2f1c30e3d6d 100644 --- a/plugins/chat/app/services/chat/update_thread_notification_settings.rb +++ b/plugins/chat/app/services/chat/update_thread_notification_settings.rb @@ -61,7 +61,7 @@ module Chat membership = thread.membership_for(guardian.user) if !membership membership = thread.add(guardian.user) - membership.update!(last_read_message_id: thread.last_reply.id) + membership.update!(last_read_message_id: thread.last_message_id) end membership.update!(notification_level: contract.notification_level) context.membership = membership diff --git a/plugins/chat/app/services/chat/update_user_thread_last_read.rb b/plugins/chat/app/services/chat/update_user_thread_last_read.rb index 030d5603557..ef4958fa143 100644 --- a/plugins/chat/app/services/chat/update_user_thread_last_read.rb +++ b/plugins/chat/app/services/chat/update_user_thread_last_read.rb @@ -49,14 +49,9 @@ module Chat def mark_thread_read(thread:, guardian:, **) query = <<~SQL UPDATE user_chat_thread_memberships - SET last_read_message_id = ( - SELECT id FROM chat_messages - WHERE thread_id = :thread_id - AND deleted_at IS NULL - ORDER BY created_at DESC, id DESC - LIMIT 1 - ) - WHERE user_id = :user_id AND thread_id = :thread_id + SET last_read_message_id = chat_threads.last_message_id + FROM chat_threads + WHERE user_id = :user_id AND thread_id = :thread_id AND chat_threads.id = :thread_id SQL DB.exec(query, thread_id: thread.id, user_id: guardian.user.id) end @@ -73,7 +68,7 @@ module Chat ::Chat::Publisher.publish_user_tracking_state!( guardian.user, thread.channel, - thread.last_reply, + thread.last_message, ) end end diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.hbs index 64642ce753d..eaaa128bd70 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.hbs @@ -1,7 +1,9 @@
-
- {{this.lastMessageFormattedDate}} -
+ {{#if @channel.lastMessage}} +
+ {{this.lastMessageFormattedDate}} +
+ {{/if}} {{#if this.unreadIndicator}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.js b/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.js index 4c234fcd92e..1ddaa3c5688 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.js @@ -6,7 +6,7 @@ export default class ChatChannelMetadata extends Component { } get lastMessageFormattedDate() { - return moment(this.args.channel.lastMessageSentAt).calendar(null, { + return moment(this.args.channel.lastMessage.createdAt).calendar(null, { sameDay: "LT", nextDay: "[Tomorrow]", nextWeek: "dddd", diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js index 60a80674b9a..dcf44e2fe4d 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js @@ -664,10 +664,6 @@ export default class ChatLivePane extends Component { } handleSentMessage(data) { - if (this.args.channel.isFollowing) { - this.args.channel.lastMessageSentAt = new Date(); - } - if (data.chat_message.user.id === this.currentUser.id && data.staged_id) { const stagedMessage = handleStagedMessage( this.args.channel, @@ -686,12 +682,14 @@ export default class ChatLivePane extends Component { // If we are at the bottom, we append the message and scroll to it const message = ChatMessage.create(this.args.channel, data.chat_message); this.args.channel.addMessages([message]); + this.args.channel.lastMessage = message; this.scrollToLatestMessage(); this.updateLastReadMessage(); } else { // If we are almost at the bottom, we append the message and notice the user const message = ChatMessage.create(this.args.channel, data.chat_message); this.args.channel.addMessages([message]); + this.args.channel.lastMessage = message; this.hasNewMessages = true; } } diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/composer/thread.js b/plugins/chat/assets/javascripts/discourse/components/chat/composer/thread.js index 00f08e854fe..5bca803ae5a 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/composer/thread.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat/composer/thread.js @@ -38,10 +38,6 @@ export default class ChatComposerThread extends ChatComposer { return I18n.t("chat.placeholder_thread"); } - get lastMessage() { - return this.args.thread.lastMessage; - } - lastUserMessage(user) { return this.args.thread.lastUserMessage(user); } diff --git a/plugins/chat/assets/javascripts/discourse/lib/fabricators.js b/plugins/chat/assets/javascripts/discourse/lib/fabricators.js index 93919babe61..1730a80c61b 100644 --- a/plugins/chat/assets/javascripts/discourse/lib/fabricators.js +++ b/plugins/chat/assets/javascripts/discourse/lib/fabricators.js @@ -54,7 +54,7 @@ function messageFabricator(args = {}) { function channelFabricator(args = {}) { const id = args.id || sequence++; - return ChatChannel.create( + const channel = ChatChannel.create( Object.assign( { id, @@ -62,7 +62,6 @@ function channelFabricator(args = {}) { args.chatable?.type || args.chatable_type || CHATABLE_TYPES.categoryChannel, - last_message_sent_at: args.last_message_sent_at, chatable_id: args.chatable?.id || args.chatable_id, title: args.title || "General", description: args.description, @@ -73,6 +72,10 @@ function channelFabricator(args = {}) { args ) ); + + channel.lastMessage = messageFabricator({ channel }); + + return channel; } function categoryFabricator(args = {}) { diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js index a580b2fc9ef..a832125d07e 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js @@ -1,4 +1,5 @@ import UserChatChannelMembership from "discourse/plugins/chat/discourse/models/user-chat-channel-membership"; +import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message"; import { TrackedSet } from "@ember-compat/tracked-built-ins"; import { escapeExpression } from "discourse/lib/utilities"; import { tracked } from "@glimmer/tracking"; @@ -61,7 +62,6 @@ export default class ChatChannel { @tracked description; @tracked status; @tracked activeThread = null; - @tracked lastMessageSentAt; @tracked canDeleteOthers; @tracked canDeleteSelf; @tracked canFlag; @@ -82,6 +82,7 @@ export default class ChatChannel { @tracked _unreadThreadIds = new TrackedSet(); @tracked _currentUserMembership; + @tracked _lastMessage; constructor(args = {}) { this.id = args.id; @@ -99,7 +100,6 @@ export default class ChatChannel { this.userSilenced = args.user_silenced; this.canModerate = args.can_moderate; this.description = args.description; - this.lastMessageSentAt = args.last_message_sent_at; this.threadingEnabled = args.threading_enabled; this.autoJoinUsers = args.auto_join_users; this.allowChannelWideMentions = args.allow_channel_wide_mentions; @@ -116,6 +116,7 @@ export default class ChatChannel { } this.tracking = new ChatTrackingState(getOwner(this)); + this.lastMessage = args.last_message; } get unreadThreadCount() { @@ -158,10 +159,6 @@ export default class ChatChannel { this.messagesManager.removeMessage(message); } - get lastMessage() { - return this.messagesManager.findLastMessage(); - } - lastUserMessage(user) { return this.messagesManager.findLastUserMessage(user); } @@ -312,6 +309,23 @@ export default class ChatChannel { } } + get lastMessage() { + return this._lastMessage; + } + + set lastMessage(message) { + if (!message) { + this._lastMessage = null; + return; + } + + if (message instanceof ChatMessage) { + this._lastMessage = message; + } else { + this._lastMessage = ChatMessage.create(this, message); + } + } + clearSelectedMessages() { this.selectedMessages.forEach((message) => (message.selected = false)); } diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js index f42ad19f551..f55e58fb849 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js @@ -151,8 +151,17 @@ export default class ChatChannelsManager extends Service { #sortDirectMessageChannels(channels) { return channels.sort((a, b) => { + if (!a.lastMessage) { + return 1; + } + + if (!b.lastMessage) { + return -1; + } + if (a.tracking.unreadCount === b.tracking.unreadCount) { - return new Date(a.lastMessageSentAt) > new Date(b.lastMessageSentAt) + return new Date(a.lastMessage.createdAt) > + new Date(b.lastMessage.createdAt) ? -1 : 1; } else { diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-subscriptions-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-subscriptions-manager.js index e65a97b1775..314907f1f71 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-subscriptions-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-subscriptions-manager.js @@ -186,16 +186,20 @@ export default class ChatSubscriptionsManager extends Service { _onNewChannelMessage(busData) { this.chatChannelsManager.find(busData.channel_id).then((channel) => { - if (busData.user_id === this.currentUser.id) { + channel.lastMessage = busData.message; + const user = busData.message.user; + if (user.id === this.currentUser.id) { // User sent message, update tracking state to no unread - channel.currentUserMembership.lastReadMessageId = busData.message_id; + channel.currentUserMembership.lastReadMessageId = + channel.lastMessage.id; } else { // Ignored user sent message, update tracking state to no unread - if (this.currentUser.ignored_users.includes(busData.username)) { - channel.currentUserMembership.lastReadMessageId = busData.message_id; + if (this.currentUser.ignored_users.includes(user.username)) { + channel.currentUserMembership.lastReadMessageId = + channel.lastMessage.id; } else { if ( - busData.message_id > + channel.lastMessage.id > (channel.currentUserMembership.lastReadMessageId || 0) ) { channel.tracking.unreadCount++; @@ -204,7 +208,7 @@ export default class ChatSubscriptionsManager extends Service { // Thread should be considered unread if not already. if (busData.thread_id) { channel.threadsManager - .find(busData.channel_id, busData.thread_id) + .find(channel.id, busData.thread_id) .then((thread) => { if (thread.currentUserMembership) { channel.unreadThreadIds.add(busData.thread_id); @@ -213,8 +217,6 @@ export default class ChatSubscriptionsManager extends Service { } } } - - channel.lastMessageSentAt = new Date(); }); } @@ -223,25 +225,29 @@ export default class ChatSubscriptionsManager extends Service { channel.threadsManager .find(busData.channel_id, busData.thread_id) .then((thread) => { - if (busData.user_id === this.currentUser.id) { + if (busData.message.user.id === this.currentUser.id) { // Thread should no longer be considered unread. if (thread.currentUserMembership) { channel.unreadThreadIds.delete(busData.thread_id); thread.currentUserMembership.lastReadMessageId = - busData.message_id; + busData.message.id; } } else { // Ignored user sent message, update tracking state to no unread - if (this.currentUser.ignored_users.includes(busData.username)) { + if ( + this.currentUser.ignored_users.includes( + busData.message.user.username + ) + ) { if (thread.currentUserMembership) { thread.currentUserMembership.lastReadMessageId = - busData.message_id; + busData.message.id; } } else { // Message from other user. Increment unread for thread tracking state. if ( thread.currentUserMembership && - busData.message_id > + busData.message.id > (thread.currentUserMembership.lastReadMessageId || 0) && !thread.currentUserMembership.isQuiet ) { diff --git a/plugins/chat/db/migrate/20230707025733_add_last_message_id_to_channel_and_thread.rb b/plugins/chat/db/migrate/20230707025733_add_last_message_id_to_channel_and_thread.rb new file mode 100644 index 00000000000..b8befcef009 --- /dev/null +++ b/plugins/chat/db/migrate/20230707025733_add_last_message_id_to_channel_and_thread.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class AddLastMessageIdToChannelAndThread < ActiveRecord::Migration[7.0] + def change + add_column :chat_channels, :last_message_id, :bigint, null: true + add_column :chat_threads, :last_message_id, :bigint, null: true + + add_index :chat_channels, :last_message_id + add_index :chat_threads, :last_message_id + end +end diff --git a/plugins/chat/db/migrate/20230707082645_backfill_chat_channel_and_thread_last_message_ids.rb b/plugins/chat/db/migrate/20230707082645_backfill_chat_channel_and_thread_last_message_ids.rb new file mode 100644 index 00000000000..cba288c9b33 --- /dev/null +++ b/plugins/chat/db/migrate/20230707082645_backfill_chat_channel_and_thread_last_message_ids.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +class BackfillChatChannelAndThreadLastMessageIds < ActiveRecord::Migration[7.0] + def up + execute <<-SQL + UPDATE chat_channels + SET last_message_id = ( + SELECT cm.id + FROM chat_messages cm + LEFT JOIN chat_threads ON chat_threads.original_message_id = cm.id + WHERE cm.chat_channel_id = chat_channels.id + AND cm.deleted_at IS NULL + AND (cm.thread_id IS NULL OR chat_threads.id IS NOT NULL) + ORDER BY cm.created_at DESC, cm.id DESC + LIMIT 1 + ); + SQL + + execute <<-SQL + UPDATE chat_threads + SET last_message_id = ( + SELECT cm.id + FROM chat_messages cm + WHERE cm.thread_id = chat_threads.id + AND cm.deleted_at IS NULL + ORDER BY cm.created_at DESC, cm.id DESC + LIMIT 1 + ); + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/plugins/chat/db/post_migrate/20230710040640_backfill_chat_channel_and_thread_last_message_ids_post_migrate.rb b/plugins/chat/db/post_migrate/20230710040640_backfill_chat_channel_and_thread_last_message_ids_post_migrate.rb new file mode 100644 index 00000000000..08384e31191 --- /dev/null +++ b/plugins/chat/db/post_migrate/20230710040640_backfill_chat_channel_and_thread_last_message_ids_post_migrate.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +class BackfillChatChannelAndThreadLastMessageIdsPostMigrate < ActiveRecord::Migration[7.0] + execute <<-SQL + UPDATE chat_channels + SET last_message_id = ( + SELECT cm.id + FROM chat_messages cm + LEFT JOIN chat_threads ON chat_threads.original_message_id = cm.id + WHERE cm.chat_channel_id = chat_channels.id + AND cm.deleted_at IS NULL + AND (cm.thread_id IS NULL OR chat_threads.id IS NOT NULL) + ORDER BY cm.created_at DESC, cm.id DESC + LIMIT 1 + ); + SQL + + execute <<-SQL + UPDATE chat_threads + SET last_message_id = ( + SELECT cm.id + FROM chat_messages cm + WHERE cm.thread_id = chat_threads.id + AND cm.deleted_at IS NULL + ORDER BY cm.created_at DESC, cm.id DESC + LIMIT 1 + ); + SQL +end diff --git a/plugins/chat/lib/chat/channel_fetcher.rb b/plugins/chat/lib/chat/channel_fetcher.rb index 17fca5377a9..4a9c1f87854 100644 --- a/plugins/chat/lib/chat/channel_fetcher.rb +++ b/plugins/chat/lib/chat/channel_fetcher.rb @@ -88,7 +88,7 @@ module Chat def self.secured_public_channel_search(guardian, options = {}) allowed_channel_ids = generate_allowed_channel_ids_sql(guardian, exclude_dm_channels: true) - channels = Chat::Channel.includes(chatable: [:topic_only_relative_url]) + channels = Chat::Channel.includes(:last_message, chatable: [:topic_only_relative_url]) channels = channels.includes(:chat_channel_archive) if options[:include_archives] channels = @@ -179,10 +179,15 @@ module Chat def self.secured_direct_message_channels_search(user_id, guardian, options = {}) query = Chat::Channel.strict_loading.includes( + last_message: [:uploads], chatable: [{ direct_message_users: [user: :user_option] }, :users], ) query = query.includes(chatable: [{ users: :user_status }]) if SiteSetting.enable_user_status query = query.joins(:user_chat_channel_memberships) + query = + query.joins( + "LEFT JOIN chat_messages last_message ON last_message.id = chat_channels.last_message_id", + ) scoped_channels = Chat::Channel @@ -223,7 +228,7 @@ module Chat query .where(chatable_type: Chat::Channel.direct_channel_chatable_types) .where(chat_channels: { id: scoped_channels }) - .order(last_message_sent_at: :desc) + .order("last_message.created_at DESC NULLS LAST") channels = query.to_a preload_fields = diff --git a/plugins/chat/lib/chat/message_creator.rb b/plugins/chat/lib/chat/message_creator.rb index 9cbd6775c32..2b1c16317f5 100644 --- a/plugins/chat/lib/chat/message_creator.rb +++ b/plugins/chat/lib/chat/message_creator.rb @@ -63,6 +63,7 @@ module Chat @chat_message.attach_uploads(uploads) Chat::Draft.where(user_id: @user.id, chat_channel_id: @chat_channel.id).destroy_all post_process_resolved_thread + update_channel_last_message Chat::Publisher.publish_new!( @chat_channel, @chat_message, @@ -71,7 +72,6 @@ module Chat ) Jobs.enqueue(Jobs::Chat::ProcessMessage, { chat_message_id: @chat_message.id }) Chat::Notifier.notify_new(chat_message: @chat_message, timestamp: @chat_message.created_at) - @chat_channel.touch(:last_message_sent_at) DiscourseEvent.trigger(:chat_message_created, @chat_message, @chat_channel, @user) rescue => error @error = error @@ -232,6 +232,7 @@ module Chat def post_process_resolved_thread return if resolved_thread.blank? + resolved_thread.update!(last_message: @chat_message) resolved_thread.increment_replies_count_cache current_user_thread_membership = resolved_thread.add(@user) current_user_thread_membership.update!(last_read_message_id: @chat_message.id) @@ -240,5 +241,10 @@ module Chat resolved_thread.add(resolved_thread.original_message_user) end end + + def update_channel_last_message + return if @chat_message.thread_reply? + @chat_channel.update!(last_message: @chat_message) + end end end diff --git a/plugins/chat/lib/chat/user_notifications_extension.rb b/plugins/chat/lib/chat/user_notifications_extension.rb index c0c87a06df2..6c73ceb1d0d 100644 --- a/plugins/chat/lib/chat/user_notifications_extension.rb +++ b/plugins/chat/lib/chat/user_notifications_extension.rb @@ -73,7 +73,7 @@ module Chat # Prioritize messages from regular channels over direct messages if channels.any? channel_notification_text( - channels.sort_by { |channel| [channel.last_message_sent_at, channel.created_at] }, + channels.sort_by { |channel| [channel.last_message.created_at, channel.created_at] }, dm_users, ) else diff --git a/plugins/chat/spec/components/chat/message_creator_spec.rb b/plugins/chat/spec/components/chat/message_creator_spec.rb index dfaec6dd54d..3da123f44f0 100644 --- a/plugins/chat/spec/components/chat/message_creator_spec.rb +++ b/plugins/chat/spec/components/chat/message_creator_spec.rb @@ -120,16 +120,14 @@ describe Chat::MessageCreator do }.to change { Chat::Message.count }.by(1) end - it "updates the channel’s last message date" do - previous_last_message_sent_at = public_chat_channel.last_message_sent_at - - described_class.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - ) - - expect(previous_last_message_sent_at).to be < public_chat_channel.reload.last_message_sent_at + it "updates the last_message for the channel" do + message = + described_class.create( + chat_channel: public_chat_channel, + user: user1, + content: "this is a message", + ).chat_message + expect(public_chat_channel.reload.last_message).to eq(message) end it "sets the last_editor_id to the user who created the message" do @@ -640,6 +638,18 @@ describe Chat::MessageCreator do expect(message.in_reply_to.thread).to eq(message.thread) expect(message.thread.original_message).to eq(reply_message) expect(message.thread.original_message_user).to eq(reply_message.user) + expect(message.thread.last_message).to eq(message) + end + + it "does not change the last_message of the channel for a thread reply" do + original_last_message = public_chat_channel.last_message + described_class.create( + chat_channel: public_chat_channel, + user: user1, + content: "this is a message", + in_reply_to_id: reply_message.id, + ) + expect(public_chat_channel.reload.last_message).to eq(original_last_message) end it "creates a user thread membership" do @@ -756,6 +766,7 @@ describe Chat::MessageCreator do }.not_to change { Chat::Thread.count } expect(message.reload.thread).to eq(existing_thread) + expect(existing_thread.reload.last_message).to eq(message) end it "creates a user thread membership if one does not exist" do diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index ac4035c5e58..359a813db6e 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/plugins/chat/spec/fabricators/chat_fabricator.rb @@ -160,6 +160,7 @@ Fabricator(:chat_thread, class_name: "Chat::Thread") do transient :with_replies transient :channel transient :original_message_user + transient :old_om original_message do |attrs| Fabricate( @@ -170,7 +171,13 @@ Fabricator(:chat_thread, class_name: "Chat::Thread") do end after_create do |thread, transients| - thread.original_message.update!(thread_id: thread.id) + attrs = { thread_id: thread.id } + + # Sometimes we make this older via created_at so any messages fabricated for this thread + # afterwards are not created earlier in time than the OM. + attrs[:created_at] = 1.week.ago if transients[:old_om] + + thread.original_message.update!(**attrs) thread.add(thread.original_message_user) if transients[:with_replies] diff --git a/plugins/chat/spec/integration/thread_replies_count_cache_accuracy_spec.rb b/plugins/chat/spec/integration/thread_replies_count_cache_accuracy_spec.rb index d829547db11..a41ad8e2ef8 100644 --- a/plugins/chat/spec/integration/thread_replies_count_cache_accuracy_spec.rb +++ b/plugins/chat/spec/integration/thread_replies_count_cache_accuracy_spec.rb @@ -46,7 +46,7 @@ RSpec.describe "Chat::Thread replies_count cache accuracy" do # Lose the cache intentionally. Chat::Thread.clear_caches!(thread.id) - message_to_destroy = thread.last_reply + message_to_destroy = thread.last_message Chat::TrashMessage.call( message_id: message_to_destroy.id, channel_id: thread.channel_id, diff --git a/plugins/chat/spec/lib/chat/channel_fetcher_spec.rb b/plugins/chat/spec/lib/chat/channel_fetcher_spec.rb index 3c405b54bec..65f02b4bd39 100644 --- a/plugins/chat/spec/lib/chat/channel_fetcher_spec.rb +++ b/plugins/chat/spec/lib/chat/channel_fetcher_spec.rb @@ -332,7 +332,7 @@ describe Chat::ChannelFetcher do end describe ".secured_direct_message_channels" do - it "includes direct message channels the user is a member of ordered by last_message_sent_at" do + it "includes direct message channels the user is a member of ordered by last_message.created_at" do Fabricate( :user_chat_channel_membership_for_dm, chat_channel: direct_message_channel1, @@ -350,8 +350,11 @@ describe Chat::ChannelFetcher do Chat::DirectMessageUser.create!(direct_message: dm_channel2, user: user1) Chat::DirectMessageUser.create!(direct_message: dm_channel2, user: user2) - direct_message_channel1.update!(last_message_sent_at: 1.day.ago) - direct_message_channel2.update!(last_message_sent_at: 1.hour.ago) + Fabricate(:chat_message, user: user1, chat_channel: direct_message_channel1) + Fabricate(:chat_message, user: user1, chat_channel: direct_message_channel2) + + direct_message_channel1.last_message.update!(created_at: 1.day.ago) + direct_message_channel2.last_message.update!(created_at: 1.hour.ago) expect(described_class.secured_direct_message_channels(user1.id, guardian).map(&:id)).to eq( [direct_message_channel2.id, direct_message_channel1.id], diff --git a/plugins/chat/spec/models/chat/channel_spec.rb b/plugins/chat/spec/models/chat/channel_spec.rb index 748f761b93b..a634588f7ad 100644 --- a/plugins/chat/spec/models/chat/channel_spec.rb +++ b/plugins/chat/spec/models/chat/channel_spec.rb @@ -186,4 +186,43 @@ RSpec.describe Chat::Channel do ) end end + + describe "#latest_not_deleted_message_id" do + fab!(:channel) { Fabricate(:category_channel) } + fab!(:old_message) { Fabricate(:chat_message, chat_channel: channel) } + fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel) } + + before { old_message.update!(created_at: 1.day.ago) } + + it "accepts an anchor message to only get messages of a lower id" do + expect(channel.latest_not_deleted_message_id(anchor_message_id: message_1.id)).to eq( + old_message.id, + ) + end + + it "gets the latest message by created_at" do + expect(channel.latest_not_deleted_message_id).to eq(message_1.id) + end + + it "does not get other channel messages" do + Fabricate(:chat_message) + expect(channel.latest_not_deleted_message_id).to eq(message_1.id) + end + + it "does not get thread replies" do + thread = Fabricate(:chat_thread, channel: channel, old_om: true) + message_1.update!(thread: thread) + expect(channel.latest_not_deleted_message_id).to eq(old_message.id) + end + + it "does get thread original message" do + thread = Fabricate(:chat_thread, channel: channel) + expect(channel.latest_not_deleted_message_id).to eq(thread.original_message_id) + end + + it "does not get deleted messages" do + message_1.trash! + expect(channel.latest_not_deleted_message_id).to eq(old_message.id) + end + end end diff --git a/plugins/chat/spec/models/chat/thread_spec.rb b/plugins/chat/spec/models/chat/thread_spec.rb index 55fa93bfc3b..1ea74c5a7a8 100644 --- a/plugins/chat/spec/models/chat/thread_spec.rb +++ b/plugins/chat/spec/models/chat/thread_spec.rb @@ -213,4 +213,33 @@ RSpec.describe Chat::Thread do end end end + + describe "#latest_not_deleted_message_id" do + fab!(:channel) { Fabricate(:category_channel) } + fab!(:thread) { Fabricate(:chat_thread, channel: channel, old_om: true) } + fab!(:old_message) { Fabricate(:chat_message, chat_channel: channel, thread: thread) } + fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel, thread: thread) } + + before { old_message.update!(created_at: 1.day.ago) } + + it "accepts an anchor message to only get messages of a lower id" do + expect(thread.latest_not_deleted_message_id(anchor_message_id: message_1.id)).to eq( + old_message.id, + ) + end + + it "gets the latest message by created_at" do + expect(thread.latest_not_deleted_message_id).to eq(message_1.id) + end + + it "does not get other channel messages" do + Fabricate(:chat_message) + expect(thread.latest_not_deleted_message_id).to eq(message_1.id) + end + + it "does not get deleted messages" do + message_1.trash! + expect(thread.latest_not_deleted_message_id).to eq(old_message.id) + end + end end diff --git a/plugins/chat/spec/services/actions/reset_user_last_read_channel_message_spec.rb b/plugins/chat/spec/services/actions/reset_user_last_read_channel_message_spec.rb index d0362ec1bbf..60b182ceac0 100644 --- a/plugins/chat/spec/services/actions/reset_user_last_read_channel_message_spec.rb +++ b/plugins/chat/spec/services/actions/reset_user_last_read_channel_message_spec.rb @@ -27,7 +27,9 @@ RSpec.describe Chat::Action::ResetUserLastReadChannelMessage do context "when there are non-deleted messages left in the channel" do before do message_3.trash! + message_3.chat_channel.update_last_message_id! message_6.trash! + message_6.chat_channel.update_last_message_id! end it "sets the matching membership last_read_message_ids to the most recently created message ID" do @@ -38,7 +40,11 @@ RSpec.describe Chat::Action::ResetUserLastReadChannelMessage do 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!) } + before do + [message_1, message_2, message_4, message_5].each(&:trash!) + channel_1.update_last_message_id! + channel_2.update_last_message_id! + end 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]) diff --git a/plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb b/plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb index 824d31452ee..e1f83745ed2 100644 --- a/plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb +++ b/plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb @@ -186,15 +186,17 @@ RSpec.describe ::Chat::LookupChannelThreads do thread_4.membership_for(current_user).update!( notification_level: ::Chat::UserChatThreadMembership.notification_levels[:muted], ) - thread_5 = Fabricate(:chat_thread, channel: channel_1) + Fabricate(:chat_thread, channel: channel_1) expect(result.threads.map(&:id)).to eq([thread_1.id, thread_2.id, thread_3.id]) end it "does not count deleted messages for sort order" do + original_last_message_id = thread_3.reload.last_message_id unread_message = Fabricate(:chat_message, chat_channel: channel_1, thread: thread_3) unread_message.update!(created_at: 2.days.ago) unread_message.trash! + thread_3.reload.update!(last_message_id: original_last_message_id) expect(result.threads.map(&:id)).to eq([thread_1.id, thread_2.id, thread_3.id]) 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 029f0baa145..d12c9bf04f4 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 @@ -109,6 +109,7 @@ RSpec.describe Chat::MarkAllUserChannelsRead do it "does not use deleted messages for the last_read_message_id" do message_2.trash! + message_2.chat_channel.update_last_message_id! result expect(membership_1.reload.last_read_message_id).to eq(message_1.id) end diff --git a/plugins/chat/spec/services/chat/message_destroyer_spec.rb b/plugins/chat/spec/services/chat/message_destroyer_spec.rb index 623014baa8d..ec16a275a9f 100644 --- a/plugins/chat/spec/services/chat/message_destroyer_spec.rb +++ b/plugins/chat/spec/services/chat/message_destroyer_spec.rb @@ -62,5 +62,11 @@ RSpec.describe Chat::MessageDestroyer do expect { message_1.reload }.to raise_exception(ActiveRecord::RecordNotFound) expect(message_2.reload).to be_present end + + it "sets the last_message_id for the channel if that message is deleted" do + expect(message_1.chat_channel.last_message_id).to eq(message_1.id) + described_class.new.destroy_in_batches(Chat::Message.where(id: message_1.id)) + expect(message_1.chat_channel.reload.last_message_id).to eq(nil) + end end end diff --git a/plugins/chat/spec/services/chat/publisher_spec.rb b/plugins/chat/spec/services/chat/publisher_spec.rb index ef59298ffab..44c497cbb09 100644 --- a/plugins/chat/spec/services/chat/publisher_spec.rb +++ b/plugins/chat/spec/services/chat/publisher_spec.rb @@ -281,10 +281,12 @@ describe Chat::Publisher do { type: "channel", channel_id: channel.id, - message_id: message_1.id, - user_id: message_1.user_id, - username: message_1.user.username, thread_id: nil, + message: + Chat::MessageSerializer.new( + message_1, + { scope: Guardian.new(nil), root: false }, + ).as_json, }, ) end @@ -340,10 +342,12 @@ describe Chat::Publisher do { type: "thread", channel_id: channel.id, - message_id: message_1.id, - user_id: message_1.user_id, - username: message_1.user.username, thread_id: thread.id, + message: + Chat::MessageSerializer.new( + message_1, + { scope: Guardian.new(nil), root: false }, + ).as_json, }, ) end diff --git a/plugins/chat/spec/services/chat/restore_message_spec.rb b/plugins/chat/spec/services/chat/restore_message_spec.rb index 80a1c077abd..cd482b6ad38 100644 --- a/plugins/chat/spec/services/chat/restore_message_spec.rb +++ b/plugins/chat/spec/services/chat/restore_message_spec.rb @@ -5,7 +5,12 @@ RSpec.describe Chat::RestoreMessage do let!(:guardian) { Guardian.new(current_user) } fab!(:message) { Fabricate(:chat_message, user: current_user) } - before { message.trash! } + before do + message.trash! + message.chat_channel.update!( + last_message_id: message.chat_channel.latest_not_deleted_message_id, + ) + end describe ".call" do subject(:result) { described_class.call(params) } @@ -45,6 +50,19 @@ RSpec.describe Chat::RestoreMessage do expect(Chat::Message.find_by(id: message.id)).not_to be_nil end + it "updates the channel last_message_id if the message is now the last one in the channel" do + expect(message.chat_channel.reload.last_message_id).to be_nil + result + expect(message.chat_channel.reload.last_message_id).to eq(message.id) + end + + it "does not update the channel last_message_id if the message is not the last one in the channel" do + next_message = Fabricate(:chat_message, chat_channel: message.chat_channel) + expect(message.chat_channel.reload.last_message_id).to eq(next_message.id) + result + expect(message.chat_channel.reload.last_message_id).to eq(next_message.id) + end + it "publishes associated Discourse and MessageBus events" do freeze_time messages = nil @@ -60,13 +78,30 @@ RSpec.describe Chat::RestoreMessage do context "when the message has a thread" do fab!(:thread) { Fabricate(:chat_thread, channel: message.chat_channel) } - before { message.update!(thread: thread) } + before do + message.update!(thread: thread) + thread.update_last_message_id! + thread.original_message.update!(created_at: message.created_at - 2.hours) + end it "increments the thread reply count" do thread.set_replies_count_cache(1) result expect(thread.replies_count_cache).to eq(2) end + + it "updates the thread last_message_id if the message is now the last one in the thread" do + expect(message.thread.reload.last_message_id).to eq(thread.original_message_id) + result + expect(message.thread.reload.last_message_id).to eq(message.id) + end + + it "does not update the thread last_message_id if the message is not the last one in the channel" do + next_message = Fabricate(:chat_message, thread: message.thread) + expect(message.thread.reload.last_message_id).to eq(next_message.id) + result + expect(message.thread.reload.last_message_id).to eq(next_message.id) + end end end end diff --git a/plugins/chat/spec/services/chat/trash_message_spec.rb b/plugins/chat/spec/services/chat/trash_message_spec.rb index a88388aca54..60772d6d9fd 100644 --- a/plugins/chat/spec/services/chat/trash_message_spec.rb +++ b/plugins/chat/spec/services/chat/trash_message_spec.rb @@ -115,10 +115,23 @@ RSpec.describe Chat::TrashMessage do expect(membership_2.reload.last_read_message_id).to be_nil end + it "updates the channel last_message_id to the previous message in the channel" do + next_message = + Fabricate(:chat_message, chat_channel: message.chat_channel, user: current_user) + params[:message_id] = next_message.id + expect(message.chat_channel.reload.last_message).to eq(next_message) + result + expect(message.chat_channel.reload.last_message).to eq(message) + end + context "when the message has a thread" do fab!(:thread) { Fabricate(:chat_thread, channel: message.chat_channel) } - before { message.update!(thread: thread) } + before do + message.update!(thread: thread) + thread.update!(last_message: message) + thread.original_message.update!(created_at: message.created_at - 2.hours) + end it "decrements the thread reply count" do thread.set_replies_count_cache(5) @@ -154,6 +167,22 @@ RSpec.describe Chat::TrashMessage do expect(membership_1.reload.last_read_message_id).to be_nil expect(membership_2.reload.last_read_message_id).to be_nil end + + it "updates the thread last_message_id to the previous message in the thread" do + next_message = Fabricate(:chat_message, thread: thread, user: current_user) + params[:message_id] = next_message.id + expect(thread.reload.last_message).to eq(next_message) + result + expect(thread.reload.last_message).to eq(message) + end + + context "when there are no other messages left in the thread except the original message" do + it "updates the thread last_message_id to the original message" do + expect(thread.last_message).to eq(message) + result + expect(thread.reload.last_message).to eq(thread.original_message) + end + end end context "when message is already deleted" do diff --git a/plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb b/plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb index e076a2cb403..f8e48fb2342 100644 --- a/plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb +++ b/plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Chat::UpdateUserThreadLastRead do fab!(:current_user) { Fabricate(:user) } fab!(:channel) { Fabricate(:chat_channel) } - fab!(:thread) { Fabricate(:chat_thread, channel: channel) } + fab!(:thread) { Fabricate(:chat_thread, channel: channel, old_om: true) } fab!(:thread_reply_1) { Fabricate(:chat_message, chat_channel: channel, thread: thread) } fab!(:thread_reply_2) { Fabricate(:chat_message, chat_channel: channel, thread: thread) } @@ -95,7 +95,7 @@ RSpec.describe Chat::UpdateUserThreadLastRead do it "updates the last_read_message_id of the thread" do result - expect(membership.reload.last_read_message_id).to eq(thread.last_reply.id) + expect(membership.reload.last_read_message_id).to eq(thread.reload.last_message.id) end end end diff --git a/plugins/chat/spec/support/api/schemas/category_chat_channel.json b/plugins/chat/spec/support/api/schemas/category_chat_channel.json index 60fe90b3fd6..2e952ebbd56 100644 --- a/plugins/chat/spec/support/api/schemas/category_chat_channel.json +++ b/plugins/chat/spec/support/api/schemas/category_chat_channel.json @@ -21,7 +21,7 @@ "chatable_url": { "type": "string" }, "title": { "type": "string" }, "chatable_id": { "type": "number" }, - "last_message_sent_at": { "type": "string" }, + "last_message": { "type": ["object", "null"] }, "status": { "type": "string" }, "chatable": { "type": "object", 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 66ce5d47e24..008d1cebddb 100644 --- a/plugins/chat/spec/system/channel_thread_message_echoing_spec.rb +++ b/plugins/chat/spec/system/channel_thread_message_echoing_spec.rb @@ -79,10 +79,10 @@ describe "Channel thread message echoing", type: :system do current_user .user_chat_channel_memberships .find_by(chat_channel: channel) - .update!(last_read_message_id: thread.last_reply.id) + .update!(last_read_message_id: thread.last_message_id) chat_page.visit_channel(channel) expect(channel_page).not_to have_css( - channel_page.message_by_id_selector(thread.last_reply.id), + channel_page.message_by_id_selector(thread.last_message_id), ) end diff --git a/plugins/chat/spec/system/chat/composer/shortcuts/thread_spec.rb b/plugins/chat/spec/system/chat/composer/shortcuts/thread_spec.rb index b37022fb487..573412efe80 100644 --- a/plugins/chat/spec/system/chat/composer/shortcuts/thread_spec.rb +++ b/plugins/chat/spec/system/chat/composer/shortcuts/thread_spec.rb @@ -59,7 +59,10 @@ RSpec.describe "Chat | composer | shortcuts | thread", type: :system do end context "when last message is deleted" do - before { last_thread_message.trash! } + before do + last_thread_message.trash! + thread_1.update_last_message_id! + end it "does not edit a message" do chat_page.visit_thread(thread_1) diff --git a/plugins/chat/spec/system/message_thread_indicator_spec.rb b/plugins/chat/spec/system/message_thread_indicator_spec.rb index 556b2955955..7f48f94d0e9 100644 --- a/plugins/chat/spec/system/message_thread_indicator_spec.rb +++ b/plugins/chat/spec/system/message_thread_indicator_spec.rb @@ -141,19 +141,19 @@ describe "Thread indicator for chat messages", type: :system do end it "shows an excerpt of the last reply in the thread" do - thread_1.last_reply.update!(message: "test for excerpt") - thread_1.last_reply.rebake! + thread_1.last_message.update!(message: "test for excerpt") + thread_1.last_message.rebake! chat_page.visit_channel(channel) expect( channel_page.message_thread_indicator(thread_1.original_message).excerpt, - ).to have_content(thread_excerpt(thread_1.last_reply)) + ).to have_content(thread_excerpt(thread_1.last_message)) end it "updates the last reply excerpt and participants when a new message is added to the thread" do new_user = Fabricate(:user) chat_system_user_bootstrap(user: new_user, channel: channel) - original_last_reply = thread_1.replies.last + original_last_reply = thread_1.last_message original_last_reply.update!(message: "test for excerpt") original_last_reply.rebake! diff --git a/plugins/chat/spec/system/single_thread_spec.rb b/plugins/chat/spec/system/single_thread_spec.rb index 83ae37a016d..761b64e3512 100644 --- a/plugins/chat/spec/system/single_thread_spec.rb +++ b/plugins/chat/spec/system/single_thread_spec.rb @@ -112,7 +112,7 @@ describe "Single thread in side panel", type: :system do expect(side_panel).to have_open_thread(thread) thread_page.send_message("new thread message") expect(thread_page).to have_message(thread_id: thread.id, text: "new thread message") - thread_message = thread.last_reply + thread_message = thread.last_message expect(thread_message.chat_channel_id).to eq(channel.id) expect(thread_message.thread.channel_id).to eq(channel.id) end diff --git a/plugins/chat/test/javascripts/chat-fixtures.js b/plugins/chat/test/javascripts/chat-fixtures.js index edaad47b023..7f4e1f83c0f 100644 --- a/plugins/chat/test/javascripts/chat-fixtures.js +++ b/plugins/chat/test/javascripts/chat-fixtures.js @@ -31,7 +31,7 @@ export const directMessageChannels = [ following: true, }, allow_channel_wide_mentions: true, - last_message_sent_at: "2021-07-20T08:14:16.950Z", + last_message: { id: 333, created_at: "2021-07-02T08:14:16.950Z" }, message_bus_last_ids: { new_mentions: 0, new_messages: 0, @@ -66,7 +66,7 @@ export const directMessageChannels = [ following: true, }, allow_channel_wide_mentions: true, - last_message_sent_at: "2021-07-05T12:04:00.850Z", + last_message: { id: 333, created_at: "2021-07-02T08:14:16.950Z" }, message_bus_last_ids: { new_mentions: 0, new_messages: 0, @@ -108,7 +108,7 @@ export const chatChannels = { status: "open", chatable: chatables[1], allow_channel_wide_mentions: true, - last_message_sent_at: "2021-07-24T08:14:16.950Z", + last_message: { id: 333, created_at: "2021-07-02T08:14:16.950Z" }, current_user_membership: { muted: false, following: true, @@ -127,7 +127,7 @@ export const chatChannels = { status: "open", chatable: chatables[1], allow_channel_wide_mentions: true, - last_message_sent_at: "2021-07-15T08:14:16.950Z", + last_message: { id: 333, created_at: "2021-07-02T08:14:16.950Z" }, current_user_membership: { muted: false, following: true, @@ -146,7 +146,7 @@ export const chatChannels = { status: "open", chatable: chatables[8], allow_channel_wide_mentions: true, - last_message_sent_at: "2021-07-14T08:14:16.950Z", + last_message: { id: 333, created_at: "2021-07-02T08:14:16.950Z" }, current_user_membership: { muted: false, following: true, @@ -165,7 +165,7 @@ export const chatChannels = { status: "read_only", chatable: chatables[8], allow_channel_wide_mentions: true, - last_message_sent_at: "2021-07-10T08:14:16.950Z", + last_message: { id: 333, created_at: "2021-07-02T08:14:16.950Z" }, current_user_membership: { muted: false, following: true, @@ -184,7 +184,7 @@ export const chatChannels = { status: "closed", chatable: chatables[8], allow_channel_wide_mentions: true, - last_message_sent_at: "2021-07-21T08:14:16.950Z", + last_message: { id: 333, created_at: "2021-07-02T08:14:16.950Z" }, current_user_membership: { muted: false, following: true, @@ -203,7 +203,7 @@ export const chatChannels = { status: "archived", chatable: chatables[8], allow_channel_wide_mentions: true, - last_message_sent_at: "2021-07-25T08:14:16.950Z", + last_message: { id: 333, created_at: "2021-07-02T08:14:16.950Z" }, current_user_membership: { muted: false, following: true, @@ -222,7 +222,7 @@ export const chatChannels = { status: "open", chatable: chatables[12], allow_channel_wide_mentions: true, - last_message_sent_at: "2021-07-02T08:14:16.950Z", + last_message: { id: 333, created_at: "2021-07-02T08:14:16.950Z" }, current_user_membership: { muted: false, following: true, diff --git a/plugins/chat/test/javascripts/components/chat-channel-metadata-test.js b/plugins/chat/test/javascripts/components/chat-channel-metadata-test.js index 09538f4b3d4..6304da0ed2d 100644 --- a/plugins/chat/test/javascripts/components/chat-channel-metadata-test.js +++ b/plugins/chat/test/javascripts/components/chat-channel-metadata-test.js @@ -8,10 +8,12 @@ import { render } from "@ember/test-helpers"; module("Discourse Chat | Component | chat-channel-metadata", function (hooks) { setupRenderingTest(hooks); - test("displays last message sent at", async function (assert) { + test("displays last message created at", async function (assert) { let lastMessageSentAt = moment().subtract(1, "day").format(); - this.channel = fabricators.directMessageChannel({ - last_message_sent_at: lastMessageSentAt, + this.channel = fabricators.directMessageChannel(); + this.channel.lastMessage = fabricators.message({ + channel: this.channel, + created_at: lastMessageSentAt, }); await render(hbs``); @@ -19,7 +21,7 @@ module("Discourse Chat | Component | chat-channel-metadata", function (hooks) { assert.dom(".chat-channel-metadata__date").hasText("Yesterday"); lastMessageSentAt = moment(); - this.channel.lastMessageSentAt = lastMessageSentAt; + this.channel.lastMessage.createdAt = lastMessageSentAt; await render(hbs``); assert diff --git a/plugins/chat/test/javascripts/components/chat-channel-row-test.js b/plugins/chat/test/javascripts/components/chat-channel-row-test.js index 14784ab6de1..e37d1887e4b 100644 --- a/plugins/chat/test/javascripts/components/chat-channel-row-test.js +++ b/plugins/chat/test/javascripts/components/chat-channel-row-test.js @@ -47,13 +47,15 @@ module("Discourse Chat | Component | chat-channel-row", function (hooks) { }); test("renders correct channel metadata", async function (assert) { - this.categoryChatChannel.lastMessageSentAt = moment().toISOString(); + this.categoryChatChannel.lastMessage = fabricators.message({ + created_at: moment().toISOString(), + }); await render(hbs``); assert .dom(".chat-channel-metadata") .hasText( - moment(this.categoryChatChannel.lastMessageSentAt).format("h:mm A") + moment(this.categoryChatChannel.lastMessage.createdAt).format("h:mm A") ); });