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.
This commit is contained in:
Blake Erickson 2020-09-02 20:12:24 -06:00
parent d9b30308f1
commit 67dec38f31
3 changed files with 36 additions and 7 deletions

View File

@ -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)

View File

@ -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

View File

@ -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