From cb5e5f3e5b4093a7791b23a6a141f9d25e328067 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Fri, 5 May 2023 17:08:33 +0200 Subject: [PATCH] UX: groups deleted messages (#21411) Any continuous series of deleted messages will now be grouped into one single expand button. --- .../discourse/components/chat-channel.js | 35 +- .../discourse/components/chat-message.hbs | 362 +++++++++--------- .../discourse/components/chat-message.js | 34 ++ .../discourse/lib/chat-message-interactor.js | 4 +- .../discourse/lib/chat-messages-manager.js | 9 + .../discourse/models/chat-channel.js | 28 +- .../discourse/models/chat-message.js | 8 +- .../stylesheets/common/chat-message.scss | 4 + plugins/chat/config/locales/client.en.yml | 5 +- .../chat/spec/system/deleted_message_spec.rb | 34 +- .../system/move_message_to_channel_spec.rb | 3 +- .../system/page_objects/chat/chat_channel.rb | 7 + .../system/page_objects/chat/chat_thread.rb | 7 + 13 files changed, 325 insertions(+), 215 deletions(-) diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js index 64ce8abe0b5..9384cd05da5 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js @@ -108,7 +108,7 @@ export default class ChatLivePane extends Component { // Technically we could keep messages to avoid re-fetching them, but // it's not worth the complexity for now - this.args.channel?.messagesManager?.clearMessages(); + this.args.channel?.clearMessages(); if (this._loadedChannelId !== this.args.channel?.id) { this._unsubscribeToUpdates(this._loadedChannelId); @@ -183,7 +183,7 @@ export default class ChatLivePane extends Component { results ); - this.args.channel.messages = messages; + this.args.channel.addMessages(messages); this.args.channel.details = meta; if (this.requestedTargetMessageId) { @@ -224,8 +224,8 @@ export default class ChatLivePane extends Component { const loadingMoreKey = `loadingMore${capitalize(direction)}`; const canLoadMore = loadingPast - ? this.#messagesManager.canLoadMorePast - : this.#messagesManager.canLoadMoreFuture; + ? this.args.channel?.canLoadMorePast + : this.args.channel?.canLoadMoreFuture; if ( !canLoadMore || @@ -276,7 +276,7 @@ export default class ChatLivePane extends Component { } this.args.channel.details = meta; - this.#messagesManager.addMessages(messages); + this.args.channel.addMessages(messages); // Edge case for IOS to avoid blank screens // and/or scrolling to bottom losing track of scroll position @@ -367,7 +367,7 @@ export default class ChatLivePane extends Component { @debounce(100) highlightOrFetchMessage(messageId) { - const message = this.#messagesManager?.findMessage(messageId); + const message = this.args.channel?.findMessage(messageId); if (message) { this.scrollToMessage(message.id, { highlight: true, @@ -388,7 +388,7 @@ export default class ChatLivePane extends Component { return; } - const message = this.#messagesManager?.findMessage(messageId); + const message = this.args.channel?.findMessage(messageId); if (message?.deletedAt && opts.autoExpand) { message.expanded = true; } @@ -485,7 +485,7 @@ export default class ChatLivePane extends Component { return; } - if (this.#messagesManager?.canLoadMoreFuture) { + if (this.args.channel?.canLoadMoreFuture) { this._fetchAndScrollToLatest(); } else if (this.args.channel.messages?.length > 0) { this.scrollToMessage( @@ -534,9 +534,9 @@ export default class ChatLivePane extends Component { } removeMessage(msgData) { - const message = this.#messagesManager.findMessage(msgData.id); + const message = this.args.channel?.findMessage(msgData.id); if (message) { - this.#messagesManager.removeMessage(message); + this.args.channel?.removeMessage(message); } } @@ -557,7 +557,7 @@ export default class ChatLivePane extends Component { if (data.chat_message.user.id === this.currentUser.id && data.staged_id) { const stagedMessage = handleStagedMessage( this.args.channel, - this.#messagesManager, + this.args.channel.messagesManager, data ); if (stagedMessage) { @@ -565,20 +565,19 @@ export default class ChatLivePane extends Component { } } - if (this.#messagesManager.canLoadMoreFuture) { + if (this.args.channel?.canLoadMoreFuture) { // If we can load more messages, we just notice the user of new messages this.hasNewMessages = true; } else if (this.#isTowardsBottom()) { // 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.#messagesManager.addMessages([message]); + this.args.channel.addMessages([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.#messagesManager.addMessages([message]); + this.args.channel.addMessages([message]); this.hasNewMessages = true; } } @@ -589,10 +588,6 @@ export default class ChatLivePane extends Component { return this.isDestroying || this.isDestroyed; } - get #messagesManager() { - return this.args.channel?.messagesManager; - } - @action onSendMessage(message) { if (message.editing) { @@ -711,7 +706,7 @@ export default class ChatLivePane extends Component { } _onSendError(id, error) { - const stagedMessage = this.#messagesManager.findStagedMessage(id); + const stagedMessage = this.args.channel.findStagedMessage(id); if (stagedMessage) { if (error.jqXHR?.responseJSON?.errors?.length) { // only network errors are retryable diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-message.hbs index b122566906b..b28891bbe1e 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message.hbs @@ -1,202 +1,206 @@ {{! template-lint-disable no-invalid-interactive }} -{{#if (eq @context "channel")}} - - -{{/if}} +{{#if this.shouldRender}} + {{#if (eq @context "channel")}} + + + {{/if}} -
- {{#if this.show}} - {{#if this.pane.selectingMessages}} - - {{/if}} - - {{#if this.deletedAndCollapsed}} -
- + {{#if this.show}} + {{#if this.pane.selectingMessages}} + -
- {{else if this.hiddenAndCollapsed}} -
- -
- {{else}} -
- {{#unless this.hideReplyToInfo}} - - {{/unless}} + {{/if}} - {{#if this.hideUserInfo}} - - {{else}} - - {{/if}} - -
- {{#unless this.hideUserInfo}} - + {{#if this.deletedAndCollapsed}} +
+ +
+ {{else if this.hiddenAndCollapsed}} +
+ +
+ {{else}} +
+ {{#unless this.hideReplyToInfo}} + {{/unless}} - - {{#if @message.reactions.length}} -
- {{#if this.reactionLabel}} -
- {{replace-emoji this.reactionLabel}} -
- {{/if}} + {{#if this.hideUserInfo}} + + {{else}} + + {{/if}} - {{#each @message.reactions as |reaction|}} - - {{/each}} +
+ {{#unless this.hideUserInfo}} + + {{/unless}} - {{#if this.chat.userCanInteractWithChat}} - {{#unless this.site.mobileView}} - + {{#if @message.reactions.length}} +
+ {{#if this.reactionLabel}} +
+ {{replace-emoji this.reactionLabel}} +
+ {{/if}} + + {{#each @message.reactions as |reaction|}} + - {{/unless}} + {{/each}} + + {{#if this.chat.userCanInteractWithChat}} + {{#unless this.site.mobileView}} + + {{/unless}} + {{/if}} +
+ {{/if}} + + + {{#if @message.error}} +
+ {{#if (eq @message.error "network_error")}} + + + {{i18n "chat.retry_staged_message.title"}} + + + {{i18n "chat.retry_staged_message.action"}} + + + {{else}} + {{@message.error}} {{/if}}
{{/if}} - - {{#if @message.error}} -
- {{#if (eq @message.error "network_error")}} - - - {{i18n "chat.retry_staged_message.title"}} + {{#if this.mentionWarning}} +
+ {{#if this.mentionWarning.invitation_sent}} + {{d-icon "check"}} + + {{i18n + "chat.mention_warning.invitations_sent" + count=this.mentionWarning.without_membership.length + }} - - {{i18n "chat.retry_staged_message.action"}} - - - {{else}} - {{@message.error}} - {{/if}} -
- {{/if}} + {{else}} + - {{#if this.mentionWarning}} -
- {{#if this.mentionWarning.invitation_sent}} - {{d-icon "check"}} - - {{i18n - "chat.mention_warning.invitations_sent" - count=this.mentionWarning.without_membership.length - }} - - {{else}} - + {{#if this.mentionWarning.cannot_see}} +

+ {{this.mentionedCannotSeeText}} +

+ {{/if}} - {{#if this.mentionWarning.cannot_see}} -

- {{this.mentionedCannotSeeText}} -

- {{/if}} + {{#if this.mentionWarning.without_membership}} +

+ {{this.mentionedWithoutMembershipText}} + + {{i18n "chat.mention_warning.invite"}} + +

+ {{/if}} + {{#if this.mentionWarning.group_mentions_disabled}} +

+ {{this.groupsWithDisabledMentions}} +

+ {{/if}} - {{#if this.mentionWarning.without_membership}} -

- {{this.mentionedWithoutMembershipText}} - - {{i18n "chat.mention_warning.invite"}} - -

- {{/if}} - {{#if this.mentionWarning.group_mentions_disabled}} -

- {{this.groupsWithDisabledMentions}} -

+ {{#if this.mentionWarning.groups_with_too_many_members}} +

{{this.groupsWithTooManyMembers}}

+ {{/if}} {{/if}} +
+ {{/if}} +
- {{#if this.mentionWarning.groups_with_too_many_members}} -

{{this.groupsWithTooManyMembers}}

- {{/if}} - {{/if}} -
+ {{#if this.showThreadIndicator}} + {{/if}}
- - {{#if this.showThreadIndicator}} - - {{/if}} -
+ {{/if}} {{/if}} - {{/if}} -
\ No newline at end of file +
+{{/if}} \ No newline at end of file diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message.js b/plugins/chat/assets/javascripts/discourse/components/chat-message.js index a399c0db713..5a3dbfe52b1 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message.js @@ -69,9 +69,43 @@ export default class ChatMessage extends Component { return !this.args.message?.expanded; } + get deletedMessageLabel() { + let count = 1; + + const recursiveCount = (message) => { + const previousMessage = message.previousMessage; + if (previousMessage?.deletedAt) { + count++; + recursiveCount(previousMessage); + } + }; + + recursiveCount(this.args.message); + + return I18n.t("chat.deleted", { count }); + } + + get shouldRender() { + return ( + this.args.message.expanded || + !this.args.message.deletedAt || + (this.args.message.deletedAt && !this.args.message.nextMessage?.deletedAt) + ); + } + @action expand() { + const recursiveExpand = (message) => { + const previousMessage = message.previousMessage; + if (previousMessage?.deletedAt) { + previousMessage.expanded = true; + recursiveExpand(previousMessage); + } + }; + this.args.message.expanded = true; + + recursiveExpand(this.args.message); } @action diff --git a/plugins/chat/assets/javascripts/discourse/lib/chat-message-interactor.js b/plugins/chat/assets/javascripts/discourse/lib/chat-message-interactor.js index 239119787fe..938dda3c4ef 100644 --- a/plugins/chat/assets/javascripts/discourse/lib/chat-message-interactor.js +++ b/plugins/chat/assets/javascripts/discourse/lib/chat-message-interactor.js @@ -215,9 +215,9 @@ export default class ChatMessageInteractor { bulkSelect(checked) { const channel = this.message.channel; const lastSelectedIndex = channel.findIndexOfMessage( - this.pane.lastSelectedMessage + this.pane.lastSelectedMessage.id ); - const newlySelectedIndex = channel.findIndexOfMessage(this.message); + const newlySelectedIndex = channel.findIndexOfMessage(this.message.id); const sortedIndices = [lastSelectedIndex, newlySelectedIndex].sort( (a, b) => a - b ); 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 0158698f339..1a12be16663 100644 --- a/plugins/chat/assets/javascripts/discourse/lib/chat-messages-manager.js +++ b/plugins/chat/assets/javascripts/discourse/lib/chat-messages-manager.js @@ -12,6 +12,7 @@ export default class ChatMessagesManager { } clearMessages() { + this.messages.forEach((message) => (message.manager = null)); this.messages.clear(); this.canLoadMoreFuture = null; @@ -19,6 +20,10 @@ export default class ChatMessagesManager { } addMessages(messages = []) { + messages.forEach((message) => { + message.manager = this; + }); + this.messages = this.messages .concat(messages) .uniqBy("id") @@ -40,4 +45,8 @@ export default class ChatMessagesManager { (message) => message.staged && message.id === stagedMessageId ); } + + findIndexOfMessage(id) { + return this.messages.findIndex((m) => m.id === id); + } } diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js index c67862f1f4f..8da9fa2a9ef 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js @@ -68,14 +68,30 @@ export default class ChatChannel extends RestModel { threadsManager = new ChatThreadsManager(getOwner(this)); messagesManager = new ChatMessagesManager(getOwner(this)); - findIndexOfMessage(message) { - return this.messages.findIndex((m) => m.id === message.id); + findIndexOfMessage(id) { + return this.messagesManager.findIndexOfMessage(id); + } + + findStagedMessage(id) { + return this.messagesManager.findStagedMessage(id); } findMessage(id) { return this.messagesManager.findMessage(id); } + addMessages(messages) { + this.messagesManager.addMessages(messages); + } + + clearMessages() { + this.messagesManager.clearMessages(); + } + + removeMessage(message) { + this.messagesManager.removeMessage(message); + } + get messages() { return this.messagesManager.messages; } @@ -88,6 +104,10 @@ export default class ChatChannel extends RestModel { return this.messagesManager.canLoadMoreFuture; } + get canLoadMorePast() { + return this.messagesManager.canLoadMorePast; + } + get escapedTitle() { return escapeExpression(this.title); } @@ -186,10 +206,10 @@ export default class ChatChannel extends RestModel { if (message.inReplyTo) { if (!this.threadingEnabled) { - this.messagesManager.addMessages([message]); + this.addMessages([message]); } } else { - this.messagesManager.addMessages([message]); + this.addMessages([message]); } } diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-message.js b/plugins/chat/assets/javascripts/discourse/models/chat-message.js index 23a64dceebd..07c3310d27b 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-message.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-message.js @@ -50,6 +50,8 @@ export default class ChatMessage { @tracked message; @tracked thread; @tracked threadReplyCount; + @tracked manager = null; + @tracked _cooked; constructor(channel, args = {}) { @@ -193,17 +195,17 @@ export default class ChatMessage { @cached get index() { - return this.channel.messages.indexOf(this); + return this.manager?.messages?.indexOf(this); } @cached get previousMessage() { - return this.channel?.messages?.objectAt?.(this.index - 1); + return this.manager?.messages?.objectAt?.(this.index - 1); } @cached get nextMessage() { - return this.channel?.messages?.objectAt?.(this.index + 1); + return this.manager?.messages?.objectAt?.(this.index + 1); } incrementVersion() { diff --git a/plugins/chat/assets/stylesheets/common/chat-message.scss b/plugins/chat/assets/stylesheets/common/chat-message.scss index bc65c9454c9..13623a93d72 100644 --- a/plugins/chat/assets/stylesheets/common/chat-message.scss +++ b/plugins/chat/assets/stylesheets/common/chat-message.scss @@ -7,6 +7,10 @@ color: var(--primary-low-mid); padding: 0.25em; + .d-button-label { + text-align: left; + } + &:hover { background: inherit; color: inherit; diff --git a/plugins/chat/config/locales/client.en.yml b/plugins/chat/config/locales/client.en.yml index 3261b7f2ac2..2b6114ddc96 100644 --- a/plugins/chat/config/locales/client.en.yml +++ b/plugins/chat/config/locales/client.en.yml @@ -81,7 +81,10 @@ en: collapse: "Collapse Chat Drawer" expand: "Expand Chat Drawer" confirm_flag: "Are you sure you want to flag %{username}'s message?" - deleted: "A message was deleted. [view]" + deleted: + one: "A message was deleted. [view]" + other: "%{count} messages were deleted. [view all]" + hidden: "A message was hidden. [view]" delete: "Delete" edited: "edited" diff --git a/plugins/chat/spec/system/deleted_message_spec.rb b/plugins/chat/spec/system/deleted_message_spec.rb index 918a1939e32..03a753aca49 100644 --- a/plugins/chat/spec/system/deleted_message_spec.rb +++ b/plugins/chat/spec/system/deleted_message_spec.rb @@ -4,7 +4,7 @@ RSpec.describe "Deleted message", type: :system, js: true do let(:chat_page) { PageObjects::Pages::Chat.new } let(:channel_page) { PageObjects::Pages::ChatChannel.new } - fab!(:current_user) { Fabricate(:user) } + fab!(:current_user) { Fabricate(:admin) } fab!(:channel_1) { Fabricate(:category_channel) } before do @@ -21,7 +21,33 @@ RSpec.describe "Deleted message", type: :system, js: true do last_message = find(".chat-message-container:last-child") channel_page.delete_message(OpenStruct.new(id: last_message["data-id"])) - expect(page).to have_content(I18n.t("js.chat.deleted")) + expect(channel_page).to have_deleted_message( + OpenStruct.new(id: last_message["data-id"]), + count: 1, + ) + end + end + + context "when deleting multiple messages" do + fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) } + fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel_1) } + fab!(:message_3) { Fabricate(:chat_message, chat_channel: channel_1) } + fab!(:message_4) { Fabricate(:chat_message, chat_channel: channel_1) } + fab!(:message_5) { Fabricate(:chat_message, chat_channel: channel_1) } + fab!(:message_6) { Fabricate(:chat_message, chat_channel: channel_1) } + + it "groups them" do + chat_page.visit_channel(channel_1) + + channel_page.delete_message(message_1) + channel_page.delete_message(message_3) + channel_page.delete_message(message_4) + channel_page.delete_message(message_6) + + expect(channel_page).to have_deleted_message(message_1) + expect(channel_page).to have_deleted_message(message_4, count: 2) + expect(channel_page).to have_deleted_message(message_6) + expect(channel_page).to have_no_message(id: message_3.id) end end @@ -65,9 +91,9 @@ RSpec.describe "Deleted message", type: :system, js: true do ) expect(channel_page).to have_no_message(id: message_1.id) - expect(channel_page).to have_no_message(id: message_2.id) + expect(channel_page).to have_deleted_message(message_2, count: 2) expect(open_thread).to have_no_message(thread_id: thread.id, id: message_4.id) - expect(open_thread).to have_no_message(thread_id: thread.id, id: message_5.id) + expect(open_thread).to have_deleted_message(message_5, count: 2) end end end diff --git a/plugins/chat/spec/system/move_message_to_channel_spec.rb b/plugins/chat/spec/system/move_message_to_channel_spec.rb index e605ce2b87b..a1b10f7c149 100644 --- a/plugins/chat/spec/system/move_message_to_channel_spec.rb +++ b/plugins/chat/spec/system/move_message_to_channel_spec.rb @@ -87,8 +87,7 @@ RSpec.describe "Move message to channel", type: :system, js: true do chat.visit_channel(channel_1) - expect(page).to have_no_content(message_1.message) - expect(page).to have_content(I18n.t("js.chat.deleted")) + expect(channel).to have_deleted_message(message_1) end end end diff --git a/plugins/chat/spec/system/page_objects/chat/chat_channel.rb b/plugins/chat/spec/system/page_objects/chat/chat_channel.rb index 7daa59c4672..ecf74ce4bb6 100644 --- a/plugins/chat/spec/system/page_objects/chat/chat_channel.rb +++ b/plugins/chat/spec/system/page_objects/chat/chat_channel.rb @@ -172,6 +172,13 @@ module PageObjects check_message_presence(exists: false, text: text, id: id) end + def has_deleted_message?(message, count: 1) + has_css?( + ".chat-channel .chat-message-container[data-id=\"#{message.id}\"] .chat-message-deleted", + text: I18n.t("js.chat.deleted", count: count), + ) + end + def check_message_presence(exists: true, text: nil, id: nil) css_method = exists ? :has_css? : :has_no_css? if text diff --git a/plugins/chat/spec/system/page_objects/chat/chat_thread.rb b/plugins/chat/spec/system/page_objects/chat/chat_thread.rb index 2ef08a9fa35..13e31b1a245 100644 --- a/plugins/chat/spec/system/page_objects/chat/chat_thread.rb +++ b/plugins/chat/spec/system/page_objects/chat/chat_thread.rb @@ -77,6 +77,13 @@ module PageObjects def message_by_id_selector(id) ".chat-thread .chat-messages-container .chat-message-container[data-id=\"#{id}\"]" end + + def has_deleted_message?(message, count: 1) + has_css?( + ".chat-thread .chat-message-container[data-id=\"#{message.id}\"] .chat-message-deleted", + text: I18n.t("js.chat.deleted", count: count), + ) + end end end end