Merge branch '42800-change-usage-of-avatar_icon' into 'master'

Change all occurrences of ApplicationHelper#avatar_icon to use a User object where possible

Closes #42800

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

app/views/projects/repositories/_feed.html.haml

@@ -11,7 +11,7 @@
     %div
       = link_to project_commits_path(@project, commit.id) do
         %code= commit.short_id
-      = image_tag avatar_icon(commit.author_email), class: "", width: 16, alt: ''
+      = image_tag avatar_icon_for_email(commit.author_email), class: "", width: 16, alt: ''
       = markdown(truncate(commit.title, length: 40), pipeline: :single_line, author: commit.author)
   %td
     %span.pull-right.cgray

app/views/search/results/_snippet_blob.html.haml

@@ -7,7 +7,7 @@
     = snippet.title
     by
     = link_to user_snippets_path(snippet.author) do
-      = image_tag avatar_icon(snippet.author), class: "avatar avatar-inline s16", alt: ''
+      = image_tag avatar_icon_for_user(snippet.author), class: "avatar avatar-inline s16", alt: ''
       = snippet.author_name
     %span.light= time_ago_with_tooltip(snippet.created_at)
   %h4.snippet-title

app/views/search/results/_snippet_title.html.haml

@@ -18,6 +18,6 @@
     %span
       by
       = link_to user_snippets_path(snippet_title.author) do
-        = image_tag avatar_icon(snippet_title.author), class: "avatar avatar-inline s16", alt: ''
+        = image_tag avatar_icon_for_user(snippet_title.author), class: "avatar avatar-inline s16", alt: ''
         = snippet_title.author_name
       %span.light= time_ago_with_tooltip(snippet_title.created_at)

app/views/shared/members/_member.html.haml

@@ -8,7 +8,7 @@
 %li.member{ class: dom_class(member), id: dom_id(member) }
   %span.list-item-name
     - if user
-      = image_tag avatar_icon(user, 40), class: "avatar s40", alt: ''
+      = image_tag avatar_icon_for_user(user, 40), class: "avatar s40", alt: ''
       .user-info
         = link_to user.name, user_path(user), class: 'member'
         %span.cgray= user.to_reference
@@ -36,7 +36,7 @@
               Expires in #{distance_of_time_in_words_to_now(member.expires_at)}
     - else
-      = image_tag avatar_icon(member.invite_email, 40), class: "avatar s40", alt: ''
+      = image_tag avatar_icon_for_email(member.invite_email, 40), class: "avatar s40", alt: ''
       .user-info
         .member= member.invite_email
         .cgray

app/views/shared/milestones/_issuable.html.haml

@@ -28,4 +28,4 @@
       - assignees.each do |assignee|
         = link_to polymorphic_path(issuable_type_args, { milestone_title: @milestone.title, assignee_id: assignee.id, state: 'all' }),
                   class: 'has-tooltip', title: "Assigned to #{assignee.name}", data: { container: 'body' } do
-          - image_tag(avatar_icon(assignee, 16), class: "avatar s16", alt: '')
+          - image_tag(avatar_icon_for_user(assignee, 16), class: "avatar s16", alt: '')

app/views/shared/milestones/_participants_tab.html.haml

@@ -2,7 +2,7 @@
   - users.each do |user|
     %li
       = link_to user, title: user.name, class: "darken" do
-        = image_tag avatar_icon(user, 32), class: "avatar s32"
+        = image_tag avatar_icon_for_user(user, 32), class: "avatar s32"
         %strong= truncate(user.name, length: 40)
         %div
           %small.cgray= user.username

app/views/shared/notes/_note.html.haml

@@ -16,7 +16,7 @@
         = icon_for_system_note(note)
       - else
         %a.image-diff-avatar-link{ href: user_path(note.author) }
-          = image_tag avatar_icon(note.author), alt: '', class: 'avatar s40'
+          = image_tag avatar_icon_for_user(note.author), alt: '', class: 'avatar s40'
           - if note.is_a?(DiffNote) && note.on_image?
             - if show_image_comment_badge && note_counter == 0
               -# Only show this for the first comment in the discussion

app/views/shared/notes/_notes_with_form.html.haml

@@ -14,7 +14,7 @@
         .timeline-icon.hidden-xs.hidden-sm
           %a.author_link{ href: user_path(current_user) }
-            = image_tag avatar_icon(current_user), alt: current_user.to_reference, class: 'avatar s40'
+            = image_tag avatar_icon_for_user(current_user), alt: current_user.to_reference, class: 'avatar s40'
         .timeline-content.timeline-content-form
           = render "shared/notes/form", view: diff_view, supports_autocomplete: autocomplete
 - elsif !current_user

app/views/shared/projects/_project.html.haml

@@ -17,7 +17,7 @@
       .avatar-container.s40
         = link_to project_path(project), class: dom_class(project) do
           - if project.creator && use_creator_avatar
-            = image_tag avatar_icon(project.creator.email, 40), class: "avatar s40", alt:''
+            = image_tag avatar_icon_for_user(project.creator, 40), class: "avatar s40", alt:''
           - else
             = project_icon(project, alt: '', class: 'avatar project-avatar s40')
     .project-details

