FIX: always update 'last_gravatar_download_attempt' when updating gravatar

app/models/user_avatar.rb

@@ -13,12 +13,10 @@ class UserAvatar < ActiveRecord::Base
   def update_gravatar!
     DistributedMutex.synchronize("update_gravatar_#{user_id}") do
       begin
-        # special logic for our system user
-        email_hash = user_id == Discourse::SYSTEM_USER_ID ? User.email_hash("info@discourse.org") : user.email_hash
-
-        self.last_gravatar_download_attempt = Time.new
+        self.update_columns(last_gravatar_download_attempt: Time.now)
 
         max = Discourse.avatar_sizes.max
+        email_hash = user_id == Discourse::SYSTEM_USER_ID ? User.email_hash("info@discourse.org") : user.email_hash
         gravatar_url = "https://www.gravatar.com/avatar/#{email_hash}.png?s=#{max}&d=404"
 
         # follow redirects in case gravatar change rules on us
@@ -42,12 +40,10 @@ class UserAvatar < ActiveRecord::Base
             type: "avatar"
           ).create_for(user_id)
 
-          upload_id = upload.id
-
-          if gravatar_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)
+                user.update!(uploaded_avatar_id: upload.id)
               end
 
               gravatar_upload&.destroy!

spec/models/user_avatar_spec.rb

@@ -8,62 +8,79 @@ describe UserAvatar 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
+    describe "when working" do
 
-    after do
-      temp.unlink
-    end
+      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
 
-    it 'can update gravatars' do
-      expect do
-        avatar.update_gravatar!
-      end.to change { Upload.count }.by(1)
+      after do
+        temp.unlink
+      end
 
-      upload = Upload.last
+      it 'can update gravatars' do
+        freeze_time Time.now
 
-      expect(avatar.gravatar_upload).to eq(upload)
-      expect(user.reload.uploaded_avatar).to eq(nil)
-    end
+        expect { avatar.update_gravatar! }.to change { Upload.count }.by(1)
 
-    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)
+        expect(avatar.last_gravatar_download_attempt).to eq(Time.now)
+        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
 
-    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)
+    describe "when failing" do
 
-        avatar.update_gravatar!
+      it "always update 'last_gravatar_download_attempt'" do
+        freeze_time Time.now
 
-        expect(Upload.find_by(id: upload.id)).to eq(nil)
+        FileHelper.expects(:download).raises(SocketError)
 
-        new_upload = Upload.last
+        expect { avatar.update_gravatar! }.to_not change { Upload.count }
 
-        expect(user.reload.uploaded_avatar).to eq(new_upload)
-        expect(avatar.reload.gravatar_upload).to eq(new_upload)
+        expect(avatar.last_gravatar_download_attempt).to eq(Time.now)
       end
+
     end
+
   end
 
   context '.import_url_for_user' do