FIX: Properly associate user_profiles background urls via upload id.

`Upload#url` is more likely and can change from time to time. When it
does changes, we don't want to have to look through multiple tables to
ensure that the URLs are all up to date. Instead, we simply associate
uploads properly to `UserProfile` so that it does not have to replicate
the URLs in the table.

app/assets/javascripts/discourse/components/user-card-contents.js.es6

@@ -131,7 +131,7 @@ export default Ember.Component.extend(
       }
     },
 
-    @observes("user.card_background")
+    @observes("user.card_background_upload_url")
     addBackground() {
       if (!this.get("allowBackgrounds")) {
         return;
@@ -142,7 +142,7 @@ export default Ember.Component.extend(
         return;
       }
 
-      const url = this.get("user.card_background");
+      const url = this.get("user.card_background_upload_url");
       const bg = Ember.isEmpty(url)
         ? ""
         : `url(${Discourse.getURLWithCDN(url)})`;

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

@@ -10,8 +10,8 @@ export default Ember.Controller.extend(PreferencesTabController, {
     "location",
     "custom_fields",
     "user_fields",
-    "profile_background",
+    "profile_background_upload_url",
-    "card_background",
+    "card_background_upload_url",
     "date_of_birth"
   ],
 

app/assets/javascripts/discourse/controllers/user.js.es6

@@ -21,8 +21,8 @@ export default Ember.Controller.extend(CanCheckEmails, {
     return !profileHidden && viewingSelf;
   },
 
-  @computed("model.profileBackground")
+  @computed("model.profileBackgroundUrl")
-  hasProfileBackground(background) {
+  hasProfileBackgroundUrl(background) {
     return !Ember.isEmpty(background.toString());
   },
 

app/assets/javascripts/discourse/models/user.js.es6

@@ -77,8 +77,8 @@ const User = RestModel.extend({
     return username;
   },
 
-  @computed("profile_background")
+  @computed("profile_background_upload_url")
-  profileBackground(bgUrl) {
+  profileBackgroundUrl(bgUrl) {
     if (
       Ember.isEmpty(bgUrl) ||
       !Discourse.SiteSettings.allow_profile_backgrounds
@@ -250,8 +250,8 @@ const User = RestModel.extend({
       "user_fields",
       "muted_usernames",
       "ignored_usernames",
-      "profile_background",
+      "profile_background_upload_url",
-      "card_background",
+      "card_background_upload_url",
       "muted_tags",
       "tracked_tags",
       "watched_tags",

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

@@ -32,7 +32,8 @@
   <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 type="profile_background"}}
+      {{image-uploader imageUrl=model.profile_background_upload_url
+                       type="profile_background"}}
     </div>
     <div class='instructions'>
       {{i18n 'user.change_profile_background.instructions'}}
@@ -42,7 +43,8 @@
   <div class="control-group pref-profile-bg">
     <label class="control-label">{{i18n 'user.change_card_background.title'}}</label>
     <div class="controls">
-      {{image-uploader imageUrl=model.card_background type="card_background"}}
+      {{image-uploader imageUrl=model.card_background_upload_url
+                       type="card_background"}}
     </div>
     <div class='instructions'>
       {{i18n 'user.change_card_background.instructions'}}

app/assets/javascripts/discourse/templates/user.hbs

@@ -1,7 +1,7 @@
 <div class="container {{if viewingSelf 'viewing-self'}}">
   {{#d-section class="user-main"}}
 
-    <section class="{{if collapsedInfo 'collapsed-info'}} about {{if hasProfileBackground 'has-background' 'no-background'}}" >
+    <section class="{{if collapsedInfo 'collapsed-info'}} about {{if hasProfileBackgroundUrl 'has-background' 'no-background'}}" >
     {{#unless collapsedInfo}}
     {{#if showStaffCounters}}
       <div class='staff-counters'>
@@ -34,7 +34,7 @@
         {{/if}}
       </div>
     {{/if}}
-    <div class="user-profile-image" style={{model.profileBackground}}></div>
+    <div class="user-profile-image" style={{model.profileBackgroundUrl}}></div>
     {{/unless}}
 
       <div class='details'>

app/controllers/session_controller.rb

@@ -69,12 +69,16 @@ class SessionController < ApplicationController
           sso.avatar_url = UrlHelper.absolute Discourse.store.cdn_url(avatar_url)
         end
 
-        if current_user.user_profile.profile_background.present?
+        if current_user.user_profile.profile_background_upload.present?
-          sso.profile_background_url = UrlHelper.absolute upload_cdn_path(current_user.user_profile.profile_background)
+          sso.profile_background_url = UrlHelper.absolute(upload_cdn_path(
+            current_user.user_profile.profile_background_upload.url
+          ))
         end
 
-        if current_user.user_profile.card_background.present?
+        if current_user.user_profile.card_background_upload.present?
-          sso.card_background_url = UrlHelper.absolute upload_cdn_path(current_user.user_profile.card_background)
+          sso.card_background_url = UrlHelper.absolute(upload_cdn_path(
+            current_user.user_profile.card_background_upload.url
+          ))
         end
 
         if request.xhr?

app/controllers/users_controller.rb

@@ -1241,8 +1241,8 @@ class UsersController < ApplicationController
       :location,
       :website,
       :dismissed_banner_key,
-      :profile_background,
+      :profile_background_upload_url,
-      :card_background
+      :card_background_upload_url
     ]
 
     permitted << { custom_fields: User.editable_user_custom_fields } unless User.editable_user_custom_fields.blank?

app/jobs/onceoff/recover_user_profile_backgrounds.rb

@@ -1,25 +0,0 @@
-module Jobs
-  class RecoverUserProfileBackgrounds < Jobs::Onceoff
-    def execute_onceoff(_)
-      base_url = Discourse.store.absolute_base_url
-      return if !base_url.match?(/s3\.dualstack/)
-
-      old = base_url.sub('s3.dualstack.', 's3-')
-      old_like = %"#{old}%"
-
-      DB.exec(<<~SQL, from: old, to: base_url, old_like: old_like)
-        UPDATE user_profiles
-        SET profile_background = replace(profile_background, :from, :to)
-        WHERE profile_background ilike :old_like
-      SQL
-
-      DB.exec(<<~SQL, from: old, to: base_url, old_like: old_like)
-        UPDATE user_profiles
-        SET card_background = replace(card_background, :from, :to)
-        WHERE card_background ilike :old_like
-      SQL
-
-      UploadRecovery.new.recover_user_profile_backgrounds
-    end
-  end
-end

app/jobs/scheduled/clean_up_uploads.rb

@@ -56,7 +56,8 @@ module Jobs
         .joins("LEFT JOIN post_uploads pu ON pu.upload_id = uploads.id")
         .joins("LEFT JOIN users u ON u.uploaded_avatar_id = uploads.id")
         .joins("LEFT JOIN user_avatars ua ON ua.gravatar_upload_id = uploads.id OR ua.custom_upload_id = uploads.id")
-        .joins("LEFT JOIN user_profiles up ON up.profile_background = uploads.url OR up.card_background = uploads.url")
+        .joins("LEFT JOIN user_profiles up2 ON up2.profile_background = uploads.url OR up2.card_background = uploads.url")
+        .joins("LEFT JOIN user_profiles up ON up.profile_background_upload_id = uploads.id OR up.card_background_upload_id = uploads.id")
         .joins("LEFT JOIN categories c ON c.uploaded_logo_id = uploads.id OR c.uploaded_background_id = uploads.id")
         .joins("LEFT JOIN custom_emojis ce ON ce.upload_id = uploads.id")
         .joins("LEFT JOIN theme_fields tf ON tf.upload_id = uploads.id")
@@ -64,7 +65,8 @@ module Jobs
         .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")
-        .where("up.profile_background IS NULL AND up.card_background IS NULL")
+        .where("up.profile_background_upload_id IS NULL AND up.card_background_upload_id IS NULL")
+        .where("up2.profile_background IS NULL AND up2.card_background IS NULL")
         .where("c.uploaded_logo_id IS NULL AND c.uploaded_background_id IS NULL")
         .where("ce.upload_id IS NULL")
         .where("tf.upload_id IS NULL")

app/models/discourse_single_sign_on.rb

@@ -266,7 +266,7 @@ class DiscourseSingleSignOn < SingleSignOn
       end
     end
 
-    profile_background_missing = user.user_profile.profile_background.blank? || Upload.get_from_url(user.user_profile.profile_background).blank?
+    profile_background_missing = user.user_profile.profile_background_upload.blank? || Upload.get_from_url(user.user_profile.profile_background_upload.url).blank?
     if (profile_background_missing || SiteSetting.sso_overrides_profile_background) && profile_background_url.present?
       profile_background_changed = sso_record.external_profile_background_url != profile_background_url
       if profile_background_changed || profile_background_missing
@@ -278,7 +278,7 @@ class DiscourseSingleSignOn < SingleSignOn
       end
     end
 
-    card_background_missing = user.user_profile.card_background.blank? || Upload.get_from_url(user.user_profile.card_background).blank?
+    card_background_missing = user.user_profile.card_background_upload.blank? || Upload.get_from_url(user.user_profile.card_background_upload.url).blank?
     if (card_background_missing || SiteSetting.sso_overrides_profile_background) && card_background_url.present?
       card_background_changed = sso_record.external_card_background_url != card_background_url
       if card_background_changed || card_background_missing

app/models/user.rb

@@ -79,6 +79,8 @@ class User < ActiveRecord::Base
 
   has_one :user_stat, dependent: :destroy
   has_one :user_profile, dependent: :destroy, inverse_of: :user
+  has_one :profile_background_upload, through: :user_profile
+  has_one :card_background_upload, through: :user_profile
   has_one :single_sign_on_record, dependent: :destroy
   belongs_to :approved_by, class_name: 'User'
   belongs_to :primary_group, class_name: 'Group'

app/models/user_profile.rb

@@ -1,7 +1,13 @@
 require_dependency 'upload_creator'
 class UserProfile < ActiveRecord::Base
+  self.ignored_columns = %w{
+    card_background
+    profile_background
+  }
 
   belongs_to :user, inverse_of: :user_profile
+  belongs_to :card_background_upload, class_name: "Upload"
+  belongs_to :profile_background_upload, class_name: "Upload"
 
   validates :bio_raw, length: { maximum: 3000 }
   validates :website, url: true, allow_blank: true, if: Proc.new { |c| c.new_record? || c.website_changed? }
@@ -9,9 +15,6 @@ class UserProfile < ActiveRecord::Base
   before_save :cook
   after_save :trigger_badges
 
-  validates :profile_background, upload_url: true, if: :profile_background_changed?
-  validates :card_background, upload_url: true, if: :card_background_changed?
-
   validate :website_domain_validator, if: Proc.new { |c| c.new_record? || c.website_changed? }
 
   has_many :user_profile_views, dependent: :destroy
@@ -40,23 +43,19 @@ class UserProfile < ActiveRecord::Base
   end
 
   def upload_card_background(upload)
-    self.card_background = upload.url
+    self.update!(card_background_upload: upload)
-    self.save!
   end
 
   def clear_card_background
-    self.card_background = ""
+    self.update!(card_background_upload: nil)
-    self.save!
   end
 
   def upload_profile_background(upload)
-    self.profile_background = upload.url
+    self.update!(profile_background_upload: upload)
-    self.save!
   end
 
   def clear_profile_background
-    self.profile_background = ""
+    self.update!(profile_background_upload: nil)
-    self.save!
   end
 
   def self.rebake_old(limit)
@@ -150,21 +149,25 @@ end
 #
 # Table name: user_profiles
 #
-#  user_id              :integer          not null, primary key
+#  user_id                      :integer          not null, primary key
-#  location             :string
+#  location                     :string
-#  website              :string
+#  website                      :string
-#  bio_raw              :text
+#  bio_raw                      :text
-#  bio_cooked           :text
+#  bio_cooked                   :text
-#  profile_background   :string(255)
+#  profile_background           :string(255)
-#  dismissed_banner_key :integer
+#  dismissed_banner_key         :integer
-#  bio_cooked_version   :integer
+#  bio_cooked_version           :integer
-#  badge_granted_title  :boolean          default(FALSE)
+#  badge_granted_title          :boolean          default(FALSE)
-#  card_background      :string(255)
+#  card_background              :string(255)
-#  views                :integer          default(0), not null
+#  views                        :integer          default(0), not null
+#  profile_background_upload_id :integer
+#  card_background_upload_id    :integer
 #
 # Indexes
 #
-#  index_user_profiles_on_bio_cooked_version  (bio_cooked_version)
+#  index_user_profiles_on_bio_cooked_version            (bio_cooked_version)
-#  index_user_profiles_on_card_background     (card_background)
+#  index_user_profiles_on_card_background               (card_background)
-#  index_user_profiles_on_profile_background  (profile_background)
+#  index_user_profiles_on_card_background_upload_id     (card_background_upload_id)
+#  index_user_profiles_on_profile_background            (profile_background)
+#  index_user_profiles_on_profile_background_upload_id  (profile_background_upload_id)
 #

app/serializers/user_serializer.rb

@@ -41,8 +41,6 @@ class UserSerializer < BasicUserSerializer
              :created_at,
              :website,
              :website_name,
-             :profile_background,
-             :card_background,
              :location,
              :can_edit,
              :can_edit_username,
@@ -80,7 +78,9 @@ class UserSerializer < BasicUserSerializer
              :second_factor_enabled,
              :second_factor_backup_enabled,
              :second_factor_remaining_backup_codes,
-             :associated_accounts
+             :associated_accounts,
+             :profile_background_upload_url,
+             :card_background_upload_url
 
   has_one :invited_by, embed: :object, serializer: BasicUserSerializer
   has_many :groups, embed: :object, serializer: BasicGroupSerializer
@@ -127,8 +127,8 @@ class UserSerializer < BasicUserSerializer
                        :location,
                        :website,
                        :website_name,
-                       :profile_background,
+                       :profile_background_upload_url,
-                       :card_background
+                       :card_background_upload_url
 
   ###
   ### ATTRIBUTES
@@ -243,14 +243,6 @@ class UserSerializer < BasicUserSerializer
     website.present?
   end
 
-  def profile_background
-    object.user_profile.profile_background
-  end
-
-  def card_background
-    object.user_profile.card_background
-  end
-
   def location
     object.user_profile.location
   end
@@ -491,4 +483,12 @@ class UserSerializer < BasicUserSerializer
     scope.is_staff?
   end
 
+  def profile_background_upload_url
+    object.profile_background_upload&.url
+  end
+
+  def card_background_upload_url
+    object.card_background_upload&.url
+  end
+
 end

app/services/user_anonymizer.rb

@@ -47,8 +47,14 @@ class UserAnonymizer
       options.save!
 
       if profile = @user.user_profile
-        profile.update(location: nil, website: nil, bio_raw: nil, bio_cooked: nil,
+        profile.update!(
-                       profile_background: nil, card_background: nil)
+          location: nil,
+          website: nil,
+          bio_raw: nil,
+          bio_cooked: nil,
+          profile_background_upload: nil,
+          card_background_upload: nil
+        )
       end
 
       @user.user_avatar.try(:destroy)

app/services/user_merger.rb

@@ -221,10 +221,10 @@ class UserMerger
         bio_raw              = COALESCE(t.bio_raw, s.bio_raw),
         bio_cooked           = COALESCE(t.bio_cooked, s.bio_cooked),
         bio_cooked_version   = COALESCE(t.bio_cooked_version, s.bio_cooked_version),
-        profile_background   = COALESCE(t.profile_background, s.profile_background),
+        profile_background_upload_id = COALESCE(t.profile_background_upload_id, s.profile_background_upload_id),
         dismissed_banner_key = COALESCE(t.dismissed_banner_key, s.dismissed_banner_key),
         badge_granted_title  = t.badge_granted_title OR s.badge_granted_title,
-        card_background      = COALESCE(t.card_background, s.card_background),
+        card_background_upload_id = COALESCE(t.card_background_upload_id, s.card_background_upload_id),
         views                = t.views + s.views
       FROM user_profiles AS s
       WHERE t.user_id = :target_user_id AND s.user_id = :source_user_id

app/services/user_updater.rb

@@ -54,8 +54,14 @@ class UserUpdater
     unless SiteSetting.enable_sso && SiteSetting.sso_overrides_bio
       user_profile.bio_raw = attributes.fetch(:bio_raw) { user_profile.bio_raw }
     end
-    user_profile.profile_background = attributes.fetch(:profile_background) { user_profile.profile_background }
+
-    user_profile.card_background = attributes.fetch(:card_background) { user_profile.card_background }
+    if upload = Upload.get_from_url(attributes[:profile_background_upload_url])
+      user_profile.upload_profile_background(upload)
+    end
+
+    if upload = Upload.get_from_url(attributes[:card_background_upload_url])
+      user_profile.upload_card_background(upload)
+    end
 
     old_user_name = user.name.present? ? user.name : ""
     user.name = attributes.fetch(:name) { user.name }

db/migrate/20190426011148_add_upload_foreign_keys_to_user_profiles.rb

@@ -0,0 +1,34 @@
+require 'migration/column_dropper'
+
+class AddUploadForeignKeysToUserProfiles < ActiveRecord::Migration[5.2]
+  def up
+    %i{profile_background card_background}.each do |column|
+      Migration::ColumnDropper.mark_readonly(:user_profiles, column)
+    end
+
+    add_column :user_profiles, :profile_background_upload_id, :integer, null: true
+    add_column :user_profiles, :card_background_upload_id, :integer, null: true
+    add_foreign_key :user_profiles, :uploads, column: :profile_background_upload_id
+    add_foreign_key :user_profiles, :uploads, column: :card_background_upload_id
+
+    execute <<~SQL
+    UPDATE user_profiles up1
+    SET profile_background_upload_id = u.id
+    FROM user_profiles up2
+    INNER JOIN uploads u ON u.url = up2.profile_background
+    WHERE up1.user_id = up2.user_id
+    SQL
+
+    execute <<~SQL
+    UPDATE user_profiles up1
+    SET card_background_upload_id = u.id
+    FROM user_profiles up2
+    INNER JOIN uploads u ON u.url = up2.card_background
+    WHERE up1.user_id = up2.user_id
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

lib/migration/column_dropper.rb

@@ -20,17 +20,19 @@ module Migration
 
       columns.each do |column|
         column = column.to_s
-
+        self.drop_readonly(table, column)
-        DB.exec <<~SQL
-          DROP FUNCTION IF EXISTS #{BaseDropper.readonly_function_name(table, column)} CASCADE;
-          -- Backward compatibility for old functions created in the public
-          -- schema
-          DROP FUNCTION IF EXISTS #{BaseDropper.old_readonly_function_name(table, column)} CASCADE;
-        SQL
-
         # safe cause it is protected on method entry, can not be passed in params
         DB.exec("ALTER TABLE #{table} DROP COLUMN IF EXISTS #{column}")
       end
     end
+
+    def self.drop_readonly(table, column)
+      DB.exec <<~SQL
+        DROP FUNCTION IF EXISTS #{BaseDropper.readonly_function_name(table, column)} CASCADE;
+        -- Backward compatibility for old functions created in the public
+        -- schema
+        DROP FUNCTION IF EXISTS #{BaseDropper.old_readonly_function_name(table, column)} CASCADE;
+      SQL
+    end
   end
 end

lib/upload_recovery.rb

@@ -44,45 +44,8 @@ class UploadRecovery
     end
   end
 
-  def recover_user_profile_backgrounds
-    UserProfile
-      .where("profile_background IS NOT NULL OR card_background IS NOT NULL")
-      .find_each do |user_profile|
-
-      %i{card_background profile_background}.each do |column|
-        background = user_profile.public_send(column)
-
-        if background.present? && !Upload.exists?(url: background)
-          data = Upload.extract_url(background)
-          next unless data
-          sha1 = data[2]
-
-          if @dry_run
-            puts "#{background}"
-          else
-            recover_user_profile_background(sha1, user_profile.user_id) do |upload|
-              user_profile.update!("#{column}" => upload.url) if upload.persisted?
-            end
-          end
-        end
-      end
-    end
-  end
-
   private
 
-  def recover_user_profile_background(sha1, user_id, &block)
-    return unless valid_sha1?(sha1)
-
-    attributes = { sha1: sha1, user_id: user_id }
-
-    if Discourse.store.external?
-      recover_from_s3(attributes, &block)
-    else
-      recover_from_local(attributes, &block)
-    end
-  end
-
   def recover_post_upload(post, sha1)
     return unless valid_sha1?(sha1)
 

lib/validators/upload_url_validator.rb

@@ -1,15 +0,0 @@
-class UploadUrlValidator < ActiveModel::EachValidator
-  def validate_each(record, attribute, value)
-    if value.present?
-      uri =
-        begin
-          URI.parse(value)
-        rescue URI::Error
-        end
-
-      unless uri && Upload.exists?(url: value)
-        record.errors.add(attribute, options[:message] || I18n.t('errors.messages.invalid'))
-      end
-    end
-  end
-end

script/import_scripts/discuz_x.rb

@@ -199,7 +199,7 @@ class ImportScripts::DiscuzX < ImportScripts::Base
                 end
               end
             end
-            if !user['spacecss'].blank? && newmember.user_profile.profile_background.blank?
+            if !user['spacecss'].blank? && newmember.user_profile.profile_background_upload.blank?
               # profile background
               if matched = user['spacecss'].match(/body\s*{[^}]*url\('?(.+?)'?\)/i)
                 body_background = matched[1].split(ORIGINAL_SITE_PREFIX, 2).last

script/import_scripts/lithium.rb

@@ -291,7 +291,7 @@ class ImportScripts::Lithium < ImportScripts::Base
 
     return if !upload.persisted?
 
-    imported_user.user_profile.update(profile_background: upload.url)
+    imported_user.user_profile.upload_profile_background(upload)
   ensure
     file.close rescue nil
     file.unlink rescue nil

script/import_scripts/nodebb/nodebb.rb

@@ -281,7 +281,7 @@ class ImportScripts::NodeBB < ImportScripts::Base
 
     return if !upload.persisted?
 
-    imported_user.user_profile.update(profile_background: upload.url)
+    imported_user.user_profile.upload_profile_background(upload)
   ensure
     string_io.close rescue nil
     file.close rescue nil

script/import_scripts/vbulletin.rb

@@ -254,7 +254,7 @@ EOM
 
     return if !upload.persisted?
 
-    imported_user.user_profile.update(profile_background: upload.url)
+    imported_user.user_profile.upload_profile_background(upload)
   ensure
     file.close rescue nil
     file.unlink rescue nil

script/import_scripts/vbulletin5.rb

@@ -164,7 +164,7 @@ class ImportScripts::VBulletin < ImportScripts::Base
 
     return if !upload.persisted?
 
-    imported_user.user_profile.update(profile_background: upload.url)
+    imported_user.user_profile.upload_profile_background(upload)
   ensure
     file.close rescue nil
     file.unlink rescue nil

spec/db/migrate/add_upload_foreign_keys_to_user_profiles_spec.rb

@@ -0,0 +1,79 @@
+require 'rails_helper'
+require 'migration/column_dropper'
+require_relative '../../../db/migrate/20190426011148_add_upload_foreign_keys_to_user_profiles'
+
+RSpec.describe AddUploadForeignKeysToUserProfiles do
+  before do
+    %i{card_background profile_background}.each do |column|
+      # In the future when the column is dropped
+      # DB.exec("ALTER TABLE user_profiles ADD COLUMN #{column} VARCHAR;")
+      Migration::ColumnDropper.drop_readonly(:user_profiles, column)
+    end
+
+    %i{card_background_upload_id profile_background_upload_id}.each do |column|
+      DB.exec("ALTER TABLE user_profiles DROP COLUMN IF EXISTS #{column}")
+    end
+  end
+
+  def select_column_from_user_profiles(column, user_id)
+    DB.query_single(<<~SQL).first
+    SELECT #{column}
+    FROM user_profiles
+    WHERE user_id = #{user_id}
+    SQL
+  end
+
+  it "should migrate the data properly" do
+    upload = Fabricate(:upload)
+    upload2 = Fabricate(:upload)
+    user = Fabricate(:user)
+    user2 = Fabricate(:user)
+    user3 = Fabricate(:user)
+
+    DB.exec(<<~SQL)
+    UPDATE user_profiles
+    SET card_background = '#{upload.url}', profile_background = '#{upload.url}'
+    WHERE user_profiles.user_id = #{user.id}
+    SQL
+
+    DB.exec(<<~SQL)
+    UPDATE user_profiles
+    SET card_background = '#{upload.url}', profile_background = '#{upload2.url}'
+    WHERE user_profiles.user_id = #{user2.id}
+    SQL
+
+    DB.exec(<<~SQL)
+    UPDATE user_profiles
+    SET card_background = '#{upload.url}'
+    WHERE user_profiles.user_id = #{user3.id}
+    SQL
+
+    silence_stdout { described_class.new.up }
+
+    %i{card_background profile_background}.each do |column|
+      expect(select_column_from_user_profiles(column, user.id))
+        .to eq(upload.url)
+    end
+
+    %i{card_background_upload_id profile_background_upload_id}.each do |column|
+      expect(select_column_from_user_profiles(column, user.id))
+        .to eq(upload.id)
+    end
+
+    expect(select_column_from_user_profiles(
+      :card_background_upload_id, user2.id
+    )).to eq(upload.id)
+
+    expect(select_column_from_user_profiles(
+      :profile_background_upload_id, user2.id
+    )).to eq(upload2.id)
+
+    expect(select_column_from_user_profiles(
+      :card_background_upload_id, user3.id
+    )).to eq(upload.id)
+
+    expect(select_column_from_user_profiles(
+      :profile_background_upload_id, user3.id
+    )).to eq(nil)
+  end
+end

spec/db/migrate/add_uploads_to_categories_spec.rb

@@ -13,32 +13,44 @@ RSpec.describe AddUploadsToCategories do
     end
   end
 
+  def select_column_from_categories(column, category_id)
+    DB.query_single(<<~SQL).first
+    SELECT #{column}
+    FROM categories
+    WHERE id = #{category_id}
+    SQL
+  end
+
   it "should migrate the data properly" do
     upload1 = Fabricate(:upload)
     upload2 = Fabricate(:upload)
+    category1 = Fabricate(:category)
+    category2 = Fabricate(:category)
 
-    category1 = Fabricate(:category,
+    DB.exec(<<~SQL)
-      logo_url: upload1.url,
+    UPDATE categories
-      background_url: upload2.url
+    SET logo_url = '#{upload1.url}', background_url = '#{upload2.url}'
-    )
+    WHERE categories.id = #{category1.id}
+    SQL
 
-    category2 = Fabricate(:category,
+    DB.exec(<<~SQL)
-      logo_url: upload2.url,
+    UPDATE categories
-      background_url: upload1.url
+    SET logo_url = '#{upload2.url}', background_url = '#{upload1.url}'
-    )
+    WHERE categories.id = #{category2.id}
+    SQL
 
     silence_stdout { described_class.new.up }
 
-    Discourse.reset_active_record_cache
+    expect(select_column_from_categories(:uploaded_logo_id, category1.id))
+      .to eq(upload1.id)
 
-    category1.reload
+    expect(select_column_from_categories(:uploaded_background_id, category1.id))
+      .to eq(upload2.id)
 
-    expect(category1.uploaded_logo_id).to eq(upload1.id)
+    expect(select_column_from_categories(:uploaded_logo_id, category2.id))
-    expect(category1.uploaded_background_id).to eq(upload2.id)
+      .to eq(upload2.id)
 
-    category2.reload
+    expect(select_column_from_categories(:uploaded_background_id, category2.id))
-
+      .to eq(upload1.id)
-    expect(category2.uploaded_logo_id).to eq(upload2.id)
-    expect(category2.uploaded_background_id).to eq(upload1.id)
   end
 end

spec/jobs/clean_up_uploads_spec.rb

@@ -156,7 +156,7 @@ describe Jobs::CleanUpUploads do
 
   it "does not delete profile background uploads" do
     profile_background_upload = fabricate_upload
-    UserProfile.last.update!(profile_background: profile_background_upload.url)
+    UserProfile.last.upload_profile_background(profile_background_upload)
 
     Jobs::CleanUpUploads.new.execute(nil)
 
@@ -166,7 +166,7 @@ describe Jobs::CleanUpUploads do
 
   it "does not delete card background uploads" do
     card_background_upload = fabricate_upload
-    UserProfile.last.update!(card_background: card_background_upload.url)
+    UserProfile.last.upload_card_background(card_background_upload)
 
     Jobs::CleanUpUploads.new.execute(nil)
 

spec/jobs/recover_user_profile_backgrounds_spec.rb

@@ -1,41 +0,0 @@
-# frozen_string_literal: true
-
-require 'rails_helper'
-
-require_dependency 'jobs/onceoff/recover_user_profile_backgrounds'
-
-RSpec.describe Jobs::RecoverUserProfileBackgrounds do
-  let(:user_profile) { Fabricate(:user).user_profile }
-
-  before do
-    SiteSetting.s3_upload_bucket = "s3-upload-bucket"
-    SiteSetting.s3_access_key_id = "s3-access-key-id"
-    SiteSetting.s3_secret_access_key = "s3-secret-access-key"
-    SiteSetting.enable_s3_uploads = true
-  end
-
-  it "corrects the URL and recovers the uploads" do
-    current_upload = Upload.create!(
-      url: '//s3-upload-bucket.s3-us-east-1.amazonaws.com/somewhere/a.png',
-      original_filename: 'a.png',
-      filesize: 100,
-      user_id: -1,
-    )
-
-    user_profile.update!(
-      profile_background: current_upload.url,
-      card_background: current_upload.url
-    )
-
-    Jobs::RecoverUserProfileBackgrounds.new.execute_onceoff({})
-
-    user_profile.reload
-
-    %i{card_background profile_background}.each do |column|
-      expect(user_profile.public_send(column)).to eq(
-        '//s3-upload-bucket.s3.dualstack.us-east-1.amazonaws.com/somewhere/a.png'
-      )
-    end
-
-  end
-end

spec/lib/upload_recovery_spec.rb

@@ -197,58 +197,4 @@ RSpec.describe UploadRecovery do
       end
     end
   end
-
-  describe "#recover_user_profile_backgrounds" do
-    before do
-      user.user_profile.update!(
-        profile_background: upload.url,
-        card_background: upload.url
-      )
-    end
-
-    it "should recover the background uploads" do
-      user_profile = user.user_profile
-      upload.destroy!
-
-      user_profile.update_columns(
-        profile_background: user_profile.profile_background.sub("default", "X"),
-        card_background: user_profile.card_background.sub("default", "X")
-      )
-
-      expect do
-        upload_recovery.recover_user_profile_backgrounds
-      end.to change { Upload.count }.by(1)
-
-      user_profile.reload
-
-      expect(user_profile.profile_background).to eq(upload.url)
-      expect(user_profile.card_background).to eq(upload.url)
-    end
-
-    describe 'for a bad upload' do
-      it 'should not update the urls' do
-        user_profile = user.user_profile
-        upload.destroy!
-
-        profile_background = user_profile.profile_background.sub("default", "X")
-        card_background = user_profile.card_background.sub("default", "X")
-
-        user_profile.update_columns(
-          profile_background: profile_background,
-          card_background: card_background
-        )
-
-        SiteSetting.authorized_extensions = ''
-
-        expect do
-          upload_recovery.recover_user_profile_backgrounds
-        end.to_not change { Upload.count }
-
-        user_profile.reload
-
-        expect(user_profile.profile_background).to eq(profile_background)
-        expect(user_profile.card_background).to eq(card_background)
-      end
-    end
-  end
 end

spec/models/discourse_single_sign_on_spec.rb

@@ -668,11 +668,11 @@ describe DiscourseSingleSignOn do
       user = sso.lookup_or_create_user(ip_address)
       user.reload
       user.user_profile.reload
-      profile_background = user.user_profile.profile_background
+      profile_background_url = user.profile_background_upload.url
 
       # initial creation ...
-      expect(profile_background).to_not eq(nil)
+      expect(profile_background_url).to_not eq(nil)
-      expect(profile_background).to_not eq('')
+      expect(profile_background_url).to_not eq('')
 
       FileHelper.stubs(:download) { raise "should not be called" }
       sso.profile_background_url = "https://some.new/avatar.png"
@@ -681,7 +681,7 @@ describe DiscourseSingleSignOn do
       user.user_profile.reload
 
       # profile_background updated but no override specified ...
-      expect(user.user_profile.profile_background).to eq(profile_background)
+      expect(user.profile_background_upload.url).to eq(profile_background_url)
     end
   end
 
@@ -704,33 +704,24 @@ describe DiscourseSingleSignOn do
     end
 
     it "deal with no profile_background_url passed for an existing user with a profile_background" do
-      Sidekiq::Testing.inline! do
+      # Deliberately not setting profile_background_url so it should not update
-        # Deliberately not setting profile_background_url so it should not update
+      sso_record.user.user_profile.clear_profile_background
-        sso_record.user.user_profile.update_columns(profile_background: '')
+      user = sso.lookup_or_create_user(ip_address)
-        user = sso.lookup_or_create_user(ip_address)
+      user.reload
-        user.reload
-        user.user_profile.reload
 
-        expect(user).to_not be_nil
+      expect(user.profile_background_upload).to eq(nil)
-        expect(user.user_profile.profile_background).to eq('')
-      end
     end
 
     it "deal with a profile_background_url passed for an existing user with a profile_background" do
-      Sidekiq::Testing.inline! do
+      url = "http://example.com/a_different_image.png"
-        FileHelper.stubs(:download).returns(logo)
+      stub_request(:get, url).to_return(body: logo)
 
-        sso_record.user.user_profile.update_columns(profile_background: '')
+      sso_record.user.user_profile.clear_profile_background
+      sso.profile_background_url = "http://example.com/a_different_image.png"
+      user = sso.lookup_or_create_user(ip_address)
+      user.reload
 
-        sso.profile_background_url = "http://example.com/a_different_image.png"
+      expect(user.profile_background_upload).to_not eq(nil)
-
-        user = sso.lookup_or_create_user(ip_address)
-        user.reload
-        user.user_profile.reload
-
-        expect(user).to_not be_nil
-        expect(user.user_profile.profile_background).to_not eq('')
-      end
     end
   end
 
@@ -744,16 +735,14 @@ describe DiscourseSingleSignOn do
       sso.username = "sam"
       sso.card_background_url = "http://awesome.com/image.png"
       sso.suppress_welcome_message = true
-
       FileHelper.stubs(:download).returns(file_from_fixtures("logo.png"))
       user = sso.lookup_or_create_user(ip_address)
       user.reload
       user.user_profile.reload
-      card_background = user.user_profile.card_background
+      card_background_url = user.user_profile.card_background_upload.url
 
       # initial creation ...
-      expect(card_background).to_not eq(nil)
+      expect(card_background_url).to be_present
-      expect(card_background).to_not eq('')
 
       FileHelper.stubs(:download) { raise "should not be called" }
       sso.card_background_url = "https://some.new/avatar.png"
@@ -762,7 +751,9 @@ describe DiscourseSingleSignOn do
       user.user_profile.reload
 
       # card_background updated but no override specified ...
-      expect(user.user_profile.card_background).to eq(card_background)
+      expect(user.user_profile.card_background_upload.url).to eq(
+        card_background_url
+      )
     end
   end
 
@@ -785,33 +776,25 @@ describe DiscourseSingleSignOn do
     end
 
     it "deal with no card_background_url passed for an existing user with a card_background" do
-      Sidekiq::Testing.inline! do
+      # Deliberately not setting card_background_url so it should not update
-        # Deliberately not setting card_background_url so it should not update
+      sso_record.user.user_profile.clear_card_background
-        sso_record.user.user_profile.update_columns(card_background: '')
+      user = sso.lookup_or_create_user(ip_address)
-        user = sso.lookup_or_create_user(ip_address)
+      user.reload
-        user.reload
-        user.user_profile.reload
 
-        expect(user).to_not be_nil
+      expect(user.user_profile.card_background_upload).to eq(nil)
-        expect(user.user_profile.card_background).to eq('')
-      end
     end
 
     it "deal with a card_background_url passed for an existing user with a card_background_url" do
-      Sidekiq::Testing.inline! do
+      url = "http://example.com/a_different_image.png"
-        FileHelper.stubs(:download).returns(logo)
+      stub_request(:get, url).to_return(body: logo)
 
-        sso_record.user.user_profile.update_columns(card_background: '')
+      sso_record.user.user_profile.clear_card_background
+      sso.card_background_url = url
 
-        sso.card_background_url = "http://example.com/a_different_image.png"
+      user = sso.lookup_or_create_user(ip_address)
+      user.reload
 
-        user = sso.lookup_or_create_user(ip_address)
+      expect(user.user_profile.card_background_upload.url).to_not eq('')
-        user.reload
-        user.user_profile.reload
-
-        expect(user).to_not be_nil
-        expect(user.user_profile.card_background).to_not eq('')
-      end
     end
   end
 

spec/models/user_profile_spec.rb

@@ -8,23 +8,6 @@ describe UserProfile do
     expect(user.user_profile).to be_present
   end
 
-  context "url validation" do
-    let(:user) { Fabricate(:user) }
-    let(:upload) { Fabricate(:upload) }
-
-    it "ensures profile_background is valid" do
-      expect(Fabricate.build(:user_profile, user: user, profile_background: "---%")).not_to be_valid
-      expect(Fabricate.build(:user_profile, user: user, profile_background: "http://example.com/made-up.jpg")).not_to be_valid
-      expect(Fabricate.build(:user_profile, user: user, profile_background: upload.url)).to be_valid
-    end
-
-    it "ensures background_url is valid" do
-      expect(Fabricate.build(:user_profile, user: user, card_background: ";test")).not_to be_valid
-      expect(Fabricate.build(:user_profile, user: user, card_background: "http://example.com/no.jpg")).not_to be_valid
-      expect(Fabricate.build(:user_profile, user: user, card_background: upload.url)).to be_valid
-    end
-  end
-
   describe 'rebaking' do
     it 'correctly rebakes bio' do
       user_profile = Fabricate(:evil_trout).user_profile
@@ -228,7 +211,7 @@ describe UserProfile do
 
         user.reload
 
-        expect(user.user_profile.profile_background).to eq(nil)
+        expect(user.profile_background_upload).to eq(nil)
       end
     end
 
@@ -240,7 +223,7 @@ describe UserProfile do
 
         user.reload
 
-        expect(user.user_profile.card_background).to eq(nil)
+        expect(user.card_background_upload).to eq(nil)
       end
     end
 

spec/requests/session_controller_spec.rb

@@ -804,9 +804,13 @@ RSpec.describe SessionController do
         )
 
         @user.update_columns(uploaded_avatar_id: upload.id)
-        @user.user_profile.update_columns(
+
-          profile_background: "//test.s3.dualstack.us-east-1.amazonaws.com/something",
+        upload1 = Fabricate(:upload_s3)
-          card_background: "//test.s3.dualstack.us-east-1.amazonaws.com/something"
+        upload2 = Fabricate(:upload_s3)
+
+        @user.user_profile.update!(
+          profile_background_upload: upload1,
+          card_background_upload: upload2
         )
 
         @user.reload

spec/requests/users_controller_spec.rb

@@ -1459,6 +1459,7 @@ describe UsersController do
       before do
         sign_in(user)
       end
+
       let(:user) { Fabricate(:user, username: 'test.test', name: "Test User") }
 
       it "should be able to update a user" do
@@ -1494,6 +1495,7 @@ describe UsersController do
 
     context 'with authenticated user' do
       context 'with permission to update' do
+        let(:upload) { Fabricate(:upload) }
         let!(:user) { sign_in(Fabricate(:user)) }
 
         it 'allows the update' do
@@ -1504,7 +1506,9 @@ describe UsersController do
           put "/u/#{user.username}.json", params: {
             name: 'Jim Tom',
             muted_usernames: "#{user2.username},#{user3.username}",
-            watched_tags: "#{tags[0].name},#{tags[1].name}"
+            watched_tags: "#{tags[0].name},#{tags[1].name}",
+            card_background_upload_url: upload.url,
+            profile_background_upload_url: upload.url
           }
 
           expect(response.status).to eq(200)
@@ -1512,8 +1516,8 @@ describe UsersController do
           user.reload
 
           expect(user.name).to eq 'Jim Tom'
-
           expect(user.muted_users.pluck(:username).sort).to eq [user2.username, user3.username].sort
+
           expect(TagUser.where(
             user: user,
             notification_level: TagUser.notification_levels[:watching]
@@ -1532,6 +1536,8 @@ describe UsersController do
           expect(user.muted_users.pluck(:username).sort).to be_empty
           expect(user.user_option.theme_ids).to eq([theme.id])
           expect(user.user_option.email_level).to eq(UserOption.email_level_types[:always])
+          expect(user.profile_background_upload).to eq(upload)
+          expect(user.card_background_upload).to eq(upload)
         end
 
         context 'a locale is chosen that differs from I18n.locale' do
@@ -1986,6 +1992,7 @@ describe UsersController do
     context 'while logged in' do
       let(:another_user) { Fabricate(:user) }
       let(:user) { Fabricate(:user) }
+
       before do
         sign_in(user)
       end
@@ -2008,7 +2015,7 @@ describe UsersController do
       it 'can clear the profile background' do
         delete "/u/#{user.username}/preferences/user_image.json", params: { type: 'profile_background' }
 
-        expect(user.reload.user_profile.profile_background).to eq("")
+        expect(user.reload.profile_background_upload).to eq(nil)
         expect(response.status).to eq(200)
       end
     end

spec/serializers/user_serializer_spec.rb

@@ -41,13 +41,11 @@ describe UserSerializer do
   end
 
   context "with a user" do
-    let(:user) { Fabricate.build(:user, user_profile: Fabricate.build(:user_profile)) }
+    let(:user) { Fabricate(:user) }
     let(:serializer) { UserSerializer.new(user, scope: Guardian.new, root: false) }
     let(:json) { serializer.as_json }
-
+    let(:upload) { Fabricate(:upload) }
-    it "produces json" do
+    let(:upload2) { Fabricate(:upload) }
-      expect(json).to be_present
-    end
 
     context "with `enable_names` true" do
       before do
@@ -69,23 +67,15 @@ describe UserSerializer do
       end
     end
 
-    context "with filled out card background" do
+    context "with filled out backgrounds" do
       before do
-        user.user_profile.card_background = 'http://card.com'
+        user.user_profile.upload_card_background(upload)
+        user.user_profile.upload_profile_background(upload2)
       end
 
       it "has a profile background" do
-        expect(json[:card_background]).to eq 'http://card.com'
+        expect(json[:card_background_upload_url]).to eq(upload.url)
-      end
+        expect(json[:profile_background_upload_url]).to eq(upload2.url)
-    end
-
-    context "with filled out profile background" do
-      before do
-        user.user_profile.profile_background = 'http://background.com'
-      end
-
-      it "has a profile background" do
-        expect(json[:profile_background]).to eq 'http://background.com'
       end
     end
 

spec/services/user_anonymizer_spec.rb

@@ -74,17 +74,23 @@ describe UserAnonymizer do
       end
 
       it "resets profile to default values" do
-        user.update(name: "Bibi", date_of_birth: 19.years.ago, title: "Super Star")
+        user.update!(
+          name: "Bibi",
+          date_of_birth: 19.years.ago,
+          title: "Super Star"
+        )
 
         profile = user.reload.user_profile
-        profile.update(
+        upload = Fabricate(:upload)
+
+        profile.update!(
           location: "Moose Jaw",
-          website: "www.bim.com",
+          website: "http://www.bim.com",
           bio_raw: "I'm Bibi from Moosejaw. I sing and dance.",
           bio_cooked: "I'm Bibi from Moosejaw. I sing and dance.",
-          profile_background: "http://example.com/bg.jpg",
+          profile_background_upload: upload,
           bio_cooked_version: 2,
-          card_background: "http://example.com/cb.jpg"
+          card_background_upload: upload
         )
 
         prev_username = user.username
@@ -104,9 +110,9 @@ describe UserAnonymizer do
         expect(profile.location).to eq(nil)
         expect(profile.website).to eq(nil)
         expect(profile.bio_cooked).to eq(nil)
-        expect(profile.profile_background).to eq(nil)
+        expect(profile.profile_background_upload).to eq(nil)
-        expect(profile.bio_cooked_version).to eq(nil)
+        expect(profile.bio_cooked_version).to eq(UserProfile::BAKED_VERSION)
-        expect(profile.card_background).to eq(nil)
+        expect(profile.card_background_upload).to eq(nil)
       end
     end
 

spec/services/user_merger_spec.rb

@@ -951,10 +951,25 @@ describe UserMerger do
   end
 
   it "updates users" do
-    walter.update_attribute(:approved_by, source_user)
+    walter.update!(approved_by: source_user)
+    upload = Fabricate(:upload)
+
+    source_user.update!(admin: true)
+
+    source_user.user_profile.update!(
+      card_background_upload: upload,
+      profile_background_upload: upload,
+    )
+
     merge_users!
 
     expect(walter.reload.approved_by).to eq(target_user)
+
+    target_user.reload
+
+    expect(target_user.admin).to eq(true)
+    expect(target_user.card_background_upload).to eq(upload)
+    expect(target_user.profile_background_upload).to eq(upload)
   end
 
   it "deletes the source user even when it's an admin" do

spec/services/user_updater_spec.rb

@@ -145,20 +145,26 @@ describe UserUpdater do
       date_of_birth = Time.zone.now
 
       theme = Fabricate(:theme, user_selectable: true)
+      upload1 = Fabricate(:upload)
+      upload2 = Fabricate(:upload)
 
       seq = user.user_option.theme_key_seq
 
-      val = updater.update(bio_raw: 'my new bio',
+      val = updater.update(
-                           email_level: UserOption.email_level_types[:always],
+        bio_raw: 'my new bio',
-                           mailing_list_mode: true,
+        email_level: UserOption.email_level_types[:always],
-                           digest_after_minutes: "45",
+        mailing_list_mode: true,
-                           new_topic_duration_minutes: 100,
+        digest_after_minutes: "45",
-                           auto_track_topics_after_msecs: 101,
+        new_topic_duration_minutes: 100,
-                           notification_level_when_replying: 3,
+        auto_track_topics_after_msecs: 101,
-                           email_in_reply_to: false,
+        notification_level_when_replying: 3,
-                           date_of_birth: date_of_birth,
+        email_in_reply_to: false,
-                           theme_ids: [theme.id],
+        date_of_birth: date_of_birth,
-                           allow_private_messages: false)
+        theme_ids: [theme.id],
+        allow_private_messages: false,
+        card_background_upload_url: upload1.url,
+        profile_background_upload_url: upload2.url
+      )
 
       expect(val).to be_truthy
 
@@ -176,6 +182,8 @@ describe UserUpdater do
       expect(user.user_option.theme_key_seq).to eq(seq + 1)
       expect(user.user_option.allow_private_messages).to eq(false)
       expect(user.date_of_birth).to eq(date_of_birth.to_date)
+      expect(user.card_background_upload).to eq(upload1)
+      expect(user.profile_background_upload).to eq(upload2)
     end
 
     it "disables email_digests when enabling mailing_list_mode" do