app/views/shared/snippets/_snippet.html.haml

 - link_project = local_assigns.fetch(:link_project, false)
 %li.snippet-row
-  = image_tag avatar_icon(snippet.author), class: "avatar s40 hidden-xs", alt: ''
+  = image_tag avatar_icon_for_user(snippet.author), class: "avatar s40 hidden-xs", alt: ''
   .title
     = link_to reliable_snippet_path(snippet) do

app/views/users/show.html.haml

@@ -35,8 +35,8 @@
     .profile-header
       .avatar-holder
-        = link_to avatar_icon(@user, 400), target: '_blank', rel: 'noopener noreferrer' do
-          = image_tag avatar_icon(@user, 90), class: "avatar s90", alt: ''
+        = link_to avatar_icon_for_user(@user, 400), target: '_blank', rel: 'noopener noreferrer' do
+          = image_tag avatar_icon_for_user(@user, 90), class: "avatar s90", alt: ''
       .user-info
         .cover-title

changelogs/unreleased/42800-change-usage-of-avatar_icon.yml

+---
+title: Use a user object in ApplicationHelper#avatar_icon where possible to avoid
+  N+1 queries.
+merge_request: 42800
+author:
+type: performance

spec/helpers/application_helper_spec.rb

@@ -63,13 +63,29 @@ describe ApplicationHelper do
     end
   end
-  describe 'avatar_icon' do
+  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(user.email).to_s)
+          expect(helper.avatar_icon_for_email(user.email).to_s)
             .to eq(user.avatar.url)
         end
       end
@@ -78,17 +94,37 @@ describe ApplicationHelper do
         it 'calls gravatar_icon' do
           expect(helper).to receive(:gravatar_icon).with('foo@example.com', 20, 2)
-          helper.avatar_icon('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
-    describe 'using a user' do
+  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(user).to_s)
+        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

spec/helpers/avatars_helper_spec.rb

@@ -29,7 +29,7 @@ describe AvatarsHelper do
       is_expected.to eq tag(
         :img,
         alt: "#{user.name}'s avatar",
-        src: avatar_icon(user, 16),
+        src: avatar_icon_for_user(user, 16),
         data: { container: 'body' },
         class: 'avatar s16 has-tooltip',
         title: user.name
@@ -43,7 +43,7 @@ describe AvatarsHelper do
         is_expected.to eq tag(
           :img,
           alt: "#{user.name}'s avatar",
-          src: avatar_icon(user, 16),
+          src: avatar_icon_for_user(user, 16),
           data: { container: 'body' },
           class: "avatar s16 #{options[:css_class]} has-tooltip",
           title: user.name
@@ -58,7 +58,7 @@ describe AvatarsHelper do
         is_expected.to eq tag(
           :img,
           alt: "#{user.name}'s avatar",
-          src: avatar_icon(user, options[:size]),
+          src: avatar_icon_for_user(user, options[:size]),
           data: { container: 'body' },
           class: "avatar s#{options[:size]} has-tooltip",
           title: user.name
@@ -89,7 +89,7 @@ describe AvatarsHelper do
           :img,
           alt: "#{user.name}'s avatar",
           src: LazyImageTagHelper.placeholder_image,
-          data: { container: 'body', src: avatar_icon(user, 16) },
+          data: { container: 'body', src: avatar_icon_for_user(user, 16) },
           class: "avatar s16 has-tooltip lazy",
           title: user.name
         )
@@ -104,7 +104,7 @@ describe AvatarsHelper do
           is_expected.to eq tag(
             :img,
             alt: "#{user.name}'s avatar",
-            src: avatar_icon(user, 16),
+            src: avatar_icon_for_user(user, 16),
             data: { container: 'body' },
             class: "avatar s16 has-tooltip",
             title: user.name
@@ -119,7 +119,7 @@ describe AvatarsHelper do
           is_expected.to eq tag(
             :img,
             alt: "#{user.name}'s avatar",
-            src: avatar_icon(user, 16),
+            src: avatar_icon_for_user(user, 16),
             class: "avatar s16",
             title: user.name
           )
@@ -137,7 +137,7 @@ describe AvatarsHelper do
           is_expected.to eq tag(
             :img,
             alt: "#{user.name}'s avatar",
-            src: avatar_icon(user, 16),
+            src: avatar_icon_for_user(user, 16),
             data: { container: 'body' },
             class: "avatar s16 has-tooltip",
             title: user.name
@@ -149,7 +149,7 @@ describe AvatarsHelper do
         is_expected.to eq tag(
           :img,
           alt: "#{options[:user_name]}'s avatar",
-          src: avatar_icon(options[:user_email], 16),
+          src: avatar_icon_for_email(options[:user_email], 16),
           data: { container: 'body' },
           class: "avatar s16 has-tooltip",
           title: options[:user_name]

spec/helpers/projects_helper_spec.rb

@@ -215,7 +215,7 @@ describe ProjectsHelper do
     let(:expected) { double }
     before do
-      expect(helper).to receive(:avatar_icon).with(user, 16).and_return(expected)
+      expect(helper).to receive(:avatar_icon_for_user).with(user, 16).and_return(expected)
     end
     it 'returns image tag for member avatar' do