Merge branch '38189-fix-user-avatar-url-cdn' into 'master'

Prevent URL concatenation for avatars

Closes #38189

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

app/helpers/avatars_helper.rb

@@ -13,22 +13,29 @@ module AvatarsHelper
     user_name = options[:user].try(:name) || options[:user_name]
     avatar_url = options[:url] || avatar_icon(options[:user] || options[:user_email], avatar_size)
     has_tooltip = options[:has_tooltip].nil? ? true : options[:has_tooltip]
-    data_attributes = {}
+    data_attributes = options[:data] || {}
     css_class = %W[avatar s#{avatar_size}].push(*options[:css_class])
 
     if has_tooltip
       css_class.push('has-tooltip')
-      data_attributes = { container: 'body' }
+      data_attributes[:container] = 'body'
     end
 
-    image_tag(
-      avatar_url,
-      class: css_class,
+    if options[:lazy]
+      css_class << 'lazy'
+      data_attributes[:src] = avatar_url
+      avatar_url = LazyImageTagHelper.placeholder_image
+    end
+
+    image_options = {
       alt:   "#{user_name}'s avatar",
-      title: user_name,
+      src:   avatar_url,
       data:  data_attributes,
-      lazy: true
-    )
+      class: css_class,
+      title: user_name
+    }
+
+    tag(:img, image_options)
   end
 
   def user_avatar(options = {})

app/views/shared/issuable/_user_dropdown_item.html.haml

@@ -4,7 +4,7 @@
 %li.filter-dropdown-item{ class: ('js-current-user' if user == current_user) }
   %button.btn.btn-link.dropdown-user{ type: :button }
     .avatar-container.s40
-      = user_avatar_without_link(user: user, lazy: avatar[:lazy], url: avatar[:url], size: 40, has_tooltip: false).gsub('/images/{{avatar_url}}','{{avatar_url}}').html_safe
+      = user_avatar_without_link(user: user, lazy: avatar[:lazy], url: avatar[:url], size: 40, has_tooltip: false)
     .dropdown-user-details
       %span
         = user.name

spec/helpers/avatars_helper_spec.rb

@@ -26,12 +26,13 @@ describe AvatarsHelper do
     subject { helper.user_avatar_without_link(options) }
 
     it 'displays user avatar' do
-      is_expected.to eq image_tag(
-        LazyImageTagHelper.placeholder_image,
-        class: 'avatar s16 has-tooltip  lazy',
+      is_expected.to eq tag(
+        :img,
         alt: "#{user.name}'s avatar",
-        title: user.name,
-        data: { container: 'body', src: avatar_icon(user, 16) }
+        src: avatar_icon(user, 16),
+        data: { container: 'body' },
+        class: 'avatar s16 has-tooltip',
+        title: user.name
       )
     end
 
@@ -39,12 +40,13 @@ describe AvatarsHelper do
       let(:options) { { user: user, css_class: '.cat-pics' } }
 
       it 'uses provided css_class' do
-        is_expected.to eq image_tag(
-          LazyImageTagHelper.placeholder_image,
-          class: "avatar s16 #{options[:css_class]} has-tooltip  lazy",
+        is_expected.to eq tag(
+          :img,
           alt: "#{user.name}'s avatar",
-          title: user.name,
-          data: { container: 'body', src: avatar_icon(user, 16) }
+          src: avatar_icon(user, 16),
+          data: { container: 'body' },
+          class: "avatar s16 #{options[:css_class]} has-tooltip",
+          title: user.name
         )
       end
     end
@@ -53,12 +55,13 @@ describe AvatarsHelper do
       let(:options) { { user: user, size: 99 } }
 
       it 'uses provided size' do
-        is_expected.to eq image_tag(
-          LazyImageTagHelper.placeholder_image,
-          class: "avatar s#{options[:size]} has-tooltip  lazy",
+        is_expected.to eq tag(
+          :img,
           alt: "#{user.name}'s avatar",
-          title: user.name,
-          data: { container: 'body', src: avatar_icon(user, options[:size]) }
+          src: avatar_icon(user, options[:size]),
+          data: { container: 'body' },
+          class: "avatar s#{options[:size]} has-tooltip",
+          title: user.name
         )
       end
     end
@@ -67,12 +70,28 @@ describe AvatarsHelper do
       let(:options) { { user: user, url: '/over/the/rainbow.png' } }
 
       it 'uses provided url' do
-        is_expected.to eq image_tag(
-          LazyImageTagHelper.placeholder_image,
-          class: 'avatar s16 has-tooltip  lazy',
+        is_expected.to eq tag(
+          :img,
           alt: "#{user.name}'s avatar",
-          title: user.name,
-          data: { container: 'body', src: options[:url] }
+          src: options[:url],
+          data: { container: 'body' },
+          class: "avatar s16 has-tooltip",
+          title: user.name
+        )
+      end
+    end
+
+    context 'with lazy parameter' do
+      let(:options) { { user: user, lazy: true } }
+
+      it 'adds `lazy` class to class list, sets `data-src` with avatar URL and `src` with placeholder image' do
+        is_expected.to eq tag(
+          :img,
+          alt: "#{user.name}'s avatar",
+          src: LazyImageTagHelper.placeholder_image,
+          data: { container: 'body', src: avatar_icon(user, 16) },
+          class: "avatar s16 has-tooltip lazy",
+          title: user.name
         )
       end
     end
@@ -82,12 +101,13 @@ describe AvatarsHelper do
         let(:options) { { user: user, has_tooltip: true } }
 
         it 'adds has-tooltip' do
-          is_expected.to eq image_tag(
-            LazyImageTagHelper.placeholder_image,
-            class: 'avatar s16 has-tooltip  lazy',
+          is_expected.to eq tag(
+            :img,
             alt: "#{user.name}'s avatar",
-            title: user.name,
-            data: { container: 'body', src: avatar_icon(user, 16) }
+            src: avatar_icon(user, 16),
+            data: { container: 'body' },
+            class: "avatar s16 has-tooltip",
+            title: user.name
           )
         end
       end
@@ -96,12 +116,12 @@ describe AvatarsHelper do
         let(:options) { { user: user, has_tooltip: false } }
 
         it 'does not add has-tooltip or data container' do
-          is_expected.to eq image_tag(
-            LazyImageTagHelper.placeholder_image,
-            class: 'avatar s16  lazy',
+          is_expected.to eq tag(
+            :img,
             alt: "#{user.name}'s avatar",
-            title: user.name,
-            data: { src: avatar_icon(user, 16) }
+            src: avatar_icon(user, 16),
+            class: "avatar s16",
+            title: user.name
           )
         end
       end
@@ -114,23 +134,25 @@ describe AvatarsHelper do
         let(:options) { { user: user, user_name: 'Tinky Winky' } }
 
         it 'prefers user parameter' do
-          is_expected.to eq image_tag(
-            LazyImageTagHelper.placeholder_image,
-            class: 'avatar s16 has-tooltip  lazy',
+          is_expected.to eq tag(
+            :img,
             alt: "#{user.name}'s avatar",
-            title: user.name,
-            data: { container: 'body', src: avatar_icon(user, 16) }
+            src: avatar_icon(user, 16),
+            data: { container: 'body' },
+            class: "avatar s16 has-tooltip",
+            title: user.name
           )
         end
       end
 
       it 'uses user_name and user_email parameter if user is not present' do
-        is_expected.to eq image_tag(
-          LazyImageTagHelper.placeholder_image,
-          class: 'avatar s16 has-tooltip  lazy',
+        is_expected.to eq tag(
+          :img,
           alt: "#{options[:user_name]}'s avatar",
-          title: options[:user_name],
-          data: { container: 'body', src: avatar_icon(options[:user_email], 16) }
+          src: avatar_icon(options[:user_email], 16),
+          data: { container: 'body' },
+          class: "avatar s16 has-tooltip",
+          title: options[:user_name]
         )
       end
     end