From a23d0f996132746b29c17644a58b3bd00c7144ae Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Wed, 17 Mar 2021 08:55:23 +0300 Subject: [PATCH] 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. --- .../addon/controllers/admin-badges-show.js | 47 +++++++- .../admin/addon/routes/admin-badges-show.js | 9 ++ .../admin/addon/templates/badges-show.hbs | 57 +++++++--- .../javascripts/discourse/app/models/badge.js | 3 +- .../acceptance/admin-badges-show-test.js | 101 ++++++++++++++++++ .../tests/fixtures/badges-fixture.js | 25 +++++ .../stylesheets/common/admin/badges.scss | 8 ++ app/controllers/admin/badges_controller.rb | 2 +- app/controllers/badges_controller.rb | 2 +- app/controllers/user_badges_controller.rb | 2 +- .../onceoff/migrate_badge_image_to_uploads.rb | 91 ++++++++++++++++ app/jobs/scheduled/clean_up_uploads.rb | 2 + app/models/badge.rb | 14 ++- app/serializers/badge_serializer.rb | 2 +- app/services/staff_action_logger.rb | 2 +- config/locales/client.en.yml | 5 +- ...311070755_add_image_upload_id_to_badges.rb | 19 ++++ spec/jobs/clean_up_uploads_spec.rb | 10 ++ .../migrate_badge_image_to_uploads_spec.rb | 65 +++++++++++ spec/models/badge_spec.rb | 10 ++ spec/requests/admin/badges_controller_spec.rb | 7 +- 21 files changed, 456 insertions(+), 27 deletions(-) create mode 100644 app/assets/javascripts/discourse/tests/acceptance/admin-badges-show-test.js create mode 100644 app/jobs/onceoff/migrate_badge_image_to_uploads.rb create mode 100644 db/migrate/20210311070755_add_image_upload_id_to_badges.rb create mode 100644 spec/jobs/migrate_badge_image_to_uploads_spec.rb diff --git a/app/assets/javascripts/admin/addon/controllers/admin-badges-show.js b/app/assets/javascripts/admin/addon/controllers/admin-badges-show.js index 19f1d6ab6d3..5e1ed5bb21a 100644 --- a/app/assets/javascripts/admin/addon/controllers/admin-badges-show.js +++ b/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", diff --git a/app/assets/javascripts/admin/addon/routes/admin-badges-show.js b/app/assets/javascripts/admin/addon/routes/admin-badges-show.js index a370cd32d85..7053be216f4 100644 --- a/app/assets/javascripts/admin/addon/routes/admin-badges-show.js +++ b/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"); diff --git a/app/assets/javascripts/admin/addon/templates/badges-show.hbs b/app/assets/javascripts/admin/addon/templates/badges-show.hbs index 429f0cbc94d..e71ed63bb32 100644 --- a/app/assets/javascripts/admin/addon/templates/badges-show.hbs +++ b/app/assets/javascripts/admin/addon/templates/badges-show.hbs @@ -15,23 +15,48 @@
- - {{icon-picker - name="icon" - value=buffered.icon - options=(hash - maximum=1 - ) - onChange=(action (mut buffered.icon)) - }} + +
+ -

{{i18n "admin.badges.icon_help"}}

-
- -
- - {{input type="text" name="image" value=buffered.image}} -

{{i18n "admin.badges.image_help"}}

