Merge branch 'refactor-duplication' into 'master'

Remove some code duplication

* remove duplicate code in uploaders
* remove duplicate code in NotificationHelper
* remove duplicate code in Repository


Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

See merge request !1800

7 files changed, 46 insertions, 93 deletions

diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb
index cf11f8e5320..499c655d2bf 100644
--- a/app/helpers/notifications_helper.rb
+++ b/app/helpers/notifications_helper.rb
@@ -16,40 +16,28 @@ module NotificationsHelper
   def notification_list_item(notification_level, user_membership)
     case notification_level
     when Notification::N_DISABLED
-      content_tag(:li, class: active_level_for(user_membership, Notification::N_DISABLED)) do
-        link_to '#', class: 'update-notification', data: { notification_level: Notification::N_DISABLED } do
-          icon('microphone-slash fw', text: 'Disabled')
-        end
-      end
+      update_notification_link(Notification::N_DISABLED, user_membership, 'Disabled', 'microphone-slash')
     when Notification::N_PARTICIPATING
-      content_tag(:li, class: active_level_for(user_membership, Notification::N_PARTICIPATING)) do
-        link_to '#', class: 'update-notification', data: { notification_level: Notification::N_PARTICIPATING } do
-          icon('volume-up fw', text: 'Participate')
-        end
-      end
+      update_notification_link(Notification::N_PARTICIPATING, user_membership, 'Participate', 'volume-up')
     when Notification::N_WATCH
-      content_tag(:li, class: active_level_for(user_membership, Notification::N_WATCH)) do
-        link_to '#', class: 'update-notification', data: { notification_level: Notification::N_WATCH } do
-          icon('eye fw', text: 'Watch')
-        end
-      end
+      update_notification_link(Notification::N_WATCH, user_membership, 'Watch', 'eye')
     when Notification::N_MENTION
-      content_tag(:li, class: active_level_for(user_membership, Notification::N_MENTION)) do
-        link_to '#', class: 'update-notification', data: { notification_level: Notification::N_MENTION }  do
-          icon('at fw', text: 'On mention')
-        end
-      end
+      update_notification_link(Notification::N_MENTION, user_membership, 'On mention', 'at')
     when Notification::N_GLOBAL
-      content_tag(:li, class: active_level_for(user_membership, Notification::N_GLOBAL)) do
-        link_to '#', class: 'update-notification', data: { notification_level: Notification::N_GLOBAL } do
-          icon('globe fw', text: 'Global')
-        end
-      end
+      update_notification_link(Notification::N_GLOBAL, user_membership, 'Global', 'globe')
     else
       # do nothing
     end
   end
 
+  def update_notification_link(notification_level, user_membership, title, icon)
+    content_tag(:li, class: active_level_for(user_membership, notification_level)) do
+      link_to '#', class: 'update-notification', data: { notification_level: notification_level } do
+        icon("#{icon} fw", text: title)
+      end
+    end
+  end
+
   def notification_label(user_membership)
     Notification.new(user_membership).to_s
   end
diff --git a/app/models/repository.rb b/app/models/repository.rb
index f8c4cb1387b..f76b770e867 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -346,8 +346,8 @@ class Repository
     end
   end
 
