Merge branch 'dm-commit-trailer-without-gravatar' into 'master'

Fix commit trailer rendering when Gravatar is disabled

Closes #45633

See merge request gitlab-org/gitlab-ce!18586

10 files changed, 250 insertions, 227 deletions

diff --git a/app/assets/javascripts/vue_shared/components/identicon.vue b/app/assets/javascripts/vue_shared/components/identicon.vue
index 0a30f467b08..23010f40f26 100644
--- a/app/assets/javascripts/vue_shared/components/identicon.vue
+++ b/app/assets/javascripts/vue_shared/components/identicon.vue
@@ -17,7 +17,7 @@ export default {
   },
   computed: {
     /**
-     * This method is based on app/helpers/application_helper.rb#project_identicon
+     * This method is based on app/helpers/avatars_helper.rb#project_identicon
      */
     identiconStyles() {
       const allowedColors = [
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 228c8d2e8f9..6aa307b4db4 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -32,80 +32,6 @@ module ApplicationHelper
     args.any? { |v| v.to_s.downcase == action_name }
   end
 
-  def project_icon(project_id, options = {})
-    project =
-      if project_id.respond_to?(:avatar_url)
-        project_id
-      else
-        Project.find_by_full_path(project_id)
-      end
-
-    if project.avatar_url
-      image_tag project.avatar_url, options
-    else # generated icon
-      project_identicon(project, options)
-    end
-  end
-
-  def project_identicon(project, options = {})
-    allowed_colors = {
-      red: 'FFEBEE',
-      purple: 'F3E5F5',
-      indigo: 'E8EAF6',
-      blue: 'E3F2FD',
-      teal: 'E0F2F1',
-      orange: 'FBE9E7',
-      gray: 'EEEEEE'
-    }
-
-    options[:class] ||= ''
-    options[:class] << ' identicon'
-    bg_key = project.id % 7
-    style = "background-color: ##{allowed_colors.values[bg_key]}; color: #555"
-
-    content_tag(:div, class: options[:class], style: style) do
-      project.name[0, 1].upcase
-    end
-  end
-
-  # Takes both user and email and returns the avatar_icon by
-  # user (preferred) or email.
-  def avatar_icon_for(user = nil, email = nil, size = nil, scale = 2, only_path: true)
-    if user
-      avatar_icon_for_user(user, size, scale, only_path: only_path)
-    elsif email
-      avatar_icon_for_email(email, size, scale, only_path: only_path)
-    else
-      default_avatar
-    end
-  end
-
-  def avatar_icon_for_email(email = nil, size = nil, scale = 2, only_path: true)
-    user = User.find_by_any_email(email.try(:downcase))
-    if user
-      avatar_icon_for_user(user, size, scale, only_path: only_path)
-    else
-      gravatar_icon(email, size, scale)
-    end
-  end
-
-  def avatar_icon_for_user(user = nil, size = nil, scale = 2, only_path: true)
-    if user
-      user.avatar_url(size: size, only_path: only_path) || default_avatar
-    else
-      gravatar_icon(nil, size, scale)
-    end
-  end
-
-  def gravatar_icon(user_email = '', size = nil, scale = 2)
-    GravatarService.new.execute(user_email, size, scale) ||
-      default_avatar
-  end
-
-  def default_avatar
-    asset_path('no_avatar.png')
-  end
-
   def last_commit(project)
     if project.repo_exists?
       time_ago_with_tooltip(project.repository.commit.committed_date)
diff --git a/app/helpers/avatars_helper.rb b/app/helpers/avatars_helper.rb
index 21b6c0a8ad5..d339c01d492 100644
--- a/app/helpers/avatars_helper.rb
+++ b/app/helpers/avatars_helper.rb
@@ -1,4 +1,78 @@
 module AvatarsHelper
+  def project_icon(project_id, options = {})
+    project =
+      if project_id.respond_to?(:avatar_url)
+        project_id
+      else
+        Project.find_by_full_path(project_id)
+      end
+
+    if project.avatar_url
+      image_tag project.avatar_url, options
+    else # generated icon
+      project_identicon(project, options)
+    end
+  end
+
+  def project_identicon(project, options = {})
+    allowed_colors = {
+      red: 'FFEBEE',
+      purple: 'F3E5F5',
+      indigo: 'E8EAF6',
+      blue: 'E3F2FD',
+      teal: 'E0F2F1',
+      orange: 'FBE9E7',
+      gray: 'EEEEEE'
+    }
+
+    options[:class] ||= ''
+    options[:class] << ' identicon'
+    bg_key = project.id % 7
+    style = "background-color: ##{allowed_colors.values[bg_key]}; color: #555"
+
+    content_tag(:div, class: options[:class], style: style) do
+      project.name[0, 1].upcase
+    end
+  end
+
+  # Takes both user and email and returns the avatar_icon by
+  # user (preferred) or email.
+  def avatar_icon_for(user = nil, email = nil, size = nil, scale = 2, only_path: true)
+    if user
+      avatar_icon_for_user(user, size, scale, only_path: only_path)
+    elsif email
+      avatar_icon_for_email(email, size, scale, only_path: only_path)
+    else
+      default_avatar
+    end
+  end
+
+  def avatar_icon_for_email(email = nil, size = nil, scale = 2, only_path: true)
+    user = User.find_by_any_email(email.try(:downcase))
+    if user
+      avatar_icon_for_user(user, size, scale, only_path: only_path)
+    else
+      gravatar_icon(email, size, scale)
+    end
+  end
+
+  def avatar_icon_for_user(user = nil, size = nil, scale = 2, only_path: true)
+    if user
+      user.avatar_url(size: size, only_path: only_path) || default_avatar
+    else
+      gravatar_icon(nil, size, scale)
+    end
+  end
+
+  def gravatar_icon(user_email = '', size = nil, scale = 2)
+    GravatarService.new.execute(user_email, size, scale) ||
+      default_avatar
+  end
+
+  def default_avatar
+    ActionController::Base.helpers.image_path('no_avatar.png')
+  end
+
   def author_avatar(commit_or_event, options = {})
     user_avatar(options.merge({
       user: commit_or_event.author,
diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb
index e4212775956..3646e08a15f 100644
--- a/app/mailers/notify.rb
+++ b/app/mailers/notify.rb
@@ -16,6 +16,7 @@ class Notify < BaseMailer
   helper BlobHelper
   helper EmailsHelper
   helper MembersHelper
+  helper AvatarsHelper
   helper GitlabRoutingHelper
 
   def test_email(recipient_email, subject, body)
diff --git a/changelogs/unreleased/dm-commit-trailer-without-gravatar.yml b/changelogs/unreleased/dm-commit-trailer-without-gravatar.yml
new file mode 100644
index 00000000000..9f057c67122
--- /dev/null
+++ b/changelogs/unreleased/dm-commit-trailer-without-gravatar.yml
@@ -0,0 +1,5 @@
+---
+title: Fix commit trailer rendering when Gravatar is disabled
+merge_request:
+author:
+type: fixed
diff --git a/lib/banzai/filter/commit_trailers_filter.rb b/lib/banzai/filter/commit_trailers_filter.rb
index ef16df1f3ae..7b55e8b36f6 100644
--- a/lib/banzai/filter/commit_trailers_filter.rb
+++ b/lib/banzai/filter/commit_trailers_filter.rb
@@ -13,7 +13,6 @@ module Banzai
     # * https://git.wiki.kernel.org/index.php/CommitMessageConventions
     class CommitTrailersFilter < HTML::Pipeline::Filter
       include ActionView::Helpers::TagHelper
-      include ApplicationHelper
       include AvatarsHelper
 
       TRAILER_REGEXP = /(?<label>[[:alpha:]-]+-by:)/i.freeze
diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb
index 43cb0dfe163..5e454f8b310 100644
--- a/spec/helpers/application_helper_spec.rb
+++ b/spec/helpers/application_helper_spec.rb
@@ -2,8 +2,6 @@
 require 'spec_helper'
 
 describe ApplicationHelper do
-  include UploadHelpers
-
   describe 'current_controller?' do
     it 'returns true when controller matches argument' do
       stub_controller_name('foo')
@@ -54,143 +52,6 @@ describe ApplicationHelper do
     end
   end
 
-  describe 'project_icon' do
-    it 'returns an url for the avatar' do
-      project = create(:project, :public, avatar: File.open(uploaded_image_temp_path))
-
-      expect(helper.project_icon(project.full_path).to_s)
-        .to eq "<img data-src=\"#{project.avatar.url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />"
-    end
-  end
-
-  describe 'avatar_icon_for' do
-    let!(:user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: 'bar@example.com') }
-    let(:email) { 'foo@example.com' }
-    let!(:another_user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: email) }
-
-    it 'prefers the user to retrieve the avatar_url' do
-      expect(helper.avatar_icon_for(user, email).to_s)
-        .to eq(user.avatar.url)
-    end
-
-    it 'falls back to email lookup if no user given' do
-      expect(helper.avatar_icon_for(nil, email).to_s)
-        .to eq(another_user.avatar.url)
-    end
-  end
-
-  describe 'avatar_icon_for_email' do
-    let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) }
-
-    context 'using an email' do
-      context 'when there is a matching user' do
-        it 'returns a relative URL for the avatar' do
-          expect(helper.avatar_icon_for_email(user.email).to_s)
-            .to eq(user.avatar.url)
-        end
-      end
-
-      context 'when no user exists for the email' do
-        it 'calls gravatar_icon' do
-          expect(helper).to receive(:gravatar_icon).with('foo@example.com', 20, 2)
-
-          helper.avatar_icon_for_email('foo@example.com', 20, 2)
-        end
-      end
-
-      context 'without an email passed' do
-        it 'calls gravatar_icon' do
-          expect(helper).to receive(:gravatar_icon).with(nil, 20, 2)
-
-          helper.avatar_icon_for_email(nil, 20, 2)
-        end
-      end
-    end
-  end
-
-  describe 'avatar_icon_for_user' do
-    let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) }
-
-    context 'with a user object passed' do
-      it 'returns a relative URL for the avatar' do
-        expect(helper.avatar_icon_for_user(user).to_s)
-          .to eq(user.avatar.url)
-      end
-    end
-
-    context 'without a user object passed' do
-      it 'calls gravatar_icon' do
-        expect(helper).to receive(:gravatar_icon).with(nil, 20, 2)
-
-        helper.avatar_icon_for_user(nil, 20, 2)
-      end
-    end
-  end
-
-  describe 'gravatar_icon' do
-    let(:user_email) { 'user@email.com' }
-
-    context 'with Gravatar disabled' do
-      before do
-        stub_application_setting(gravatar_enabled?: false)
-      end
-
-      it 'returns a generic avatar' do
-        expect(helper.gravatar_icon(user_email)).to match_asset_path('no_avatar.png')
-      end
-    end
-
-    context 'with Gravatar enabled' do
-      before do
-        stub_application_setting(gravatar_enabled?: true)
-      end
-
-      it 'returns a generic avatar when email is blank' do
-        expect(helper.gravatar_icon('')).to match_asset_path('no_avatar.png')
-      end
-
-      it 'returns a valid Gravatar URL' do
-        stub_config_setting(https: false)
-
-        expect(helper.gravatar_icon(user_email))
-          .to match('https://www.gravatar.com/avatar/b58c6f14d292556214bd64909bcdb118')
-      end
-
-      it 'uses HTTPs when configured' do
-        stub_config_setting(https: true)
-
-        expect(helper.gravatar_icon(user_email))
-          .to match('https://secure.gravatar.com')
-      end
-
-      it 'returns custom gravatar path when gravatar_url is set' do
-        stub_gravatar_setting(plain_url: 'http://example.local/?s=%{size}&hash=%{hash}')
-
-        expect(gravatar_icon(user_email, 20))
-          .to eq('http://example.local/?s=40&hash=b58c6f14d292556214bd64909bcdb118')
-      end
-
-      it 'accepts a custom size argument' do
-        expect(helper.gravatar_icon(user_email, 64)).to include '?s=128'
-      end
-
-      it 'defaults size to 40@2x when given an invalid size' do
-        expect(helper.gravatar_icon(user_email, nil)).to include '?s=80'
-      end
-
-      it 'accepts a scaling factor' do
-        expect(helper.gravatar_icon(user_email, 40, 3)).to include '?s=120'
-      end
-
-      it 'ignores case and surrounding whitespace' do
-        normal = helper.gravatar_icon('foo@example.com')
-        upcase = helper.gravatar_icon(' FOO@EXAMPLE.COM ')
-
-        expect(normal).to eq upcase
-      end
-    end
-  end
-
   describe 'simple_sanitize' do
     let(:a_tag) { '<a href="#">Foo</a>' }
 
