diff --git a/plugins/chat/app/controllers/chat/api/reads_controller.rb b/plugins/chat/app/controllers/chat/api/channels_read_controller.rb similarity index 80% rename from plugins/chat/app/controllers/chat/api/reads_controller.rb rename to plugins/chat/app/controllers/chat/api/channels_read_controller.rb index 7d25138ac5d..0d34f09af13 100644 --- a/plugins/chat/app/controllers/chat/api/reads_controller.rb +++ b/plugins/chat/app/controllers/chat/api/channels_read_controller.rb @@ -1,17 +1,15 @@ # frozen_string_literal: true -class Chat::Api::ReadsController < Chat::ApiController +class Chat::Api::ChannelsReadController < Chat::ApiController def update - params.require(%i[channel_id message_id]) - - with_service(Chat::UpdateUserLastRead) do + with_service(Chat::UpdateUserChannelLastRead) do on_success { render(json: success_json) } on_failure { render(json: failed_json, status: 422) } on_failed_policy(:ensure_message_id_recency) do raise Discourse::InvalidParameters.new(:message_id) end on_model_not_found(:message) { raise Discourse::NotFound } - on_model_not_found(:active_membership) { raise Discourse::NotFound } + on_model_not_found(:membership) { raise Discourse::NotFound } on_model_not_found(:channel) { raise Discourse::NotFound } on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess } on_failed_contract do |contract| diff --git a/plugins/chat/app/controllers/chat/api/thread_reads_controller.rb b/plugins/chat/app/controllers/chat/api/channels_threads_read_controller.rb similarity index 79% rename from plugins/chat/app/controllers/chat/api/thread_reads_controller.rb rename to plugins/chat/app/controllers/chat/api/channels_threads_read_controller.rb index 66620d7ddff..f957d06991c 100644 --- a/plugins/chat/app/controllers/chat/api/thread_reads_controller.rb +++ b/plugins/chat/app/controllers/chat/api/channels_threads_read_controller.rb @@ -1,13 +1,12 @@ # frozen_string_literal: true -class Chat::Api::ThreadReadsController < Chat::ApiController +class Chat::Api::ChannelsThreadsReadController < Chat::ApiController def update - params.require(%i[channel_id thread_id]) - with_service(Chat::UpdateUserThreadLastRead) do on_success { render(json: success_json) } on_failure { render(json: failed_json, status: 422) } on_model_not_found(:thread) { raise Discourse::NotFound } + on_model_not_found(:message) { raise Discourse::NotFound } on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess } on_failed_contract do |contract| render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400) diff --git a/plugins/chat/app/models/chat/thread.rb b/plugins/chat/app/models/chat/thread.rb index 4caab5a7884..8de438c01e4 100644 --- a/plugins/chat/app/models/chat/thread.rb +++ b/plugins/chat/app/models/chat/thread.rb @@ -55,12 +55,6 @@ module Chat user_chat_thread_memberships.find_by(user: user) end - def mark_read_for_user!(user, last_read_message_id: nil) - membership_for(user)&.update!( - last_read_message_id: last_read_message_id || self.last_message_id, - ) - end - def replies self.chat_messages.where.not(id: self.original_message_id).order("created_at ASC, id ASC") end diff --git a/plugins/chat/app/models/chat/user_chat_thread_membership.rb b/plugins/chat/app/models/chat/user_chat_thread_membership.rb index 49d2bdfd181..20bfd426b90 100644 --- a/plugins/chat/app/models/chat/user_chat_thread_membership.rb +++ b/plugins/chat/app/models/chat/user_chat_thread_membership.rb @@ -9,6 +9,10 @@ module Chat belongs_to :thread, class_name: "Chat::Thread", foreign_key: :thread_id enum :notification_level, Chat::NotificationLevels.all + + def mark_read!(new_last_read_id = nil) + update!(last_read_message_id: new_last_read_id || thread.last_message_id) + end end end diff --git a/plugins/chat/app/services/chat/list_channel_thread_messages.rb b/plugins/chat/app/services/chat/list_channel_thread_messages.rb index 5dd920e6fb3..6096d8a8ca2 100644 --- a/plugins/chat/app/services/chat/list_channel_thread_messages.rb +++ b/plugins/chat/app/services/chat/list_channel_thread_messages.rb @@ -69,9 +69,9 @@ module Chat end def determine_target_message_id(contract:, membership:, guardian:) - if contract.fetch_from_last_read + if contract.fetch_from_last_read || !contract.target_message_id context.target_message_id = membership&.last_read_message_id - else + elsif contract.target_message_id context.target_message_id = contract.target_message_id end end diff --git a/plugins/chat/app/services/chat/update_user_last_read.rb b/plugins/chat/app/services/chat/update_user_channel_last_read.rb similarity index 64% rename from plugins/chat/app/services/chat/update_user_last_read.rb rename to plugins/chat/app/services/chat/update_user_channel_last_read.rb index fae8773ea9d..94600d55ab8 100644 --- a/plugins/chat/app/services/chat/update_user_last_read.rb +++ b/plugins/chat/app/services/chat/update_user_channel_last_read.rb @@ -4,9 +4,9 @@ module Chat # Service responsible for updating the last read message id of a membership. # # @example - # Chat::UpdateUserLastRead.call(channel_id: 2, message_id: 3, guardian: guardian) + # Chat::UpdateUserChannelLastRead.call(channel_id: 2, message_id: 3, guardian: guardian) # - class UpdateUserLastRead + class UpdateUserChannelLastRead include ::Service::Base # @!method call(channel_id:, message_id:, guardian:) @@ -17,7 +17,7 @@ module Chat contract model :channel - model :active_membership + model :membership policy :invalid_access model :message policy :ensure_message_id_recency @@ -41,31 +41,30 @@ module Chat ::Chat::Channel.find_by(id: contract.channel_id) end - def fetch_active_membership(guardian:, channel:) + def fetch_membership(guardian:, channel:) ::Chat::ChannelMembershipManager.new(channel).find_for_user(guardian.user, following: true) end - def invalid_access(guardian:, active_membership:) - guardian.can_join_chat_channel?(active_membership.chat_channel) + def invalid_access(guardian:, membership:) + guardian.can_join_chat_channel?(membership.chat_channel) end def fetch_message(channel:, contract:) ::Chat::Message.with_deleted.find_by(chat_channel_id: channel.id, id: contract.message_id) end - def ensure_message_id_recency(message:, active_membership:) - !active_membership.last_read_message_id || - message.id >= active_membership.last_read_message_id + def ensure_message_id_recency(message:, membership:) + !membership.last_read_message_id || message.id >= membership.last_read_message_id end - def update_membership_state(message:, active_membership:) - active_membership.update!(last_read_message_id: message.id, last_viewed_at: Time.zone.now) + def update_membership_state(message:, membership:) + membership.update!(last_read_message_id: message.id, last_viewed_at: Time.zone.now) end - def mark_associated_mentions_as_read(active_membership:, message:) + def mark_associated_mentions_as_read(membership:, message:) ::Chat::Action::MarkMentionsRead.call( - active_membership.user, - channel_ids: [active_membership.chat_channel.id], + membership.user, + channel_ids: [membership.chat_channel.id], message_id: message.id, ) end 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 76a2792b626..5004416503a 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 @@ -2,13 +2,10 @@ module Chat # Service responsible for marking messages in a thread - # as read. For now this just marks any mentions in the thread - # as read and marks the entire thread as read. - # As we add finer-grained user tracking state to threads it - # will work in a similar way to Chat::UpdateUserLastRead. + # as read. # # @example - # Chat::UpdateUserThreadLastRead.call(channel_id: 2, thread_id: 3, guardian: guardian) + # Chat::UpdateUserThreadLastRead.call(channel_id: 2, thread_id: 3, message_id: 4, guardian: guardian) # class UpdateUserThreadLastRead include ::Service::Base @@ -16,20 +13,25 @@ module Chat # @!method call(channel_id:, thread_id:, guardian:) # @param [Integer] channel_id # @param [Integer] thread_id + # @param [Integer] message_id # @param [Guardian] guardian # @return [Service::Base::Context] contract model :thread policy :invalid_access + model :membership + model :message + policy :ensure_valid_message step :mark_associated_mentions_as_read step :mark_thread_read step :publish_new_last_read_to_clients # @!visibility private class Contract - attribute :thread_id, :integer attribute :channel_id, :integer + attribute :thread_id, :integer + attribute :message_id, :integer validates :thread_id, :channel_id, presence: true end @@ -40,31 +42,41 @@ module Chat ::Chat::Thread.find_by(id: contract.thread_id, channel_id: contract.channel_id) end + def fetch_message(contract:, thread:) + ::Chat::Message.with_deleted.find_by( + id: contract.message_id || thread.last_message_id, + thread_id: contract.thread_id, + chat_channel_id: contract.channel_id, + ) + end + + def fetch_membership(guardian:, thread:) + thread.membership_for(guardian.user) + end + def invalid_access(guardian:, thread:) guardian.can_join_chat_channel?(thread.channel) end - # NOTE: In future we will pass in a specific last_read_message_id - # to the service, so this will need to change because currently it's - # just using the thread's last_message_id. - def mark_thread_read(thread:, guardian:) - thread.mark_read_for_user!(guardian.user) + def ensure_valid_message(message:, membership:) + !membership.last_read_message_id || message.id >= membership.last_read_message_id end - def mark_associated_mentions_as_read(thread:, guardian:) + def mark_thread_read(membership:, message:) + membership.mark_read!(message.id) + end + + def mark_associated_mentions_as_read(thread:, guardian:, message:) ::Chat::Action::MarkMentionsRead.call( guardian.user, channel_ids: [thread.channel_id], thread_id: thread.id, + message_id: message.id, ) end - def publish_new_last_read_to_clients(guardian:, thread:) - ::Chat::Publisher.publish_user_tracking_state!( - guardian.user, - thread.channel, - thread.last_message, - ) + def publish_new_last_read_to_clients(guardian:, thread:, message:) + ::Chat::Publisher.publish_user_tracking_state!(guardian.user, thread.channel, message) end end end diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-channel.gjs index 63d6d239c41..a39ab3b4010 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel.gjs @@ -1,7 +1,6 @@ import Component from "@glimmer/component"; import { cached, tracked } from "@glimmer/tracking"; import { getOwner } from "@ember/application"; -import { hash } from "@ember/helper"; import { action } from "@ember/object"; import didInsert from "@ember/render-modifiers/modifiers/did-insert"; import didUpdate from "@ember/render-modifiers/modifiers/did-update"; @@ -28,10 +27,7 @@ import { READ_INTERVAL_MS, } from "discourse/plugins/chat/discourse/lib/chat-constants"; import ChatMessagesLoader from "discourse/plugins/chat/discourse/lib/chat-messages-loader"; -import { - checkMessageBottomVisibility, - checkMessageTopVisibility, -} from "discourse/plugins/chat/discourse/lib/check-message-visibility"; +import { checkMessageTopVisibility } from "discourse/plugins/chat/discourse/lib/check-message-visibility"; import DatesSeparatorsPositioner from "discourse/plugins/chat/discourse/lib/dates-separators-positioner"; import { extractCurrentTopicInfo } from "discourse/plugins/chat/discourse/lib/extract-current-topic-info"; import { @@ -40,14 +36,14 @@ import { } from "discourse/plugins/chat/discourse/lib/scroll-helpers"; import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message"; import { stackingContextFix } from "../lib/chat-ios-hacks"; -import ChatOnResize from "../modifiers/chat/on-resize"; -import ChatScrollableList from "../modifiers/chat/scrollable-list"; import ChatComposerChannel from "./chat/composer/channel"; import ChatScrollToBottomArrow from "./chat/scroll-to-bottom-arrow"; import ChatSelectionManager from "./chat/selection-manager"; import ChatChannelPreviewCard from "./chat-channel-preview-card"; import ChatMentionWarnings from "./chat-mention-warnings"; import Message from "./chat-message"; +import ChatMessagesContainer from "./chat-messages-container"; +import ChatMessagesScroller from "./chat-messages-scroller"; import ChatNotices from "./chat-notices"; import ChatSkeleton from "./chat-skeleton"; import ChatUploadDropZone from "./chat-upload-drop-zone"; @@ -78,7 +74,7 @@ export default class ChatChannel extends Component { @tracked uploadDropZone; @tracked isScrolling = false; - scrollable = null; + scroller = null; _mentionWarningsSeen = {}; _unreachableGroupMentions = []; _overMembersLimitGroupMentions = []; @@ -101,8 +97,8 @@ export default class ChatChannel extends Component { } @action - setScrollable(element) { - this.scrollable = element; + registerScroller(element) { + this.scroller = element; } @action @@ -117,7 +113,8 @@ export default class ChatChannel extends Component { @action didResizePane() { this.debounceFillPaneAttempt(); - DatesSeparatorsPositioner.apply(this.scrollable); + this.debouncedUpdateLastReadMessage(); + DatesSeparatorsPositioner.apply(this.scroller); } @action @@ -180,7 +177,7 @@ export default class ChatChannel extends Component { return; } - stackingContextFix(this.scrollable, () => { + stackingContextFix(this.scroller, () => { this.messagesManager.addMessages([message]); }); this.debouncedUpdateLastReadMessage(); @@ -244,7 +241,7 @@ export default class ChatChannel extends Component { } const targetMessageId = this.messagesManager.messages.lastObject.id; - stackingContextFix(this.scrollable, () => { + stackingContextFix(this.scroller, () => { this.messagesManager.addMessages(messages); }); @@ -261,14 +258,14 @@ export default class ChatChannel extends Component { @action async scrollToBottom() { this._ignoreNextScroll = true; - await scrollListToBottom(this.scrollable); + await scrollListToBottom(this.scroller); this.debouncedUpdateLastReadMessage(); } scrollToMessageId(messageId, options = {}) { this._ignoreNextScroll = true; const message = this.messagesManager.findMessage(messageId); - scrollListToMessage(this.scrollable, message, options); + scrollListToMessage(this.scroller, message, options); } debounceFillPaneAttempt() { @@ -309,12 +306,12 @@ export default class ChatChannel extends Component { schedule("afterRender", () => { const firstMessageId = this.messagesManager.messages.firstObject?.id; - const messageContainer = this.scrollable.querySelector( + const messageContainer = this.scroller.querySelector( `.chat-message-container[data-id="${firstMessageId}"]` ); if ( messageContainer && - checkMessageTopVisibility(this.scrollable, messageContainer) + checkMessageTopVisibility(this.scroller, messageContainer) ) { this.fetchMoreMessages({ direction: PAST }); } @@ -415,41 +412,28 @@ export default class ChatChannel extends Component { return; } - schedule("afterRender", () => { - let lastFullyVisibleMessageNode = null; + if (!this.lastFullyVisibleMessageId) { + return; + } - this.scrollable - .querySelectorAll(".chat-message-container") - .forEach((item) => { - if (checkMessageBottomVisibility(this.scrollable, item)) { - lastFullyVisibleMessageNode = item; - } - }); + let lastUnreadVisibleMessage = this.messagesManager.findMessage( + this.lastFullyVisibleMessageId + ); - if (!lastFullyVisibleMessageNode) { - return; - } + if (!lastUnreadVisibleMessage) { + return; + } - let lastUnreadVisibleMessage = this.messagesManager.findMessage( - lastFullyVisibleMessageNode.dataset.id - ); + const lastReadId = + this.args.channel.currentUserMembership?.lastReadMessageId; + if (lastReadId >= lastUnreadVisibleMessage.id) { + return; + } - if (!lastUnreadVisibleMessage) { - return; - } - - const lastReadId = - this.args.channel.currentUserMembership?.lastReadMessageId; - // we don't return early if === as we want to ensure different tabs will do the check - if (lastReadId > lastUnreadVisibleMessage.id) { - return; - } - - return this.chatApi.markChannelAsRead( - this.args.channel.id, - lastUnreadVisibleMessage.id - ); - }); + return this.chatApi.markChannelAsRead( + this.args.channel.id, + lastUnreadVisibleMessage.id + ); } @action @@ -457,7 +441,7 @@ export default class ChatChannel extends Component { if (this.messagesLoader.canLoadMoreFuture) { this.fetchMessages(); } else if (this.messagesManager.messages.length > 0) { - this.scrollToBottom(this.scrollable); + this.scrollToBottom(this.scroller); } } @@ -468,7 +452,7 @@ export default class ChatChannel extends Component { return; } - DatesSeparatorsPositioner.apply(this.scrollable); + DatesSeparatorsPositioner.apply(this.scroller); this.needsArrow = (this.messagesLoader.fetchedOnce && @@ -476,6 +460,7 @@ export default class ChatChannel extends Component { (state.distanceToBottom.pixels > 250 && !state.atBottom); this.isScrolling = true; this.debouncedUpdateLastReadMessage(); + this.lastFullyVisibleMessageId = state.lastVisibleId; if ( state.atTop || @@ -499,6 +484,7 @@ export default class ChatChannel extends Component { (state.distanceToBottom.pixels > 250 && !state.atBottom); this.isScrolling = false; this.atBottom = state.atBottom; + this.lastFullyVisibleMessageId = state.lastVisibleId; if (state.atBottom) { this.fetchMoreMessages({ direction: FUTURE }); @@ -537,7 +523,7 @@ export default class ChatChannel extends Component { this.resetComposerMessage(); try { - stackingContextFix(this.scrollable, async () => { + stackingContextFix(this.scroller, async () => { await this.chatApi.editMessage(this.args.channel.id, message.id, data); }); } catch (e) { @@ -553,7 +539,7 @@ export default class ChatChannel extends Component { resetIdle(); - stackingContextFix(this.scrollable, async () => { + stackingContextFix(this.scroller, async () => { await this.args.channel.stageMessage(message); }); @@ -712,19 +698,12 @@ export default class ChatChannel extends Component { -
-
+ {{#each this.messagesManager.messages key="id" as |message|}} {{/unless}} {{/each}} -
+ {{! at bottom even if shown at top due to column-reverse }} {{#if this.messagesLoader.loadedPast}} @@ -746,7 +725,7 @@ export default class ChatChannel extends Component { {{i18n "chat.all_loaded"}}
{{/if}} - + {{/if}} {{/if}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-messages-container.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-messages-container.gjs new file mode 100644 index 00000000000..629ff1cc819 --- /dev/null +++ b/plugins/chat/assets/javascripts/discourse/components/chat-messages-container.gjs @@ -0,0 +1,13 @@ +import { hash } from "@ember/helper"; +import ChatOnResize from "../modifiers/chat/on-resize"; + +const ChatMessagesContainer = ; + +export default ChatMessagesContainer; diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-messages-scroller.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-messages-scroller.gjs new file mode 100644 index 00000000000..5eac5e3ee7a --- /dev/null +++ b/plugins/chat/assets/javascripts/discourse/components/chat-messages-scroller.gjs @@ -0,0 +1,17 @@ +import { hash } from "@ember/helper"; +import didInsert from "@ember/render-modifiers/modifiers/did-insert"; +import ChatScrollableList from "../modifiers/chat/scrollable-list"; + +const ChatMessagesScroller = ; + +export default ChatMessagesScroller; diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-thread.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-thread.gjs index 622ce76ea49..cce9cb6dabc 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-thread.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-thread.gjs @@ -1,7 +1,6 @@ import Component from "@glimmer/component"; import { cached, tracked } from "@glimmer/tracking"; import { getOwner } from "@ember/application"; -import { hash } from "@ember/helper"; import { action } from "@ember/object"; import didInsert from "@ember/render-modifiers/modifiers/did-insert"; import willDestroy from "@ember/render-modifiers/modifiers/will-destroy"; @@ -30,12 +29,12 @@ import { } from "discourse/plugins/chat/discourse/lib/scroll-helpers"; import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message"; import UserChatThreadMembership from "discourse/plugins/chat/discourse/models/user-chat-thread-membership"; -import ChatOnResize from "../modifiers/chat/on-resize"; -import ChatScrollableList from "../modifiers/chat/scrollable-list"; import ChatComposerThread from "./chat/composer/thread"; import ChatScrollToBottomArrow from "./chat/scroll-to-bottom-arrow"; import ChatSelectionManager from "./chat/selection-manager"; import Message from "./chat-message"; +import ChatMessagesContainer from "./chat-messages-container"; +import ChatMessagesScroller from "./chat-messages-scroller"; import ChatSkeleton from "./chat-skeleton"; import ChatUploadDropZone from "./chat-upload-drop-zone"; @@ -58,7 +57,7 @@ export default class ChatThread extends Component { @tracked needsArrow = false; @tracked uploadDropZone; - scrollable = null; + scroller = null; @action resetIdle() { @@ -116,7 +115,7 @@ export default class ChatThread extends Component { return; } - DatesSeparatorsPositioner.apply(this.scrollable); + DatesSeparatorsPositioner.apply(this.scroller); this.needsArrow = (this.messagesLoader.fetchedOnce && @@ -124,6 +123,7 @@ export default class ChatThread extends Component { (state.distanceToBottom.pixels > 250 && !state.atBottom); this.isScrolling = true; this.debounceUpdateLastReadMessage(); + this.lastFullyVisibleMessageId = state.lastVisibleId; if ( state.atTop || @@ -148,6 +148,7 @@ export default class ChatThread extends Component { this.resetIdle(); this.atBottom = state.atBottom; this.args.setFullTitle?.(state.atTop); + this.lastFullyVisibleMessageId = state.lastVisibleId; if (state.atBottom) { this.fetchMoreMessages({ direction: FUTURE }); @@ -164,16 +165,37 @@ export default class ChatThread extends Component { @bind updateLastReadMessage() { - // HACK: We don't have proper scroll visibility over - // what message we are looking at, don't have the lastReadMessageId - // for the thread, and this updateLastReadMessage function is only - // called when scrolling all the way to the bottom. - this.markThreadAsRead(); + if (!this.args.thread?.currentUserMembership) { + return; + } + + if (!this.lastFullyVisibleMessageId) { + return; + } + + const lastUnreadVisibleMessage = this.messagesManager.findMessage( + this.lastFullyVisibleMessageId + ); + + if (!lastUnreadVisibleMessage) { + return; + } + + const lastReadId = this.args.thread.currentUserMembership.lastReadMessageId; + if (lastReadId >= lastUnreadVisibleMessage.id) { + return; + } + + return this.chatApi.markThreadAsRead( + this.args.thread.channel.id, + this.args.thread.id, + lastUnreadVisibleMessage.id + ); } @action - setScrollable(element) { - this.scrollable = element; + registerScroller(element) { + this.scroller = element; } @action @@ -191,7 +213,7 @@ export default class ChatThread extends Component { this._ignoreNextScroll = true; this.debounceFillPaneAttempt(); this.debounceUpdateLastReadMessage(); - DatesSeparatorsPositioner.apply(this.scrollable); + DatesSeparatorsPositioner.apply(this.scroller); } async fetchMessages(findArgs = {}) { @@ -215,17 +237,13 @@ export default class ChatThread extends Component { } const [messages, meta] = this.processMessages(this.args.thread, result); - stackingContextFix(this.scrollable, () => { + stackingContextFix(this.scroller, () => { this.messagesManager.addMessages(messages); }); this.args.thread.details = meta; - if (this.args.targetMessageId) { - this.scrollToMessageId(this.args.targetMessageId, { highlight: true }); - } else if (this.args.thread.currentUserMembership?.lastReadMessageId) { - this.scrollToMessageId( - this.args.thread.currentUserMembership?.lastReadMessageId - ); + if (meta.target_message_id) { + this.scrollToMessageId(meta.target_message_id, { highlight: true }); } else { this.scrollToTop(); } @@ -249,7 +267,7 @@ export default class ChatThread extends Component { return; } - stackingContextFix(this.scrollable, () => { + stackingContextFix(this.scroller, () => { this.messagesManager.addMessages(messages); }); this.args.thread.details = meta; @@ -311,7 +329,7 @@ export default class ChatThread extends Component { ) { this._ignoreNextScroll = true; const message = this.messagesManager.findMessage(messageId); - scrollListToMessage(this.scrollable, message, opts); + scrollListToMessage(this.scroller, message, opts); } @bind @@ -322,7 +340,7 @@ export default class ChatThread extends Component { return; } - stackingContextFix(this.scrollable, () => { + stackingContextFix(this.scroller, () => { this.messagesManager.addMessages([message]); }); } @@ -345,20 +363,6 @@ export default class ChatThread extends Component { return [messages, result.meta]; } - // NOTE: At some point we want to do this based on visible messages - // and scrolling; for now it's enough to do it when the thread panel - // opens/messages are loaded since we have no pagination for threads. - markThreadAsRead() { - if (!this.args.thread) { - return; - } - - return this.chatApi.markThreadAsRead( - this.args.thread.channel.id, - this.args.thread.id - ); - } - @action async onSendMessage(message) { resetIdle(); @@ -424,7 +428,7 @@ export default class ChatThread extends Component { this.chatThreadPane.sending = true; this._ignoreNextScroll = true; - stackingContextFix(this.scrollable, async () => { + stackingContextFix(this.scroller, async () => { await this.args.thread.stageMessage(message); }); this.resetComposerMessage(); @@ -495,13 +499,13 @@ export default class ChatThread extends Component { @action async scrollToBottom() { this._ignoreNextScroll = true; - await scrollListToBottom(this.scrollable); + await scrollListToBottom(this.scroller); } @action async scrollToTop() { this._ignoreNextScroll = true; - await scrollListToTop(this.scrollable); + await scrollListToTop(this.scroller); } @action @@ -538,19 +542,12 @@ export default class ChatThread extends Component { {{didInsert this.setup}} {{willDestroy this.teardown}} > -
-
+ {{#each this.messagesManager.messages key="id" as |message|}} {{/if}} {{/unless}} -
-
+ + {{/if}} diff --git a/plugins/chat/assets/javascripts/discourse/lib/chat-channel-subscription-manager.js b/plugins/chat/assets/javascripts/discourse/lib/chat-channel-subscription-manager.js index 591052ec4de..efac29cb41d 100644 --- a/plugins/chat/assets/javascripts/discourse/lib/chat-channel-subscription-manager.js +++ b/plugins/chat/assets/javascripts/discourse/lib/chat-channel-subscription-manager.js @@ -36,7 +36,6 @@ export default class ChatChannelSubscriptionManager { teardown() { this.messageBus.unsubscribe(this.messageBusChannel, this.onMessage); - this.modelId = null; } @bind @@ -194,7 +193,7 @@ export default class ChatChannelSubscriptionManager { if (message) { message.deletedAt = null; } else { - const newMessage = ChatMessage.create(this.model, data.chat_message); + const newMessage = ChatMessage.create(this.channel, data.chat_message); newMessage.manager = this.messagesManager; this.messagesManager.addMessages([newMessage]); } diff --git a/plugins/chat/assets/javascripts/discourse/modifiers/chat/scrollable-list.js b/plugins/chat/assets/javascripts/discourse/modifiers/chat/scrollable-list.js index 0c24cecc3eb..c020dccd179 100644 --- a/plugins/chat/assets/javascripts/discourse/modifiers/chat/scrollable-list.js +++ b/plugins/chat/assets/javascripts/discourse/modifiers/chat/scrollable-list.js @@ -27,6 +27,8 @@ export default class ChatScrollableList extends Modifier { this.element.addEventListener("wheel", this.handleWheel, { passive: true, }); + + this.throttleComputeScroll(); } @bind diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-api.js b/plugins/chat/assets/javascripts/discourse/services/chat-api.js index 3f0d420b37e..40014c29ac4 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-api.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-api.js @@ -493,21 +493,25 @@ export default class ChatApi extends Service { * @returns {Promise} */ markChannelAsRead(channelId, messageId = null) { - return this.#putRequest(`/channels/${channelId}/read/${messageId}`); + return this.#putRequest( + `/channels/${channelId}/read?message_id=${messageId}` + ); } /** - * Marks all messages and mentions in a thread as read. This is quite - * far-reaching for now, and is not granular since there is no membership/ - * read state per-user for threads. In future this will be expanded to - * also pass message ID in the same way as markChannelAsRead + * Marks messages for a single user chat thread membership as read. If no + * message ID is provided, then the latest message for the channel is fetched + * on the server and used for the last read message. * * @param {number} channelId - The ID of the channel for the thread being marked as read. * @param {number} threadId - The ID of the thread being marked as read. + * @param {number} messageId - The ID of the message being marked as read. * @returns {Promise} */ - markThreadAsRead(channelId, threadId) { - return this.#putRequest(`/channels/${channelId}/threads/${threadId}/read`); + markThreadAsRead(channelId, threadId, messageId) { + return this.#putRequest( + `/channels/${channelId}/threads/${threadId}/read?message_id=${messageId}` + ); } /** diff --git a/plugins/chat/assets/stylesheets/common/base-common.scss b/plugins/chat/assets/stylesheets/common/base-common.scss index 48f42a0a76b..e383e5a65c8 100644 --- a/plugins/chat/assets/stylesheets/common/base-common.scss +++ b/plugins/chat/assets/stylesheets/common/base-common.scss @@ -195,7 +195,7 @@ body.has-full-page-chat { top: var(--header-offset); } - .chat-messages-scroll { + .chat-messages-scroller { box-sizing: border-box; height: 100%; } diff --git a/plugins/chat/assets/stylesheets/common/chat-channel.scss b/plugins/chat/assets/stylesheets/common/chat-channel.scss index fcd271f1fd9..99a85aa9532 100644 --- a/plugins/chat/assets/stylesheets/common/chat-channel.scss +++ b/plugins/chat/assets/stylesheets/common/chat-channel.scss @@ -9,31 +9,18 @@ min-width: 250px; @include chat-height(var(--chat-header-offset, 0px)); - .chat-messages-scroll { - flex-grow: 1; - overflow-y: scroll; - overscroll-behavior: contain; - display: flex; - flex-direction: column-reverse; - z-index: 1; - margin: 0 1px 0 0; - will-change: transform; - @include chat-scrollbar(); - min-height: 1px; + .join-channel-btn.in-float { + position: absolute; + transform: translateX(-50%); + left: 50%; + top: 10px; + z-index: 10; + } - .join-channel-btn.in-float { - position: absolute; - transform: translateX(-50%); - left: 50%; - top: 10px; - z-index: 10; - } - - .all-loaded-message { - text-align: center; - color: var(--primary-medium); - font-size: var(--font-down-1); - padding: 0.5em 0.25em 0.25em; - } + .all-loaded-message { + text-align: center; + color: var(--primary-medium); + font-size: var(--font-down-1); + padding: 0.5em 0.25em 0.25em; } } diff --git a/plugins/chat/assets/stylesheets/common/chat-messages-scroller.scss b/plugins/chat/assets/stylesheets/common/chat-messages-scroller.scss new file mode 100644 index 00000000000..572d4e4b04a --- /dev/null +++ b/plugins/chat/assets/stylesheets/common/chat-messages-scroller.scss @@ -0,0 +1,14 @@ +.chat-messages-scroller { + flex-grow: 1; + overflow-y: scroll; + overscroll-behavior: contain; + display: flex; + flex-direction: column-reverse; + z-index: 1; + margin: 0 1px 0 0; + will-change: transform; + @include chat-scrollbar(); + min-height: 1px; + box-sizing: border-box; + transition: padding-top 0.2s ease-in-out; +} diff --git a/plugins/chat/assets/stylesheets/common/chat-thread.scss b/plugins/chat/assets/stylesheets/common/chat-thread.scss index cecafac5e69..0a16859210f 100644 --- a/plugins/chat/assets/stylesheets/common/chat-thread.scss +++ b/plugins/chat/assets/stylesheets/common/chat-thread.scss @@ -3,15 +3,4 @@ flex-direction: column; position: relative; @include chat-height(var(--chat-header-expanded-offset, 0px)); - - &__body { - overflow-y: scroll; - @include chat-scrollbar(); - box-sizing: border-box; - flex-grow: 1; - overscroll-behavior: contain; - display: flex; - flex-direction: column-reverse; - transition: padding-top 0.2s ease-in-out; - } } diff --git a/plugins/chat/assets/stylesheets/common/index.scss b/plugins/chat/assets/stylesheets/common/index.scss index 8c3bd0b9c3f..a9becca82ff 100644 --- a/plugins/chat/assets/stylesheets/common/index.scss +++ b/plugins/chat/assets/stylesheets/common/index.scss @@ -69,3 +69,4 @@ @import "chat-thread-title"; @import "chat-audio-upload"; @import "chat-message-text"; +@import "chat-messages-scroller"; diff --git a/plugins/chat/assets/stylesheets/mobile/chat-channel.scss b/plugins/chat/assets/stylesheets/mobile/chat-channel.scss index 5eabe9eec06..be841c30990 100644 --- a/plugins/chat/assets/stylesheets/mobile/chat-channel.scss +++ b/plugins/chat/assets/stylesheets/mobile/chat-channel.scss @@ -1,5 +1,5 @@ .chat-channel { - .chat-messages-scroll { + .chat-messages-scroller { padding-bottom: 5px; } } diff --git a/plugins/chat/assets/stylesheets/mobile/chat-thread.scss b/plugins/chat/assets/stylesheets/mobile/chat-thread.scss index 7659acf76b8..6c8baa11000 100644 --- a/plugins/chat/assets/stylesheets/mobile/chat-thread.scss +++ b/plugins/chat/assets/stylesheets/mobile/chat-thread.scss @@ -3,7 +3,7 @@ margin: 0 1px 0 0; } - .chat-messages-scroll { + .chat-messages-scroller { padding: 10px 10px 0 10px; } } diff --git a/plugins/chat/config/routes.rb b/plugins/chat/config/routes.rb index 8e49e12f4c8..f4f5f3ca92c 100644 --- a/plugins/chat/config/routes.rb +++ b/plugins/chat/config/routes.rb @@ -7,8 +7,8 @@ Chat::Engine.routes.draw do get "/me/channels" => "current_user_channels#index" get "/me/threads" => "current_user_threads#index" post "/channels" => "channels#create" - put "/channels/read/" => "reads#update_all" - put "/channels/:channel_id/read/:message_id" => "reads#update" + put "/channels/read" => "channels_read#update_all" + put "/channels/:channel_id/read" => "channels_read#update" post "/channels/:channel_id/messages/:message_id/flags" => "channels_messages_flags#create" post "/channels/:channel_id/drafts" => "channels_drafts#create" delete "/channels/:channel_id" => "channels#destroy" @@ -45,7 +45,7 @@ Chat::Engine.routes.draw do put "/channels/:channel_id/threads/:thread_id" => "channel_threads#update" get "/channels/:channel_id/threads/:thread_id" => "channel_threads#show" get "/channels/:channel_id/threads/:thread_id/messages" => "channel_thread_messages#index" - put "/channels/:channel_id/threads/:thread_id/read" => "thread_reads#update" + put "/channels/:channel_id/threads/:thread_id/read" => "channels_threads_read#update" post "/channels/:channel_id/threads/:thread_id/drafts" => "channels_threads_drafts#create" put "/channels/:channel_id/threads/:thread_id/notifications-settings/me" => "channel_threads_current_user_notifications_settings#update" diff --git a/plugins/chat/spec/requests/chat/api/reads_controller_spec.rb b/plugins/chat/spec/requests/chat/api/channels_read_controller_spec.rb similarity index 91% rename from plugins/chat/spec/requests/chat/api/reads_controller_spec.rb rename to plugins/chat/spec/requests/chat/api/channels_read_controller_spec.rb index 85add0ad9c3..74b247b7e71 100644 --- a/plugins/chat/spec/requests/chat/api/reads_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/channels_read_controller_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -RSpec.describe Chat::Api::ReadsController do +RSpec.describe Chat::Api::ChannelsReadController do fab!(:current_user) { Fabricate(:user) } before do @@ -17,7 +17,7 @@ RSpec.describe Chat::Api::ReadsController do fab!(:message_2) { Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) } it "returns a 404 when the user is not a channel member" do - put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json" + put "/chat/api/channels/#{chat_channel.id}/read?message_id=#{message_1.id}.json" expect(response.status).to eq(404) end @@ -29,7 +29,7 @@ RSpec.describe Chat::Api::ReadsController do following: false, ) - put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json" + put "/chat/api/channels/#{chat_channel.id}/read?message_id=#{message_1.id}.json" expect(response.status).to eq(404) end @@ -49,7 +49,7 @@ RSpec.describe Chat::Api::ReadsController do before { membership.update!(last_read_message_id: message_2.id) } it "raises an invalid request" do - put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json" + put "/chat/api/channels/#{chat_channel.id}/read?message_id=#{message_1.id}.json" expect(response.status).to eq(400) expect(response.parsed_body["errors"][0]).to match(/message_id/) end @@ -59,14 +59,14 @@ RSpec.describe Chat::Api::ReadsController do before { message_1.trash!(Discourse.system_user) } it "works" do - put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}" + put "/chat/api/channels/#{chat_channel.id}/read?message_id=#{message_1.id}" expect(response.status).to eq(200) end end it "updates timing records" do expect { - put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json" + put "/chat/api/channels/#{chat_channel.id}/read?message_id=#{message_1.id}.json" }.not_to change { Chat::UserChatChannelMembership.count } membership.reload @@ -78,7 +78,7 @@ RSpec.describe Chat::Api::ReadsController do it "marks all mention notifications as read for the channel" do notification = create_notification_and_mention_for(current_user, other_user, message_1) - put "/chat/api/channels/#{chat_channel.id}/read/#{message_2.id}.json" + put "/chat/api/channels/#{chat_channel.id}/read?message_id=#{message_2.id}.json" expect(response.status).to eq(200) expect(notification.reload.read).to eq(true) end @@ -87,7 +87,7 @@ RSpec.describe Chat::Api::ReadsController do message_3 = Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) notification = create_notification_and_mention_for(current_user, other_user, message_3) - put "/chat/api/channels/#{chat_channel.id}/read/#{message_2.id}.json" + put "/chat/api/channels/#{chat_channel.id}/read?message_id=#{message_2.id}.json" expect(response.status).to eq(200) expect(notification.reload.read).to eq(false) end diff --git a/plugins/chat/spec/requests/chat/api/channels_threads_read_controller_spec.rb b/plugins/chat/spec/requests/chat/api/channels_threads_read_controller_spec.rb new file mode 100644 index 00000000000..d41cf8d10c8 --- /dev/null +++ b/plugins/chat/spec/requests/chat/api/channels_threads_read_controller_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +RSpec.describe Chat::Api::ChannelsThreadsReadController do + fab!(:current_user) { Fabricate(:user) } + + before do + SiteSetting.chat_enabled = true + SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone] + sign_in(current_user) + end + + describe "#update" do + context "with valid params" do + fab!(:thread_1) { Fabricate(:chat_thread) } + + before { thread_1.add(current_user) } + + it "is a success" do + put "/chat/api/channels/#{thread_1.channel.id}/threads/#{thread_1.id}/read.json" + + expect(response.status).to eq(200) + end + + context "when a message_id is provided" do + fab!(:message_1) do + Fabricate(:chat_message, thread: thread_1, chat_channel: thread_1.channel) + end + + it "updates the last read" do + expect { + put "/chat/api/channels/#{thread_1.channel.id}/threads/#{thread_1.id}/read?message_id=#{message_1.id}.json" + }.to change { thread_1.membership_for(current_user).last_read_message_id }.from(nil).to( + message_1.id, + ) + + expect(response.status).to eq(200) + end + end + end + + context "when the thread doesn't exist" do + fab!(:channel_1) { Fabricate(:chat_channel) } + + it "raises a not found" do + put "/chat/api/channels/#{channel_1.id}/threads/-999/read.json" + + expect(response.status).to eq(404) + end + end + + context "when the user can't join associated channel" do + fab!(:channel_1) { Fabricate(:private_category_channel) } + fab!(:thread_1) { Fabricate(:chat_thread, channel: channel_1) } + + before { thread_1.add(current_user) } + + it "raises a not found" do + put "/chat/api/channels/#{thread_1.channel.id}/threads/#{thread_1.id}/read.json" + + expect(response.status).to eq(403) + end + end + end +end diff --git a/plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb b/plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb deleted file mode 100644 index 75f4a079bfe..00000000000 --- a/plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb +++ /dev/null @@ -1,72 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe Chat::Api::ThreadReadsController do - fab!(:current_user) { Fabricate(:user) } - - before do - SiteSetting.chat_enabled = true - SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone] - sign_in(current_user) - end - - describe "#update" do - describe "marking the thread messages as read" do - fab!(:channel) { Fabricate(:chat_channel) } - fab!(:other_user) { Fabricate(:user) } - fab!(:thread) { Fabricate(:chat_thread, channel: channel) } - fab!(:message_1) do - Fabricate(:chat_message, chat_channel: channel, user: other_user, thread: thread) - end - fab!(:message_2) do - Fabricate(:chat_message, chat_channel: channel, user: other_user, thread: thread) - end - - context "when the user cannot access the channel" do - fab!(:channel) { Fabricate(:private_category_channel) } - fab!(:thread) { Fabricate(:chat_thread, channel: channel) } - it "raises invalid access" do - put "/chat/api/channels/#{channel.id}/threads/#{thread.id}/read.json" - expect(response.status).to eq(403) - end - end - - context "when the channel_id and thread_id params do not match" do - it "raises a not found" do - put "/chat/api/channels/#{Fabricate(:chat_channel).id}/threads/#{thread.id}/read.json" - expect(response.status).to eq(404) - end - end - - it "marks all mention notifications as read for the thread" do - notification_1 = create_notification_and_mention_for(current_user, other_user, message_1) - notification_2 = create_notification_and_mention_for(current_user, other_user, message_2) - - put "/chat/api/channels/#{channel.id}/threads/#{thread.id}/read.json" - expect(response.status).to eq(200) - expect(notification_1.reload.read).to eq(true) - expect(notification_2.reload.read).to eq(true) - end - end - end - - def create_notification_and_mention_for(user, sender, msg) - Notification - .create!( - notification_type: Notification.types[:chat_mention], - user: user, - high_priority: true, - read: false, - data: { - message: "chat.mention_notification", - chat_message_id: msg.id, - chat_channel_id: msg.chat_channel_id, - chat_channel_title: msg.chat_channel.title(user), - chat_channel_slug: msg.chat_channel.slug, - mentioned_by_username: sender.username, - }.to_json, - ) - .tap do |notification| - Chat::UserMention.create!(user: user, chat_message: msg, notifications: [notification]) - end - end -end 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 d8a03d962fe..a0618336903 100644 --- a/plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb +++ b/plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb @@ -170,9 +170,9 @@ RSpec.describe ::Chat::LookupChannelThreads do [thread_4, thread_5, thread_6, thread_7].each do |t| t.add(current_user) - t.mark_read_for_user!(current_user) + t.membership_for(current_user).mark_read! end - [thread_1, thread_2, thread_3].each { |t| t.mark_read_for_user!(current_user) } + [thread_1, thread_2, thread_3].each { |t| t.membership_for(current_user).mark_read! } # The old unread messages. Fabricate(:chat_message, chat_channel: channel_1, thread: thread_7).update!( diff --git a/plugins/chat/spec/services/chat/update_user_last_read_spec.rb b/plugins/chat/spec/services/chat/update_user_channel_last_read_spec.rb similarity index 95% rename from plugins/chat/spec/services/chat/update_user_last_read_spec.rb rename to plugins/chat/spec/services/chat/update_user_channel_last_read_spec.rb index 935430d18ec..854890c0131 100644 --- a/plugins/chat/spec/services/chat/update_user_last_read_spec.rb +++ b/plugins/chat/spec/services/chat/update_user_channel_last_read_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true -RSpec.describe Chat::UpdateUserLastRead do - describe Chat::UpdateUserLastRead::Contract, type: :model do +RSpec.describe Chat::UpdateUserChannelLastRead do + describe Chat::UpdateUserChannelLastRead::Contract, type: :model do it { is_expected.to validate_presence_of :channel_id } it { is_expected.to validate_presence_of :message_id } end @@ -32,7 +32,7 @@ RSpec.describe Chat::UpdateUserLastRead do context "when user has no membership" do before { membership.destroy! } - it { is_expected.to fail_to_find_a_model(:active_membership) } + it { is_expected.to fail_to_find_a_model(:membership) } end context "when user can’t access the channel" 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 39020398638..e94a8de922d 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,15 +11,23 @@ RSpec.describe Chat::UpdateUserThreadLastRead do fab!(:chatters) { Fabricate(:group) } fab!(:current_user) { Fabricate(:user, group_ids: [chatters.id]) } - fab!(:channel) { Fabricate(:chat_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) } + fab!(:thread) { Fabricate(:chat_thread, old_om: true) } + fab!(:reply_1) { Fabricate(:chat_message, thread: thread, chat_channel_id: thread.channel.id) } let(:guardian) { Guardian.new(current_user) } - let(:params) { { guardian: guardian, channel_id: channel.id, thread_id: thread.id } } + let(:params) do + { + message_id: reply_1.id, + guardian: guardian, + channel_id: thread.channel.id, + thread_id: thread.id, + } + end - before { SiteSetting.chat_allowed_groups = [chatters] } + before do + thread.add(current_user) + SiteSetting.chat_allowed_groups = [chatters] + end context "when params are not valid" do before { params.delete(:thread_id) } @@ -27,81 +35,108 @@ RSpec.describe Chat::UpdateUserThreadLastRead do it { is_expected.to fail_a_contract } end + context "when thread cannot be found" do + before { params[:channel_id] = Fabricate(:chat_channel).id } + + it { is_expected.to fail_to_find_a_model(:thread) } + end + + context "when user has no membership" do + before { thread.remove(current_user) } + + it { is_expected.to fail_to_find_a_model(:membership) } + end + + context "when user can’t access the channel" do + fab!(:channel) { Fabricate(:private_category_channel) } + fab!(:thread) { Fabricate(:chat_thread, channel: channel) } + + it { is_expected.to fail_a_policy(:invalid_access) } + end + context "when params are valid" do - context "when user can’t access the channel" do - fab!(:channel) { Fabricate(:private_category_channel) } - fab!(:thread) { Fabricate(:chat_thread, channel: channel) } - - it { is_expected.to fail_a_policy(:invalid_access) } + it "sets the service result as successful" do + expect(result).to be_a_success end - context "when thread cannot be found" do - before { params[:channel_id] = Fabricate(:chat_channel).id } - - it { is_expected.to fail_to_find_a_model(:thread) } + it "publishes new last read to clients" do + messages = MessageBus.track_publish { result } + expect(messages.map(&:channel)).to include("/chat/user-tracking-state/#{current_user.id}") end - context "when everything is fine" do - fab!(:notification_1) do - Fabricate( - :notification, - notification_type: Notification.types[:chat_mention], - user: current_user, - ) + context "when the user is a member of the thread" do + fab!(:membership) do + Fabricate(:user_chat_thread_membership, user: current_user, thread: thread) end - fab!(:notification_2) do - Fabricate( - :notification, - notification_type: Notification.types[:chat_mention], - user: current_user, + + it "updates the last_read_message_id of the thread" do + expect { result }.to change { membership.reload.last_read_message_id }.from(nil).to( + reply_1.id, ) end - let(:messages) { MessageBus.track_publish { result } } + context "when the provided last read id is before the existing one" do + fab!(:reply_2) { Fabricate(:chat_message, thread: thread) } - before do - Jobs.run_immediately! - Chat::UserMention.create!( - notifications: [notification_1], - user: current_user, - chat_message: Fabricate(:chat_message, chat_channel: channel, thread: thread), - ) - Chat::UserMention.create!( - notifications: [notification_2], - user: current_user, - chat_message: Fabricate(:chat_message, chat_channel: channel, thread: thread), - ) + before { thread.membership_for(current_user).update!(last_read_message_id: reply_2.id) } + + it { is_expected.to fail_a_policy(:ensure_valid_message) } end - it "sets the service result as successful" do - expect(result).to be_a_success - end - - it "marks existing notifications related to all messages in the thread as read" do - expect { result }.to change { - Notification.where( - notification_type: Notification.types[:chat_mention], - user: current_user, - read: false, - ).count - }.by(-2) - end - - it "publishes new last read to clients" do - expect(messages.map(&:channel)).to include("/chat/user-tracking-state/#{current_user.id}") - end - - context "when the user is a member of the thread" do - fab!(:membership) do - Fabricate(:user_chat_thread_membership, user: current_user, thread: thread) - end - - it "updates the last_read_message_id of the thread" do - result - expect(membership.reload.last_read_message_id).to eq(thread.reload.last_message.id) + context "when the message doesn’t exist" do + it "fails" do + params[:message_id] = 999 + is_expected.to fail_to_find_a_model(:message) end end end end + + context "when unread messages have associated notifications" do + before_all do + Jobs.run_immediately! + thread.channel.add(current_user) + end + + fab!(:reply_2) do + Fabricate( + :chat_message, + thread: thread, + message: "hi @#{current_user.username}", + use_service: true, + ) + end + + fab!(:reply_3) do + Fabricate( + :chat_message, + thread: thread, + message: "hi @#{current_user.username}", + use_service: true, + ) + end + + it "marks notifications as read" do + params[:message_id] = reply_2.id + + expect { described_class.call(params) }.to change { + ::Notification + .where(notification_type: Notification.types[:chat_mention]) + .where(user: current_user) + .where(read: false) + .count + }.by(-1) + + params[:message_id] = reply_3.id + + expect { described_class.call(params) }.to change { + ::Notification + .where(notification_type: Notification.types[:chat_mention]) + .where(user: current_user) + .where(read: false) + .count + }.by(-1) + end + end end end diff --git a/plugins/chat/spec/system/chat_channel_spec.rb b/plugins/chat/spec/system/chat_channel_spec.rb index 5c9c03ed99c..48960115335 100644 --- a/plugins/chat/spec/system/chat_channel_spec.rb +++ b/plugins/chat/spec/system/chat_channel_spec.rb @@ -146,7 +146,7 @@ RSpec.describe "Chat channel", type: :system do end context "when returning to a channel where last read is not last message" do - it "jumps to the bottom of the channel" do + it "scrolls to the correct last read message" do channel_1.membership_for(current_user).update!(last_read_message: message_1) messages = Fabricate.times(50, :chat_message, chat_channel: channel_1) chat_page.visit_channel(channel_1) @@ -348,7 +348,7 @@ RSpec.describe "Chat channel", type: :system do ".chat-message-actions-container[data-id='#{last_message["data-id"]}']", ) - find(".chat-messages-scroll").scroll_to(0, -1000) + find(".chat-messages-scroller").scroll_to(0, -1000) expect(page).to have_no_css( ".chat-message-actions-container[data-id='#{last_message["data-id"]}']", diff --git a/plugins/chat/spec/system/page_objects/chat/components/messages.rb b/plugins/chat/spec/system/page_objects/chat/components/messages.rb index 24f6472d73b..239bdc77030 100644 --- a/plugins/chat/spec/system/page_objects/chat/components/messages.rb +++ b/plugins/chat/spec/system/page_objects/chat/components/messages.rb @@ -6,7 +6,7 @@ module PageObjects class Messages < PageObjects::Components::Base attr_reader :context - SELECTOR = ".chat-messages-scroll" + SELECTOR = ".chat-messages-scroller" def initialize(context) @context = context diff --git a/plugins/chat/spec/system/single_thread_spec.rb b/plugins/chat/spec/system/single_thread_spec.rb index d38a20da396..64b9da4b257 100644 --- a/plugins/chat/spec/system/single_thread_spec.rb +++ b/plugins/chat/spec/system/single_thread_spec.rb @@ -34,6 +34,17 @@ describe "Single thread in side panel", type: :system do fab!(:channel) { Fabricate(:chat_channel, threading_enabled: true) } fab!(:thread) { chat_thread_chain_bootstrap(channel: channel, users: [current_user, user_2]) } + context "when returning to a thread where last read is not last message" do + it "scrolls to the correct last read message" do + message_1 = Fabricate(:chat_message, thread: thread, chat_channel: channel) + thread.membership_for(current_user).update!(last_read_message: message_1) + messages = Fabricate.times(50, :chat_message, thread: thread, chat_channel: channel) + chat_page.visit_thread(thread) + + expect(page).to have_css("[data-id='#{message_1.id}'].-highlighted") + end + end + context "when in full page" do context "when switching channel" do fab!(:channel_2) { Fabricate(:chat_channel, threading_enabled: true) } diff --git a/plugins/chat/spec/system/update_last_read_spec.rb b/plugins/chat/spec/system/update_last_read_spec.rb index 5f41e3f55ff..8ef66f20612 100644 --- a/plugins/chat/spec/system/update_last_read_spec.rb +++ b/plugins/chat/spec/system/update_last_read_spec.rb @@ -23,8 +23,8 @@ RSpec.describe "Update last read", type: :system do chat_page.visit_channel(channel_1) try_until_success do - page.execute_script("document.querySelector('.chat-messages-scroll').scrollTo(0, 1)") - page.execute_script("document.querySelector('.chat-messages-scroll').scrollTo(0, 0)") + page.execute_script("document.querySelector('.chat-messages-scroller').scrollTo(0, 1)") + page.execute_script("document.querySelector('.chat-messages-scroller').scrollTo(0, 0)") expect(membership.reload.last_read_message_id).to eq(last_message.id) end end