+ +
+ {{#if imageUploaderSelected}} + {{image-uploader + imageUrl=buffered.image_url + onUploadDone=(action "setImage") + onUploadDeleted=(action "removeImage") + type="badge_image" + class="no-repeat contain-image"}} +
+

{{i18n "admin.badges.image_help"}}

+
+ {{else if iconSelectorSelected}} + {{icon-picker + name="icon" + value=buffered.icon + options=(hash maximum=1) + onChange=(action (mut buffered.icon)) + }} + {{/if}}
diff --git a/app/assets/javascripts/discourse/app/models/badge.js b/app/assets/javascripts/discourse/app/models/badge.js index 99648d7558a..f9493129a5a 100644 --- a/app/assets/javascripts/discourse/app/models/badge.js +++ b/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() { diff --git a/app/assets/javascripts/discourse/tests/acceptance/admin-badges-show-test.js b/app/assets/javascripts/discourse/tests/acceptance/admin-badges-show-test.js new file mode 100644 index 00000000000..3da3a7a4806 --- /dev/null +++ b/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"); + }); +}); diff --git a/app/assets/javascripts/discourse/tests/fixtures/badges-fixture.js b/app/assets/javascripts/discourse/tests/fixtures/badges-fixture.js index eff532d4964..ee4ce84b425 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/badges-fixture.js +++ b/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", + }, + ] } }; diff --git a/app/assets/stylesheets/common/admin/badges.scss b/app/assets/stylesheets/common/admin/badges.scss index 816e21d3c9e..d573295cfdd 100644 --- a/app/assets/stylesheets/common/admin/badges.scss +++ b/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; diff --git a/app/controllers/admin/badges_controller.rb b/app/controllers/admin/badges_controller.rb index 067837f5fe2..d0206999893 100644 --- a/app/controllers/admin/badges_controller.rb +++ b/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, diff --git a/app/controllers/badges_controller.rb b/app/controllers/badges_controller.rb index 14ff402160d..3b90fce090a 100644 --- a/app/controllers/badges_controller.rb +++ b/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 diff --git a/app/controllers/user_badges_controller.rb b/app/controllers/user_badges_controller.rb index 2771e443f1b..05d0a693011 100644 --- a/app/controllers/user_badges_controller.rb +++ b/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) diff --git a/app/jobs/onceoff/migrate_badge_image_to_uploads.rb b/app/jobs/onceoff/migrate_badge_image_to_uploads.rb new file mode 100644 index 00000000000..5585a508c2f --- /dev/null +++ b/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 diff --git a/app/jobs/scheduled/clean_up_uploads.rb b/app/jobs/scheduled/clean_up_uploads.rb index 218f50b9628..7b3ee54f075 100644 --- a/app/jobs/scheduled/clean_up_uploads.rb +++ b/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? diff --git a/app/models/badge.rb b/app/models/badge.rb index fc89d8d253c..606ecb02d2d 100644 --- a/app/models/badge.rb +++ b/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 diff --git a/app/serializers/badge_serializer.rb b/app/serializers/badge_serializer.rb index d75ef6e1c9d..cd837ac1cd9 100644 --- a/app/serializers/badge_serializer.rb +++ b/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 diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index a5cd2b7f157..772de24bb68 100644 --- a/app/services/staff_action_logger.rb +++ b/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} diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 15a7322e24b..dc23536138c 100644 --- a/config/locales/client.en.yml +++ b/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 diff --git a/db/migrate/20210311070755_add_image_upload_id_to_badges.rb b/db/migrate/20210311070755_add_image_upload_id_to_badges.rb new file mode 100644 index 00000000000..ed0379bc671 --- /dev/null +++ b/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 diff --git a/spec/jobs/clean_up_uploads_spec.rb b/spec/jobs/clean_up_uploads_spec.rb index feb7d18245c..26275cac180 100644 --- a/spec/jobs/clean_up_uploads_spec.rb +++ b/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 diff --git a/spec/jobs/migrate_badge_image_to_uploads_spec.rb b/spec/jobs/migrate_badge_image_to_uploads_spec.rb new file mode 100644 index 00000000000..702821f3a49 --- /dev/null +++ b/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 diff --git a/spec/models/badge_spec.rb b/spec/models/badge_spec.rb index 73cd39ef0ee..f20a5b82a66 100644 --- a/spec/models/badge_spec.rb +++ b/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') diff --git a/spec/requests/admin/badges_controller_spec.rb b/spec/requests/admin/badges_controller_spec.rb index c93b7f5dd21..b6d26dc3b21 100644 --- a/spec/requests/admin/badges_controller_spec.rb +++ b/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