diff --git a/spec/helpers/avatars_helper_spec.rb b/spec/helpers/avatars_helper_spec.rb
index 04c6d259135..5856bccb5b8 100644
--- a/spec/helpers/avatars_helper_spec.rb
+++ b/spec/helpers/avatars_helper_spec.rb
@@ -1,10 +1,147 @@
 require 'rails_helper'
 
 describe AvatarsHelper do
-  include ApplicationHelper
+  include UploadHelpers
 
   let(:user) { create(:user) }
 
+  describe '#project_icon' do
+    it 'returns an url for the avatar' do
+      project = create(:project, :public, avatar: File.open(uploaded_image_temp_path))
+
+      expect(helper.project_icon(project.full_path).to_s)
+        .to eq "<img data-src=\"#{project.avatar.url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />"
+    end
+  end
+
+  describe '#avatar_icon_for' do
+    let!(:user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: 'bar@example.com') }
+    let(:email) { 'foo@example.com' }
+    let!(:another_user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: email) }
+
+    it 'prefers the user to retrieve the avatar_url' do
+      expect(helper.avatar_icon_for(user, email).to_s)
+        .to eq(user.avatar.url)
+    end
+
+    it 'falls back to email lookup if no user given' do
+      expect(helper.avatar_icon_for(nil, email).to_s)
+        .to eq(another_user.avatar.url)
+    end
+  end
+
+  describe '#avatar_icon_for_email' do
+    let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) }
+
+    context 'using an email' do
+      context 'when there is a matching user' do
+        it 'returns a relative URL for the avatar' do
+          expect(helper.avatar_icon_for_email(user.email).to_s)
+            .to eq(user.avatar.url)
+        end
+      end
+
+      context 'when no user exists for the email' do
+        it 'calls gravatar_icon' do
+          expect(helper).to receive(:gravatar_icon).with('foo@example.com', 20, 2)
+
+          helper.avatar_icon_for_email('foo@example.com', 20, 2)
+        end
+      end
+
+      context 'without an email passed' do
+        it 'calls gravatar_icon' do
+          expect(helper).to receive(:gravatar_icon).with(nil, 20, 2)
+
+          helper.avatar_icon_for_email(nil, 20, 2)
+        end
+      end
+    end
+  end
+
+  describe '#avatar_icon_for_user' do
+    let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) }
+
+    context 'with a user object passed' do
+      it 'returns a relative URL for the avatar' do
+        expect(helper.avatar_icon_for_user(user).to_s)
+          .to eq(user.avatar.url)
+      end
+    end
+
+    context 'without a user object passed' do
+      it 'calls gravatar_icon' do
+        expect(helper).to receive(:gravatar_icon).with(nil, 20, 2)
+
+        helper.avatar_icon_for_user(nil, 20, 2)
+      end
+    end
+  end
+
+  describe '#gravatar_icon' do
+    let(:user_email) { 'user@email.com' }
+
+    context 'with Gravatar disabled' do
+      before do
+        stub_application_setting(gravatar_enabled?: false)
+      end
+
+      it 'returns a generic avatar' do
+        expect(helper.gravatar_icon(user_email)).to match_asset_path('no_avatar.png')
+      end
+    end
+
+    context 'with Gravatar enabled' do
+      before do
+        stub_application_setting(gravatar_enabled?: true)
+      end
+
+      it 'returns a generic avatar when email is blank' do
+        expect(helper.gravatar_icon('')).to match_asset_path('no_avatar.png')
+      end
+
+      it 'returns a valid Gravatar URL' do
+        stub_config_setting(https: false)
+
+        expect(helper.gravatar_icon(user_email))
+          .to match('https://www.gravatar.com/avatar/b58c6f14d292556214bd64909bcdb118')
+      end
+
+      it 'uses HTTPs when configured' do
+        stub_config_setting(https: true)
+
+        expect(helper.gravatar_icon(user_email))
+          .to match('https://secure.gravatar.com')
+      end
+
+      it 'returns custom gravatar path when gravatar_url is set' do
+        stub_gravatar_setting(plain_url: 'http://example.local/?s=%{size}&hash=%{hash}')
+
+        expect(gravatar_icon(user_email, 20))
+          .to eq('http://example.local/?s=40&hash=b58c6f14d292556214bd64909bcdb118')
+      end
+
+      it 'accepts a custom size argument' do
+        expect(helper.gravatar_icon(user_email, 64)).to include '?s=128'
+      end
+
+      it 'defaults size to 40@2x when given an invalid size' do
+        expect(helper.gravatar_icon(user_email, nil)).to include '?s=80'
+      end
+
+      it 'accepts a scaling factor' do
+        expect(helper.gravatar_icon(user_email, 40, 3)).to include '?s=120'
+      end
+
+      it 'ignores case and surrounding whitespace' do
+        normal = helper.gravatar_icon('foo@example.com')
+        upcase = helper.gravatar_icon(' FOO@EXAMPLE.COM ')
+
+        expect(normal).to eq upcase
+      end
+    end
+  end
+
   describe '#user_avatar' do
     subject { helper.user_avatar(user: user) }
 
