DEV: Add uploadHandler support to composer-upload-uppy mixin (#14692)

DEV: Add uploadHandler support to composer-upload-uppy mixin (#14692)

This commit adds uploadHandler support to composer uploads using uppy. The only things we have that are using this are discourse-brightcove and discourse-video, which both pop modal windows to handle the file upload and completely leave out all the composer-type flows. This implementation simply follows the existing one, where if a single file is uploaded and there is a matching upload handler we take control away from uppy and hand it off to the upload handler.

Trying to get this kind of thing working within uppy would require a few changes because they have no way to restrict uploaders to certain file types and with the way their uploaders are run it doesn’t look like it would be easy to add this either, so I don’t think this is worth the work unless at some point in the future we plan to have more upload handler integrations.

I also fixed an issue with cleanUpComposerUploadHandler which is used in tests to reset the state of uploadHandlers in the composer. This was doing uploadHandlers = [] to clear that array, but that creates a brand new array so anything else referencing the original array will lose that reference. Better to set uploadHandlers.length = 0 to clear it. This was breaking the tests I added to see if upload handlers were working.

diff --git a/app/assets/javascripts/discourse/app/components/composer-editor.js b/app/assets/javascripts/discourse/app/components/composer-editor.js
index 1e01a57..2aed00d 100644
--- a/app/assets/javascripts/discourse/app/components/composer-editor.js
+++ b/app/assets/javascripts/discourse/app/components/composer-editor.js
@@ -48,7 +48,14 @@ export function addComposerUploadHandler(extensions, method) {
   });
 }
 export function cleanUpComposerUploadHandler() {
-  uploadHandlers = [];
+  // we cannot set this to uploadHandlers = [] because that messes with
+  // the references to the original array that the component has. this only
+  // really affects tests, but without doing this you could addComposerUploadHandler
+  // in a beforeEach function in a test but then it's not adding to the
+  // existing reference that the component has, because an earlier test ran
+  // cleanUpComposerUploadHandler and lost it. setting the length to 0 empties
+  // the array but keeps the reference
+  uploadHandlers.length = 0;
 }
 
 let uploadProcessorQueue = [];
@@ -688,6 +695,14 @@ export default Component.extend(ComposerUpload, {
     }
   },
 
