From ba1b95c9f4bc6c67b9c18b244a63a0de53118653 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 9 Mar 2023 18:17:54 +1000 Subject: [PATCH] FIX: Uploading multiple files to chat could cause canellations (#20605) When we introduced `existingUploads` as an arg to the ChatComposerUploads component, we also introduced a bug where if multiple uploads were being done at once, and the draft was saved, then because of didReceiveAttrs we would cancel the currently uploading files because the draft uploads became the existingUploads. To work around this, since we do want to keep this on didReceiveAttrs for cases when the user opens a draft or edits another message, the easiest thing to do is to just not save uploads into the chat draft if there are still uploads in progress. That way only when all uploads are complete do we make them a part of the draft. There is a small risk that the user could do something to lose their uploads in the draft, but it's a better gamble to have that happen rather than in progress uploads to be cancelled while the user is waiting for them to be done because of the draft. Also changes the uploads system spec back to the old way of attaching multiple files since that is why it was failing. --- .../discourse/components/chat-composer-uploads.js | 14 ++++++++++---- .../discourse/components/chat-composer.js | 4 ++-- .../discourse/components/chat-live-pane.js | 14 ++++++++++++-- plugins/chat/spec/system/uploads_spec.rb | 9 +-------- 4 files changed, 25 insertions(+), 16 deletions(-) diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-composer-uploads.js b/plugins/chat/assets/javascripts/discourse/components/chat-composer-uploads.js index 2b76d566810..a6b7f6568fc 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-composer-uploads.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-composer-uploads.js @@ -26,7 +26,7 @@ export default Component.extend(UppyUploadMixin, { didReceiveAttrs() { this._super(...arguments); - if (this._uppyInstance?.getState?.()?.totalProgress > 0) { + if (this.inProgressUploads?.length > 0) { this._uppyInstance?.cancelAll(); } @@ -53,7 +53,7 @@ export default Component.extend(UppyUploadMixin, { uploadDone(upload) { this.uploads.pushObject(upload); - this.onUploadChanged(this.uploads); + this._triggerUploadsChanged(); }, @discourseComputed("uploads.length", "inProgressUploads.length") @@ -67,13 +67,13 @@ export default Component.extend(UppyUploadMixin, { fileId: upload.id, }); this.uploads.removeObject(upload); - this.onUploadChanged(this.uploads); + this._triggerUploadsChanged(); }, @action removeUpload(upload) { this.uploads.removeObject(upload); - this.onUploadChanged(this.uploads); + this._triggerUploadsChanged(); }, _uploadDropTargetOptions() { @@ -134,4 +134,10 @@ export default Component.extend(UppyUploadMixin, { this._addFiles([...event.clipboardData.files], { pasted: true }); } }, + + _triggerUploadsChanged() { + this.onUploadChanged(this.uploads, { + inProgressUploadsCount: this.inProgressUploads?.length, + }); + }, }); diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-composer.js b/plugins/chat/assets/javascripts/discourse/components/chat-composer.js index b66d897dee1..eb7d434c4db 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-composer.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-composer.js @@ -719,9 +719,9 @@ export default Component.extend(TextareaTextManipulation, { }, @action - uploadsChanged(uploads) { + uploadsChanged(uploads, { inProgressUploadsCount }) { this.set("_uploads", cloneJSON(uploads)); - this.onValueChange?.({ uploads: this._uploads }); + this.onValueChange?.({ uploads: this._uploads, inProgressUploadsCount }); }, @action diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.js b/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.js index 817a1c4cc2f..f3d303c3f55 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.js @@ -1005,14 +1005,24 @@ export default class ChatLivePane extends Component { } @action - composerValueChanged({ value, uploads, replyToMsg }) { + composerValueChanged({ value, uploads, replyToMsg, inProgressUploadsCount }) { if (!this.editingMessage && !this.args.channel.isDraft) { if (typeof value !== "undefined") { this.args.channel.draft.message = value; } - if (typeof uploads !== "undefined") { + + // only save the uploads to the draft if we are not still uploading other + // ones, otherwise we get into a cycle where we pass the draft uploads as + // existingUploads back to the upload component and cause in progress ones + // to be cancelled + if ( + typeof uploads !== "undefined" && + inProgressUploadsCount !== "undefined" && + inProgressUploadsCount === 0 + ) { this.args.channel.draft.uploads = uploads; } + if (typeof replyToMsg !== "undefined") { this.args.channel.draft.replyToMsg = replyToMsg; } diff --git a/plugins/chat/spec/system/uploads_spec.rb b/plugins/chat/spec/system/uploads_spec.rb index 09dee0e400d..90bcb05d4f7 100644 --- a/plugins/chat/spec/system/uploads_spec.rb +++ b/plugins/chat/spec/system/uploads_spec.rb @@ -38,17 +38,10 @@ describe "Uploading files in chat messages", type: :system, js: true do chat.visit_channel(channel_1) file_path_1 = file_from_fixtures("logo.png", "images").path - attach_file([file_path_1]) do - channel.open_action_menu - channel.click_action_button("chat-upload-btn") - find(".chat-composer-input").click - end - file_path_2 = file_from_fixtures("logo.jpg", "images").path - attach_file([file_path_2]) do + attach_file([file_path_1, file_path_2]) do channel.open_action_menu channel.click_action_button("chat-upload-btn") - find(".chat-composer-input").click end expect(page).to have_css(".chat-composer-upload .preview .preview-img", count: 2)