diff --git a/spec/lib/banzai/filter/commit_trailers_filter_spec.rb b/spec/lib/banzai/filter/commit_trailers_filter_spec.rb
index 1fd145116df..068cdc85a07 100644
--- a/spec/lib/banzai/filter/commit_trailers_filter_spec.rb
+++ b/spec/lib/banzai/filter/commit_trailers_filter_spec.rb
@@ -47,16 +47,36 @@ describe Banzai::Filter::CommitTrailersFilter do
       )
     end
 
-    it 'non GitLab users and replaces them with mailto links' do
-      _, message_html = build_commit_message(
-        trailer: trailer,
-        name: FFaker::Name.name,
-        email: email
-      )
+    context 'non GitLab users' do
+      shared_examples 'mailto links' do
+        it 'replaces them with mailto links' do
+          _, message_html = build_commit_message(
+            trailer: trailer,
+            name: FFaker::Name.name,
+            email: email
+          )
 
-      doc = filter(message_html)
+          doc = filter(message_html)
 
-      expect_to_have_mailto_link(doc, email: email, trailer: trailer)
+          expect_to_have_mailto_link_with_avatar(doc, email: email, trailer: trailer)
+        end
+      end
+
+      context 'when Gravatar is disabled' do
+        before do
+          stub_application_setting(gravatar_enabled: false)
+        end
+
+        it_behaves_like 'mailto links'
+      end
+
+      context 'when Gravatar is enabled' do
+        before do
+          stub_application_setting(gravatar_enabled: true)
+        end
+
+        it_behaves_like 'mailto links'
+      end
     end
 
     it 'multiple trailers in the same message' do
