UX: Add image uploader widget for uploading badge images (#12377)

Currently the process of adding a custom image to badge is quite clunky; you have to upload your image to a topic, and then copy the image URL and pasting it in a text field. Besides being clucky, if the topic or post that contains the image is deleted, the image will be garbage-collected in a few days and the badge will lose the image because the application is not that the image is referenced by a badge.

This commit improves that by adding a proper image uploader widget for badge images.

app/assets/javascripts/admin/addon/controllers/admin-badges-show.js

@@ -5,19 +5,27 @@ import bootbox from "bootbox";
 import { bufferedProperty } from "discourse/mixins/buffered-content";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import { propertyNotEqual } from "discourse/lib/computed";
-import { reads } from "@ember/object/computed";
+import { equal, reads } from "@ember/object/computed";
 import { run } from "@ember/runloop";
+import { action } from "@ember/object";
+import getURL from "discourse-common/lib/get-url";
+
+const IMAGE = "image";
+const ICON = "icon";
 
 export default Controller.extend(bufferedProperty("model"), {
   adminBadges: controller(),
   saving: false,
   savingStatus: "",
+  selectedGraphicType: null,
   badgeTypes: reads("adminBadges.badgeTypes"),
   badgeGroupings: reads("adminBadges.badgeGroupings"),
   badgeTriggers: reads("adminBadges.badgeTriggers"),
   protectedSystemFields: reads("adminBadges.protectedSystemFields"),
   readOnly: reads("buffered.system"),
   showDisplayName: propertyNotEqual("name", "displayName"),
+  iconSelectorSelected: equal("selectedGraphicType", ICON),
+  imageUploaderSelected: equal("selectedGraphicType", IMAGE),
 
   init() {
     this._super(...arguments);
@@ -67,6 +75,41 @@ export default Controller.extend(bufferedProperty("model"), {
     this.set("savingStatus", "");
   },
 
+  showIconSelector() {
+    this.set("selectedGraphicType", ICON);
+  },
+
+  showImageUploader() {
+    this.set("selectedGraphicType", IMAGE);
+  },
+
+  @action
+  changeGraphicType(newType) {
+    if (newType === IMAGE) {
+      this.showImageUploader();
+    } else if (newType === ICON) {
+      this.showIconSelector();
+    } else {
+      throw new Error(`Unknown badge graphic type "${newType}"`);
+    }
+  },
+
+  @action
+  setImage(upload) {
+    this.buffered.setProperties({
+      image_upload_id: upload.id,
+      image_url: getURL(upload.url),
+    });
+  },
+
+  @action
+  removeImage() {
+    this.buffered.setProperties({
+      image_upload_id: null,
+      image_url: null,
+    });
+  },
+
   actions: {
     save() {
       if (!this.saving) {
@@ -82,7 +125,7 @@ export default Controller.extend(bufferedProperty("model"), {
           "description",
           "long_description",
           "icon",
-          "image",
+          "image_upload_id",
           "query",
           "badge_grouping_id",
           "trigger",

app/assets/javascripts/admin/addon/routes/admin-badges-show.js

@@ -23,6 +23,15 @@ export default Route.extend({
     );
   },
 
+  setupController(controller, model) {
+    this._super(...arguments);
+    if (model.image_url) {
+      controller.showImageUploader();
+    } else if (model.icon) {
+      controller.showIconSelector();
+    }
+  },
+
   actions: {
     saveError(e) {
       let msg = I18n.t("generic_error");

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

@@ -15,23 +15,48 @@
     </div>
 
     <div>
-      <label for="icon">{{i18n "admin.badges.icon"}}</label>
+      <label for="graphic">{{i18n "admin.badges.graphic"}}</label>
-      {{icon-picker
+      <div class="radios">
-        name="icon"
+        <label class="radio-label" for="badge-icon">
-        value=buffered.icon
+          {{radio-button
-        options=(hash
+            name="badge-icon"
-          maximum=1
+            id="badge-icon"
-        )
+            value="icon"
-        onChange=(action (mut buffered.icon))
+            selection=selectedGraphicType
-      }}
+            onChange=(action "changeGraphicType")
+          }}
+          <span>{{i18n "admin.badges.select_an_icon"}}</span>
+        </label>
 
-      <p class="help">{{i18n "admin.badges.icon_help"}}</p>
+        <label class="radio-label" for="badge-image">
-    </div>
+          {{radio-button
-
+            name="badge-image"
-    <div>
+            id="badge-image"
-      <label for="image">{{i18n "admin.badges.image"}}</label>
+            value="image"
-      {{input type="text" name="image" value=buffered.image}}
+            selection=selectedGraphicType
-      <p class="help">{{i18n "admin.badges.image_help"}}</p>
+            onChange=(action "changeGraphicType")
+          }}
+          <span>{{i18n "admin.badges.upload_an_image"}}</span>
+        </label>
+      </div>
+      {{#if imageUploaderSelected}}
+        {{image-uploader
+          imageUrl=buffered.image_url
+          onUploadDone=(action "setImage")
+          onUploadDeleted=(action "removeImage")
+          type="badge_image"
+          class="no-repeat contain-image"}}
+        <div class="control-instructions">
+          <p class="help">{{i18n "admin.badges.image_help"}}</p>
+        </div>
+      {{else if iconSelectorSelected}}
+        {{icon-picker
+          name="icon"
+          value=buffered.icon
+          options=(hash maximum=1)
+          onChange=(action (mut buffered.icon))
+        }}
+      {{/if}}
     </div>
 
     <div>

app/assets/javascripts/discourse/app/models/badge.js

@@ -5,10 +5,11 @@ import RestModel from "discourse/models/rest";
 import { ajax } from "discourse/lib/ajax";
 import discourseComputed from "discourse-common/utils/decorators";
 import getURL from "discourse-common/lib/get-url";
-import { none } from "@ember/object/computed";
+import { alias, none } from "@ember/object/computed";
 
 const Badge = RestModel.extend({
   newBadge: none("id"),
+  image: alias("image_url"),
 
   @discourseComputed
   url() {

app/assets/javascripts/discourse/tests/acceptance/admin-badges-show-test.js

@@ -0,0 +1,101 @@
+import {
+  acceptance,
+  exists,
+  query,
+} from "discourse/tests/helpers/qunit-helpers";
+import { click, visit } from "@ember/test-helpers";
+import { test } from "qunit";
+
+acceptance("Admin - Badges - Show", function (needs) {
+  needs.user();
+  test("new badge page", async function (assert) {
+    await visit("/admin/badges/new");
+    assert.ok(
+      !query("input#badge-icon").checked,
+      "radio button for selecting an icon is off initially"
+    );
+    assert.ok(
+      !query("input#badge-image").checked,
+      "radio button for uploading an image is off initially"
+    );
+    assert.ok(!exists(".icon-picker"), "icon picker is not visible");
+    assert.ok(!exists(".image-uploader"), "image uploader is not visible");
+
+    await click("input#badge-icon");
+    assert.ok(
+      exists(".icon-picker"),
+      "icon picker is visible after clicking the select icon radio button"
+    );
+    assert.ok(!exists(".image-uploader"), "image uploader remains hidden");
+
+    await click("input#badge-image");
+    assert.ok(
+      !exists(".icon-picker"),
+      "icon picker is hidden after clicking the upload image radio button"
+    );
+    assert.ok(
+      exists(".image-uploader"),
+      "image uploader becomes visible after clicking the upload image radio button"
+    );
+  });
+
+  test("existing badge that has an icon", async function (assert) {
+    await visit("/admin/badges/1");
+    assert.ok(
+      query("input#badge-icon").checked,
+      "radio button for selecting an icon is on"
+    );
+    assert.ok(
+      !query("input#badge-image").checked,
+      "radio button for uploading an image is off"
+    );
+    assert.ok(exists(".icon-picker"), "icon picker is visible");
+    assert.ok(!exists(".image-uploader"), "image uploader is not visible");
+    assert.equal(query(".icon-picker").textContent.trim(), "fa-rocket");
+  });
+
+  test("existing badge that has an image URL", async function (assert) {
+    await visit("/admin/badges/2");
+    assert.ok(
+      !query("input#badge-icon").checked,
+      "radio button for selecting an icon is off"
+    );
+    assert.ok(
+      query("input#badge-image").checked,
+      "radio button for uploading an image is on"
+    );
+    assert.ok(!exists(".icon-picker"), "icon picker is not visible");
+    assert.ok(exists(".image-uploader"), "image uploader is visible");
+    assert.ok(
+      query(".image-uploader a.lightbox").href.endsWith(
+        "/assets/some-image.png"
+      ),
+      "image uploader shows the right image"
+    );
+  });
+
+  test("existing badge that has both an icon and image URL", async function (assert) {
+    await visit("/admin/badges/3");
+    assert.ok(
+      !query("input#badge-icon").checked,
+      "radio button for selecting an icon is off because image overrides icon"
+    );
+    assert.ok(
+      query("input#badge-image").checked,
+      "radio button for uploading an image is on because image overrides icon"
+    );
+    assert.ok(!exists(".icon-picker"), "icon picker is not visible");
+    assert.ok(exists(".image-uploader"), "image uploader is visible");
+    assert.ok(
+      query(".image-uploader a.lightbox").href.endsWith(
+        "/assets/some-image.png"
+      ),
+      "image uploader shows the right image"
+    );
+
+    await click("input#badge-icon");
+    assert.ok(exists(".icon-picker"), "icon picker is becomes visible");
+    assert.ok(!exists(".image-uploader"), "image uploader bcomes hidden");
+    assert.equal(query(".icon-picker").textContent.trim(), "fa-rocket");
+  });
+});

app/assets/javascripts/discourse/tests/fixtures/badges-fixture.js

@@ -1703,5 +1703,30 @@ export default {
         granted_by_id: -1
       }
     ]
+  },
+
+  "/admin/badges.json": {
+    admin_badges: {
+      triggers: []
+    },
+    badge_groupings: [],
+    badges: [
+      {
+        id: 1,
+        name: "Only icon",
+        icon: "fa-rocket",
+      },
+      {
+        id: 2,
+        name: "Only image",
+        image_url: "/assets/some-image.png",
+      },
+      {
+        id: 3,
+        name: "Both image and icon",
+        icon: "fa-rocket",
+        image_url: "/assets/some-image.png",
+      },
+    ]
   }
 };

app/assets/stylesheets/common/admin/badges.scss

@@ -81,6 +81,14 @@
     }
   }
   .form-horizontal {
+    .radios {
+      display: flex;
+
+      .radio-label {
+        margin-right: 1.5em;
+      }
+    }
+
     .ace-wrapper {
       position: relative;
       height: 270px;

app/controllers/admin/badges_controller.rb

@@ -9,7 +9,7 @@ class Admin::BadgesController < Admin::AdminController
       badge_types: BadgeType.all.order(:id).to_a,
       badge_groupings: BadgeGrouping.all.order(:position).to_a,
       badges: Badge.includes(:badge_grouping)
-        .includes(:badge_type)
+        .includes(:badge_type, :image_upload)
         .references(:badge_grouping)
         .order('badge_groupings.position, badge_type_id, badges.name').to_a,
       protected_system_fields: Badge.protected_system_fields,

app/controllers/badges_controller.rb

@@ -17,7 +17,7 @@ class BadgesController < ApplicationController
     if (params[:only_listable] == "true") || !request.xhr?
       # NOTE: this is sorted client side if needed
       badges = badges.includes(:badge_grouping)
-        .includes(:badge_type)
+        .includes(:badge_type, :image_upload)
         .where(enabled: true, listable: true)
     end
 

app/controllers/user_badges_controller.rb

@@ -41,7 +41,7 @@ class UserBadgesController < ApplicationController
         .select(UserBadge.attribute_names.map { |x| "MAX(#{x}) AS #{x}" }, 'COUNT(*) AS "count"')
     end
 
-    user_badges = user_badges.includes(badge: [:badge_grouping, :badge_type])
+    user_badges = user_badges.includes(badge: [:badge_grouping, :badge_type, :image_upload])
       .includes(post: :topic)
       .includes(:granted_by)
 

app/jobs/onceoff/migrate_badge_image_to_uploads.rb

@@ -0,0 +1,91 @@
+# frozen_string_literal: true
+require 'uri'
+
+module Jobs
+  class MigrateBadgeImageToUploads < ::Jobs::Onceoff
+    def execute_onceoff(args)
+      column_exists = DB.exec(<<~SQL) == 1
+        SELECT 1
+        FROM INFORMATION_SCHEMA.COLUMNS
+        WHERE
+          table_schema = 'public' AND
+          table_name = 'badges' AND
+          column_name = 'image_upload_id'
+      SQL
+      return unless column_exists
+
+      Badge.where.not(image: nil).select(:id, :image_upload_id, :image).each do |badge|
+        if badge.image_upload.present?
+          DB.exec("UPDATE badges SET image = NULL WHERE id = ?", badge.id)
+          next
+        end
+
+        image_url = badge[:image]
+        next if image_url.blank? || image_url !~ URI.regexp
+
+        count = 0
+        file = nil
+        sleep_interval = 5
+
+        loop do
+          url = UrlHelper.absolute_without_cdn(image_url)
+
+          begin
+            file = FileHelper.download(
+              url,
+              max_file_size: [
+                SiteSetting.max_image_size_kb.kilobytes,
+                20.megabytes
+              ].max,
+              tmp_file_name: 'tmp_badge_image_upload',
+              skip_rate_limit: true,
+              follow_redirect: true
+            )
+          rescue OpenURI::HTTPError,
+                 OpenSSL::SSL::SSLError,
+                 Net::OpenTimeout,
+                 Net::ReadTimeout,
+                 Errno::ECONNREFUSED,
+                 EOFError,
+                 SocketError,
+                 Discourse::InvalidParameters => e
+
+            logger.error(
+              "Error encountered when trying to download from URL '#{image_url}' " +
+              "for badge '#{badge[:id]}'.\n#{e.class}: #{e.message}\n#{e.backtrace.join("\n")}"
+            )
+          end
+
+          count += 1
+          break if file
+
+          logger.warn(
+            "Failed to download image from #{url} for badge '#{badge[:id]}'. Retrying (#{count}/3)..."
+          )
+          break if count >= 3
+          sleep(count * sleep_interval)
+        end
+
+        next if file.blank?
+
+        upload = UploadCreator.new(
+          file,
+          "image_for_badge_#{badge[:id]}",
+          origin: UrlHelper.absolute(image_url)
+        ).create_for(Discourse.system_user.id)
+
+        if upload.errors.count > 0 || upload&.id.blank?
+          logger.error("Failed to create an upload for the image of badge '#{badge[:id]}'. Error: #{upload.errors.full_messages}")
+        else
+          DB.exec("UPDATE badges SET image = NULL, image_upload_id = ? WHERE id = ?", upload.id, badge[:id])
+        end
+      end
+    end
+
+    private
+
+    def logger
+      Rails.logger
+    end
+  end
+end

app/jobs/scheduled/clean_up_uploads.rb

@@ -43,6 +43,7 @@ module Jobs
         .joins("LEFT JOIN theme_fields tf ON tf.upload_id = uploads.id")
         .joins("LEFT JOIN user_exports ue ON ue.upload_id = uploads.id")
         .joins("LEFT JOIN groups g ON g.flair_upload_id = uploads.id")
+        .joins("LEFT JOIN badges b ON b.image_upload_id = uploads.id")
         .where("pu.upload_id IS NULL")
         .where("u.uploaded_avatar_id IS NULL")
         .where("ua.gravatar_upload_id IS NULL AND ua.custom_upload_id IS NULL")
@@ -52,6 +53,7 @@ module Jobs
         .where("tf.upload_id IS NULL")
         .where("ue.upload_id IS NULL")
         .where("g.flair_upload_id IS NULL")
+        .where("b.image_upload_id IS NULL")
         .where("ss.value IS NULL")
 
       if SiteSetting.selectable_avatars.present?

app/models/badge.rb

@@ -1,6 +1,11 @@
 # frozen_string_literal: true
 
 class Badge < ActiveRecord::Base
+  # TODO: Drop in July 2021
+  self.ignored_columns = %w{image}
+
+  include GlobalPath
+
   # NOTE: These badge ids are not in order! They are grouped logically.
   #       When picking an id, *search* for it.
 
@@ -100,6 +105,7 @@ class Badge < ActiveRecord::Base
 
   belongs_to :badge_type
   belongs_to :badge_grouping
+  belongs_to :image_upload, class_name: 'Upload'
 
   has_many :user_badges, dependent: :destroy
 
@@ -236,7 +242,7 @@ class Badge < ActiveRecord::Base
   end
 
   def default_icon=(val)
-    unless self.image
+    if self.image_upload_id.blank?
       self.icon ||= val
       self.icon = val if self.icon == "fa-certificate"
     end
@@ -293,6 +299,12 @@ class Badge < ActiveRecord::Base
     @i18n_name ||= self.class.i18n_name(name)
   end
 
+  def image_url
+    if image_upload_id.present?
+      upload_cdn_path(image_upload.url)
+    end
+  end
+
   protected
 
   def ensure_not_system

app/serializers/badge_serializer.rb

@@ -2,7 +2,7 @@
 
 class BadgeSerializer < ApplicationSerializer
   attributes :id, :name, :description, :grant_count, :allow_title,
-             :multiple_grant, :icon, :image, :listable, :enabled, :badge_grouping_id,
+             :multiple_grant, :icon, :image_url, :listable, :enabled, :badge_grouping_id,
              :system, :long_description, :slug, :has_badge, :manually_grantable?
 
   has_one :badge_type

app/services/staff_action_logger.rb

@@ -323,7 +323,7 @@ class StaffActionLogger
     ))
   end
 
-  BADGE_FIELDS ||= %i{id name description long_description icon image badge_type_id
+  BADGE_FIELDS ||= %i{id name description long_description icon image_upload_id badge_type_id
     badge_grouping_id query allow_title multiple_grant listable target_posts
     enabled auto_revoke show_posts system}
 

config/locales/client.en.yml

@@ -5094,8 +5094,11 @@ en:
         enabled: Enable badge
         icon: Icon
         image: Image
+        graphic: Graphic
         icon_help: "Enter a Font Awesome icon name (use prefix 'far-' for regular icons and 'fab-' for brand icons)"
-        image_help: "Enter the URL of the image (overrides icon field if both are set)"
+        image_help: "Uploading an image overrides icon field if both are set."
+        select_an_icon: "Select an Icon"
+        upload_an_image: "Upload an Image"
         read_only_setting_help: "Customize text"
         query: Badge Query (SQL)
         target_posts: Query targets posts

db/migrate/20210311070755_add_image_upload_id_to_badges.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+class AddImageUploadIdToBadges < ActiveRecord::Migration[6.0]
+  def change
+    add_column :badges, :image_upload_id, :integer
+    reversible do |dir|
+      dir.up do
+        DB.exec <<~SQL
+          UPDATE badges b1
+          SET image_upload_id = u.id
+          FROM badges b2
+          INNER JOIN uploads u
+          ON b2.image ~ CONCAT('/', u.sha1, '\\.\\w')
+          WHERE b1.id = b2.id
+        SQL
+      end
+    end
+  end
+end

spec/jobs/clean_up_uploads_spec.rb

@@ -295,4 +295,14 @@ describe Jobs::CleanUpUploads do
     expect(Upload.exists?(id: expired_upload.id)).to eq(false)
     expect(Upload.exists?(id: theme_upload.id)).to eq(true)
   end
+
+  it "does not delete badges uploads" do
+    badge_image = fabricate_upload
+    badge = Fabricate(:badge, image_upload_id: badge_image.id)
+
+    Jobs::CleanUpUploads.new.execute(nil)
+
+    expect(Upload.exists?(id: expired_upload.id)).to eq(false)
+    expect(Upload.exists?(id: badge_image.id)).to eq(true)
+  end
 end

spec/jobs/migrate_badge_image_to_uploads_spec.rb

@@ -0,0 +1,65 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Jobs::MigrateBadgeImageToUploads do
+  let(:image_url) { "https://omg.aws.somestack/test.png" }
+  let(:badge) { Fabricate(:badge) }
+
+  before do
+    @orig_logger = Rails.logger
+    Rails.logger = @fake_logger = FakeLogger.new
+  end
+
+  after do
+    Rails.logger = @orig_logger
+  end
+
+  it 'should migrate to the new badge `image_upload_id` column correctly' do
+    stub_request(:get, image_url).to_return(
+      status: 200, body: file_from_fixtures("smallest.png").read
+    )
+    DB.exec(<<~SQL, flair_url: image_url, id: badge.id)
+      UPDATE badges SET image = :flair_url WHERE id = :id
+    SQL
+
+    expect do
+      described_class.new.execute_onceoff({})
+    end.to change { Upload.count }.by(1)
+
+    badge.reload
+    upload = Upload.last
+    expect(badge.image_upload).to eq(upload)
+    expect(badge.image_url).to eq(upload.url)
+    expect(badge[:image]).to eq(nil)
+  end
+
+  it 'should skip badges with invalid flair URLs' do
+    DB.exec("UPDATE badges SET image = 'abc' WHERE id = ?", badge.id)
+    described_class.new.execute_onceoff({})
+    expect(Rails.logger.warnings.count).to eq(0)
+    expect(Rails.logger.errors.count).to eq(0)
+  end
+
+  # this case has a couple of hacks that are needed to test this behavior, so if it
+  # starts failing randomly in the future, I'd just delete it and not bother with it
+  it 'should not keep retrying forever if download fails' do
+    stub_request(:get, image_url).to_return(status: 403)
+    instance = described_class.new
+    instance.expects(:sleep).times(2)
+
+    DB.exec(<<~SQL, flair_url: image_url, id: badge.id)
+      UPDATE badges SET image = :flair_url WHERE id = :id
+    SQL
+
+    expect do
+      instance.execute_onceoff({})
+    end.not_to change { Upload.count }
+
+    badge.reload
+    expect(badge.image_upload).to eq(nil)
+    expect(badge.image_url).to eq(nil)
+    expect(Badge.where(id: badge.id).select(:image).first[:image]).to eq(image_url)
+    expect(Rails.logger.warnings.count).to eq(3)
+  end
+end

spec/models/badge_spec.rb

@@ -71,6 +71,16 @@ describe Badge do
     end
   end
 
+  describe '#image_url' do
+    it 'has CDN url' do
+      SiteSetting.enable_s3_uploads = true
+      SiteSetting.s3_cdn_url = "https://some-s3-cdn.amzn.com"
+      upload = Fabricate(:upload_s3)
+      badge = Fabricate(:badge, image_upload_id: upload.id)
+      expect(badge.image_url).to start_with("https://some-s3-cdn.amzn.com")
+    end
+  end
+
   describe '.i18n_name' do
     it 'transforms to lower case letters, and replaces spaces with underscores' do
       expect(Badge.i18n_name('Basic User')).to eq('basic_user')

spec/requests/admin/badges_controller_spec.rb

@@ -138,6 +138,7 @@ describe Admin::BadgesController do
       it 'updates the badge' do
         SiteSetting.enable_badge_sql = true
         sql = "select id user_id, created_at granted_at from users"
+        image = Fabricate(:upload)
 
         put "/admin/badges/#{badge.id}.json", params: {
           name: "123456",
@@ -145,13 +146,17 @@ describe Admin::BadgesController do
           badge_type_id: badge.badge_type_id,
           allow_title: false,
           multiple_grant: false,
-          enabled: true
+          enabled: true,
+          image_upload_id: image.id,
+          icon: "fa-rocket",
         }
 
         expect(response.status).to eq(200)
         badge.reload
         expect(badge.name).to eq('123456')
         expect(badge.query).to eq(sql)
+        expect(badge.image_upload.id).to eq(image.id)
+        expect(badge.icon).to eq("fa-rocket")
       end
 
       context 'when there is a user with a title granted using the badge' do