From e51bbfa4e813bb3ef7ed969118b78c1375d047f4 Mon Sep 17 00:00:00 2001 From: Jan Cernik <66427541+jancernik@users.noreply.github.com> Date: Tue, 20 Jun 2023 10:58:38 -0300 Subject: [PATCH] FEATURE: Scroll to first message when clicking date in chat (#21926) This PR adds a new parameter to fetch chat messages: `target_date`. It can be used to fetch messages by a specific date string. Note that it does not need to be the `created_at` date of an existing message, it can be any date. Similar to `target_message_id`, it retrieves an array of past and future messages following the query limits. --- .../chat/api/channels_controller.rb | 10 ++++- .../chat/app/queries/chat/messages_query.rb | 45 ++++++++++++++++--- .../app/services/chat/channel_view_builder.rb | 8 ++-- .../discourse/components/chat-channel.hbs | 2 +- .../discourse/components/chat-channel.js | 27 +++++++++-- .../chat-message-separator-date.hbs | 10 ++++- .../components/chat-message-separator-new.hbs | 2 +- .../discourse/components/chat-message.hbs | 5 ++- .../discourse/lib/chat-messages-manager.js | 7 +++ .../discourse/models/chat-channel.js | 4 ++ .../discourse/models/chat-message.js | 14 +++++- .../discourse/services/chat-api.js | 4 ++ .../common/chat-message-separator.scss | 28 ++++++++++++ .../spec/queries/chat/messages_query_spec.rb | 16 +++++++ .../chat/channel_view_builder_spec.rb | 22 +++++++++ .../chat-message-separator-date-test.js | 7 ++- 16 files changed, 187 insertions(+), 24 deletions(-) diff --git a/plugins/chat/app/controllers/chat/api/channels_controller.rb b/plugins/chat/app/controllers/chat/api/channels_controller.rb index 54fd4c25067..47283654fee 100644 --- a/plugins/chat/app/controllers/chat/api/channels_controller.rb +++ b/plugins/chat/app/controllers/chat/api/channels_controller.rb @@ -68,14 +68,14 @@ class Chat::Api::ChannelsController < Chat::ApiController end def show - if params[:target_message_id].present? || params[:include_messages].present? || - params[:fetch_from_last_read].present? + if should_build_channel_view? with_service( Chat::ChannelViewBuilder, **params.permit( :channel_id, :target_message_id, :thread_id, + :target_date, :page_size, :direction, :fetch_from_last_read, @@ -83,6 +83,7 @@ class Chat::Api::ChannelsController < Chat::ApiController :channel_id, :target_message_id, :thread_id, + :target_date, :page_size, :direction, :fetch_from_last_read, @@ -160,4 +161,9 @@ class Chat::Api::ChannelsController < Chat::ApiController permitted_params += CATEGORY_CHANNEL_EDITABLE_PARAMS if channel.category_channel? params.require(:channel).permit(*permitted_params) end + + def should_build_channel_view? + params[:target_message_id].present? || params[:target_date].present? || + params[:include_messages].present? || params[:fetch_from_last_read].present? + end end diff --git a/plugins/chat/app/queries/chat/messages_query.rb b/plugins/chat/app/queries/chat/messages_query.rb index aaf9ec11f79..ea8be51aa7a 100644 --- a/plugins/chat/app/queries/chat/messages_query.rb +++ b/plugins/chat/app/queries/chat/messages_query.rb @@ -3,10 +3,10 @@ module Chat # Queries messages for a specific channel. This can be used in two modes: # - # 1. Query messages around a target_message_id. This is used when a user - # needs to jump to the middle of a messages stream or load around their - # last read message ID. There is no pagination or direction here, just - # a limit on past and future messages. + # 1. Query messages around a target_message_id or target_date. This is used + # when a user needs to jump to the middle of a messages stream or load + # around a target. There is no pagination or direction + # here, just a limit on past and future messages. # 2. Query messages with paginations and direction. This is used for normal # scrolling of the messages stream of a channel. # @@ -28,6 +28,7 @@ module Chat # @param thread_id [Integer] (optional) The thread ID to filter messages by. # @param target_message_id [Integer] (optional) The message ID to query around. # It is assumed that the caller already checked if this exists. + # @param target_date [String] (optional) The date to query around. # @param include_thread_messages [Boolean] (optional) Whether to include messages # that are linked to a thread. # @param page_size [Integer] (optional) The number of messages to fetch when not @@ -42,7 +43,8 @@ module Chat target_message_id: nil, include_thread_messages: false, page_size: PAST_MESSAGE_LIMIT + FUTURE_MESSAGE_LIMIT, - direction: nil + direction: nil, + target_date: nil ) messages = base_query(channel: channel) messages = messages.with_deleted if guardian.can_moderate_chat?(channel.chatable) @@ -61,7 +63,11 @@ module Chat if target_message_id.present? && direction.blank? query_around_target(target_message_id, channel, messages) else - query_paginated_messages(direction, page_size, channel, messages, target_message_id) + if target_date.present? + query_by_date(target_date, channel, messages) + else + query_paginated_messages(direction, page_size, channel, messages, target_message_id) + end end end @@ -151,5 +157,32 @@ module Chat can_load_more_future: can_load_more_future, } end + + def self.query_by_date(target_date, channel, messages) + past_messages = + messages + .where("created_at <= ?", target_date) + .order(created_at: :desc) + .limit(PAST_MESSAGE_LIMIT) + .to_a + + future_messages = + messages + .where("created_at > ?", target_date) + .order(created_at: :asc) + .limit(FUTURE_MESSAGE_LIMIT) + .to_a + + can_load_more_past = past_messages.size == PAST_MESSAGE_LIMIT + can_load_more_future = future_messages.size == FUTURE_MESSAGE_LIMIT + + { + past_messages: past_messages, + future_messages: future_messages, + target_date: target_date, + can_load_more_past: can_load_more_past, + can_load_more_future: can_load_more_future, + } + end end end diff --git a/plugins/chat/app/services/chat/channel_view_builder.rb b/plugins/chat/app/services/chat/channel_view_builder.rb index ea6e5b65b34..f37656c611a 100644 --- a/plugins/chat/app/services/chat/channel_view_builder.rb +++ b/plugins/chat/app/services/chat/channel_view_builder.rb @@ -51,6 +51,7 @@ module Chat attribute :direction, :string # (optional) attribute :page_size, :integer # (optional) attribute :fetch_from_last_read, :boolean # (optional) + attribute :target_date, :string # (optional) validates :channel_id, presence: true validates :direction, @@ -128,16 +129,17 @@ module Chat include_thread_messages: include_thread_messages, page_size: contract.page_size, direction: contract.direction, + target_date: contract.target_date, ) context.can_load_more_past = messages_data[:can_load_more_past] context.can_load_more_future = messages_data[:can_load_more_future] - if !messages_data[:target_message] + if !messages_data[:target_message] && !messages_data[:target_date] context.messages = messages_data[:messages] else messages_data[:target_message] = ( - if !include_thread_messages && messages_data[:target_message].thread_reply? + if !include_thread_messages && messages_data[:target_message]&.thread_reply? [] else [messages_data[:target_message]] @@ -148,7 +150,7 @@ module Chat messages_data[:past_messages].reverse, messages_data[:target_message], messages_data[:future_messages], - ].reduce([], :concat) + ].reduce([], :concat).compact end end diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-channel.hbs index 117129d0d7d..68219b1c3a7 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel.hbs @@ -36,12 +36,12 @@ class="chat-messages-container" {{chat/on-resize this.didResizePane (hash delay=100 immediate=true)}} > - {{#if this.loadedOnce}} {{#each @channel.messages key="id" as |message|}} {{/each}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js index 1f18c64e01a..76e45319e33 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js @@ -183,6 +183,8 @@ export default class ChatLivePane extends Component { if (this.requestedTargetMessageId) { findArgs.targetMessageId = this.requestedTargetMessageId; scrollToMessageId = this.requestedTargetMessageId; + } else if (this.requestedTargetDate) { + findArgs.targetDate = this.requestedTargetDate; } else if (fetchingFromLastRead) { findArgs.fetchFromLastRead = true; scrollToMessageId = @@ -230,6 +232,15 @@ export default class ChatLivePane extends Component { highlight: true, }); return; + } else if (this.requestedTargetDate) { + const message = this.args.channel?.findFirstMessageOfDay( + this.requestedTargetDate + ); + + this.scrollToMessage(message.id, { + highlight: true, + }); + return; } if ( @@ -240,7 +251,6 @@ export default class ChatLivePane extends Component { this.scrollToMessage(scrollToMessageId); return; } - this.scrollToBottom(); }) .catch(this._handleErrors) @@ -251,6 +261,7 @@ export default class ChatLivePane extends Component { this.loadedOnce = true; this.requestedTargetMessageId = null; + this.requestedTargetDate = null; this.loadingMorePast = false; this.debounceFillPaneAttempt(); this.updateLastReadMessage(); @@ -364,6 +375,16 @@ export default class ChatLivePane extends Component { } @bind + fetchMessagesByDate(date) { + const message = this.args.channel?.findFirstMessageOfDay(date); + if (message.firstOfResults && this.args.channel?.canLoadMorePast) { + this.requestedTargetDate = date; + this.debounceFetchMessages(); + } else { + this.highlightOrFetchMessage(message.id); + } + } + fillPaneAttempt() { if (this._selfDeleted) { return; @@ -392,9 +413,7 @@ export default class ChatLivePane extends Component { let foundFirstNew = false; result.chat_messages.forEach((messageData, index) => { - if (index === 0) { - messageData.firstOfResults = true; - } + messageData.firstOfResults = index === 0; if (this.currentUser.ignored_users) { // If a message has been hidden it is because the current user is ignoring diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-separator-date.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-message-separator-date.hbs index f824ef4b9a5..2249396840e 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message-separator-date.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message-separator-date.hbs @@ -1,16 +1,22 @@ -{{#if @message.firstMessageOfTheDayAt}} +{{#if @message.formattedFirstMessageDate}}
- {{@message.firstMessageOfTheDayAt}} + {{@message.formattedFirstMessageDate}} {{#if @message.newest}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-separator-new.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-message-separator-new.hbs index 26607178d86..7aaba0ccb65 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message-separator-new.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message-separator-new.hbs @@ -1,4 +1,4 @@ -{{#if (and @message.newest (not @message.firstMessageOfTheDayAt))}} +{{#if (and @message.newest (not @message.formattedFirstMessageDate))}}
diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-message.hbs index 9006e4ffee6..c074f8e528d 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message.hbs @@ -2,7 +2,10 @@ {{#if this.shouldRender}} {{#if (eq @context "channel")}} - + {{/if}} diff --git a/plugins/chat/assets/javascripts/discourse/lib/chat-messages-manager.js b/plugins/chat/assets/javascripts/discourse/lib/chat-messages-manager.js index 67a237394cd..4dcb0352cb2 100644 --- a/plugins/chat/assets/javascripts/discourse/lib/chat-messages-manager.js +++ b/plugins/chat/assets/javascripts/discourse/lib/chat-messages-manager.js @@ -35,6 +35,13 @@ export default class ChatMessagesManager { ); } + findFirstMessageOfDay(messageDate) { + const targetDay = new Date(messageDate).toDateString(); + return this.messages.find( + (message) => new Date(message.createdAt).toDateString() === targetDay + ); + } + removeMessage(message) { return this.messages.removeObject(message); } diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js index 289eb54ddd2..df8db7fa335 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js @@ -157,6 +157,10 @@ export default class ChatChannel { return this.messagesManager.findMessage(id); } + findFirstMessageOfDay(date) { + return this.messagesManager.findFirstMessageOfDay(date); + } + addMessages(messages) { this.messagesManager.addMessages(messages); } diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-message.js b/plugins/chat/assets/javascripts/discourse/models/chat-message.js index 9dbf7342a19..a53336ffd62 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-message.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-message.js @@ -194,7 +194,7 @@ export default class ChatMessage { get firstMessageOfTheDayAt() { if (!this.previousMessage) { - return this.#calendarDate(this.createdAt); + return this.#startOfDay(this.createdAt); } if ( @@ -203,7 +203,13 @@ export default class ChatMessage { new Date(this.createdAt) ) ) { - return this.#calendarDate(this.createdAt); + return this.#startOfDay(this.createdAt); + } + } + + get formattedFirstMessageDate() { + if (this.firstMessageOfTheDayAt) { + return this.#calendarDate(this.firstMessageOfTheDayAt); } } @@ -363,4 +369,8 @@ export default class ChatMessage { a.getDate() === b.getDate() ); } + + #startOfDay(date) { + return moment(date).startOf("day").format(); + } } diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-api.js b/plugins/chat/assets/javascripts/discourse/services/chat-api.js index d03d7dafef3..0d4be6ba794 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-api.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-api.js @@ -46,6 +46,10 @@ export default class ChatApi extends Service { if (data.threadId) { args.thread_id = data.threadId; } + + if (data.targetDate) { + args.target_date = data.targetDate; + } } return this.#getRequest(`/channels/${channelId}`, args).then((result) => { diff --git a/plugins/chat/assets/stylesheets/common/chat-message-separator.scss b/plugins/chat/assets/stylesheets/common/chat-message-separator.scss index f68f12882ce..f34c90afd53 100644 --- a/plugins/chat/assets/stylesheets/common/chat-message-separator.scss +++ b/plugins/chat/assets/stylesheets/common/chat-message-separator.scss @@ -69,6 +69,10 @@ border-radius: 4px; color: var(--primary-800); background: var(--primary-50); + + &:hover { + border: 1px solid var(--secondary-high); + } } .chat-message-separator__last-visit { @@ -94,6 +98,30 @@ padding: 0.25rem 0.5rem; box-sizing: border-box; display: flex; + cursor: pointer; + pointer-events: all; + + .no-touch & { + &:hover { + border: 1px solid var(--secondary-high); + border-radius: 4px; + color: var(--primary-800); + background: var(--primary-50); + } + } + + .touch & { + &:active { + border: 1px solid var(--secondary-high); + border-radius: 4px; + color: var(--primary-800); + background: var(--primary-50); + } + } + + &:active { + transform: scale(0.98); + } } & + .chat-message-separator__line-container { diff --git a/plugins/chat/spec/queries/chat/messages_query_spec.rb b/plugins/chat/spec/queries/chat/messages_query_spec.rb index ee1a31328e0..be46468822e 100644 --- a/plugins/chat/spec/queries/chat/messages_query_spec.rb +++ b/plugins/chat/spec/queries/chat/messages_query_spec.rb @@ -9,6 +9,7 @@ RSpec.describe Chat::MessagesQuery do let(:page_size) { nil } let(:direction) { nil } let(:target_message_id) { nil } + let(:target_date) { nil } let(:options) do { thread_id: thread_id, @@ -16,6 +17,7 @@ RSpec.describe Chat::MessagesQuery do page_size: page_size, direction: direction, target_message_id: target_message_id, + target_date: target_date, } end @@ -118,6 +120,20 @@ RSpec.describe Chat::MessagesQuery do end end + context "when target_date provided" do + let(:target_date) { 1.day.ago } + + it "queries messages in the channel and finds the past and future messages" do + expect(subject).to eq( + past_messages: [message_1], + future_messages: [message_2, message_3], + target_date: target_date, + can_load_more_past: false, + can_load_more_future: false, + ) + end + end + context "when target_message_id not provided" do it "queries messages in the channel" do expect(subject).to eq( diff --git a/plugins/chat/spec/services/chat/channel_view_builder_spec.rb b/plugins/chat/spec/services/chat/channel_view_builder_spec.rb index d61d6ee0138..99a9eb9e917 100644 --- a/plugins/chat/spec/services/chat/channel_view_builder_spec.rb +++ b/plugins/chat/spec/services/chat/channel_view_builder_spec.rb @@ -21,6 +21,7 @@ RSpec.describe Chat::ChannelViewBuilder do let(:direction) { nil } let(:thread_id) { nil } let(:fetch_from_last_read) { nil } + let(:target_date) { nil } let(:params) do { guardian: guardian, @@ -30,6 +31,7 @@ RSpec.describe Chat::ChannelViewBuilder do page_size: page_size, direction: direction, thread_id: thread_id, + target_date: target_date, } end @@ -54,6 +56,7 @@ RSpec.describe Chat::ChannelViewBuilder do include_thread_messages: true, page_size: page_size, direction: direction, + target_date: target_date, ) .returns({ messages: [] }) subject @@ -337,5 +340,24 @@ RSpec.describe Chat::ChannelViewBuilder do end end end + + context "when target_date provided" do + fab!(:past_message) do + msg = Fabricate(:chat_message, chat_channel: channel) + msg.update!(created_at: 3.days.ago) + msg + end + fab!(:future_message) do + msg = Fabricate(:chat_message, chat_channel: channel) + msg.update!(created_at: 1.days.ago) + msg + end + + let(:target_date) { 2.days.ago } + + it "includes past and future messages" do + expect(subject.view.chat_messages).to eq([past_message, future_message]) + end + end end end diff --git a/plugins/chat/test/javascripts/components/chat-message-separator-date-test.js b/plugins/chat/test/javascripts/components/chat-message-separator-date-test.js index 415413df7ac..bac862aa8ea 100644 --- a/plugins/chat/test/javascripts/components/chat-message-separator-date-test.js +++ b/plugins/chat/test/javascripts/components/chat-message-separator-date-test.js @@ -11,9 +11,12 @@ module( test("first message of the day", async function (assert) { this.set("date", moment().format("LLL")); - this.set("message", { firstMessageOfTheDayAt: this.date }); + this.set("message", { formattedFirstMessageDate: this.date }); + this.set("fetchMessagesByDate", () => {}); - await render(hbs``); + await render( + hbs`` + ); assert.strictEqual( query(".chat-message-separator-date").innerText.trim(),