FIX: Gravatar download attempt if user is missing their email

FIX: Gravatar download attempt if user is missing their email

It is possible that a user could exist without an email, if so we should not enqueue a job to download their gravatar.

This commit resolves this error that can occur:

Job exception: undefined method `email' for nil:NilClass
/var/www/discourse/app/models/user.rb:1204:in `email'
/var/www/discourse/app/jobs/regular/update_gravatar.rb:12:in `execute'

This commit also fixes the original spec which actually was wrong. The job never enqueued in the original spec and so the gravatar was never actually updated and the test was checking if the two values were the same, but they were both null and never updated, so of course they were the same!

A new test has also been added to make sure the gravatar job isn’t enqueued when a user’s email is missing.

diff --git a/app/jobs/regular/update_gravatar.rb b/app/jobs/regular/update_gravatar.rb
index 7ae7217..de954b4 100644
--- a/app/jobs/regular/update_gravatar.rb
+++ b/app/jobs/regular/update_gravatar.rb
@@ -9,7 +9,7 @@ module Jobs
       user = User.find_by(id: args[:user_id])
       avatar = UserAvatar.find_by(id: args[:avatar_id])
 
-      if user && avatar && avatar.user&.id == user.id && user.email.present?
+      if user && avatar && avatar.user&.id == user.id && user.primary_email.present?
         avatar.update_gravatar!
         if !user.uploaded_avatar_id && avatar.gravatar_upload_id
           user.update_column(:uploaded_avatar_id, avatar.gravatar_upload_id)
diff --git a/app/models/user.rb b/app/models/user.rb
index b5d4d63..865b667 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -1077,7 +1077,10 @@ class User < ActiveRecord::Base
 
     avatar = user_avatar || create_user_avatar
 
-    if SiteSetting.automatically_download_gravatars? && !avatar.last_gravatar_download_attempt
+    if self.primary_email.present? &&
+        SiteSetting.automatically_download_gravatars? &&
+        !avatar.last_gravatar_download_attempt
+
       Jobs.cancel_scheduled_job(:update_gravatar, user_id: self.id, avatar_id: avatar.id)
       Jobs.enqueue_in(1.second, :update_gravatar, user_id: self.id, avatar_id: avatar.id)
     end
diff --git a/spec/jobs/update_gravatar_spec.rb b/spec/jobs/update_gravatar_spec.rb
index 3be1af2..0f0de83 100644
--- a/spec/jobs/update_gravatar_spec.rb
+++ b/spec/jobs/update_gravatar_spec.rb
@@ -3,22 +3,48 @@
 require 'rails_helper'
 
 describe Jobs::UpdateGravatar do
+  fab!(:user) { Fabricate(:user) }
+  let(:temp) { Tempfile.new('test') }
+  fab!(:upload) { Fabricate(:upload, user: user) }
+  let(:avatar) { user.create_user_avatar! }
 
   it "picks gravatar if system avatar is picked and gravatar was just downloaded" do
-    user = User.create!(username: "bob", name: "bob", email: "a@a.com")
+    temp.binmode
+    # tiny valid png
+    temp.write(Base64.decode64("iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg=="))
+    temp.rewind
+    FileHelper.expects(:download).returns(temp)
+
+    Jobs.run_immediately!
+
     expect(user.uploaded_avatar_id).to eq(nil)
     expect(user.user_avatar.gravatar_upload_id).to eq(nil)
 
-    png = Base64.decode64("R0lGODlhAQABALMAAAAAAIAAAACAAICAAAAAgIAAgACAgMDAwICAgP8AAAD/AP//AAAA//8A/wD//wBiZCH5BAEAAA8ALAAAAAABAAEAAAQC8EUAOw==")
-    url = "https://www.gravatar.com/avatar/d10ca8d11301c2f4993ac2279ce4b930.png?s=360&d=404"
-    stub_request(:get, url).to_return(body: png)
-
     SiteSetting.automatically_download_gravatars = true
 
     user.refresh_avatar
     user.reload
 
+    expect(user.uploaded_avatar_id).to_not eq(nil)
     expect(user.uploaded_avatar_id).to eq(user.user_avatar.gravatar_upload_id)
+
+    temp.unlink
   end
 
+  it "does not enqueue a job when user is missing their email" do
+    user.primary_email.destroy
+    user.reload
+
+    expect(user.uploaded_avatar_id).to eq(nil)
+    expect(user.user_avatar.gravatar_upload_id).to eq(nil)
+
+    SiteSetting.automatically_download_gravatars = true
+
+    expect { user.refresh_avatar }
+      .to change { Jobs::UpdateGravatar.jobs.count }.by(0)
+    user.reload
+
+    expect(user.uploaded_avatar_id).to eq(nil)
+    expect(user.user_avatar.gravatar_upload_id).to eq(nil)
+  end
 end

GitHub sha: 67dec38f

1 Like

Your commit messages are glorious :heart_eyes:

1 Like