diff --git a/app/jobs/onceoff/fix_out_of_sync_user_uploaded_avatar.rb b/app/jobs/onceoff/fix_out_of_sync_user_uploaded_avatar.rb new file mode 100644 index 00000000000..522ba9c0da7 --- /dev/null +++ b/app/jobs/onceoff/fix_out_of_sync_user_uploaded_avatar.rb @@ -0,0 +1,23 @@ +module Jobs + class FixOutOfSyncUserUploadedAvatar < Jobs::Onceoff + def execute_onceoff(args) + DB.exec(<<~SQL) + WITH X AS ( + SELECT + u.id AS user_id, + ua.gravatar_upload_id AS gravatar_upload_id + FROM users u + JOIN user_avatars ua ON ua.user_id = u.id + LEFT JOIN uploads ON uploads.id = u.uploaded_avatar_id + WHERE u.uploaded_avatar_id IS NOT NULL + AND uploads.id IS NULL + ) + UPDATE users + SET uploaded_avatar_id = X.gravatar_upload_id + FROM X + WHERE users.id = X.user_id + AND coalesce(uploaded_avatar_id,-1) <> coalesce(X.gravatar_upload_id,-1) + SQL + end + end +end diff --git a/app/models/user_avatar.rb b/app/models/user_avatar.rb index 731c15981a6..e1a5da4b8d0 100644 --- a/app/models/user_avatar.rb +++ b/app/models/user_avatar.rb @@ -42,10 +42,18 @@ class UserAvatar < ActiveRecord::Base type: "avatar" ).create_for(user_id) - if gravatar_upload_id != upload.id - gravatar_upload&.destroy! - self.gravatar_upload = upload - save! + upload_id = upload.id + + if gravatar_upload_id != upload_id + User.transaction do + if gravatar_upload_id && user.uploaded_avatar_id == gravatar_upload_id + user.update!(uploaded_avatar_id: upload_id) + end + + gravatar_upload&.destroy! + self.gravatar_upload = upload + save! + end end end rescue OpenURI::HTTPError diff --git a/spec/jobs/fix_out_of_sync_user_uploaded_avatar_spec.rb b/spec/jobs/fix_out_of_sync_user_uploaded_avatar_spec.rb new file mode 100644 index 00000000000..a2323b31525 --- /dev/null +++ b/spec/jobs/fix_out_of_sync_user_uploaded_avatar_spec.rb @@ -0,0 +1,42 @@ +require 'rails_helper' + +RSpec.describe Jobs::FixOutOfSyncUserUploadedAvatar do + it 'should fix out of sync user uploaded avatars' do + user_with_custom_upload = Fabricate(:user) + custom_upload1 = Fabricate(:upload, user: user_with_custom_upload) + gravatar_upload1 = Fabricate(:upload, user: user_with_custom_upload) + user_with_custom_upload.update!(uploaded_avatar: custom_upload1) + + user_with_custom_upload.user_avatar.update!( + custom_upload: custom_upload1, + gravatar_upload: gravatar_upload1 + ) + + user_out_of_sync = Fabricate(:user) + custom_upload2 = Fabricate(:upload, user: user_out_of_sync) + gravatar_upload2 = Fabricate(:upload, user: user_out_of_sync) + prev_gravatar_upload = Fabricate(:upload, user: user_out_of_sync) + user_out_of_sync.update!(uploaded_avatar: prev_gravatar_upload) + prev_gravatar_upload.destroy! + + user_out_of_sync.user_avatar.update!( + custom_upload: custom_upload2, + gravatar_upload: gravatar_upload2 + ) + + user_without_uploaded_avatar = Fabricate(:user) + gravatar_upload3 = Fabricate(:upload, user: user_without_uploaded_avatar) + + user_without_uploaded_avatar.user_avatar.update!( + gravatar_upload: gravatar_upload3 + ) + + described_class.new.execute_onceoff({}) + + expect(user_with_custom_upload.reload.uploaded_avatar).to eq(custom_upload1) + expect(user_out_of_sync.reload.uploaded_avatar).to eq(gravatar_upload2) + + expect(user_without_uploaded_avatar.reload.uploaded_avatar) + .to eq(nil) + end +end diff --git a/spec/models/user_avatar_spec.rb b/spec/models/user_avatar_spec.rb index 75f31ac8db8..7bc27537a2e 100644 --- a/spec/models/user_avatar_spec.rb +++ b/spec/models/user_avatar_spec.rb @@ -4,16 +4,66 @@ describe UserAvatar do let(:user) { Fabricate(:user) } let(:avatar) { user.create_user_avatar! } - it 'can update gravatars' do - temp = Tempfile.new('test') - temp.binmode - # tiny valid png - temp.write(Base64.decode64("R0lGODlhAQABALMAAAAAAIAAAACAAICAAAAAgIAAgACAgMDAwICAgP8AAAD/AP//AAAA//8A/wD//wBiZCH5BAEAAA8ALAAAAAABAAEAAAQC8EUAOw==")) - temp.rewind - FileHelper.expects(:download).returns(temp) - avatar.update_gravatar! - temp.unlink - expect(avatar.gravatar_upload).not_to eq(nil) + describe '#update_gravatar!' do + let(:temp) { Tempfile.new('test') } + let(:upload) { Fabricate(:upload, user: user) } + + before do + temp.binmode + # tiny valid png + temp.write(Base64.decode64("R0lGODlhAQABALMAAAAAAIAAAACAAICAAAAAgIAAgACAgMDAwICAgP8AAAD/AP//AAAA//8A/wD//wBiZCH5BAEAAA8ALAAAAAABAAEAAAQC8EUAOw==")) + temp.rewind + FileHelper.expects(:download).returns(temp) + end + + after do + temp.unlink + end + + it 'can update gravatars' do + expect do + avatar.update_gravatar! + end.to change { Upload.count }.by(1) + + upload = Upload.last + + expect(avatar.gravatar_upload).to eq(upload) + expect(user.reload.uploaded_avatar).to eq(nil) + end + + describe 'when user has an existing custom upload' do + it "should not change the user's uploaded avatar" do + user.update!(uploaded_avatar: upload) + + avatar.update!( + custom_upload: upload, + gravatar_upload: Fabricate(:upload, user: user) + ) + + avatar.update_gravatar! + + expect(upload.reload).to eq(upload) + expect(user.reload.uploaded_avatar).to eq(upload) + expect(avatar.reload.custom_upload).to eq(upload) + expect(avatar.gravatar_upload).to eq(Upload.last) + end + end + + describe 'when user has an existing gravatar' do + it "should update the user's uploaded avatar correctly" do + user.update!(uploaded_avatar: upload) + avatar.update!(gravatar_upload: upload) + + avatar.update_gravatar! + + expect(Upload.find_by(id: upload.id)).to eq(nil) + + new_upload = Upload.last + + expect(user.reload.uploaded_avatar).to eq(new_upload) + expect(avatar.reload.gravatar_upload).to eq(new_upload) + end + end end context '.import_url_for_user' do