@@ -69,7 +89,7 @@ describe Banzai::Filter::CommitTrailersFilter do
       doc = filter(message)
 
       expect_to_have_user_link_with_avatar(doc, user: user, trailer: trailer)
-      expect_to_have_mailto_link(doc, email: email, trailer: different_trailer)
+      expect_to_have_mailto_link_with_avatar(doc, email: email, trailer: different_trailer)
     end
 
     context 'special names' do
@@ -90,7 +110,7 @@ describe Banzai::Filter::CommitTrailersFilter do
 
           doc = filter(message_html)
 
-          expect_to_have_mailto_link(doc, email: email, trailer: trailer)
+          expect_to_have_mailto_link_with_avatar(doc, email: email, trailer: trailer)
           expect(doc.text).to match Regexp.escape(message)
         end
       end
diff --git a/spec/support/commit_trailers_spec_helper.rb b/spec/support/commit_trailers_spec_helper.rb
index add359946db..efa317fd2f9 100644
--- a/spec/support/commit_trailers_spec_helper.rb
+++ b/spec/support/commit_trailers_spec_helper.rb
@@ -8,7 +8,7 @@ module CommitTrailersSpecHelper
     expect(wrapper.attribute('data-user').value).to eq user.id.to_s
   end
 
-  def expect_to_have_mailto_link(doc, email:, trailer:)
+  def expect_to_have_mailto_link_with_avatar(doc, email:, trailer:)
     wrapper = find_user_wrapper(doc, trailer)
 
     expect_to_have_links_with_url_and_avatar(wrapper, "mailto:#{CGI.escape_html(email)}", email)