+  _findMatchingUploadHandler(fileName) {
+    return this.uploadHandlers.find((handler) => {
+      const ext = handler.extensions.join("|");
+      const regex = new RegExp(`\\.(${ext})$`, "i");
+      return regex.test(fileName);
+    });
+  },
+
   actions: {
     importQuote(toolbarEvent) {
       this.importQuote(toolbarEvent);
diff --git a/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js b/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js
index c5f1e08..1538efa 100644
--- a/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js
+++ b/app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js
@@ -67,6 +67,12 @@ export default Mixin.create(ExtendableUploader, {
     }
   },
 
+  _abortAndReset() {
+    this.appEvents.trigger(`${this.eventPrefix}:uploads-aborted`);
+    this._reset();
+    return false;
+  },
+
   _bindUploadTarget() {
     this.placeholders = {};
     this._inProgressUploads = 0;
@@ -118,6 +124,20 @@ export default Mixin.create(ExtendableUploader, {
         const fileCount = Object.keys(files).length;
         const maxFiles = this.siteSettings.simultaneous_uploads;
 
+        // Look for a matching file upload handler contributed from a plugin.
+        // It is not ideal that this only works for single file uploads, but
+        // at this time it is all we need. In future we may want to devise a
+        // nicer way of doing this. Uppy plugins are out of the question because
+        // there is no way to define which uploader plugin handles which file
+        // extensions at this time.
+        if (fileCount === 1) {
+          const file = Object.values(files)[0];
+          const matchingHandler = this._findMatchingUploadHandler(file.name);
+          if (matchingHandler && !matchingHandler.method(file.data, this)) {
+            return this._abortAndReset();
+          }
+        }
+
         // Limit the number of simultaneous uploads
         if (maxFiles > 0 && fileCount > maxFiles) {
           bootbox.alert(
@@ -125,9 +145,7 @@ export default Mixin.create(ExtendableUploader, {
               count: maxFiles,
             })
           );
-          this.appEvents.trigger(`${this.eventPrefix}:uploads-aborted`);
-          this._reset();
-          return false;
+          return this._abortAndReset();
         }
       },
     });
diff --git a/app/assets/javascripts/discourse/app/mixins/composer-upload.js b/app/assets/javascripts/discourse/app/mixins/composer-upload.js
index 7c5446c..24fae9e 100644
--- a/app/assets/javascripts/discourse/app/mixins/composer-upload.js
+++ b/app/assets/javascripts/discourse/app/mixins/composer-upload.js
@@ -199,9 +199,10 @@ export default Mixin.create({
 
     $element.on("fileuploadsubmit", (e, data) => {
       const max = this.siteSettings.simultaneous_uploads;
+      const fileCount = data.files.length;
 
       // Limit the number of simultaneous uploads
-      if (max > 0 && data.files.length > max) {
+      if (max > 0 && fileCount > max) {
         bootbox.alert(
           I18n.t("post.errors.too_many_dragged_and_dropped_files", {
             count: max,
@@ -211,15 +212,10 @@ export default Mixin.create({
       }
 
       // Look for a matching file upload handler contributed from a plugin
-      const matcher = (handler) => {
-        const ext = handler.extensions.join("|");
-        const regex = new RegExp(`\\.(${ext})$`, "i");
-        return regex.test(data.files[0].name);
-      };
-
-      const matchingHandler = this.uploadHandlers.find(matcher);
-      if (data.files.length === 1 && matchingHandler) {
-        if (!matchingHandler.method(data.files[0], this)) {
+      if (fileCount === 1) {
+        const file = data.files[0];
+        const matchingHandler = this._findMatchingUploadHandler(file.name);
+        if (matchingHandler && !matchingHandler.method(file, this)) {
           return false;
         }
       }
diff --git a/app/assets/javascripts/discourse/tests/acceptance/composer-attachment-test.js b/app/assets/javascripts/discourse/tests/acceptance/composer-attachment-test.js
index c4c21df..1fe8845 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/composer-attachment-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/composer-attachment-test.js
@@ -4,7 +4,9 @@ import {
   query,
   queryAll,
 } from "discourse/tests/helpers/qunit-helpers";
+import { withPluginApi } from "discourse/lib/plugin-api";
 import { click, fillIn, visit } from "@ember/test-helpers";
+import bootbox from "bootbox";
 import { test } from "qunit";
 
 function pretender(server, helper) {
@@ -239,20 +241,51 @@ acceptance("Composer Attachment - Upload Placeholder", function (needs) {
       "![ima++ge|300x400](/images/avatar.png?4)\n"
     );
   });
+});
 
-  function createImage(name, url, width, height) {
-    const file = new Blob([""], { type: "image/png" });
-    file.name = name;
-    return {
-      files: [file],
-      result: {
-        original_filename: name,
-        thumbnail_width: width,
-        thumbnail_height: height,
-        url: url,
-      },
-    };
-  }
+function createImage(name, url, width, height) {
+  const file = new Blob([""], { type: "image/png" });
+  file.name = name;
+  return {
+    files: [file],
+    result: {
+      original_filename: name,
+      thumbnail_width: width,
+      thumbnail_height: height,
+      url,
+    },
+  };
+}
+
+acceptance("Composer Attachment - Upload Handler", function (needs) {
+  needs.user();
+  needs.hooks.beforeEach(() => {
+    withPluginApi("0.8.14", (api) => {
+      api.addComposerUploadHandler(["png"], (file) => {
+        bootbox.alert(`This is an upload handler test for ${file.name}`);
+      });
+    });
+  });
+
+  test("should handle a single file being uploaded with the extension handler", async function (assert) {
+    await visit("/");
+    await click("#create-topic");
+    const image = createImage(
+      "handlertest.png",
+      "/images/avatar.png?1",
+      200,
+      300
+    );
+    await fillIn(".d-editor-input", "This is a handler test.");
+
+    await queryAll(".wmd-controls").trigger("fileuploadsubmit", image);
+    assert.equal(
+      queryAll(".bootbox .modal-body").html(),
+      "This is an upload handler test for handlertest.png",
+      "it should show the bootbox triggered by the upload handler"
+    );
+    await click(".modal-footer .btn");
+  });
 });
 
 acceptance("Composer Attachment - File input", function (needs) {
diff --git a/app/assets/javascripts/discourse/tests/acceptance/composer-uploads-uppy-test.js b/app/assets/javascripts/discourse/tests/acceptance/composer-uploads-uppy-test.js
index 6b94f03..8148f1b 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/composer-uploads-uppy-test.js

[... diff too long, it was truncated ...]

GitHub sha: f6528afa019ded81012947efdf2835324488183b

This commit appears in #14692 which was approved by eviltrout. It was merged by martin.