Explicit use of avatar_icon_* calls depending on situation.

We want to drop the generic #avatar_icon helper that supports both an email
and a user object being passed in. Instead, we want to explicitly use the #avatar_icon_for_user
and #avatar_icon_for_email helpers depending on what we have at hand.
This allows us to avoid unnecessary database queries (e.g. call
User.find_by_any_email if we already have the user).

In situations like here, this makes it less convenient to use.

4 files changed, 33 insertions, 13 deletions

diff --git a/app/helpers/avatars_helper.rb b/app/helpers/avatars_helper.rb
index be11d453898..16e9fbfd3aa 100644
--- a/app/helpers/avatars_helper.rb
+++ b/app/helpers/avatars_helper.rb
@@ -11,7 +11,15 @@ module AvatarsHelper
   def user_avatar_without_link(options = {})
     avatar_size = options[:size] || 16
     user_name = options[:user].try(:name) || options[:user_name]
-    avatar_url = options[:url] || avatar_icon(options[:user] || options[:user_email], avatar_size)
+
+    avatar_url = if options[:url]
+                   options[:url]
+                 elsif options[:user]
+                   avatar_icon_for_user(options[:user], avatar_size)
+                 else
+                   avatar_icon_for_email(options[:user_email], avatar_size)
+                 end
+
     has_tooltip = options[:has_tooltip].nil? ? true : options[:has_tooltip]
     data_attributes = options[:data] || {}
     css_class = %W[avatar s#{avatar_size}].push(*options[:css_class])
diff --git a/app/views/notify/pipeline_failed_email.html.haml b/app/views/notify/pipeline_failed_email.html.haml
index 13b591533ba..7ec10982b5b 100644
--- a/app/views/notify/pipeline_failed_email.html.haml
+++ b/app/views/notify/pipeline_failed_email.html.haml
@@ -60,7 +60,10 @@
               %tbody
                 %tr
                   %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;vertical-align:middle;padding-right:5px;" }
-                    %img.avatar{ height: "24", src: avatar_icon(commit.author || commit.author_email, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
+                    - if commit.author
+                      %img.avatar{ height: "24", src: avatar_icon_for_user(commit.author, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
+                    - else
+                      %img.avatar{ height: "24", src: avatar_icon_for_email(commit.author_email, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
                   %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;vertical-align:middle;" }
                     - if commit.author
                       %a.muted{ href: user_url(commit.author), style: "color:#333333;text-decoration:none;" }
@@ -76,7 +79,10 @@
                 %tbody
                   %tr
                     %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;vertical-align:middle;padding-right:5px;" }
-                      %img.avatar{ height: "24", src: avatar_icon(commit.committer || commit.committer_email, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
+                      - if commit.commiter
+                        %img.avatar{ height: "24", src: avatar_icon_for_user(commit.committer, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
+                      - else
+                        %img.avatar{ height: "24", src: avatar_icon_for_email(commit.committer_email, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
                     %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;vertical-align:middle;" }
                       - if commit.committer
                         %a.muted{ href: user_url(commit.committer), style: "color:#333333;text-decoration:none;" }
diff --git a/app/views/notify/pipeline_success_email.html.haml b/app/views/notify/pipeline_success_email.html.haml
index a4ca8269c36..04f40285df9 100644
--- a/app/views/notify/pipeline_success_email.html.haml
+++ b/app/views/notify/pipeline_success_email.html.haml
@@ -60,7 +60,10 @@
               %tbody
                 %tr
                   %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;vertical-align:middle;padding-right:5px;" }
-                    %img.avatar{ height: "24", src: avatar_icon(commit.author || commit.author_email, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
+                    - if commit.author
+                      %img.avatar{ height: "24", src: avatar_icon_for_user(commit.author, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
+                    - else
+                      %img.avatar{ height: "24", src: avatar_icon_for_email(commit.author_email, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
                   %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;vertical-align:middle;" }
                     - if commit.author
                       %a.muted{ href: user_url(commit.author), style: "color:#333333;text-decoration:none;" }
@@ -76,7 +79,10 @@
                 %tbody
                   %tr
                     %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;vertical-align:middle;padding-right:5px;" }
-                      %img.avatar{ height: "24", src: avatar_icon(commit.committer || commit.committer_email, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
+                      - if commit.committer
+                        %img.avatar{ height: "24", src: avatar_icon_for_user(commit.committer, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
+                      - else
+                        %img.avatar{ height: "24", src: avatar_icon_for_email(commit.committer_email, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "" }/
                     %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;vertical-align:middle;" }
                       - if commit.committer
                         %a.muted{ href: user_url(commit.committer), style: "color:#333333;text-decoration:none;" }
diff --git a/spec/helpers/avatars_helper_spec.rb b/spec/helpers/avatars_helper_spec.rb
index f44e7ef6843..04c6d259135 100644
--- a/spec/helpers/avatars_helper_spec.rb
+++ b/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]