DEV: Make site setting type uploaded_image_list use upload IDs (#10401)

It used to be a list of concatenated upload URLs which was prone to
break.

app/assets/javascripts/admin/addon/controllers/modals/admin-uploaded-image-list.js

@@ -7,7 +7,7 @@ export default Controller.extend(ModalFunctionality, {
   @observes("model.value")
   _setup() {
     const value = this.get("model.value");
-    this.set("images", value && value.length ? value.split("\n") : []);
+    this.set("images", value && value.length ? value.split("|") : []);
   },
 
   actions: {
@@ -20,7 +20,7 @@ export default Controller.extend(ModalFunctionality, {
     },
 
     close() {
-      this.save(this.images.join("\n"));
+      this.save(this.images.join("|"));
       this.send("closeModal");
     },
   },

app/assets/javascripts/discourse/app/controllers/avatar-selector.js

@@ -11,6 +11,31 @@ export default Controller.extend(ModalFunctionality, {
   gravatarBaseUrl: setting("gravatar_base_url"),
   gravatarLoginUrl: setting("gravatar_login_url"),
 
+  @discourseComputed(
+    "siteSettings.selectable_avatars_enabled",
+    "siteSettings.selectable_avatars"
+  )
+  selectableAvatars(enabled, list) {
+    if (enabled) {
+      return list ? list.split("|") : [];
+    }
+  },
+
+  @discourseComputed(
+    "user.avatar_template",
+    "user.system_avatar_template",
+    "user.gravatar_avatar_template"
+  )
+  selected(avatarTemplate, systemAvatarTemplate, gravatarAvatarTemplate) {
+    if (avatarTemplate === systemAvatarTemplate) {
+      return "system";
+    } else if (avatarTemplate === gravatarAvatarTemplate) {
+      return "gravatar";
+    } else {
+      return "custom";
+    }
+  },
+
   @discourseComputed(
     "selected",
     "user.system_avatar_upload_id",
@@ -55,7 +80,7 @@ export default Controller.extend(ModalFunctionality, {
 
   actions: {
     uploadComplete() {
-      this.set("selected", "uploaded");
+      this.set("selected", "custom");
     },
 
     refreshGravatar() {

app/assets/javascripts/discourse/app/initializers/avatar-select.js

@@ -1,36 +0,0 @@
-import showModal from "discourse/lib/show-modal";
-import { ajax } from "discourse/lib/ajax";
-
-export default {
-  name: "avatar-select",
-
-  initialize(container) {
-    this.selectableAvatarsEnabled = container.lookup(
-      "site-settings:main"
-    ).selectable_avatars_enabled;
-
-    container
-      .lookup("service:app-events")
-      .on("show-avatar-select", this, "_showAvatarSelect");
-  },
-
-  _showAvatarSelect(user) {
-    const avatarTemplate = user.avatar_template;
-    let selected = "uploaded";
-
-    if (avatarTemplate === user.system_avatar_template) {
-      selected = "system";
-    } else if (avatarTemplate === user.gravatar_avatar_template) {
-      selected = "gravatar";
-    }
-
-    const modal = showModal("avatar-selector");
-    modal.setProperties({ user, selected });
-
-    if (this.selectableAvatarsEnabled) {
-      ajax("/site/selectable-avatars.json").then((avatars) =>
-        modal.set("selectableAvatars", avatars)
-      );
-    }
-  },
-};

app/assets/javascripts/discourse/app/routes/preferences-account.js

@@ -1,3 +1,4 @@
+import showModal from "discourse/lib/show-modal";
 import UserBadge from "discourse/models/user-badge";
 import RestrictedUserRoute from "discourse/routes/restricted-user";
 
@@ -33,7 +34,7 @@ export default RestrictedUserRoute.extend({
 
   actions: {
     showAvatarSelector(user) {
-      this.appEvents.trigger("show-avatar-select", user);
+      showModal("avatar-selector").setProperties({ user });
     },
   },
 });

app/assets/javascripts/discourse/tests/unit/controllers/avatar-selector-test.js

@@ -1,6 +1,7 @@
+import EmberObject from "@ember/object";
+import { mapRoutes } from "discourse/mapping-router";
 import { moduleFor } from "ember-qunit";
 import { test } from "qunit";
-import { mapRoutes } from "discourse/mapping-router";
 
 moduleFor("controller:avatar-selector", "controller:avatar-selector", {
   beforeEach() {
@@ -12,29 +13,33 @@ moduleFor("controller:avatar-selector", "controller:avatar-selector", {
 test("avatarTemplate", function (assert) {
   const avatarSelectorController = this.subject();
 
-  avatarSelectorController.setProperties({
+  const user = EmberObject.create({
-    selected: "system",
+    avatar_template: "avatar",
-    user: {
+    system_avatar_template: "system",
+    gravatar_avatar_template: "gravatar",
+
     system_avatar_upload_id: 1,
     gravatar_avatar_upload_id: 2,
     custom_avatar_upload_id: 3,
-    },
   });
 
+  avatarSelectorController.setProperties({ user });
+
+  user.set("avatar_template", "system");
   assert.equal(
     avatarSelectorController.get("selectedUploadId"),
     1,
     "we are using system by default"
   );
 
-  avatarSelectorController.set("selected", "gravatar");
+  user.set("avatar_template", "gravatar");
   assert.equal(
     avatarSelectorController.get("selectedUploadId"),
     2,
     "we are using gravatar when set"
   );
 
-  avatarSelectorController.set("selected", "custom");
+  user.set("avatar_template", "avatar");
   assert.equal(
     avatarSelectorController.get("selectedUploadId"),
     3,

app/controllers/admin/site_settings_controller.rb

@@ -16,6 +16,10 @@ class Admin::SiteSettingsController < Admin::AdminController
     value.strip! if value.is_a?(String)
     raise_access_hidden_setting(id)
 
+    if SiteSetting.type_supervisor.get_type(id) == :uploaded_image_list
+      value = Upload.get_from_urls(value.split("|")).to_a
+    end
+
     if SiteSetting.type_supervisor.get_type(id) == :upload
       value = Upload.get_from_url(value) || ""
     end

app/controllers/site_controller.rb

@@ -25,16 +25,6 @@ class SiteController < ApplicationController
     render json: custom_emoji
   end
 
-  def selectable_avatars
-    avatars = if SiteSetting.selectable_avatars_enabled?
-      (SiteSetting.selectable_avatars.presence || "").split("\n")
-    else
-      []
-    end
-
-    render json: avatars, root: false
-  end
-
   def basic_info
     results = {
       logo_url: UrlHelper.absolute(SiteSetting.site_logo_url),

app/controllers/users_controller.rb

@@ -1156,7 +1156,7 @@ class UsersController < ApplicationController
       return render json: failed_json, status: 422
     end
 
-    unless SiteSetting.selectable_avatars[upload.sha1]
+    unless SiteSetting.selectable_avatars.include?(upload)
       return render json: failed_json, status: 422
     end
 

app/jobs/scheduled/clean_up_uploads.rb

@@ -25,23 +25,6 @@ module Jobs
       s3_hostname = URI.parse(base_url).hostname
       s3_cdn_hostname = URI.parse(SiteSetting.Upload.s3_cdn_url || "").hostname
 
-      # Any URLs in site settings are fair game
-      ignore_urls = [
-        *SiteSetting.selectable_avatars.split("\n"),
-      ].flatten.map do |url|
-        if url.present?
-          url = url.dup
-
-          if s3_cdn_hostname.present? && s3_hostname.present?
-            url.gsub!(s3_cdn_hostname, s3_hostname)
-          end
-
-          url[base_url] && url[url.index(base_url)..-1]
-        else
-          nil
-        end
-      end.compact.uniq
-
       result = Upload.by_users
         .where("uploads.retain_hours IS NULL OR uploads.created_at < current_timestamp - interval '1 hour' * uploads.retain_hours")
         .where("uploads.created_at < ?", grace_period.hour.ago)
@@ -71,7 +54,9 @@ module Jobs
         .where("g.flair_upload_id IS NULL")
         .where("ss.value IS NULL")
 
-      result = result.where("uploads.url NOT IN (?)", ignore_urls) if ignore_urls.present?
+      if SiteSetting.selectable_avatars.present?
+        result = result.where.not(id: SiteSetting.selectable_avatars.map(&:id))
+      end
 
       result.find_each do |upload|
         if upload.sha1.present?

app/models/concerns/has_url.rb

@@ -44,5 +44,29 @@ module HasUrl
 
       result || self.find_by("url LIKE ?", "%#{data[1]}")
     end
+
+    def get_from_urls(upload_urls)
+      urls = []
+      sha1s = []
+
+      upload_urls.each do |url|
+        next if url.blank?
+
+        uri = begin
+          URI(UrlHelper.unencode(url))
+        rescue URI::Error
+        end
+
+        next if uri&.path.blank?
+        urls << uri.path
+
+        if data = extract_url(uri.path).presence
+          urls << data[1]
+          sha1s << data[2] if self.name == "Upload"
+        end
+      end
+
+      self.where(url: urls).or(self.where(sha1: sha1s))
+    end
   end
 end

app/models/user.rb

@@ -1193,16 +1193,13 @@ class User < ActiveRecord::Base
   end
 
   def set_random_avatar
-    if SiteSetting.selectable_avatars_enabled? && SiteSetting.selectable_avatars.present?
+    if SiteSetting.selectable_avatars_enabled?
-      urls = SiteSetting.selectable_avatars.split("\n")
+      if upload = SiteSetting.selectable_avatars.sample
-      if urls.present?
-        if upload = Upload.get_from_url(urls.sample)
         update_column(:uploaded_avatar_id, upload.id)
         UserAvatar.create!(user_id: id, custom_upload_id: upload.id)
       end
     end
   end
-  end
 
   def anonymous?
     SiteSetting.allow_anonymous_posting &&

config/routes.rb

@@ -66,7 +66,6 @@ Discourse::Application.routes.draw do
 
     get "site/basic-info" => 'site#basic_info'
     get "site/statistics" => 'site#statistics'
-    get "site/selectable-avatars" => "site#selectable_avatars"
 
     get "srv/status" => "forums#status"
 

config/site_settings.yml

@@ -1339,6 +1339,7 @@ files:
     validator: "SelectableAvatarsEnabledValidator"
   selectable_avatars:
     default: ""
+    client: true
     type: uploaded_image_list
   allow_all_attachments_for_group_messages: false
   png_to_jpg_quality:

db/migrate/20200810194943_change_selectable_avatars_site_setting.rb

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+class ChangeSelectableAvatarsSiteSetting < ActiveRecord::Migration[6.0]
+  def up
+    selectable_avatars = execute("SELECT value FROM site_settings WHERE name = 'selectable_avatars'")
+    return if selectable_avatars.cmd_tuples == 0
+
+    # Keep old site setting value as a backup
+    execute <<~SQL
+      UPDATE site_settings
+      SET name = 'selectable_avatars_urls'
+      WHERE name = 'selectable_avatars'
+    SQL
+
+    # Extract SHA1s from URLs and then use them for upload ID lookups
+    urls = []
+    sha1s = []
+    selectable_avatars.first["value"].split("\n").each do |url|
+      match = url.match(/(\/original\/\dX[\/\.\w]*\/(\h+)[\.\w]*)/)
+      if match.present?
+        urls << match[1]
+        sha1s << match[2]
+      else
+        STDERR.puts "Could not extract a SHA1 from #{url}"
+      end
+    end
+
+    # Ensure at least one URL or SHA1 exists so the query below can be valid
+    return if urls.size == 0 && sha1s.size == 0
+
+    uploads_query = []
+    uploads_query << "url IN (#{urls.map { |url| ActiveRecord::Base.connection.quote(url) }.join(',')})" if urls.size > 0
+    uploads_query << "sha1 IN (#{sha1s.map { |sha1| ActiveRecord::Base.connection.quote(sha1) }.join(',')})" if sha1s.size > 0
+    uploads_query = "SELECT DISTINCT id FROM uploads WHERE #{uploads_query.join(" OR ")}"
+
+    upload_ids = execute(uploads_query).map { |row| row["id"] }
+    return if upload_ids.size == 0
+
+    execute <<~SQL
+      INSERT INTO site_settings(name, data_type, value, created_at, updated_at)
+      SELECT 'selectable_avatars', data_type, '#{upload_ids.join("|")}', created_at, NOW()
+      FROM site_settings
+      WHERE name = 'selectable_avatars_urls'
+    SQL
+  end
+
+  def down
+    execute("DELETE FROM site_settings WHERE name = 'selectable_avatars'")
+    execute("UPDATE site_settings SET name = 'selectable_avatars' WHERE name = 'selectable_avatars_urls'")
+  end
+end

lib/site_setting_extension.rb

@@ -209,6 +209,7 @@ module SiteSettingExtension
     MultiJson.dump(Hash[*@client_settings.map do |name|
       value = self.public_send(name)
       value = value.to_s if type_supervisor.get_type(name) == :upload
+      value = value.map(&:to_s).join("|") if type_supervisor.get_type(name) == :uploaded_image_list
       [name, value]
     end.flatten])
   end
@@ -233,10 +234,12 @@ module SiteSettingExtension
       .reject { |s, _| !include_hidden && hidden_settings.include?(s) }
       .map do |s, v|
 
-      value = public_send(s)
       type_hash = type_supervisor.type_hash(s)
       default = defaults.get(s, default_locale).to_s
 
+      value = public_send(s)
+      value = value.map(&:to_s).join("|") if type_hash[:type].to_s == "uploaded_image_list"
+
       if type_hash[:type].to_s == "upload" &&
          default.to_i < Upload::SEEDED_ID_THRESHOLD
 
@@ -482,7 +485,21 @@ module SiteSettingExtension
   def setup_methods(name)
     clean_name = name.to_s.sub("?", "").to_sym
 
-    if type_supervisor.get_type(name) == :upload
+    if type_supervisor.get_type(name) == :uploaded_image_list
+      define_singleton_method clean_name do
+        uploads_list = uploads[name]
+        return uploads_list if uploads_list
+
+        if (value = current[name]).nil?
+          refresh!
+          value = current[name]
+        end
+
+        value = value.split("|").map(&:to_i)
+        uploads_list = Upload.where(id: value).to_a
+        uploads[name] = uploads_list if uploads_list
+      end
+    elsif type_supervisor.get_type(name) == :upload
       define_singleton_method clean_name do
         upload = uploads[name]
         return upload if upload
@@ -552,7 +569,7 @@ module SiteSettingExtension
   end
 
   def clear_uploads_cache(name)
-    if type_supervisor.get_type(name) == :upload && uploads.has_key?(name)
+    if (type_supervisor.get_type(name) == :upload || type_supervisor.get_type(name) == :uploaded_image_list) && uploads.has_key?(name)
       uploads.delete(name)
     end
   end

lib/site_settings/type_supervisor.rb

@@ -182,6 +182,8 @@ class SiteSettings::TypeSupervisor
       type = get_data_type(name, val)
     elsif type == self.class.types[:enum]
       val = @defaults_provider[name].is_a?(Integer) ? val.to_i : val.to_s
+    elsif type == self.class.types[:uploaded_image_list] && val.present?
+      val = val.is_a?(String) ? val : val.map(&:id).join("|")
     elsif type == self.class.types[:upload] && val.present?
       val = val.is_a?(Integer) ? val : val.id
     end

lib/validators/selectable_avatars_enabled_validator.rb

@@ -6,7 +6,7 @@ class SelectableAvatarsEnabledValidator
   end
 
   def valid_value?(value)
-    value == "f" || SiteSetting.selectable_avatars.split("\n").size > 1
+    value == "f" || SiteSetting.selectable_avatars.size > 1
   end
 
   def error_message

spec/components/validators/selectable_avatars_enabled_validator_spec.rb

@@ -10,12 +10,12 @@ describe SelectableAvatarsEnabledValidator do
       SiteSetting.selectable_avatars = ""
       expect(validator.valid_value?("f")).to eq(true)
 
-      SiteSetting.selectable_avatars = [Fabricate(:image_upload).url, Fabricate(:image_upload).url].join("\n")
+      SiteSetting.selectable_avatars = [Fabricate(:image_upload), Fabricate(:image_upload)]
       expect(validator.valid_value?("f")).to eq(true)
     end
 
     it "returns true when there are at least two selectable avatars" do
-      SiteSetting.selectable_avatars = [Fabricate(:image_upload).url, Fabricate(:image_upload).url].join("\n")
+      SiteSetting.selectable_avatars = [Fabricate(:image_upload), Fabricate(:image_upload)]
       expect(validator.valid_value?("t")).to eq(true)
     end
 
@@ -23,7 +23,7 @@ describe SelectableAvatarsEnabledValidator do
       SiteSetting.selectable_avatars = ""
       expect(validator.valid_value?("t")).to eq(false)
 
-      SiteSetting.selectable_avatars = Fabricate(:image_upload).url
+      SiteSetting.selectable_avatars = [Fabricate(:image_upload)]
       expect(validator.valid_value?("t")).to eq(false)
     end
   end

spec/jobs/clean_up_uploads_spec.rb

@@ -126,11 +126,11 @@ describe Jobs::CleanUpUploads do
     end
   end
 
-  it "does not clean up uploads with URLs used in site settings" do
+  it "does not clean up selectable avatars" do
     avatar1_upload = fabricate_upload
     avatar2_upload = fabricate_upload
 
-    SiteSetting.selectable_avatars = [avatar1_upload.url, avatar2_upload.url].join("\n")
+    SiteSetting.selectable_avatars = [avatar1_upload, avatar2_upload]
 
     Jobs::CleanUpUploads.new.execute(nil)
 

spec/models/upload_spec.rb

@@ -229,6 +229,56 @@ describe Upload do
     end
   end
 
+  context ".get_from_urls" do
+    let(:upload) { Fabricate(:upload, sha1: "10f73034616a796dfd70177dc54b6def44c4ba6f") }
+    let(:upload2) { Fabricate(:upload, sha1: "2a7081e615f9075befd87a9a6d273935c0262cd5") }
+
+    it "works with multiple uploads" do
+      expect(Upload.get_from_urls([upload.url, upload2.url])).to contain_exactly(upload, upload2)
+    end
+
+    it "works for an extensionless URL" do
+      url = upload.url.sub('.png', '')
+      upload.update!(url: url)
+      expect(Upload.get_from_urls([url])).to contain_exactly(upload)
+    end
+
+    it "works with uploads with mismatched URLs" do
+      upload.update!(url: "/uploads/default/12345/971308e535305c51.png")
+      expect(Upload.get_from_urls([upload.url])).to contain_exactly(upload)
+      expect(Upload.get_from_urls(["/uploads/default/123131/971308e535305c51.png"])).to be_empty
+    end
+
+    it "works with an upload with a URL containing a deep tree" do
+      upload.update!(url: Discourse.store.get_path_for("original", 16001, upload.sha1, ".#{upload.extension}"))
+      expect(Upload.get_from_urls([upload.url])).to contain_exactly(upload)
+    end
+
+    it "works when using a CDN" do
+      begin
+        original_asset_host = Rails.configuration.action_controller.asset_host
+        Rails.configuration.action_controller.asset_host = 'http://my.cdn.com'
+
+        expect(Upload.get_from_urls([
+          URI.join("http://my.cdn.com", upload.url).to_s
+        ])).to contain_exactly(upload)
+      ensure
+        Rails.configuration.action_controller.asset_host = original_asset_host
+      end
+    end
+
+    it "works with full URLs" do
+      expect(Upload.get_from_urls([
+        URI.join("http://discourse.some.com:3000/", upload.url).to_s
+      ])).to contain_exactly(upload)
+    end
+
+    it "handles invalid URIs" do
+      urls = ["http://ip:port/index.html", "mailto:admin%40example.com", "mailto:example"]
+      expect { Upload.get_from_urls(urls) }.not_to raise_error
+    end
+  end
+
   describe '.generate_digest' do
     it "should return the right digest" do
       expect(Upload.generate_digest(image.path)).to eq('bc975735dfc6409c1c2aa5ebf2239949bcbdbd65')

spec/models/user_spec.rb

@@ -2050,7 +2050,7 @@ describe User do
     it "sets a random avatar when selectable avatars is enabled" do
       avatar1 = Fabricate(:upload)
       avatar2 = Fabricate(:upload)
-      SiteSetting.selectable_avatars = [avatar1.url, avatar2.url].join("\n")
+      SiteSetting.selectable_avatars = [avatar1, avatar2]
       SiteSetting.selectable_avatars_enabled = true
 
       user = Fabricate(:user)

spec/requests/site_controller_spec.rb

@@ -64,30 +64,4 @@ describe SiteController do
       expect(response).to redirect_to '/'
     end
   end
-
-  describe '.selectable_avatars' do
-    before do
-      SiteSetting.selectable_avatars = "https://www.discourse.org\nhttps://meta.discourse.org"
-    end
-
-    it 'returns empty array when selectable avatars is disabled' do
-      SiteSetting.selectable_avatars_enabled = false
-
-      get "/site/selectable-avatars.json"
-      json = response.parsed_body
-
-      expect(response.status).to eq(200)
-      expect(json).to eq([])
-    end
-
-    it 'returns an array when selectable avatars is enabled' do
-      SiteSetting.selectable_avatars_enabled = true
-
-      get "/site/selectable-avatars.json"
-      json = response.parsed_body
-
-      expect(response.status).to eq(200)
-      expect(json).to contain_exactly("https://www.discourse.org", "https://meta.discourse.org")
-    end
-  end
 end

spec/requests/users_controller_spec.rb

@@ -2368,7 +2368,7 @@ describe UsersController do
       context 'selectable avatars is enabled' do
 
         before do
-          SiteSetting.selectable_avatars = [avatar1.url, avatar2.url].join("\n")
+          SiteSetting.selectable_avatars = [avatar1, avatar2]
           SiteSetting.selectable_avatars_enabled = true
         end