diff --git a/app/assets/javascripts/discourse/components/user-card-contents.js.es6 b/app/assets/javascripts/discourse/components/user-card-contents.js.es6 index 83e83ac9b0..d78d1d6db2 100644 --- a/app/assets/javascripts/discourse/components/user-card-contents.js.es6 +++ b/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)})`; diff --git a/app/assets/javascripts/discourse/controllers/preferences/profile.js.es6 b/app/assets/javascripts/discourse/controllers/preferences/profile.js.es6 index 87fe1fa3c4..803eea2d87 100644 --- a/app/assets/javascripts/discourse/controllers/preferences/profile.js.es6 +++ b/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", - "card_background", + "profile_background_upload_url", + "card_background_upload_url", "date_of_birth" ], diff --git a/app/assets/javascripts/discourse/controllers/user.js.es6 b/app/assets/javascripts/discourse/controllers/user.js.es6 index 038dfccfbd..85e6c3fa67 100644 --- a/app/assets/javascripts/discourse/controllers/user.js.es6 +++ b/app/assets/javascripts/discourse/controllers/user.js.es6 @@ -21,8 +21,8 @@ export default Ember.Controller.extend(CanCheckEmails, { return !profileHidden && viewingSelf; }, - @computed("model.profileBackground") - hasProfileBackground(background) { + @computed("model.profileBackgroundUrl") + hasProfileBackgroundUrl(background) { return !Ember.isEmpty(background.toString()); }, diff --git a/app/assets/javascripts/discourse/models/user.js.es6 b/app/assets/javascripts/discourse/models/user.js.es6 index e80e060cef..4af96febe8 100644 --- a/app/assets/javascripts/discourse/models/user.js.es6 +++ b/app/assets/javascripts/discourse/models/user.js.es6 @@ -77,8 +77,8 @@ const User = RestModel.extend({ return username; }, - @computed("profile_background") - profileBackground(bgUrl) { + @computed("profile_background_upload_url") + 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", - "card_background", + "profile_background_upload_url", + "card_background_upload_url", "muted_tags", "tracked_tags", "watched_tags", diff --git a/app/assets/javascripts/discourse/templates/preferences/profile.hbs b/app/assets/javascripts/discourse/templates/preferences/profile.hbs index b906cfe75d..3d5a2bcc75 100644 --- a/app/assets/javascripts/discourse/templates/preferences/profile.hbs +++ b/app/assets/javascripts/discourse/templates/preferences/profile.hbs @@ -32,7 +32,8 @@
- {{image-uploader imageUrl=model.profile_background type="profile_background"}} + {{image-uploader imageUrl=model.profile_background_upload_url + type="profile_background"}}
{{i18n 'user.change_profile_background.instructions'}} @@ -42,7 +43,8 @@
- {{image-uploader imageUrl=model.card_background type="card_background"}} + {{image-uploader imageUrl=model.card_background_upload_url + type="card_background"}}
{{i18n 'user.change_card_background.instructions'}} diff --git a/app/assets/javascripts/discourse/templates/user.hbs b/app/assets/javascripts/discourse/templates/user.hbs index afee93b845..04a4ab8dca 100644 --- a/app/assets/javascripts/discourse/templates/user.hbs +++ b/app/assets/javascripts/discourse/templates/user.hbs @@ -1,7 +1,7 @@
{{#d-section class="user-main"}} -
+
{{#unless collapsedInfo}} {{#if showStaffCounters}}
@@ -34,7 +34,7 @@ {{/if}}
{{/if}} - + {{/unless}}
diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 47d6c5c93a..52594e2772 100644 --- a/app/controllers/session_controller.rb +++ b/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? - sso.profile_background_url = UrlHelper.absolute upload_cdn_path(current_user.user_profile.profile_background) + if current_user.user_profile.profile_background_upload.present? + 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? - sso.card_background_url = UrlHelper.absolute upload_cdn_path(current_user.user_profile.card_background) + if current_user.user_profile.card_background_upload.present? + sso.card_background_url = UrlHelper.absolute(upload_cdn_path( + current_user.user_profile.card_background_upload.url + )) end if request.xhr? diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 972c1b6e44..bd04b320f8 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1241,8 +1241,8 @@ class UsersController < ApplicationController :location, :website, :dismissed_banner_key, - :profile_background, - :card_background + :profile_background_upload_url, + :card_background_upload_url ] permitted << { custom_fields: User.editable_user_custom_fields } unless User.editable_user_custom_fields.blank? diff --git a/app/jobs/onceoff/recover_user_profile_backgrounds.rb b/app/jobs/onceoff/recover_user_profile_backgrounds.rb deleted file mode 100644 index b96523bcee..0000000000 --- a/app/jobs/onceoff/recover_user_profile_backgrounds.rb +++ /dev/null @@ -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 diff --git a/app/jobs/scheduled/clean_up_uploads.rb b/app/jobs/scheduled/clean_up_uploads.rb index 031d18d5b7..2367ae5e8a 100644 --- a/app/jobs/scheduled/clean_up_uploads.rb +++ b/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") diff --git a/app/models/discourse_single_sign_on.rb b/app/models/discourse_single_sign_on.rb index ef7b925eac..00d4bad049 100644 --- a/app/models/discourse_single_sign_on.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 7245e80996..79d55de023 100644 --- a/app/models/user.rb +++ b/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' diff --git a/app/models/user_profile.rb b/app/models/user_profile.rb index 4d36ab5565..a810c30866 100644 --- a/app/models/user_profile.rb +++ b/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.save! + self.update!(card_background_upload: upload) end def clear_card_background - self.card_background = "" - self.save! + self.update!(card_background_upload: nil) end def upload_profile_background(upload) - self.profile_background = upload.url - self.save! + self.update!(profile_background_upload: upload) end def clear_profile_background - self.profile_background = "" - self.save! + self.update!(profile_background_upload: nil) end def self.rebake_old(limit) @@ -150,21 +149,25 @@ end # # Table name: user_profiles # -# user_id :integer not null, primary key -# location :string -# website :string -# bio_raw :text -# bio_cooked :text -# profile_background :string(255) -# dismissed_banner_key :integer -# bio_cooked_version :integer -# badge_granted_title :boolean default(FALSE) -# card_background :string(255) -# views :integer default(0), not null +# user_id :integer not null, primary key +# location :string +# website :string +# bio_raw :text +# bio_cooked :text +# profile_background :string(255) +# dismissed_banner_key :integer +# bio_cooked_version :integer +# badge_granted_title :boolean default(FALSE) +# card_background :string(255) +# 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_card_background (card_background) -# index_user_profiles_on_profile_background (profile_background) +# index_user_profiles_on_bio_cooked_version (bio_cooked_version) +# index_user_profiles_on_card_background (card_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) # diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index 74ac617e86..5a1b0a0bcd 100644 --- a/app/serializers/user_serializer.rb +++ b/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, - :card_background + :profile_background_upload_url, + :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 diff --git a/app/services/user_anonymizer.rb b/app/services/user_anonymizer.rb index bde3d3c609..a5bef8ba05 100644 --- a/app/services/user_anonymizer.rb +++ b/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_background: nil, card_background: nil) + profile.update!( + location: nil, + website: nil, + bio_raw: nil, + bio_cooked: nil, + profile_background_upload: nil, + card_background_upload: nil + ) end @user.user_avatar.try(:destroy) diff --git a/app/services/user_merger.rb b/app/services/user_merger.rb index 7635c03b81..b192aa24aa 100644 --- a/app/services/user_merger.rb +++ b/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 diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb index eda38fb073..1caf4eae5e 100644 --- a/app/services/user_updater.rb +++ b/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 } diff --git a/db/migrate/20190426011148_add_upload_foreign_keys_to_user_profiles.rb b/db/migrate/20190426011148_add_upload_foreign_keys_to_user_profiles.rb new file mode 100644 index 0000000000..2c6d78f782 --- /dev/null +++ b/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 diff --git a/lib/migration/column_dropper.rb b/lib/migration/column_dropper.rb index a2c421ab52..ee892af6e0 100644 --- a/lib/migration/column_dropper.rb +++ b/lib/migration/column_dropper.rb @@ -20,17 +20,19 @@ module Migration columns.each do |column| column = column.to_s - - 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 - + self.drop_readonly(table, column) # 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 diff --git a/lib/upload_recovery.rb b/lib/upload_recovery.rb index eba9849104..83acb51ace 100644 --- a/lib/upload_recovery.rb +++ b/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) diff --git a/lib/validators/upload_url_validator.rb b/lib/validators/upload_url_validator.rb deleted file mode 100644 index e1f1b790b8..0000000000 --- a/lib/validators/upload_url_validator.rb +++ /dev/null @@ -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 diff --git a/script/import_scripts/discuz_x.rb b/script/import_scripts/discuz_x.rb index 71094ec46a..5530a605e8 100644 --- a/script/import_scripts/discuz_x.rb +++ b/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 diff --git a/script/import_scripts/lithium.rb b/script/import_scripts/lithium.rb index 9398a6e8c7..ff9654e657 100644 --- a/script/import_scripts/lithium.rb +++ b/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 diff --git a/script/import_scripts/nodebb/nodebb.rb b/script/import_scripts/nodebb/nodebb.rb index d198d5d8c2..86ba05632e 100644 --- a/script/import_scripts/nodebb/nodebb.rb +++ b/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 diff --git a/script/import_scripts/vbulletin.rb b/script/import_scripts/vbulletin.rb index 092d8d536b..47c87d2584 100644 --- a/script/import_scripts/vbulletin.rb +++ b/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 diff --git a/script/import_scripts/vbulletin5.rb b/script/import_scripts/vbulletin5.rb index 1bab839561..424e921053 100644 --- a/script/import_scripts/vbulletin5.rb +++ b/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 diff --git a/spec/db/migrate/add_upload_foreign_keys_to_user_profiles_spec.rb b/spec/db/migrate/add_upload_foreign_keys_to_user_profiles_spec.rb new file mode 100644 index 0000000000..78417a7d60 --- /dev/null +++ b/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 diff --git a/spec/db/migrate/add_uploads_to_categories_spec.rb b/spec/db/migrate/add_uploads_to_categories_spec.rb index b349a3172f..b89054348f 100644 --- a/spec/db/migrate/add_uploads_to_categories_spec.rb +++ b/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, - logo_url: upload1.url, - background_url: upload2.url - ) + DB.exec(<<~SQL) + UPDATE categories + SET logo_url = '#{upload1.url}', background_url = '#{upload2.url}' + WHERE categories.id = #{category1.id} + SQL - category2 = Fabricate(:category, - logo_url: upload2.url, - background_url: upload1.url - ) + DB.exec(<<~SQL) + UPDATE categories + 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(category1.uploaded_background_id).to eq(upload2.id) + expect(select_column_from_categories(:uploaded_logo_id, category2.id)) + .to eq(upload2.id) - category2.reload - - expect(category2.uploaded_logo_id).to eq(upload2.id) - expect(category2.uploaded_background_id).to eq(upload1.id) + expect(select_column_from_categories(:uploaded_background_id, category2.id)) + .to eq(upload1.id) end end diff --git a/spec/jobs/clean_up_uploads_spec.rb b/spec/jobs/clean_up_uploads_spec.rb index 0f7e20704b..934ca6eb21 100644 --- a/spec/jobs/clean_up_uploads_spec.rb +++ b/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) diff --git a/spec/jobs/recover_user_profile_backgrounds_spec.rb b/spec/jobs/recover_user_profile_backgrounds_spec.rb deleted file mode 100644 index 42a2ec2608..0000000000 --- a/spec/jobs/recover_user_profile_backgrounds_spec.rb +++ /dev/null @@ -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 diff --git a/spec/lib/upload_recovery_spec.rb b/spec/lib/upload_recovery_spec.rb index 0d6892365e..422baaa609 100644 --- a/spec/lib/upload_recovery_spec.rb +++ b/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 diff --git a/spec/models/discourse_single_sign_on_spec.rb b/spec/models/discourse_single_sign_on_spec.rb index 80bb92184a..6787866f1d 100644 --- a/spec/models/discourse_single_sign_on_spec.rb +++ b/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).to_not eq('') + expect(profile_background_url).to_not eq(nil) + 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 - sso_record.user.user_profile.update_columns(profile_background: '') - user = sso.lookup_or_create_user(ip_address) - user.reload - user.user_profile.reload + # Deliberately not setting profile_background_url so it should not update + sso_record.user.user_profile.clear_profile_background + user = sso.lookup_or_create_user(ip_address) + user.reload - expect(user).to_not be_nil - expect(user.user_profile.profile_background).to eq('') - end + expect(user.profile_background_upload).to eq(nil) end it "deal with a profile_background_url passed for an existing user with a profile_background" do - Sidekiq::Testing.inline! do - FileHelper.stubs(:download).returns(logo) + url = "http://example.com/a_different_image.png" + 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" - - 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 + expect(user.profile_background_upload).to_not eq(nil) 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).to_not eq('') + expect(card_background_url).to be_present 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 - sso_record.user.user_profile.update_columns(card_background: '') - user = sso.lookup_or_create_user(ip_address) - user.reload - user.user_profile.reload + # Deliberately not setting card_background_url so it should not update + sso_record.user.user_profile.clear_card_background + user = sso.lookup_or_create_user(ip_address) + user.reload - expect(user).to_not be_nil - expect(user.user_profile.card_background).to eq('') - end + expect(user.user_profile.card_background_upload).to eq(nil) end it "deal with a card_background_url passed for an existing user with a card_background_url" do - Sidekiq::Testing.inline! do - FileHelper.stubs(:download).returns(logo) + url = "http://example.com/a_different_image.png" + 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) - user.reload - user.user_profile.reload - - expect(user).to_not be_nil - expect(user.user_profile.card_background).to_not eq('') - end + expect(user.user_profile.card_background_upload.url).to_not eq('') end end diff --git a/spec/models/user_profile_spec.rb b/spec/models/user_profile_spec.rb index 0f661f1fb7..2d19cb1363 100644 --- a/spec/models/user_profile_spec.rb +++ b/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 diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index db93aa94d0..7bee8e8433 100644 --- a/spec/requests/session_controller_spec.rb +++ b/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", - card_background: "//test.s3.dualstack.us-east-1.amazonaws.com/something" + + upload1 = Fabricate(:upload_s3) + upload2 = Fabricate(:upload_s3) + + @user.user_profile.update!( + profile_background_upload: upload1, + card_background_upload: upload2 ) @user.reload diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index a3f5ad4ad5..97fed892e2 100644 --- a/spec/requests/users_controller_spec.rb +++ b/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 diff --git a/spec/serializers/user_serializer_spec.rb b/spec/serializers/user_serializer_spec.rb index 7a7f3472c5..c409cba146 100644 --- a/spec/serializers/user_serializer_spec.rb +++ b/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 } - - it "produces json" do - expect(json).to be_present - end + let(:upload) { Fabricate(:upload) } + let(:upload2) { Fabricate(:upload) } 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' - end - 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' + expect(json[:card_background_upload_url]).to eq(upload.url) + expect(json[:profile_background_upload_url]).to eq(upload2.url) end end diff --git a/spec/services/user_anonymizer_spec.rb b/spec/services/user_anonymizer_spec.rb index 3855647ffe..8e57e110d9 100644 --- a/spec/services/user_anonymizer_spec.rb +++ b/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.bio_cooked_version).to eq(nil) - expect(profile.card_background).to eq(nil) + expect(profile.profile_background_upload).to eq(nil) + expect(profile.bio_cooked_version).to eq(UserProfile::BAKED_VERSION) + expect(profile.card_background_upload).to eq(nil) end end diff --git a/spec/services/user_merger_spec.rb b/spec/services/user_merger_spec.rb index 2e6576f8c5..f63d9238d3 100644 --- a/spec/services/user_merger_spec.rb +++ b/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 diff --git a/spec/services/user_updater_spec.rb b/spec/services/user_updater_spec.rb index a0ca6ab2ad..e646b0478e 100644 --- a/spec/services/user_updater_spec.rb +++ b/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', - email_level: UserOption.email_level_types[:always], - mailing_list_mode: true, - digest_after_minutes: "45", - new_topic_duration_minutes: 100, - auto_track_topics_after_msecs: 101, - notification_level_when_replying: 3, - email_in_reply_to: false, - date_of_birth: date_of_birth, - theme_ids: [theme.id], - allow_private_messages: false) + val = updater.update( + bio_raw: 'my new bio', + email_level: UserOption.email_level_types[:always], + mailing_list_mode: true, + digest_after_minutes: "45", + new_topic_duration_minutes: 100, + auto_track_topics_after_msecs: 101, + notification_level_when_replying: 3, + email_in_reply_to: false, + date_of_birth: date_of_birth, + 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