Merge branch 'group-icon-performance' into 'master'

Prevent database query each time we render group avatar

Fixes #2118

See merge request !1688

10 files changed, 39 insertions, 33 deletions

diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index a81e41819b7..8ed6d59c20d 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -86,15 +86,6 @@ module ApplicationHelper
     end
   end
 
-  def group_icon(group_path)
-    group = Group.find_by(path: group_path)
-    if group && group.avatar.present?
-      group.avatar.url
-    else
-      image_path('no_group_avatar.png')
-    end
-  end
-
   def avatar_icon(user_email = '', size = nil)
     user = User.find_by(email: user_email)
 
diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb
index 03fd461a462..2d0d0b494f6 100644
--- a/app/helpers/groups_helper.rb
+++ b/app/helpers/groups_helper.rb
@@ -40,4 +40,16 @@ module GroupsHelper
       false
     end
   end
+
+  def group_icon(group)
+    if group.is_a?(String)
+      group = Group.find_by(path: group)
+    end
+
+    if group && group.avatar.present?
+      group.avatar.url
+    else
+      image_path('no_group_avatar.png')
+    end
+  end
 end
diff --git a/app/helpers/namespaces_helper.rb b/app/helpers/namespaces_helper.rb
index 2bcfde62830..b3132a1f3ba 100644
--- a/app/helpers/namespaces_helper.rb
+++ b/app/helpers/namespaces_helper.rb
@@ -28,7 +28,7 @@ module NamespacesHelper
 
   def namespace_icon(namespace, size = 40)
     if namespace.kind_of?(Group)
-      group_icon(namespace.path)
+      group_icon(namespace)
     else
       avatar_icon(namespace.owner.email, size)
     end
diff --git a/app/views/admin/groups/show.html.haml b/app/views/admin/groups/show.html.haml
index bb7f1972925..3040faa722b 100644
--- a/app/views/admin/groups/show.html.haml
+++ b/app/views/admin/groups/show.html.haml
@@ -12,7 +12,7 @@
         Group info:
       %ul.well-list
         %li
-          = image_tag group_icon(@group.path), class: "avatar s60"
+          = image_tag group_icon(@group), class: "avatar s60"
         %li
           %span.light Name:
           %strong= @group.name
diff --git a/app/views/dashboard/groups/index.html.haml b/app/views/dashboard/groups/index.html.haml
index 50e90b1c170..f7df5352512 100644
--- a/app/views/dashboard/groups/index.html.haml
+++ b/app/views/dashboard/groups/index.html.haml
@@ -27,7 +27,7 @@
               %i.fa.fa-sign-out
               Leave
 
-        = image_tag group_icon(group.path), class: "avatar s40 avatar-tile"
+        = image_tag group_icon(group), class: "avatar s40 avatar-tile"
         = link_to group, class: 'group-name' do
           %strong= group.name
 
diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml
index c4eb00e8925..838290e4aca 100644
--- a/app/views/groups/edit.html.haml
+++ b/app/views/groups/edit.html.haml
@@ -12,7 +12,7 @@
       .form-group
         .col-sm-2
         .col-sm-10
-          = image_tag group_icon(@group.to_param), alt: '', class: 'avatar group-avatar s160'
+          = image_tag group_icon(@group), alt: '', class: 'avatar group-avatar s160'
           %p.light
             - if @group.avatar?
               You can change your group avatar here
diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml
index a453889f744..25efe973d4f 100644
--- a/app/views/groups/show.html.haml
+++ b/app/views/groups/show.html.haml
@@ -1,6 +1,6 @@
 .dashboard
   %div
-    = image_tag group_icon(@group.path), class: "avatar group-avatar s90"
+    = image_tag group_icon(@group), class: "avatar group-avatar s90"
     .clearfix
       %h2
         = @group.name
diff --git a/app/views/users/_groups.html.haml b/app/views/users/_groups.html.haml
index cb84570a6d5..f360fbb3d5d 100644
--- a/app/views/users/_groups.html.haml
+++ b/app/views/users/_groups.html.haml
@@ -1,4 +1,4 @@
 .clearfix
   - groups.each do |group|
     = link_to group, class: 'profile-groups-avatars inline', title: group.name do
-      = image_tag group_icon(group.path), class: 'avatar group-avatar s40'
+      = image_tag group_icon(group), class: 'avatar group-avatar s40'
diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb
index 99ff8a32ea5..4c11709ed6e 100644
--- a/spec/helpers/application_helper_spec.rb
+++ b/spec/helpers/application_helper_spec.rb
@@ -39,24 +39,6 @@ describe ApplicationHelper do
     end
   end
 
-  describe 'group_icon' do
-    avatar_file_path = File.join(Rails.root, 'public', 'gitlab_logo.png')
-
-    it 'should return an url for the avatar' do
-      group = create(:group)
-      group.avatar = File.open(avatar_file_path)
-      group.save!
-      expect(group_icon(group.path).to_s).
-        to match("/uploads/group/avatar/#{ group.id }/gitlab_logo.png")
-    end
-
-    it 'should give default avatar_icon when no avatar is present' do
-      group = create(:group)
-      group.save!
-      expect(group_icon(group.path)).to match('group_avatar.png')
-    end
-  end
-
   describe 'project_icon' do
     avatar_file_path = File.join(Rails.root, 'public', 'gitlab_logo.png')
 
diff --git a/spec/helpers/groups_helper.rb b/spec/helpers/groups_helper.rb
new file mode 100644
index 00000000000..3e99ab84ec9
--- /dev/null
+++ b/spec/helpers/groups_helper.rb
@@ -0,0 +1,21 @@
+require 'spec_helper'
+
+describe GroupsHelper do
+  describe 'group_icon' do
+    avatar_file_path = File.join(Rails.root, 'public', 'gitlab_logo.png')
+
+    it 'should return an url for the avatar' do
+      group = create(:group)
+      group.avatar = File.open(avatar_file_path)
+      group.save!
+      expect(group_icon(group.path).to_s).
+        to match("/uploads/group/avatar/#{ group.id }/gitlab_logo.png")
+    end
+
+    it 'should give default avatar_icon when no avatar is present' do
+      group = create(:group)
+      group.save!
+      expect(group_icon(group.path)).to match('group_avatar.png')
+    end
+  end
+end