FIX: UserAvatar#update_gravatar! does not update User#uploaded_avatar.

https://meta.discourse.org/t/missing-user-profile-pictures/93844/4

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

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

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

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