Merge branch 'fix-email-avatars' into 'master'

Fix avatar images in pipeline emails

Closes gitlab-ee#2696

See merge request !12310

app/helpers/application_helper.rb

@@ -68,7 +68,7 @@ module ApplicationHelper
     end
   end
 
-  def avatar_icon(user_or_email = nil, size = nil, scale = 2)
+  def avatar_icon(user_or_email = nil, size = nil, scale = 2, only_path: true)
     user =
       if user_or_email.is_a?(User)
         user_or_email
@@ -77,7 +77,7 @@ module ApplicationHelper
       end
 
     if user
-      user.avatar_url(size: size) || default_avatar
+      user.avatar_url(size: size, only_path: only_path) || default_avatar
     else
       gravatar_icon(user_or_email, size, scale)
     end

app/views/notify/pipeline_failed_email.html.haml

@@ -60,7 +60,7 @@
               %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), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "Avatar" }/
+                    %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: "Avatar" }/
                   %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 +76,7 @@
                 %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), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "Avatar" }/
+                      %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: "Avatar" }/
                     %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;" }
@@ -100,7 +100,7 @@
             triggered by
           - if @pipeline.user
             %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;vertical-align:middle;padding-right:5px;padding-left:5px", width: "24" }
-              %img.avatar{ height: "24", src: avatar_icon(@pipeline.user, 24), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "Avatar" }/
+              %img.avatar{ height: "24", src: avatar_icon(@pipeline.user, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "Avatar" }/
             %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;font-weight:500;line-height:1.4;vertical-align:baseline;" }
               %a.muted{ href: user_url(@pipeline.user), style: "color:#333333;text-decoration:none;" }
                 = @pipeline.user.name

app/views/notify/pipeline_success_email.html.haml

@@ -60,7 +60,7 @@
               %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), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "Avatar" }/
+                    %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: "Avatar" }/
                   %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 +76,7 @@
                 %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), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "Avatar" }/
+                      %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: "Avatar" }/
                     %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;" }
@@ -100,7 +100,7 @@
             triggered by
           - if @pipeline.user
             %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;line-height:1.4;vertical-align:middle;padding-right:5px;padding-left:5px", width: "24" }
-              %img.avatar{ height: "24", src: avatar_icon(@pipeline.user, 24), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "Avatar" }/
+              %img.avatar{ height: "24", src: avatar_icon(@pipeline.user, 24, only_path: false), style: "display:block;border-radius:12px;margin:-2px 0;", width: "24", alt: "Avatar" }/
             %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:15px;font-weight:500;line-height:1.4;vertical-align:baseline;" }
               %a.muted{ href: user_url(@pipeline.user), style: "color:#333333;text-decoration:none;" }
                 = @pipeline.user.name

spec/helpers/application_helper_spec.rb

@@ -82,42 +82,71 @@ describe ApplicationHelper do
   end
 
   describe 'avatar_icon' do
-    it 'returns an url for the avatar' do
-      user = create(:user, avatar: File.open(uploaded_image_temp_path))
-
-      avatar_url = "/uploads/system/user/avatar/#{user.id}/banana_sample.gif"
-
-      expect(helper.avatar_icon(user.email).to_s).to match(avatar_url)
-
-      allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host)
-      avatar_url = "#{gitlab_host}/uploads/system/user/avatar/#{user.id}/banana_sample.gif"
-
-      expect(helper.avatar_icon(user.email).to_s).to match(avatar_url)
-    end
-
-    it 'returns an url for the avatar with relative url' do
-      stub_config_setting(relative_url_root: '/gitlab')
-      # Must be stubbed after the stub above, and separately
-      stub_config_setting(url: Settings.send(:build_gitlab_url))
-
-      user = create(:user, avatar: File.open(uploaded_image_temp_path))
-
-      expect(helper.avatar_icon(user.email).to_s).
-        to match("/gitlab/uploads/system/user/avatar/#{user.id}/banana_sample.gif")
-    end
+    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).
+            to eq("/uploads/system/user/avatar/#{user.id}/banana_sample.gif")
+        end
+
+        context 'when an asset_host is set in the config' do
+          let(:asset_host) { 'http://assets' }
+
+          before do
+            allow(ActionController::Base).to receive(:asset_host).and_return(asset_host)
+          end
+
+          it 'returns an absolute URL on that asset host' do
+            expect(helper.avatar_icon(user.email, only_path: false).to_s).
+              to eq("#{asset_host}/uploads/system/user/avatar/#{user.id}/banana_sample.gif")
+          end
+        end
+
+        context 'when only_path is set to false' do
+          it 'returns an absolute URL for the avatar' do
+            expect(helper.avatar_icon(user.email, only_path: false).to_s).
+              to eq("#{gitlab_host}/uploads/system/user/avatar/#{user.id}/banana_sample.gif")
+          end
+        end
+
+        context 'when the GitLab instance is at a relative URL' do
+          before do
+            stub_config_setting(relative_url_root: '/gitlab')
+            # Must be stubbed after the stub above, and separately
+            stub_config_setting(url: Settings.send(:build_gitlab_url))
+          end
+
+          it 'returns a relative URL with the correct prefix' do
+            expect(helper.avatar_icon(user.email).to_s).
+              to eq("/gitlab/uploads/system/user/avatar/#{user.id}/banana_sample.gif")
+          end
+        end
+      end
 
-    it 'calls gravatar_icon when no User exists with the given email' do
-      expect(helper).to receive(:gravatar_icon).with('foo@example.com', 20, 2)
+      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('foo@example.com', 20, 2)
+          helper.avatar_icon('foo@example.com', 20, 2)
+        end
+      end
     end
 
-    describe 'using a User' do
-      it 'returns an URL for the avatar' do
-        user = create(:user, avatar: File.open(uploaded_image_temp_path))
+    describe 'using a user' do
+      context 'when only_path is true' do
+        it 'returns a relative URL for the avatar' do
+          expect(helper.avatar_icon(user, only_path: true).to_s).
+            to eq("/uploads/system/user/avatar/#{user.id}/banana_sample.gif")
+        end
+      end
 
-        expect(helper.avatar_icon(user).to_s).
-          to match("/uploads/system/user/avatar/#{user.id}/banana_sample.gif")
+      context 'when only_path is false' do
+        it 'returns an absolute URL for the avatar' do
+          expect(helper.avatar_icon(user, only_path: false).to_s).
+            to eq("#{gitlab_host}/uploads/system/user/avatar/#{user.id}/banana_sample.gif")
+        end
       end
     end
   end