FIX: call composer reset with correct params (#21777)

We were calling reset without the proper params which was causing errors in the console. This commit does the following changes:

- ensures `composer.cancel()` is the only way to cancel editing/reply
- adds a `draftSaved` property to chat message to allow for better tests
- writes a spec to ensure the flow is correct
- adds more page objects for better tests
- homogenize the default state of objects on chat message

Co-authored-by: Martin Brennan <martin@discourse.org>

app/assets/javascripts/discourse/app/mixins/uppy-upload.js

@@ -73,7 +73,6 @@ export default Mixin.create(UppyS3Multipart, ExtendableUploader, {
     });
     this.set("allowMultipleFiles", this.fileInputEl.multiple);
     this.set("inProgressUploads", []);
-    this._triggerInProgressUploadsEvent();
 
     this._bindFileInputChange();
 
@@ -298,9 +297,11 @@ export default Mixin.create(UppyS3Multipart, ExtendableUploader, {
     this._uppyInstance.on("cancel-all", () => {
       this.appEvents.trigger(`upload-mixin:${this.id}:uploads-cancelled`);
       if (!this.isDestroyed && !this.isDestroying) {
+        if (this.inProgressUploads.length) {
           this.set("inProgressUploads", []);
           this._triggerInProgressUploadsEvent();
         }
+      }
     });
 
     this.appEvents.on(`upload-mixin:${this.id}:add-files`, this._addFiles);

plugins/chat/app/controllers/chat/chat_controller.rb

@@ -90,7 +90,7 @@ module Chat
       raise Discourse::InvalidAccess unless @user_chat_channel_membership
 
       reply_to_msg_id = params[:in_reply_to_id]
-      if reply_to_msg_id
+      if reply_to_msg_id.present?
         rm = Chat::Message.find(reply_to_msg_id)
         raise Discourse::NotFound if rm.chat_channel_id != @chat_channel.id
       end

plugins/chat/assets/javascripts/discourse/components/chat-channel.hbs

@@ -8,9 +8,9 @@
   {{did-insert this.setUploadDropZone}}
   {{did-insert this.setupListeners}}
   {{will-destroy this.teardownListeners}}
-  {{did-insert this.updateChannel}}
   {{did-update this.loadMessages @targetMessageId}}
-  {{did-update this.updateChannel @channel.id}}
+  {{did-insert this.didUpdateChannel}}
+  {{did-update this.didUpdateChannel @channel.id}}
   {{did-insert this.addAutoFocusEventListener}}
   {{will-destroy this.removeAutoFocusEventListener}}
   data-id={{@channel.id}}

plugins/chat/assets/javascripts/discourse/components/chat-channel.js

@@ -105,19 +105,32 @@ export default class ChatLivePane extends Component {
   }
 
   @action
-  updateChannel() {
+  didUpdateChannel() {
     this.#cancelHandlers();
 
     this.loadedOnce = false;
 
+    if (!this.args.channel) {
+      return;
+    }
+
     // Technically we could keep messages to avoid re-fetching them, but
     // it's not worth the complexity for now
-    this.args.channel?.clearMessages();
+    this.args.channel.clearMessages();
 
-    if (this._loadedChannelId !== this.args.channel?.id) {
+    if (this._loadedChannelId !== this.args.channel.id) {
       this.unsubscribeToUpdates(this._loadedChannelId);
       this.chatChannelPane.selectingMessages = false;
-      this._loadedChannelId = this.args.channel?.id;
+      this._loadedChannelId = this.args.channel.id;
+    }
+
+    const existingDraft = this.chatDraftsManager.get({
+      channelId: this.args.channel.id,
+    });
+    if (existingDraft) {
+      this.chatChannelComposer.message = existingDraft;
+    } else {
+      this.resetComposer();
     }
 
     this.loadMessages();

plugins/chat/assets/javascripts/discourse/components/chat-composer.hbs

@@ -22,12 +22,11 @@
       (if this.pane.sending "is-sending")
       (if this.sendEnabled "is-send-enabled" "is-send-disabled")
       (if this.disabled "is-disabled" "is-enabled")
+      (if this.currentMessage.draftSaved "is-draft-saved" "is-draft-unsaved")
     }}
     {{did-update this.didUpdateMessage this.currentMessage}}
     {{did-update this.didUpdateInReplyTo this.currentMessage.inReplyTo}}
     {{did-insert this.setup}}
-    {{did-insert this.didUpdateChannel}}
-    {{did-update this.didUpdateChannel @channel.id}}
     {{will-destroy this.teardown}}
     {{will-destroy this.cancelPersistDraft}}
   >

plugins/chat/assets/javascripts/discourse/components/chat-composer.js

@@ -18,7 +18,6 @@ import { translations } from "pretty-text/emoji/data";
 import { setupHashtagAutocomplete } from "discourse/lib/hashtag-autocomplete";
 import { isEmpty, isPresent } from "@ember/utils";
 import { Promise } from "rsvp";
-import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message";
 import User from "discourse/models/user";
 import ChatMessageInteractor from "discourse/plugins/chat/discourse/lib/chat-message-interactor";
 
@@ -142,22 +141,6 @@ export default class ChatComposer extends Component {
     );
   }
 
-  @action
-  didUpdateChannel() {
-    this.cancelPersistDraft();
-
-    if (!this.args.channel) {
-      this.composer.message = null;
-      return;
-    }
-
-    this.composer.message =
-      this.chatDraftsManager.get({ channelId: this.args.channel.id }) ||
-      ChatMessage.createDraftMessage(this.args.channel, {
-        user: this.currentUser,
-      });
-  }
-
   @action
   setup() {
     this.appEvents.on("chat:modify-selection", this, "modifySelection");
@@ -211,6 +194,7 @@ export default class ChatComposer extends Component {
 
   @action
   onInput(event) {
+    this.currentMessage.draftSaved = false;
     this.currentMessage.message = event.target.value;
     this.textareaInteractor.refreshHeight();
     this.reportReplyingPresence();
@@ -224,6 +208,8 @@ export default class ChatComposer extends Component {
       return;
     }
 
+    this.currentMessage.draftSaved = false;
+
     this.inProgressUploadsCount = inProgressUploadsCount || 0;
 
     if (
@@ -376,7 +362,7 @@ export default class ChatComposer extends Component {
       if (this.currentMessage?.inReplyTo) {
         this.reset();
       } else if (this.currentMessage?.editing) {
-        this.composer.onCancelEditing();
+        this.composer.cancel();
       } else {
         event.target.blur();
       }
@@ -385,7 +371,7 @@ export default class ChatComposer extends Component {
 
   @action
   reset() {
-    this.composer.reset(this.args.channel);
+    this.composer.reset(this.args.channel, this.args.thread);
   }
 
   @action

plugins/chat/assets/javascripts/discourse/components/chat-thread.hbs

@@ -6,10 +6,8 @@
   }}
   data-id={{this.thread.id}}
   {{did-insert this.setUploadDropZone}}
-  {{did-insert this.subscribeToUpdates}}
+  {{did-insert this.didUpdateThread}}
-  {{did-update this.subscribeToUpdates this.thread.id}}
+  {{did-update this.didUpdateThread this.thread.id}}
-  {{did-insert this.loadMessages}}
-  {{did-update this.loadMessages this.thread}}
   {{will-destroy this.unsubscribeFromUpdates}}
 >
   {{#if @includeHeader}}

plugins/chat/assets/javascripts/discourse/components/chat-thread.js

@@ -39,6 +39,13 @@ export default class ChatThreadPanel extends Component {
     return this.thread?.channel;
   }
 
+  @action
+  didUpdateThread() {
+    this.subscribeToUpdates();
+    this.loadMessages();
+    this.resetComposer();
+  }
+
   @action
   setUploadDropZone(element) {
     this.uploadDropZone = element;
@@ -121,13 +128,6 @@ export default class ChatThreadPanel extends Component {
 
   @action
   loadMessages() {
-    const message = ChatMessage.createDraftMessage(this.channel, {
-      user: this.currentUser,
-    });
-    message.thread = this.thread;
-    message.inReplyTo = this.thread.originalMessage;
-    this.chatChannelThreadComposer.message = message;
-
     this.thread.messagesManager.clearMessages();
     this.fetchMessages();
   }
@@ -253,11 +253,13 @@ export default class ChatThreadPanel extends Component {
     return this.chatApi
       .sendMessage(this.channel.id, {
         message: message.message,
-        in_reply_to_id: message.inReplyTo?.id,
+        in_reply_to_id: message.thread.staged
+          ? message.thread.originalMessage.id
+          : null,
         staged_id: message.id,
         upload_ids: message.uploads.map((upload) => upload.id),
-        thread_id: this.thread.staged ? null : message.thread.id,
+        thread_id: message.thread.staged ? null : message.thread.id,
-        staged_thread_id: this.thread.staged ? message.thread.id : null,
+        staged_thread_id: message.thread.staged ? message.thread.id : null,
       })
       .catch((error) => {
         this.#onSendError(message.id, error);

plugins/chat/assets/javascripts/discourse/components/chat/composer/channel.js

@@ -37,7 +37,11 @@ export default class ChatComposerChannel extends ChatComposer {
 
   @action
   _debouncedPersistDraft(channelId, jsonDraft) {
-    this.chatApi.saveDraft(channelId, jsonDraft);
+    this.chatApi.saveDraft(channelId, jsonDraft).then(() => {
+      if (this.currentMessage) {
+        this.currentMessage.draftSaved = true;
+      }
+    });
   }
 
   get lastMessage() {

plugins/chat/assets/javascripts/discourse/models/chat-message.js

@@ -24,8 +24,9 @@ export default class ChatMessage {
   @tracked error;
   @tracked selected;
   @tracked channel;
-  @tracked staged = false;
+  @tracked staged;
-  @tracked draft = false;
+  @tracked draftSaved;
+  @tracked draft;
   @tracked channelId;
   @tracked createdAt;
   @tracked deletedAt;
@@ -38,34 +39,35 @@ export default class ChatMessage {
   @tracked expanded;
   @tracked bookmark;
   @tracked userFlagStatus;
-  @tracked hidden = false;
+  @tracked hidden;
   @tracked version = 0;
-  @tracked edited = false;
+  @tracked edited;
-  @tracked editing = false;
+  @tracked editing;
   @tracked chatWebhookEvent = new TrackedObject();
   @tracked mentionWarning;
   @tracked availableFlags;
-  @tracked newest = false;
+  @tracked newest;
-  @tracked highlighted = false;
+  @tracked highlighted;
-  @tracked firstOfResults = false;
+  @tracked firstOfResults;
   @tracked message;
   @tracked thread;
   @tracked threadReplyCount;
-  @tracked manager = null;
+  @tracked manager;
-  @tracked threadTitle = null;
+  @tracked threadTitle;
 
   @tracked _cooked;
 
   constructor(channel, args = {}) {
     // when modifying constructor, be sure to update duplicate function accordingly
     this.id = args.id;
-    this.newest = args.newest;
+    this.newest = args.newest || false;
-    this.firstOfResults = args.firstOfResults;
+    this.draftSaved = args.draftSaved || args.draft_saved || false;
-    this.staged = args.staged;
+    this.firstOfResults = args.firstOfResults || args.first_of_results || false;
-    this.edited = args.edited;
+    this.staged = args.staged || false;
-    this.editing = args.editing;
+    this.edited = args.edited || false;
+    this.editing = args.editing || false;
     this.availableFlags = args.availableFlags || args.available_flags;
-    this.hidden = args.hidden;
+    this.hidden = args.hidden || false;
     this.threadReplyCount = args.threadReplyCount || args.thread_reply_count;
     this.threadTitle = args.threadTitle || args.thread_title;
     this.chatWebhookEvent = args.chatWebhookEvent || args.chat_webhook_event;

plugins/chat/assets/javascripts/discourse/services/chat-channel-composer.js

@@ -7,6 +7,11 @@ export default class ChatChannelComposer extends ChatComposer {
   @service chat;
   @service router;
 
+  @action
+  cancelEditing() {
+    this.reset(this.message.channel);
+  }
+
   @action
   reset(channel) {
     this.message = ChatMessage.createDraftMessage(channel, {
@@ -31,7 +36,7 @@ export default class ChatChannelComposer extends ChatComposer {
         message.thread = thread;
       }
 
-      this.chat.activeMessage = null;
+      this.reset(channel);
       this.router.transitionTo("chat.channel.thread", ...thread.routeModels);
     } else {
       this.message.inReplyTo = message;

plugins/chat/assets/javascripts/discourse/services/chat-composer.js

@@ -11,12 +11,17 @@ export default class ChatComposer extends Service {
   @action
   cancel() {
     if (this.message.editing) {
-      this.reset();
+      this.cancelEditing();
     } else if (this.message.inReplyTo) {
-      this.message.inReplyTo = null;
+      this.cancelReply();
     }
   }
 
+  @action
+  cancelReply() {
+    this.message.inReplyTo = null;
+  }
+
   @action
   clear() {
     this.message.message = "";
@@ -28,9 +33,4 @@ export default class ChatComposer extends Service {
     message.editing = true;
     this.message = message;
   }
-
-  @action
-  onCancelEditing() {
-    this.reset();
-  }
 }

plugins/chat/spec/system/chat_composer_draft_spec.rb

@@ -72,6 +72,17 @@ RSpec.describe "Chat composer draft", type: :system, js: true do
 
         expect(channel_page.composer).to be_editing_message(message_1)
       end
+
+      context "when canceling editing" do
+        it "resets the draft" do
+          chat_page.visit_channel(channel_1)
+          channel_page.composer.message_details.cancel_edit
+
+          expect(channel_page.composer).to be_blank
+          expect(channel_page.composer).to have_unsaved_draft
+          expect(channel_page.composer).to have_saved_draft
+        end
+      end
     end
 
     context "with uploads" do

plugins/chat/spec/system/message_thread_indicator_spec.rb

@@ -102,13 +102,18 @@ describe "Thread indicator for chat messages", type: :system, js: true do
 
     it "increments the indicator when a new reply is sent in the thread" do
       chat_page.visit_channel(channel)
+
       expect(channel_page.message_thread_indicator(thread_1.original_message)).to have_css(
         ".chat-message-thread-indicator__replies-count",
         text: I18n.t("js.chat.thread.replies", count: 3),
       )
+
       channel_page.message_thread_indicator(thread_1.original_message).click
+
       expect(side_panel).to have_open_thread(thread_1)
-      open_thread.send_message("new thread message")
+
+      open_thread.send_message
+
       expect(channel_page.message_thread_indicator(thread_1.original_message)).to have_css(
         ".chat-message-thread-indicator__replies-count",
         text: I18n.t("js.chat.thread.replies", count: 4),

plugins/chat/spec/system/page_objects/chat/chat_channel.rb

@@ -128,8 +128,9 @@ module PageObjects
       def send_message(text = nil)
         text ||= Faker::Lorem.characters(number: SiteSetting.chat_minimum_message_length)
         text = text.chomp if text.present? # having \n on the end of the string counts as an Enter keypress
-        fill_composer(text)
+        composer.fill_in(with: text)
         click_send_message
+        messages.has_message?(text: text, persisted: true)
         click_composer
         has_no_loading_skeleton?
         text

plugins/chat/spec/system/page_objects/chat/chat_thread.rb

@@ -60,8 +60,9 @@ module PageObjects
       def send_message(text = nil)
         text ||= Faker::Lorem.characters(number: SiteSetting.chat_minimum_message_length)
         text = text.chomp if text.present? # having \n on the end of the string counts as an Enter keypress
-        fill_composer(text)
+        composer.fill_in(with: text)
         click_send_message
+        messages.has_message?(text: text, persisted: true)
         click_composer
         text
       end

plugins/chat/spec/system/page_objects/chat/components/composer.rb

@@ -14,12 +14,28 @@ module PageObjects
           @context = context
         end
 
+        def blank?
+          input.value.blank?
+        end
+
+        def has_saved_draft?
+          component.has_css?(".chat-composer.is-draft-saved")
+        end
+
+        def has_unsaved_draft?
+          component.has_css?(".chat-composer.is-draft-unsaved")
+        end
+
         def message_details
           @message_details ||= PageObjects::Components::Chat::ComposerMessageDetails.new(context)
         end
 
         def input
-          find(context).find(SELECTOR).find(".chat-composer__input")
+          component.find(".chat-composer__input")
+        end
+
+        def component
+          find(context).find(SELECTOR)
         end
 
         def fill_in(**args)

plugins/chat/spec/system/page_objects/chat/components/composer_message_details.rb

@@ -36,6 +36,10 @@ module PageObjects
         def replying_to?(message)
           has_message?(id: message.id, action: :reply)
         end
+
+        def cancel_edit
+          component.find(".cancel-message-action").click
+        end
       end
     end
   end

plugins/chat/spec/system/reply_to_message/drawer_spec.rb

@@ -26,6 +26,7 @@ RSpec.describe "Reply to message - channel - drawer", type: :system, js: true do
       chat_page.open_from_header
       drawer_page.open_channel(channel_1)
       channel_page.reply_to(original_message)
+
       expect(drawer_page).to have_open_thread
 
       thread_page.fill_composer("reply to message")

plugins/chat/spec/system/reply_to_message/mobile_spec.rb

@@ -54,15 +54,7 @@ RSpec.describe "Reply to message - channel - mobile", type: :system, js: true, m
 
   context "when the message has an existing thread" do
     fab!(:message_1) do
-      creator =
+      Fabricate(:chat_message, chat_channel: channel_1, in_reply_to: original_message)
-        Chat::MessageCreator.new(
-          chat_channel: channel_1,
-          in_reply_to_id: original_message.id,
-          user: Fabricate(:user),
-          content: Faker::Lorem.paragraph,
-        )
-      creator.create
-      creator.chat_message
     end
 
     it "replies to the existing thread" do
@@ -71,11 +63,7 @@ RSpec.describe "Reply to message - channel - mobile", type: :system, js: true, m
       expect(channel_page).to have_thread_indicator(original_message, text: "1")
 
       channel_page.reply_to(original_message)
-
+      thread_page.send_message("reply to message")
-      expect(side_panel_page).to have_open_thread
-
-      thread_page.fill_composer("reply to message")
-      thread_page.click_send_message
 
       expect(thread_page).to have_message(text: message_1.message)
       expect(thread_page).to have_message(text: "reply to message")
@@ -83,7 +71,7 @@ RSpec.describe "Reply to message - channel - mobile", type: :system, js: true, m
       thread_page.close
 
       expect(channel_page).to have_thread_indicator(original_message, text: "2")
-      expect(channel_page).to have_no_message(text: "reply to message")
+      expect(channel_page.messages).to have_no_message(text: "reply to message")
     end
   end
 

plugins/chat/spec/system/uploads_spec.rb

@@ -6,7 +6,7 @@ describe "Uploading files in chat messages", type: :system, js: true do
   fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
 
   let(:chat) { PageObjects::Pages::Chat.new }
-  let(:channel) { PageObjects::Pages::ChatChannel.new }
+  let(:channel_page) { PageObjects::Pages::ChatChannel.new }
 
   before { chat_system_bootstrap }
 
@@ -20,16 +20,16 @@ describe "Uploading files in chat messages", type: :system, js: true do
       chat.visit_channel(channel_1)
       file_path = file_from_fixtures("logo.png", "images").path
       attach_file(file_path) do
-        channel.open_action_menu
+        channel_page.open_action_menu
-        channel.click_action_button("chat-upload-btn")
+        channel_page.click_action_button("chat-upload-btn")
       end
 
       expect(page).to have_css(".chat-composer-upload .preview .preview-img")
 
-      channel.send_message("upload testing")
+      channel_page.send_message("upload testing")
 
       expect(page).not_to have_css(".chat-composer-upload")
-      expect(channel).to have_message(text: "upload testing")
+      expect(channel_page.messages).to have_message(text: "upload testing", persisted: true)
       expect(Chat::Message.last.uploads.count).to eq(1)
     end
 
@@ -39,15 +39,15 @@ describe "Uploading files in chat messages", type: :system, js: true do
       file_path_1 = file_from_fixtures("logo.png", "images").path
       file_path_2 = file_from_fixtures("logo.jpg", "images").path
       attach_file([file_path_1, file_path_2]) do
-        channel.open_action_menu
+        channel_page.open_action_menu
-        channel.click_action_button("chat-upload-btn")
+        channel_page.click_action_button("chat-upload-btn")
       end
 
       expect(page).to have_css(".chat-composer-upload .preview .preview-img", count: 2)
-      channel.send_message("upload testing")
+      channel_page.send_message("upload testing")
 
       expect(page).not_to have_css(".chat-composer-upload")
-      expect(channel).to have_message(text: "upload testing")
+      expect(channel_page.messages).to have_message(text: "upload testing", persisted: true)
       expect(Chat::Message.last.uploads.count).to eq(2)
     end
 
@@ -56,8 +56,8 @@ describe "Uploading files in chat messages", type: :system, js: true do
       chat.visit_channel(channel_1)
       file_path = file_from_fixtures("huge.jpg", "images").path
       attach_file(file_path) do
-        channel.open_action_menu
+        channel_page.open_action_menu
-        channel.click_action_button("chat-upload-btn")
+        channel_page.click_action_button("chat-upload-btn")
       end
 
       expect(find(".chat-composer-upload")).to have_content("Processing")
@@ -66,10 +66,10 @@ describe "Uploading files in chat messages", type: :system, js: true do
       # to complete then the upload to complete as well
       expect(page).to have_css(".chat-composer-upload .preview .preview-img", wait: 25)
 
-      channel.send_message("upload testing")
+      channel_page.send_message("upload testing")
 
       expect(page).not_to have_css(".chat-composer-upload")
-      expect(channel).to have_message(text: "upload testing")
+      expect(channel_page.messages).to have_message(text: "upload testing", persisted: true)
       expect(Chat::Message.last.uploads.count).to eq(1)
     end
   end
@@ -95,27 +95,27 @@ describe "Uploading files in chat messages", type: :system, js: true do
 
     it "allows deleting uploads" do
       chat.visit_channel(channel_1)
-      channel.open_edit_message(message_2)
+      channel_page.open_edit_message(message_2)
       find(".chat-composer-upload").hover
       find(".chat-composer-upload__remove-btn").click
-      channel.click_send_message
+      channel_page.click_send_message
-      expect(channel.message_by_id(message_2.id)).not_to have_css(".chat-uploads")
+      expect(channel_page.message_by_id(message_2.id)).not_to have_css(".chat-uploads")
       try_until_success(timeout: 5) { expect(message_2.reload.uploads).to be_empty }
     end
 
     it "allows adding more uploads" do
       chat.visit_channel(channel_1)
-      channel.open_edit_message(message_2)
+      channel_page.open_edit_message(message_2)
 
       file_path = file_from_fixtures("logo.png", "images").path
       attach_file(file_path) do
-        channel.open_action_menu
+        channel_page.open_action_menu
-        channel.click_action_button("chat-upload-btn")
+        channel_page.click_action_button("chat-upload-btn")
       end
 
       expect(page).to have_css(".chat-composer-upload .preview .preview-img", count: 2)
 
-      channel.click_send_message
+      channel_page.click_send_message
 
       expect(page).not_to have_css(".chat-composer-upload")
       expect(page).to have_css(".chat-img-upload", count: 2)
@@ -138,13 +138,13 @@ describe "Uploading files in chat messages", type: :system, js: true do
 
     it "does not show the action button for uploading files in public channels" do
       chat.visit_channel(channel_1)
-      channel.open_action_menu
+      channel_page.open_action_menu
       expect(page).not_to have_css(".chat-upload-btn")
     end
 
     it "does not show the action button for uploading files in direct message channels" do
       chat.visit_channel(direct_message_channel_1)
-      channel.open_action_menu
+      channel_page.open_action_menu
       expect(page).not_to have_css(".chat-upload-btn")
     end
   end