From c8fff19b9971ed1456f94dc11813678a5b96caa3 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Fri, 15 Sep 2023 18:09:45 +0200 Subject: [PATCH] DEV: removes the notion of staged thread (#23612) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While very fast and powerful staged threads forces a lot of gymnastic and edge cases. This patch adds a new service `Chat::CreateThread` and uses it to create a thread unconditionally when a user replies to a message in a threading enabled channel. If the user actually doesn’t send a message we will have a thread with no messages which has no important impact and could even be periodically cleaned if necessary. Note that this commit also moves message actions to .gjs as it was the original goal of this PR to correctly check for staged thread to show the menu or not. --- .../chat/api/channel_threads_controller.rb | 24 ++- .../chat/app/services/chat/create_message.rb | 13 +- .../chat/app/services/chat/create_thread.rb | 84 ++++++++ plugins/chat/app/services/chat/publisher.rb | 23 +- .../chat-message-actions-desktop.gjs | 195 +++++++++++++++++ .../chat-message-actions-desktop.hbs | 79 ------- .../chat-message-actions-desktop.js | 101 --------- .../chat-message-actions-mobile.gjs | 197 ++++++++++++++++++ .../chat-message-actions-mobile.hbs | 93 --------- .../components/chat-message-actions-mobile.js | 90 -------- .../discourse/components/chat-message.gjs | 2 +- .../discourse/components/chat-thread.hbs | 1 - .../discourse/components/chat-thread.js | 17 +- .../discourse/models/chat-channel.js | 18 -- .../discourse/models/chat-message.js | 32 +-- .../discourse/routes/chat-channel-thread.js | 19 -- .../discourse/services/chat-api.js | 16 +- .../services/chat-channel-composer.js | 14 +- .../chat-pane-base-subscriptions-manager.js | 41 +--- .../services/chat-staged-thread-mapping.js | 34 --- plugins/chat/config/routes.rb | 1 + .../api/channel_threads_controller_spec.rb | 59 ++++++ .../chat/api/messages_controller_spec.rb | 26 --- .../spec/services/chat/create_message_spec.rb | 7 +- .../spec/services/chat/create_thread_spec.rb | 101 +++++++++ .../chat/spec/services/chat/publisher_spec.rb | 26 --- .../unit/models/chat-message-test.js | 23 ++ 27 files changed, 733 insertions(+), 603 deletions(-) create mode 100644 plugins/chat/app/services/chat/create_thread.rb create mode 100644 plugins/chat/assets/javascripts/discourse/components/chat-message-actions-desktop.gjs delete mode 100644 plugins/chat/assets/javascripts/discourse/components/chat-message-actions-desktop.hbs delete mode 100644 plugins/chat/assets/javascripts/discourse/components/chat-message-actions-desktop.js create mode 100644 plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.gjs delete mode 100644 plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.hbs delete mode 100644 plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.js delete mode 100644 plugins/chat/assets/javascripts/discourse/services/chat-staged-thread-mapping.js create mode 100644 plugins/chat/spec/services/chat/create_thread_spec.rb create mode 100644 plugins/chat/test/javascripts/unit/models/chat-message-test.js diff --git a/plugins/chat/app/controllers/chat/api/channel_threads_controller.rb b/plugins/chat/app/controllers/chat/api/channel_threads_controller.rb index 2a25950a475..b21f6c58618 100644 --- a/plugins/chat/app/controllers/chat/api/channel_threads_controller.rb +++ b/plugins/chat/app/controllers/chat/api/channel_threads_controller.rb @@ -18,7 +18,6 @@ class Chat::Api::ChannelThreadsController < Chat::ApiController root: false, ) end - on_failed_policy(:threaded_discussions_enabled) { raise Discourse::NotFound } on_failed_policy(:threading_enabled_for_channel) { raise Discourse::NotFound } on_failed_policy(:can_view_channel) { raise Discourse::InvalidAccess } on_model_not_found(:channel) { raise Discourse::NotFound } @@ -39,7 +38,7 @@ class Chat::Api::ChannelThreadsController < Chat::ApiController participants: result.participants, ) end - on_failed_policy(:threaded_discussions_enabled) { raise Discourse::NotFound } + on_failed_policy(:threading_enabled_for_channel) { raise Discourse::NotFound } on_model_not_found(:thread) { raise Discourse::NotFound } end @@ -47,7 +46,6 @@ class Chat::Api::ChannelThreadsController < Chat::ApiController def update with_service(::Chat::UpdateThread) do - on_failed_policy(:threaded_discussions_enabled) { raise Discourse::NotFound } on_failed_policy(:threading_enabled_for_channel) { raise Discourse::NotFound } on_failed_policy(:can_view_channel) { raise Discourse::InvalidAccess } on_failed_policy(:can_edit_thread) { raise Discourse::InvalidAccess } @@ -57,4 +55,24 @@ class Chat::Api::ChannelThreadsController < Chat::ApiController end end end + + def create + with_service(::Chat::CreateThread) do + on_success do + render_serialized( + result.thread, + ::Chat::ThreadSerializer, + root: false, + membership: result.membership, + include_thread_original_message: true, + ) + end + on_failed_policy(:threading_enabled_for_channel) { raise Discourse::NotFound } + on_failed_policy(:can_view_channel) { raise Discourse::InvalidAccess } + on_failed_step(:create_thread) do + render json: failed_json.merge(errors: [result["result.step.create_thread"].error]), + status: 422 + end + end + end end diff --git a/plugins/chat/app/services/chat/create_message.rb b/plugins/chat/app/services/chat/create_message.rb index 1d131446aad..2c13c0ec8b2 100644 --- a/plugins/chat/app/services/chat/create_message.rb +++ b/plugins/chat/app/services/chat/create_message.rb @@ -9,7 +9,7 @@ module Chat class CreateMessage include Service::Base - # @!method call(chat_channel_id:, guardian:, in_reply_to_id:, message:, staged_id:, upload_ids:, thread_id:, staged_thread_id:, incoming_chat_webhook:) + # @!method call(chat_channel_id:, guardian:, in_reply_to_id:, message:, staged_id:, upload_ids:, thread_id:, incoming_chat_webhook:) # @param guardian [Guardian] # @param chat_channel_id [Integer] # @param message [String] @@ -17,7 +17,6 @@ module Chat # @param thread_id [Integer] ID of a thread to reply to # @param upload_ids [Array] IDs of uploaded documents # @param staged_id [String] arbitrary string that will be sent back to the client - # @param staged_thread_id [String] arbitrary string that will be sent back to the client (for a new thread) # @param incoming_chat_webhook [Chat::IncomingWebhook] policy :no_silenced_user @@ -53,7 +52,6 @@ module Chat attribute :staged_id, :string attribute :upload_ids, :array attribute :thread_id, :string - attribute :staged_thread_id, :string attribute :incoming_chat_webhook validates :chat_channel_id, presence: true @@ -167,16 +165,11 @@ module Chat def publish_new_thread(reply:, contract:, channel:, thread:, **) return unless channel.threading_enabled? return unless reply&.thread_id_previously_changed?(from: nil) - Chat::Publisher.publish_thread_created!(channel, reply, thread.id, contract.staged_thread_id) + Chat::Publisher.publish_thread_created!(channel, reply, thread.id) end def publish_new_message_events(channel:, message:, contract:, guardian:, **) - Chat::Publisher.publish_new!( - channel, - message, - contract.staged_id, - staged_thread_id: contract.staged_thread_id, - ) + Chat::Publisher.publish_new!(channel, message, contract.staged_id) Jobs.enqueue(Jobs::Chat::ProcessMessage, { chat_message_id: message.id }) Chat::Notifier.notify_new(chat_message: message, timestamp: message.created_at) DiscourseEvent.trigger(:chat_message_created, message, channel, guardian.user) diff --git a/plugins/chat/app/services/chat/create_thread.rb b/plugins/chat/app/services/chat/create_thread.rb new file mode 100644 index 00000000000..3b83352ac37 --- /dev/null +++ b/plugins/chat/app/services/chat/create_thread.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +module Chat + # Creates a thread. + # + # @example + # Chat::CreateThread.call(channel_id: 2, original_message_id: 3, guardian: guardian, title: "Restaurant for Saturday") + # + class CreateThread + include Service::Base + + # @!method call(thread_id:, channel_id:, guardian:, **params_to_edit) + # @param [Integer] original_message_id + # @param [Integer] channel_id + # @param [Guardian] guardian + # @option params_to_create [String,nil] title + # @return [Service::Base::Context] + + contract + model :channel + policy :can_view_channel + policy :threading_enabled_for_channel + model :original_message + transaction do + step :create_thread + step :associate_thread_to_message + step :fetch_membership + step :publish_new_thread + end + + # @!visibility private + class Contract + attribute :original_message_id, :integer + attribute :channel_id, :integer + attribute :title, :string + + validates :original_message_id, :channel_id, presence: true + validates :title, length: { maximum: Chat::Thread::MAX_TITLE_LENGTH } + end + + private + + def fetch_channel(contract:, **) + ::Chat::Channel.find_by(id: contract.channel_id) + end + + def fetch_original_message(channel:, contract:, **) + ::Chat::Message.find_by( + id: contract.original_message_id, + chat_channel_id: contract.channel_id, + ) + end + + def can_view_channel(guardian:, channel:, **) + guardian.can_preview_chat_channel?(channel) + end + + def threading_enabled_for_channel(channel:, **) + channel.threading_enabled + end + + def create_thread(channel:, original_message:, contract:, **) + context.thread = + channel.threads.create( + title: contract.title, + original_message: original_message, + original_message_user: original_message.user, + ) + fail!(context.thread.errors.full_messages.join(", ")) if context.thread.invalid? + end + + def associate_thread_to_message(original_message:, **) + original_message.update(thread: context.thread) + end + + def fetch_membership(guardian:, **) + context.membership = context.thread.membership_for(guardian.user) + end + + def publish_new_thread(channel:, original_message:, **) + ::Chat::Publisher.publish_thread_created!(channel, original_message, context.thread.id) + end + end +end diff --git a/plugins/chat/app/services/chat/publisher.rb b/plugins/chat/app/services/chat/publisher.rb index 9eb41d9dd0b..af0f56b644b 100644 --- a/plugins/chat/app/services/chat/publisher.rb +++ b/plugins/chat/app/services/chat/publisher.rb @@ -14,7 +14,7 @@ module Chat "#{root_message_bus_channel(chat_channel_id)}/thread/#{thread_id}" end - def self.calculate_publish_targets(channel, message, staged_thread_id: nil) + def self.calculate_publish_targets(channel, message) return [root_message_bus_channel(channel.id)] if !allow_publish_to_thread?(channel) if message.thread_om? @@ -22,9 +22,8 @@ module Chat root_message_bus_channel(channel.id), thread_message_bus_channel(channel.id, message.thread_id), ] - elsif staged_thread_id || message.thread_reply? + elsif message.thread_reply? targets = [thread_message_bus_channel(channel.id, message.thread_id)] - targets << thread_message_bus_channel(channel.id, staged_thread_id) if staged_thread_id targets else [root_message_bus_channel(channel.id)] @@ -35,16 +34,12 @@ module Chat channel.threading_enabled end - def self.publish_new!(chat_channel, chat_message, staged_id, staged_thread_id: nil) - message_bus_targets = - calculate_publish_targets(chat_channel, chat_message, staged_thread_id: staged_thread_id) + def self.publish_new!(chat_channel, chat_message, staged_id) + message_bus_targets = calculate_publish_targets(chat_channel, chat_message) publish_to_targets!( message_bus_targets, chat_channel, - serialize_message_with_type(chat_message, :sent).merge( - staged_id: staged_id, - staged_thread_id: staged_thread_id, - ), + serialize_message_with_type(chat_message, :sent).merge(staged_id: staged_id), ) if !chat_message.thread_reply? || !allow_publish_to_thread?(chat_channel) @@ -100,14 +95,10 @@ module Chat ) end - def self.publish_thread_created!(chat_channel, chat_message, thread_id, staged_thread_id) + def self.publish_thread_created!(chat_channel, chat_message, thread_id) publish_to_channel!( chat_channel, - serialize_message_with_type( - chat_message, - :thread_created, - { thread_id: thread_id, staged_thread_id: staged_thread_id }, - ), + serialize_message_with_type(chat_message, :thread_created, { thread_id: thread_id }), ) end diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-desktop.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-desktop.gjs new file mode 100644 index 00000000000..c845e6319dd --- /dev/null +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-desktop.gjs @@ -0,0 +1,195 @@ +import Component from "@glimmer/component"; +import { inject as service } from "@ember/service"; +import ChatMessageInteractor from "discourse/plugins/chat/discourse/lib/chat-message-interactor"; +import { getOwner } from "@ember/application"; +import { schedule } from "@ember/runloop"; +import { createPopper } from "@popperjs/core"; +import chatMessageContainer from "discourse/plugins/chat/discourse/lib/chat-message-container"; +import { action } from "@ember/object"; +import { tracked } from "@glimmer/tracking"; +import didInsert from "@ember/render-modifiers/modifiers/did-insert"; +import didUpdate from "@ember/render-modifiers/modifiers/did-update"; +import willDestroy from "@ember/render-modifiers/modifiers/will-destroy"; +import concatClass from "discourse/helpers/concat-class"; +import BookmarkIcon from "discourse/components/bookmark-icon"; +import ChatMessageReaction from "discourse/plugins/chat/discourse/components/chat-message-reaction"; +import DButton from "discourse/components/d-button"; +import DropdownSelectBox from "select-kit/components/dropdown-select-box"; +import { on } from "@ember/modifier"; +import and from "truth-helpers/helpers/and"; +import { concat, hash } from "@ember/helper"; + +const MSG_ACTIONS_VERTICAL_PADDING = -10; +const FULL = "full"; +const REDUCED = "reduced"; +const REDUCED_WIDTH_THRESHOLD = 500; + +export default class ChatMessageActionsDesktop extends Component { + + + @service chat; + @service chatEmojiPickerManager; + @service site; + + @tracked size = FULL; + + popper = null; + + get message() { + return this.chat.activeMessage.model; + } + + get context() { + return this.chat.activeMessage.context; + } + + get messageInteractor() { + return new ChatMessageInteractor( + getOwner(this), + this.message, + this.context + ); + } + + get shouldRenderFavoriteReactions() { + return this.size === FULL; + } + + @action + onWheel() { + // prevents menu to stop scroll on the list of messages + this.chat.activeMessage = null; + } + + @action + setup(element) { + this.popper?.destroy(); + + schedule("afterRender", () => { + const messageContainer = chatMessageContainer( + this.message.id, + this.context + ); + + if (!messageContainer) { + return; + } + + const viewport = messageContainer.closest(".popper-viewport"); + this.size = + viewport.clientWidth < REDUCED_WIDTH_THRESHOLD ? REDUCED : FULL; + + if (!messageContainer) { + return; + } + + this.popper = createPopper(messageContainer, element, { + placement: "top-end", + strategy: "fixed", + modifiers: [ + { + name: "flip", + enabled: true, + options: { + boundary: viewport, + fallbackPlacements: ["bottom-end"], + }, + }, + { name: "hide", enabled: true }, + { name: "eventListeners", options: { scroll: false } }, + { + name: "offset", + options: { offset: [-2, MSG_ACTIONS_VERTICAL_PADDING] }, + }, + ], + }); + }); + } + + @action + teardown() { + this.popper?.destroy(); + this.popper = null; + } +} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-desktop.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-desktop.hbs deleted file mode 100644 index 6fdde4039a0..00000000000 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-desktop.hbs +++ /dev/null @@ -1,79 +0,0 @@ -{{#if (and this.site.desktopView this.chat.activeMessage.model.id)}} -
-
- {{#if this.shouldRenderFavoriteReactions}} - {{#each - this.messageInteractor.emojiReactions key="emoji" - as |reaction| - }} - - {{/each}} - {{/if}} - - {{#if this.messageInteractor.canInteractWithMessage}} - - {{/if}} - - {{#if this.messageInteractor.canBookmark}} - - - - {{/if}} - - {{#if this.messageInteractor.canReply}} - - {{/if}} - - {{#if - (and - this.messageInteractor.message - this.messageInteractor.secondaryActions.length - ) - }} - - {{/if}} -
-
-{{/if}} \ No newline at end of file diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-desktop.js b/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-desktop.js deleted file mode 100644 index d2fad1c7c4c..00000000000 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-desktop.js +++ /dev/null @@ -1,101 +0,0 @@ -import Component from "@glimmer/component"; -import { inject as service } from "@ember/service"; -import ChatMessageInteractor from "discourse/plugins/chat/discourse/lib/chat-message-interactor"; -import { getOwner } from "@ember/application"; -import { schedule } from "@ember/runloop"; -import { createPopper } from "@popperjs/core"; -import chatMessageContainer from "discourse/plugins/chat/discourse/lib/chat-message-container"; -import { action } from "@ember/object"; -import { tracked } from "@glimmer/tracking"; - -const MSG_ACTIONS_VERTICAL_PADDING = -10; -const FULL = "full"; -const REDUCED = "reduced"; -const REDUCED_WIDTH_THRESHOLD = 500; - -export default class ChatMessageActionsDesktop extends Component { - @service chat; - @service chatEmojiPickerManager; - @service site; - - @tracked size = FULL; - - popper = null; - - get message() { - return this.chat.activeMessage.model; - } - - get context() { - return this.chat.activeMessage.context; - } - - get messageInteractor() { - return new ChatMessageInteractor( - getOwner(this), - this.message, - this.context - ); - } - - get shouldRenderFavoriteReactions() { - return this.size === FULL; - } - - @action - onWheel() { - // prevents menu to stop scroll on the list of messages - this.chat.activeMessage = null; - } - - @action - setup(element) { - this.popper?.destroy(); - - schedule("afterRender", () => { - const messageContainer = chatMessageContainer( - this.message.id, - this.context - ); - - if (!messageContainer) { - return; - } - - const viewport = messageContainer.closest(".popper-viewport"); - this.size = - viewport.clientWidth < REDUCED_WIDTH_THRESHOLD ? REDUCED : FULL; - - if (!messageContainer) { - return; - } - - this.popper = createPopper(messageContainer, element, { - placement: "top-end", - strategy: "fixed", - modifiers: [ - { - name: "flip", - enabled: true, - options: { - boundary: viewport, - fallbackPlacements: ["bottom-end"], - }, - }, - { name: "hide", enabled: true }, - { name: "eventListeners", options: { scroll: false } }, - { - name: "offset", - options: { offset: [-2, MSG_ACTIONS_VERTICAL_PADDING] }, - }, - ], - }); - }); - } - - @action - teardown() { - this.popper?.destroy(); - this.popper = null; - } -} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.gjs new file mode 100644 index 00000000000..31baed3481f --- /dev/null +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.gjs @@ -0,0 +1,197 @@ +import Component from "@glimmer/component"; +import ChatMessageInteractor from "discourse/plugins/chat/discourse/lib/chat-message-interactor"; +import { getOwner } from "discourse-common/lib/get-owner"; +import { tracked } from "@glimmer/tracking"; +import discourseLater from "discourse-common/lib/later"; +import { action } from "@ember/object"; +import { isTesting } from "discourse-common/config/environment"; +import { inject as service } from "@ember/service"; +import and from "truth-helpers/helpers/and"; +import didInsert from "@ember/render-modifiers/modifiers/did-insert"; +import concatClass from "discourse/helpers/concat-class"; +import DButton from "discourse/components/d-button"; +import { on } from "@ember/modifier"; +import ChatUserAvatar from "discourse/plugins/chat/discourse/components/chat/user-avatar"; +import ChatMessageReaction from "discourse/plugins/chat/discourse/components/chat-message-reaction"; +import { fn } from "@ember/helper"; +import or from "truth-helpers/helpers/or"; +import BookmarkIcon from "discourse/components/bookmark-icon"; + +export default class ChatMessageActionsMobile extends Component { + + + @service chat; + @service site; + @service capabilities; + + @tracked hasExpandedReply = false; + @tracked showFadeIn = false; + + get message() { + return this.chat.activeMessage.model; + } + + get context() { + return this.chat.activeMessage.context; + } + + get messageInteractor() { + return new ChatMessageInteractor( + getOwner(this), + this.message, + this.context + ); + } + + @action + fadeAndVibrate() { + discourseLater(this.#addFadeIn.bind(this)); + + if (this.capabilities.canVibrate && !isTesting()) { + navigator.vibrate(5); + } + } + + @action + expandReply(event) { + event.stopPropagation(); + this.hasExpandedReply = true; + } + + @action + collapseMenu(event) { + event.preventDefault(); + this.#onCloseMenu(); + } + + @action + actAndCloseMenu(fnId) { + this.messageInteractor[fnId](); + this.#onCloseMenu(); + } + + @action + openEmojiPicker(_, event) { + this.messageInteractor.openEmojiPicker(_, event); + this.#onCloseMenu(); + } + + #onCloseMenu() { + this.#removeFadeIn(); + + // we don't want to remove the component right away as it's animating + // 200 is equal to the duration of the css animation + discourseLater(() => { + if (this.isDestroying || this.isDestroyed) { + return; + } + + // by ensuring we are not hovering any message anymore + // we also ensure the menu is fully removed + this.chat.activeMessage = null; + }, 200); + } + + #addFadeIn() { + this.showFadeIn = true; + } + + #removeFadeIn() { + this.showFadeIn = false; + } +} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.hbs deleted file mode 100644 index 887156208fa..00000000000 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.hbs +++ /dev/null @@ -1,93 +0,0 @@ -{{#if (and this.site.mobileView this.chat.activeMessage)}} -
-
-
- -
-
-
- - - {{this.message.message}} - -
-
- -
    - {{#each this.messageInteractor.secondaryActions as |button|}} -
  • - -
    • - {{/each}} -
- - {{#if - (or this.messageInteractor.canReact this.messageInteractor.canReply) - }} -
- {{#if this.messageInteractor.canReact}} - {{#each this.messageInteractor.emojiReactions as |reaction|}} - - {{/each}} - - - {{/if}} - - {{#if this.messageInteractor.canBookmark}} - - - - {{/if}} - - {{#if this.messageInteractor.canReply}} - - {{/if}} -
- {{/if}} -
-
-{{/if}} \ No newline at end of file diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.js b/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.js deleted file mode 100644 index 43c2373c121..00000000000 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message-actions-mobile.js +++ /dev/null @@ -1,90 +0,0 @@ -import Component from "@glimmer/component"; -import ChatMessageInteractor from "discourse/plugins/chat/discourse/lib/chat-message-interactor"; -import { getOwner } from "discourse-common/lib/get-owner"; -import { tracked } from "@glimmer/tracking"; -import discourseLater from "discourse-common/lib/later"; -import { action } from "@ember/object"; -import { isTesting } from "discourse-common/config/environment"; -import { inject as service } from "@ember/service"; - -export default class ChatMessageActionsMobile extends Component { - @service chat; - @service site; - @service capabilities; - - @tracked hasExpandedReply = false; - @tracked showFadeIn = false; - - get message() { - return this.chat.activeMessage.model; - } - - get context() { - return this.chat.activeMessage.context; - } - - get messageInteractor() { - return new ChatMessageInteractor( - getOwner(this), - this.message, - this.context - ); - } - - @action - fadeAndVibrate() { - discourseLater(this.#addFadeIn.bind(this)); - - if (this.capabilities.canVibrate && !isTesting()) { - navigator.vibrate(5); - } - } - - @action - expandReply(event) { - event.stopPropagation(); - this.hasExpandedReply = true; - } - - @action - collapseMenu(event) { - event.preventDefault(); - this.#onCloseMenu(); - } - - @action - actAndCloseMenu(fnId) { - this.messageInteractor[fnId](); - this.#onCloseMenu(); - } - - @action - openEmojiPicker(_, event) { - this.messageInteractor.openEmojiPicker(_, event); - this.#onCloseMenu(); - } - - #onCloseMenu() { - this.#removeFadeIn(); - - // we don't want to remove the component right away as it's animating - // 200 is equal to the duration of the css animation - discourseLater(() => { - if (this.isDestroying || this.isDestroyed) { - return; - } - - // by ensuring we are not hovering any message anymore - // we also ensure the menu is fully removed - this.chat.activeMessage = null; - }, 200); - } - - #addFadeIn() { - this.showFadeIn = true; - } - - #removeFadeIn() { - this.showFadeIn = false; - } -} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs index c93132059b5..ba0922ebfbe 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs @@ -460,7 +460,7 @@ export default class ChatMessage extends Component { @action onLongPressStart(element, event) { - if (!this.args.message.expanded) { + if (!this.args.message.expanded || !this.args.message.persisted) { return; } diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-thread.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-thread.hbs index 0d5fb60712d..8446960a181 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-thread.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-thread.hbs @@ -2,7 +2,6 @@ class={{concat-class "chat-thread" (if this.messagesLoader.loading "loading") - (if @thread.staged "staged") }} data-id={{@thread.id}} {{did-insert this.setUploadDropZone}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-thread.js b/plugins/chat/assets/javascripts/discourse/components/chat-thread.js index 98261307bcb..7908b0b526e 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-thread.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-thread.js @@ -177,14 +177,6 @@ export default class ChatThread extends Component { } async fetchMessages(findArgs = {}) { - if (this.args.thread.staged) { - const message = this.args.thread.originalMessage; - message.thread = this.args.thread; - message.manager = this.messagesManager; - this.messagesManager.addMessages([message]); - return; - } - if (this.messagesLoader.loading) { return; } @@ -326,7 +318,7 @@ export default class ChatThread extends Component { // 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 || this.args.thread.staged) { + if (!this.args.thread) { return; } @@ -374,13 +366,10 @@ export default class ChatThread extends Component { this.args.thread.channel.id, { message: message.message, - in_reply_to_id: message.thread.staged - ? message.thread.originalMessage?.id - : null, + in_reply_to_id: null, staged_id: message.id, upload_ids: message.uploads.map((upload) => upload.id), - thread_id: message.thread.staged ? null : message.thread.id, - staged_thread_id: message.thread.staged ? message.thread.id : null, + thread_id: message.thread.id, } ); diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js index 056c0356133..40600163531 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js @@ -7,7 +7,6 @@ import ChatThreadsManager from "discourse/plugins/chat/discourse/lib/chat-thread import ChatMessagesManager from "discourse/plugins/chat/discourse/lib/chat-messages-manager"; import { getOwner } from "discourse-common/lib/get-owner"; import guid from "pretty-text/guid"; -import ChatThread from "discourse/plugins/chat/discourse/models/chat-thread"; import ChatDirectMessage from "discourse/plugins/chat/discourse/models/chat-direct-message"; import ChatChannelArchive from "discourse/plugins/chat/discourse/models/chat-channel-archive"; import Category from "discourse/models/category"; @@ -190,23 +189,6 @@ export default class ChatChannel { return this.meta.can_join_chat_channel; } - createStagedThread(message) { - const clonedMessage = message.duplicate(); - - const thread = new ChatThread(this, { - id: `staged-thread-${message.channel.id}-${message.id}`, - original_message: message, - staged: true, - created_at: moment.utc().format(), - }); - - clonedMessage.thread = thread; - clonedMessage.manager = thread.messagesManager; - thread.messagesManager.addMessages([clonedMessage]); - - return thread; - } - async stageMessage(message) { message.id = guid(); message.staged = true; diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-message.js b/plugins/chat/assets/javascripts/discourse/models/chat-message.js index 71c033306c6..f9c0290e018 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-message.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-message.js @@ -57,7 +57,6 @@ export default class ChatMessage { @tracked _thread; constructor(channel, args = {}) { - // when modifying constructor, be sure to update duplicate function accordingly this.id = args.id; this.channel = channel; this.manager = args.manager; @@ -99,35 +98,8 @@ export default class ChatMessage { } } - duplicate() { - // This is important as a message can exist in the context of a channel or a thread - // The current strategy is to have a different message object in each cases to avoid - // side effects - const message = new ChatMessage(this.channel, { - id: this.id, - newest: this.newest, - staged: this.staged, - edited: this.edited, - availableFlags: this.availableFlags, - hidden: this.hidden, - chatWebhookEvent: this.chatWebhookEvent, - createdAt: this.createdAt, - deletedAt: this.deletedAt, - excerpt: this.excerpt, - reviewableId: this.reviewableId, - userFlagStatus: this.userFlagStatus, - draft: this.draft, - message: this.message, - cooked: this.cooked, - }); - - message.reactions = this.reactions; - message.user = this.user; - message.inReplyTo = this.inReplyTo; - message.bookmark = this.bookmark; - message.uploads = this.uploads; - - return message; + get persisted() { + return !!this.id && !this.staged; } get replyable() { diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-channel-thread.js b/plugins/chat/assets/javascripts/discourse/routes/chat-channel-thread.js index 5a04de8e906..dc358406ac6 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat-channel-thread.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-channel-thread.js @@ -6,7 +6,6 @@ export default class ChatChannelThread extends DiscourseRoute { @service router; @service chatStateManager; @service chat; - @service chatStagedThreadMapping; @service chatThreadPane; model(params, transition) { @@ -46,24 +45,6 @@ export default class ChatChannelThread extends DiscourseRoute { return; } - // This is a very special logic to attempt to reconciliate a staged thread id - // it happens after creating a new thread and having a temp ID in the URL - // if users presses reload at this moment, we would have a 404 - // replacing the ID in the URL sooner would also cause a reload - const { threadId } = this.paramsFor(this.routeName); - - if (threadId?.startsWith("staged-thread-")) { - const mapping = this.chatStagedThreadMapping.getMapping(); - - if (mapping[threadId]) { - transition.abort(); - return this.router.transitionTo( - this.routeName, - ...[...channel.routeModels, mapping[threadId]] - ); - } - } - const { messageId } = this.paramsFor(this.routeName + ".near-message"); if ( !messageId && diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-api.js b/plugins/chat/assets/javascripts/discourse/services/chat-api.js index e8677485470..daae4cc8b19 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-api.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-api.js @@ -153,7 +153,6 @@ export default class ChatApi extends Service { * @param {number} [data.in_reply_to_id] - The ID of the replied-to message. * @param {number} [data.staged_id] - The staged ID of the message before it was persisted. * @param {number} [data.thread_id] - The ID of the thread where this message should be posted. - * @param {number} [data.staged_thread_id] - The staged ID of the thread before it was persisted. * @param {Array.} [data.upload_ids] - Array of upload ids linked to the message. * @returns {Promise} */ @@ -204,6 +203,21 @@ export default class ChatApi extends Service { return this.#putRequest(`/channels/${channelId}`, { channel: data }); } + /** + * Creates a thread. + * @param {number} channelId - The ID of the channel. + * @param {number} originalMessageId - The ID of the original message. + * @param {object} data - Params of the thread. + * @param {string} [data.title] - Title of the thread. + * @returns {Promise} + */ + createThread(channelId, originalMessageId, data = {}) { + return this.#postRequest(`/channels/${channelId}/threads`, { + title: data.title, + original_message_id: originalMessageId, + }); + } + /** * Updates the status of a channel. * @param {number} channelId - The ID of the channel. diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-channel-composer.js b/plugins/chat/assets/javascripts/discourse/services/chat-channel-composer.js index b745b153dff..fdf3823ae37 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-channel-composer.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-channel-composer.js @@ -5,9 +5,11 @@ import { tracked } from "@glimmer/tracking"; export default class ChatChannelComposer extends Service { @service chat; + @service chatApi; @service currentUser; @service router; @service("chat-thread-composer") threadComposer; + @service loadingSlider; @tracked message; @tracked textarea; @@ -54,7 +56,17 @@ export default class ChatChannelComposer extends Service { if (message.channel.threadingEnabled) { if (!message.thread?.id) { - message.thread = message.channel.createStagedThread(message); + this.loadingSlider.transitionStarted(); + const threadObject = await this.chatApi.createThread( + message.channel.id, + message.id + ); + this.loadingSlider.transitionEnded(); + + message.thread = message.channel.threadsManager.add( + message.channel, + threadObject + ); } this.reset(message.channel); diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js index daa1e79b315..09d9365f07f 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js @@ -36,7 +36,6 @@ export function handleStagedMessage(channel, messagesManager, data) { export default class ChatPaneBaseSubscriptionsManager extends Service { @service chat; @service currentUser; - @service chatStagedThreadMapping; messageBusChannel = null; messageBusLastId = null; @@ -215,40 +214,14 @@ export default class ChatPaneBaseSubscriptionsManager extends Service { handleNewThreadCreated(data) { this.model.threadsManager - .find(this.model.id, data.staged_thread_id, { fetchIfNotFound: false }) - .then((stagedThread) => { - if (stagedThread) { - this.chatStagedThreadMapping.setMapping( - data.thread_id, - stagedThread.id - ); - stagedThread.staged = false; - stagedThread.id = data.thread_id; - stagedThread.originalMessage.thread = stagedThread; - stagedThread.originalMessage.thread.preview.replyCount ??= 1; + .find(this.model.id, data.thread_id, { fetchIfNotFound: true }) + .then((thread) => { + const channelOriginalMessage = this.model.messagesManager.findMessage( + thread.originalMessage.id + ); - // We have to do this because the thread manager cache is keyed by - // staged_thread_id, but the thread_id is what we want to use to - // look up the thread, otherwise calls to .find() will not return - // the thread by its actual ID, and we will end up with double-ups - // in places like the thread list when .add() is called. - this.model.threadsManager.remove({ id: data.staged_thread_id }); - this.model.threadsManager.add(this.model, stagedThread, { - replace: true, - }); - } else if (data.thread_id) { - this.model.threadsManager - .find(this.model.id, data.thread_id, { fetchIfNotFound: true }) - .then((thread) => { - const channelOriginalMessage = - this.model.messagesManager.findMessage( - thread.originalMessage.id - ); - - if (channelOriginalMessage) { - channelOriginalMessage.thread = thread; - } - }); + if (channelOriginalMessage) { + channelOriginalMessage.thread = thread; } }); } diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-staged-thread-mapping.js b/plugins/chat/assets/javascripts/discourse/services/chat-staged-thread-mapping.js deleted file mode 100644 index 747e92e3df0..00000000000 --- a/plugins/chat/assets/javascripts/discourse/services/chat-staged-thread-mapping.js +++ /dev/null @@ -1,34 +0,0 @@ -import KeyValueStore from "discourse/lib/key-value-store"; -import Service from "@ember/service"; - -export default class ChatStagedThreadMapping extends Service { - STORE_NAMESPACE = "discourse_chat_"; - KEY = "staged_thread"; - - store = new KeyValueStore(this.STORE_NAMESPACE); - - constructor() { - super(...arguments); - - if (!this.store.getObject(this.USER_EMOJIS_STORE_KEY)) { - this.storedFavorites = []; - } - } - - getMapping() { - return JSON.parse(this.store.getObject(this.KEY) || "{}"); - } - - setMapping(id, stagedId) { - const mapping = {}; - mapping[stagedId] = id; - this.store.setObject({ - key: this.KEY, - value: JSON.stringify(mapping), - }); - } - - reset() { - this.store.setObject({ key: this.KEY, value: "{}" }); - } -} diff --git a/plugins/chat/config/routes.rb b/plugins/chat/config/routes.rb index a4a8c8e3432..7404c0a59d4 100644 --- a/plugins/chat/config/routes.rb +++ b/plugins/chat/config/routes.rb @@ -30,6 +30,7 @@ Chat::Engine.routes.draw do get "/mentions/groups" => "hints#check_group_mentions", :format => :json get "/channels/:channel_id/threads" => "channel_threads#index" + post "/channels/:channel_id/threads" => "channel_threads#create" 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" diff --git a/plugins/chat/spec/requests/chat/api/channel_threads_controller_spec.rb b/plugins/chat/spec/requests/chat/api/channel_threads_controller_spec.rb index 31ca9760f5a..cad5329c68a 100644 --- a/plugins/chat/spec/requests/chat/api/channel_threads_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/channel_threads_controller_spec.rb @@ -234,4 +234,63 @@ RSpec.describe Chat::Api::ChannelThreadsController do end end end + + describe "create" do + fab!(:channel_1) { Fabricate(:chat_channel, threading_enabled: true) } + fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) } + + let(:title) { "a very nice cat" } + let(:params) { { title: title, original_message_id: message_1.id } } + let(:channel_id) { channel_1.id } + + context "when channel does not exist" do + it "returns 404" do + channel_1.destroy! + post "/chat/api/channels/#{channel_id}", params: params + + expect(response.status).to eq(404) + end + end + + context "when channel exists" do + it "creates the thread" do + post "/chat/api/channels/#{channel_id}/threads", params: params + + expect(response.status).to eq(200) + expect(response.parsed_body["title"]).to eq(title) + end + + context "when user cannot view the channel" do + let(:channel_id) { Fabricate(:private_category_channel).id } + + it "returns 403" do + post "/chat/api/channels/#{channel_id}/threads", params: params + + expect(response.status).to eq(403) + end + end + + context "when the title is too long" do + let(:title) { "x" * Chat::Thread::MAX_TITLE_LENGTH + "x" } + + it "returns 400" do + post "/chat/api/channels/#{channel_id}/threads", params: params + + expect(response.status).to eq(400) + expect(response.parsed_body["errors"]).to eq( + ["Title is too long (maximum is #{Chat::Thread::MAX_TITLE_LENGTH} characters)"], + ) + end + end + end + + context "when channel does not have threading enabled" do + fab!(:channel_1) { Fabricate(:chat_channel, threading_enabled: false) } + + it "returns 404" do + post "/chat/api/channels/#{channel_id}/threads", params: params + expect(response.status).to eq(404) + end + end + end end diff --git a/plugins/chat/spec/requests/chat/api/messages_controller_spec.rb b/plugins/chat/spec/requests/chat/api/messages_controller_spec.rb index 274a8493922..e60204b42c6 100644 --- a/plugins/chat/spec/requests/chat/api/messages_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/messages_controller_spec.rb @@ -301,32 +301,6 @@ RSpec.describe Chat::Api::ChannelMessagesController do expect(messages.first.data["last_read_message_id"]).to eq(Chat::Message.last.id) end - context "when sending a message in a staged thread" do - it "creates the thread and publishes with the staged id" do - sign_in(user) - chat_channel.update!(threading_enabled: true) - - messages = - MessageBus.track_publish do - post "/chat/#{chat_channel.id}.json", - params: { - message: message, - in_reply_to_id: message_1.id, - staged_thread_id: "stagedthreadid", - } - end - - expect(response.status).to eq(200) - - thread_event = messages.find { |m| m.data["type"] == "thread_created" } - expect(thread_event.data["staged_thread_id"]).to eq("stagedthreadid") - expect(Chat::Thread.find(thread_event.data["thread_id"])).to be_persisted - - sent_event = messages.find { |m| m.data["type"] == "sent" } - expect(sent_event.data["staged_thread_id"]).to eq("stagedthreadid") - end - end - context "when sending a message in a thread" do fab!(:thread) do Fabricate(:chat_thread, channel: chat_channel, original_message: message_1) diff --git a/plugins/chat/spec/services/chat/create_message_spec.rb b/plugins/chat/spec/services/chat/create_message_spec.rb index 57f4268d618..41a668176f6 100644 --- a/plugins/chat/spec/services/chat/create_message_spec.rb +++ b/plugins/chat/spec/services/chat/create_message_spec.rb @@ -69,12 +69,7 @@ RSpec.describe Chat::CreateMessage do end it "publishes the new message" do - Chat::Publisher.expects(:publish_new!).with( - channel, - instance_of(Chat::Message), - nil, - staged_thread_id: nil, - ) + Chat::Publisher.expects(:publish_new!).with(channel, instance_of(Chat::Message), nil) result end diff --git a/plugins/chat/spec/services/chat/create_thread_spec.rb b/plugins/chat/spec/services/chat/create_thread_spec.rb new file mode 100644 index 00000000000..f687c67e5b8 --- /dev/null +++ b/plugins/chat/spec/services/chat/create_thread_spec.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +RSpec.describe Chat::CreateThread do + describe Chat::CreateThread::Contract, type: :model do + it { is_expected.to validate_presence_of :channel_id } + it { is_expected.to validate_presence_of :original_message_id } + end + + describe ".call" do + subject(:result) { described_class.call(params) } + + fab!(:current_user) { Fabricate(:user) } + fab!(:channel_1) { Fabricate(:chat_channel, threading_enabled: true) } + fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) } + + let(:guardian) { Guardian.new(current_user) } + let(:title) { nil } + let(:params) do + { + guardian: guardian, + original_message_id: message_1.id, + channel_id: channel_1.id, + title: title, + } + end + + context "when all steps pass" do + it "sets the service result as successful" do + expect(result).to be_a_success + end + + it "creates a thread" do + result + expect(result.thread).to be_persisted + end + + it "associates the original message to the thread" do + expect { + result + message_1.reload + }.to change { message_1.thread_id }.from(nil).to(result.thread.id) + end + + it "fetches the membership" do + result + expect(result.membership).to eq(result.thread.membership_for(current_user)) + end + + it "publishes a `thread_created` MessageBus event" do + message = MessageBus.track_publish("/chat/#{channel_1.id}") { result }.first + expect(message.data["type"]).to eq("thread_created") + end + + it "sets the title when existing" do + params[:title] = "Restaurant for Saturday" + result + expect(result.thread.title).to eq(params[:title]) + end + end + + context "when params are not valid" do + before { params.delete(:original_message_id) } + + it { is_expected.to fail_a_contract } + end + + context "when title is too long" do + let(:title) { "a" * Chat::Thread::MAX_TITLE_LENGTH + "a" } + + it { is_expected.to fail_a_contract } + end + + context "when original message is not found" do + fab!(:channel_2) { Fabricate(:chat_channel, threading_enabled: true) } + + before { params[:channel_id] = channel_2.id } + + it { is_expected.to fail_to_find_a_model(:original_message) } + end + + context "when original message is not found" do + before { message_1.destroy! } + + it { is_expected.to fail_to_find_a_model(:original_message) } + end + + context "when user cannot see channel" do + fab!(:private_channel_1) { Fabricate(:private_category_channel, group: Fabricate(:group)) } + + before { params[:channel_id] = private_channel_1.id } + + it { is_expected.to fail_a_policy(:can_view_channel) } + end + + context "when threading is not enabled for the channel" do + before { channel_1.update!(threading_enabled: false) } + + it { is_expected.to fail_a_policy(:threading_enabled_for_channel) } + end + end +end diff --git a/plugins/chat/spec/services/chat/publisher_spec.rb b/plugins/chat/spec/services/chat/publisher_spec.rb index 0385cb0e95d..27bbd016dd3 100644 --- a/plugins/chat/spec/services/chat/publisher_spec.rb +++ b/plugins/chat/spec/services/chat/publisher_spec.rb @@ -181,32 +181,6 @@ describe Chat::Publisher do end end - context "when a staged thread has been provided" do - fab!(:thread) do - Fabricate( - :chat_thread, - original_message: Fabricate(:chat_message, chat_channel: channel), - channel: channel, - ) - end - - before { message_1.update!(thread: thread) } - - it "generates the correct targets" do - targets = - described_class.calculate_publish_targets( - channel, - message_1, - staged_thread_id: "stagedthreadid", - ) - - expect(targets).to contain_exactly( - "/chat/#{channel.id}/thread/#{thread.id}", - "/chat/#{channel.id}/thread/stagedthreadid", - ) - end - end - context "when the message is a thread reply" do fab!(:thread) do Fabricate( diff --git a/plugins/chat/test/javascripts/unit/models/chat-message-test.js b/plugins/chat/test/javascripts/unit/models/chat-message-test.js new file mode 100644 index 00000000000..091d5bfd4e4 --- /dev/null +++ b/plugins/chat/test/javascripts/unit/models/chat-message-test.js @@ -0,0 +1,23 @@ +import { module, test } from "qunit"; +import fabricators from "discourse/plugins/chat/discourse/lib/fabricators"; +import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message"; + +module("Discourse Chat | Unit | Models | chat-message", function () { + test(".persisted", function (assert) { + const channel = fabricators.channel(); + let message = ChatMessage.create(channel, { id: null }); + assert.strictEqual(message.persisted, false); + + message = ChatMessage.create(channel, { + id: 1, + staged: true, + }); + assert.strictEqual(message.persisted, false); + + message = ChatMessage.create(channel, { + id: 1, + staged: false, + }); + assert.strictEqual(message.persisted, true); + }); +});