-  def branch_names_contains(sha)
-    args = %W(#{Gitlab.config.git.bin_path} branch --contains #{sha})
+  def refs_contains_sha(ref_type, sha)
+    args = %W(#{Gitlab.config.git.bin_path} #{ref_type} --contains #{sha})
     names = Gitlab::Popen.popen(args, path_to_repo).first
 
     if names.respond_to?(:split)
@@ -363,21 +363,12 @@ class Repository
     end
   end
 
-  def tag_names_contains(sha)
-    args = %W(#{Gitlab.config.git.bin_path} tag --contains #{sha})
-    names = Gitlab::Popen.popen(args, path_to_repo).first
-
-    if names.respond_to?(:split)
-      names = names.split("\n").map(&:strip)
-
-      names.each do |name|
-        name.slice! '* '
-      end
+  def branch_names_contains(sha)
+    refs_contains_sha('branch', sha)
+  end
 
-      names
-    else
-      []
-    end
+  def tag_names_contains(sha)
+    refs_contains_sha('tag', sha)
   end
 
   def branches
diff --git a/app/uploaders/attachment_uploader.rb b/app/uploaders/attachment_uploader.rb
index a9691bee46e..a65a896e41e 100644
--- a/app/uploaders/attachment_uploader.rb
+++ b/app/uploaders/attachment_uploader.rb
@@ -1,26 +1,11 @@
 # encoding: utf-8
 
 class AttachmentUploader < CarrierWave::Uploader::Base
+  include UploaderHelper
+
   storage :file
 
   def store_dir
     "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
   end
-
-  def image?
-    img_ext = %w(png jpg jpeg gif bmp tiff)
-    if file.respond_to?(:extension)
-      img_ext.include?(file.extension.downcase)
-    else
-      # Not all CarrierWave storages respond to :extension
-      ext = file.path.split('.').last.downcase
-      img_ext.include?(ext)
-    end
-  rescue
-    false
-  end
-
-  def file_storage?
-    self.class.storage == CarrierWave::Storage::File
-  end
 end
diff --git a/app/uploaders/avatar_uploader.rb b/app/uploaders/avatar_uploader.rb
index 7cad044555b..6135c3ad96f 100644
--- a/app/uploaders/avatar_uploader.rb
+++ b/app/uploaders/avatar_uploader.rb
@@ -1,6 +1,8 @@
 # encoding: utf-8
 
 class AvatarUploader < CarrierWave::Uploader::Base
+  include UploaderHelper
+
   storage :file
 
   after :store, :reset_events_cache
@@ -9,23 +11,6 @@ class AvatarUploader < CarrierWave::Uploader::Base
     "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
   end
 
-  def image?
-    img_ext = %w(png jpg jpeg gif bmp tiff)
-    if file.respond_to?(:extension)
-      img_ext.include?(file.extension.downcase)
-    else
-      # Not all CarrierWave storages respond to :extension
-      ext = file.path.split('.').last.downcase
-      img_ext.include?(ext)
-    end
-  rescue
-    false
-  end
-
-  def file_storage?
-    self.class.storage == CarrierWave::Storage::File
-  end
-
   def reset_events_cache(file)
     model.reset_events_cache if model.is_a?(User)
   end
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb
index e8211585834..ac920119a85 100644
--- a/app/uploaders/file_uploader.rb
+++ b/app/uploaders/file_uploader.rb
@@ -1,5 +1,7 @@
 # encoding: utf-8
 class FileUploader < CarrierWave::Uploader::Base
+  include UploaderHelper
+
   storage :file
 
   attr_accessor :project, :secret
@@ -28,21 +30,4 @@ class FileUploader < CarrierWave::Uploader::Base
   def secure_url
     File.join("/uploads", @secret, file.filename)
   end
-
-  def file_storage?
-    self.class.storage == CarrierWave::Storage::File
-  end
-
-  def image?
-    img_ext = %w(png jpg jpeg gif bmp tiff)
-    if file.respond_to?(:extension)
-      img_ext.include?(file.extension.downcase)
-    else
-      # Not all CarrierWave storages respond to :extension
-      ext = file.path.split('.').last.downcase
-      img_ext.include?(ext)
-    end
-  rescue
-    false
-  end
 end
diff --git a/app/uploaders/uploader_helper.rb b/app/uploaders/uploader_helper.rb
new file mode 100644
index 00000000000..5ef440f3367
--- /dev/null
+++ b/app/uploaders/uploader_helper.rb
@@ -0,0 +1,19 @@
+# Extra methods for uploader
+module UploaderHelper
+  def image?
+    img_ext = %w(png jpg jpeg gif bmp tiff)
+    if file.respond_to?(:extension)
+      img_ext.include?(file.extension.downcase)
+    else
+      # Not all CarrierWave storages respond to :extension
+      ext = file.path.split('.').last.downcase
+      img_ext.include?(ext)
+    end
+  rescue
+    false
+  end
+
+  def file_storage?
+    self.class.storage == CarrierWave::Storage::File
+  end
+end
diff --git a/lib/tasks/flay.rake b/lib/tasks/flay.rake
index 5efffc2cdac..dfb9df4772a 100644
--- a/lib/tasks/flay.rake
+++ b/lib/tasks/flay.rake
@@ -1,6 +1,6 @@
 desc 'Code duplication analyze via flay'
 task :flay do
-  output = %x(bundle exec flay app/ lib/gitlab/)
+  output = %x(bundle exec flay --mass 30 app/ lib/gitlab/)
 
   if output.include? "Similar code found"
     puts output