FEATURE: Change all core to use uppy-image-uploader (#14428)

Instead of using image-uploader, which relies on the old
UploadMixin, we can now use the uppy-image-uploader which
uses the new UppyUploadMixin which is stable enough and
supports both regular XHR uploads and direct S3 uploads,
controlled by a site setting (default to XHR).

At some point it may make sense to rename uppy-image-uploader
back to image-uploader, once we have gone through plugins
etc. and given a bit of deprecation time period.

This commit also fixes `for_private_message`, `for_site_setting`,
and `pasted` flags not being sent via uppy uploads onto the
UploadCreator, both via regular XHR uploads and also through
external/multipart uploads.

The uploaders changed are:

* site setting images
* badge images
* category logo
* category background
* group flair
* profile background
* profile card background

app/assets/javascripts/admin/addon/components/site-settings-image-uploader.js

@@ -1,6 +0,0 @@
-import ImageUploader from "discourse/components/image-uploader";
-
-export default ImageUploader.extend({
-  layoutName: "components/image-uploader",
-  uploadUrlParams: "&for_site_setting=true",
-});

app/assets/javascripts/admin/addon/templates/badges-show.hbs

@@ -40,12 +40,14 @@
         </label>
       </div>
       {{#if imageUploaderSelected}}
-        {{image-uploader
+        {{uppy-image-uploader
+          id="badge-image-uploader"
           imageUrl=buffered.image_url
+          type="badge_image"
           onUploadDone=(action "setImage")
           onUploadDeleted=(action "removeImage")
-          type="badge_image"
-          class="no-repeat contain-image"}}
+          class="no-repeat contain-image"
+        }}
         <div class="control-instructions">
           <p class="help">{{i18n "admin.badges.image_help"}}</p>
         </div>

app/assets/javascripts/admin/addon/templates/components/site-settings/upload.hbs

@@ -1,2 +1,8 @@
-{{site-settings-image-uploader imageUrl=value placeholderUrl=setting.placeholder type="site_setting"}}
+{{uppy-image-uploader
+  imageUrl=value
+  placeholderUrl=setting.placeholder
+  additionalParams=(hash for_site_setting=true)
+  type="site_setting"
+  id=(concat "site-setting-image-uploader-" setting.setting)
+}}
 <div class="desc">{{html-safe setting.description}}</div>

app/assets/javascripts/discourse/app/components/image-uploader.js

@@ -1,4 +1,5 @@
 import Component from "@ember/component";
+import deprecated from "discourse-common/lib/deprecated";
 import UploadMixin from "discourse/mixins/upload";
 import { ajax } from "discourse/lib/ajax";
 import discourseComputed from "discourse-common/utils/decorators";
@@ -14,6 +15,10 @@ export default Component.extend(UploadMixin, {
 
   init() {
     this._super(...arguments);
+    // TODO (martin) (2022-01-22) Remove this component.
+    deprecated(
+      "image-uploader will be removed in a future version, use uppy-image-uploader instead (the API is the same)"
+    );
     this._applyLightbox();
   },
 

app/assets/javascripts/discourse/app/components/uppy-image-uploader.js

@@ -69,8 +69,6 @@ export default Component.extend(UppyUploadMixin, {
 
   uploadDone(upload) {
     this.setProperties({
-      imageUrl: upload.url,
-      imageId: upload.id,
       imageFilesize: upload.human_filesize,
       imageFilename: upload.original_filename,
       imageWidth: upload.width,
@@ -79,8 +77,13 @@ export default Component.extend(UppyUploadMixin, {
 
     this._applyLightbox();
 
+    // the value of the property used for imageUrl should be set
+    // in this callback. this should be done in cases where imageUrl
+    // is bound to a computed property of the parent component.
     if (this.onUploadDone) {
       this.onUploadDone(upload);
+    } else {
+      this.set("imageUrl", upload.url);
     }
   },
 
@@ -123,13 +126,16 @@ export default Component.extend(UppyUploadMixin, {
     },
 
     trash() {
-      this.setProperties({ imageUrl: null, imageId: null });
-
       // uppy needs to be reset to allow for more uploads
       this._reset();
 
+      // the value of the property used for imageUrl should be cleared
+      // in this callback. this should be done in cases where imageUrl
+      // is bound to a computed property of the parent component.
       if (this.onUploadDeleted) {
         this.onUploadDeleted();
+      } else {
+        this.setProperties({ imageUrl: null });
       }
     },
   },

app/assets/javascripts/discourse/app/controllers/preferences/profile.js

@@ -68,10 +68,6 @@ export default Controller.extend({
     "model.can_upload_user_card_background"
   ),
 
-  experimentalUserCardImageUpload: readOnly(
-    "siteSettings.enable_experimental_image_uploader"
-  ),
-
   actions: {
     showFeaturedTopicModal() {
       showModal("feature-topic-on-profile", {

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

@@ -136,7 +136,6 @@ export default Mixin.create(ExtendableUploader, {
       this._instrumentUploadTimings();
     }
 
-    // hidden setting like enable_experimental_image_uploader
     if (this.siteSettings.enable_direct_s3_uploads) {
       this._useS3MultipartUploads();
     } else {
@@ -482,6 +481,8 @@ export default Mixin.create(ExtendableUploader, {
           data: JSON.stringify({
             parts,
             unique_identifier: file.meta.unique_identifier,
+            pasted: file.meta.pasted,
+            for_private_message: file.meta.for_private_message,
           }),
           // uppy is inconsistent, an error here fires the upload-error event
         }).then((responseData) => {
@@ -570,14 +571,14 @@ export default Mixin.create(ExtendableUploader, {
       }
 
       if (event && event.clipboardData && event.clipboardData.files) {
-        this._addFiles([...event.clipboardData.files]);
+        this._addFiles([...event.clipboardData.files], { pasted: true });
       }
     }.bind(this);
 
     this.element.addEventListener("paste", this.pasteEventListener);
   },
 
-  _addFiles(files) {
+  _addFiles(files, opts = {}) {
     files = Array.isArray(files) ? files : [files];
     try {
       this._uppyInstance.addFiles(
@@ -587,6 +588,7 @@ export default Mixin.create(ExtendableUploader, {
             name: file.name,
             type: file.type,
             data: file,
+            meta: { pasted: opts.pasted },
           };
         })
       );

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

@@ -25,10 +25,6 @@ export default Mixin.create({
   autoStartUploads: true,
   id: null,
 
-  // TODO (martin): this is only used in one place, consider just using
-  // form data/meta instead uploadUrlParams: "&for_site_setting=true",
-  uploadUrlParams: "",
-
   // TODO (martin): currently used for backups to turn on auto upload and PUT/XML requests
   // and for emojis to do sequential uploads, when we get to replacing those
   // with uppy make sure this is used when initializing uppy
@@ -83,7 +79,11 @@ export default Mixin.create({
 
       // need to use upload_type because uppy overrides type with the
       // actual file type
-      meta: deepMerge({ upload_type: this.type }, this.data || {}),
+      meta: deepMerge(
+        { upload_type: this.type },
+        this.additionalParams || {},
+        this.data || {}
+      ),
 
       onBeforeFileAdded: (currentFile) => {
         const validationOpts = deepMerge(
@@ -160,7 +160,6 @@ export default Mixin.create({
       this._reset();
     });
 
-    // hidden setting like enable_experimental_image_uploader
     if (this.siteSettings.enable_direct_s3_uploads) {
       this._useS3Uploads();
     } else {
@@ -224,8 +223,7 @@ export default Mixin.create({
     return (
       getUrl(this.getWithDefault("uploadUrl", "/uploads")) +
       ".json?client_id=" +
-      (this.messageBus && this.messageBus.clientId) +
-      this.uploadUrlParams
+      this.messageBus?.clientId
     );
   },
 
@@ -252,9 +250,10 @@ export default Mixin.create({
   _completeExternalUpload(file) {
     return ajax(getUrl("/uploads/complete-external-upload"), {
       type: "POST",
-      data: {
-        unique_identifier: file.meta.uniqueUploadIdentifier,
-      },
+      data: deepMerge(
+        { unique_identifier: file.meta.uniqueUploadIdentifier },
+        this.additionalParams || {}
+      ),
     });
   },
 

app/assets/javascripts/discourse/app/templates/components/edit-category-images.hbs

@@ -1,18 +1,22 @@
 <section class="field category-logo">
   <label>{{i18n "category.logo"}}</label>
-  {{image-uploader
+  {{uppy-image-uploader
     imageUrl=logoImageUrl
     onUploadDone=(action "logoUploadDone")
     onUploadDeleted=(action "logoUploadDeleted")
     type="category_logo"
-    class="no-repeat contain-image"}}
+    class="no-repeat contain-image"
+    id="category-logo-uploader"
+  }}
 </section>
 
 <section class="field category-background-image">
   <label>{{i18n "category.background_image"}}</label>
-  {{image-uploader
+  {{uppy-image-uploader
     imageUrl=backgroundImageUrl
     onUploadDone=(action "backgroundUploadDone")
     onUploadDeleted=(action "backgroundUploadDeleted")
-    type="category_background"}}
+    type="category_background"
+    id="category-background-uploader"
+  }}
 </section>

app/assets/javascripts/discourse/app/templates/components/group-flair-inputs.hbs

@@ -21,11 +21,12 @@
       onChange=(action (mut model.flair_icon))
     }}
   {{else if flairPreviewImage}}
-    {{image-uploader
+    {{uppy-image-uploader
       imageUrl=flairImageUrl
       onUploadDone=(action "setFlairImage")
       onUploadDeleted=(action "removeFlairImage")
       type="group_flair"
+      id="group-flair-uploader"
       class="no-repeat contain-image"}}
     <div class="control-instructions">
       {{i18n "groups.flair_upload_description"}}

app/assets/javascripts/discourse/app/templates/preferences/profile.hbs

@@ -47,8 +47,11 @@
     <div class="control-group pref-profile-bg">
       <label class="control-label">{{i18n "user.change_profile_background.title"}}</label>
       <div class="controls">
-        {{image-uploader imageUrl=model.profile_background_upload_url
-          type="profile_background"}}
+        {{uppy-image-uploader
+          imageUrl=model.profile_background_upload_url
+          type="profile_background"
+          id="profile-background-uploader"
+        }}
       </div>
       <div class="instructions">
         {{i18n "user.change_profile_background.instructions"}}
@@ -59,15 +62,11 @@
     <div class="control-group pref-profile-bg">
       <label class="control-label">{{i18n "user.change_card_background.title"}}</label>
       <div class="controls">
-        {{#if experimentalUserCardImageUpload}}
-          {{uppy-image-uploader
-            imageUrl=model.card_background_upload_url
-            type="card_background"
-            id="profile-card-background"
-          }}
-        {{else}}
-          {{image-uploader imageUrl=model.card_background_upload_url type="card_background"}}
-        {{/if}}
+        {{uppy-image-uploader
+          imageUrl=model.card_background_upload_url
+          type="card_background"
+          id="profile-card-background-uploader"
+        }}
       </div>
       <div class="instructions">
         {{i18n "user.change_card_background.instructions"}}

app/controllers/uploads_controller.rb

@@ -253,7 +253,13 @@ class UploadsController < ApplicationController
   end
 
   def complete_external_upload_via_manager(external_upload_stub)
-    external_upload_manager = ExternalUploadManager.new(external_upload_stub)
+    opts = {
+      for_private_message: params[:for_private_message]&.to_s == "true",
+      for_site_setting: params[:for_site_setting]&.to_s == "true",
+      pasted: params[:pasted]&.to_s == "true",
+    }
+
+    external_upload_manager = ExternalUploadManager.new(external_upload_stub, opts)
     hijack do
       begin
         upload = external_upload_manager.promote_to_upload!

app/services/external_upload_manager.rb

@@ -20,8 +20,9 @@ class ExternalUploadManager
     Discourse.redis.get("#{BAN_USER_REDIS_PREFIX}#{user.id}") == "1"
   end
 
-  def initialize(external_upload_stub)
+  def initialize(external_upload_stub, upload_create_opts = {})
     @external_upload_stub = external_upload_stub
+    @upload_create_opts = upload_create_opts
   end
 
   def can_promote?
@@ -67,18 +68,13 @@ class ExternalUploadManager
     end
 
     # TODO (martin): See if these additional opts will be needed
-    #
-    # for_private_message: for_private_message,
-    # for_site_setting: for_site_setting,
-    # pasted: pasted,
-    #
-    # also check if retain_hours is needed
+    # - check if retain_hours is needed
     opts = {
       type: external_upload_stub.upload_type,
       existing_external_upload_key: external_upload_stub.key,
       external_upload_too_big: external_size > DOWNLOAD_LIMIT,
       filesize: external_size
-    }
+    }.merge(@upload_create_opts)
 
     UploadCreator.new(tempfile, external_upload_stub.original_filename, opts).create_for(
       external_upload_stub.created_by_id

db/migrate/20211018234219_remove_enable_experimental_image_uploader_site_setting.rb

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+class RemoveEnableExperimentalImageUploaderSiteSetting < ActiveRecord::Migration[6.1]
+  def up
+    execute <<~SQL
+      DELETE FROM site_settings
+      WHERE name = 'enable_experimental_image_uploader'
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end