Merge branch 'refactor-complex-methods' into 'master'

Refactor complex methods

Make flog part of CI check which is not allowed to fail. I used high score (70) and refactored most complex method. In future releases we should lower acceptable score to something like 40..50

Part of #3444 

See merge request !1794

.gitlab-ci.yml

@@ -80,7 +80,6 @@ flog:
   tags:
     - ruby
     - mysql
-  allow_failure: true
 
 flay:
   script:

app/helpers/events_helper.rb

@@ -108,6 +108,11 @@ module EventsHelper
         end
       end
     elsif event.push?
+      push_event_feed_url(event)
+    end
+  end
+
+  def push_event_feed_url(event)
     if event.push_with_commits? && event.md_ref?
       if event.commits_count > 1
         namespace_project_compare_url(event.project.namespace, event.project,
@@ -122,7 +127,6 @@ module EventsHelper
                                     event.ref_name)
     end
   end
-  end
 
   def event_feed_summary(event)
     if event.issue?

app/services/issues/update_service.rb

@@ -22,6 +22,15 @@ module Issues
             issue, issue.labels - old_labels, old_labels - issue.labels)
         end
 
+        handle_changes(issue)
+        issue.create_new_cross_references!(current_user)
+        execute_hooks(issue, 'update')
+      end
+
+      issue
+    end
+
+    def handle_changes(issue)
       if issue.previous_changes.include?('milestone_id')
         create_milestone_note(issue)
       end
@@ -34,12 +43,6 @@ module Issues
       if issue.previous_changes.include?('title')
         create_title_change_note(issue, issue.previous_changes['title'].first)
       end
-
-        issue.create_new_cross_references!(current_user)
-        execute_hooks(issue, 'update')
-      end
-
-      issue
     end
   end
 end

app/services/merge_requests/update_service.rb

@@ -35,6 +35,15 @@ module MergeRequests
           )
         end
 
+        handle_changes(merge_request)
+        merge_request.create_new_cross_references!(current_user)
+        execute_hooks(merge_request, 'update')
+      end
+
+      merge_request
+    end
+
+    def handle_changes(merge_request)
       if merge_request.previous_changes.include?('target_branch')
         create_branch_change_note(merge_request, 'target',
                                   merge_request.previous_changes['target_branch'].first,
@@ -58,12 +67,6 @@ module MergeRequests
           merge_request.previous_changes.include?('source_branch')
         merge_request.mark_as_unchecked
       end
-
-        merge_request.create_new_cross_references!(current_user)
-        execute_hooks(merge_request, 'update')
-      end
-
-      merge_request
     end
   end
 end

app/services/system_hooks_service.rb

@@ -33,17 +33,7 @@ class SystemHooksService
         )
       end
     when Project
-      owner = model.owner
-
-      data.merge!({
-        name: model.name,
-        path: model.path,
-        path_with_namespace: model.path_with_namespace,
-        project_id: model.id,
-        owner_name: owner.name,
-        owner_email: owner.respond_to?(:email) ?  owner.email : "",
-        project_visibility: Project.visibility_levels.key(model.visibility_level_field).downcase
-      })
+      data.merge!(project_data(model))
     when User
       data.merge!({
         name: model.name,
@@ -51,16 +41,7 @@ class SystemHooksService
         user_id: model.id
       })
     when ProjectMember
-      data.merge!({
-        project_name: model.project.name,
-        project_path: model.project.path,
-        project_path_with_namespace: model.project.path_with_namespace,
-        project_id: model.project.id,
-        user_name: model.user.name,
-        user_email: model.user.email,
-        access_level: model.human_access,
-        project_visibility: Project.visibility_levels.key(model.project.visibility_level_field).downcase
-      })
+      data.merge!(project_member_data(model))
     when Group
       owner = model.owner
 
@@ -72,15 +53,7 @@ class SystemHooksService
         owner_email: owner.respond_to?(:email) ? owner.email : nil,
       )
     when GroupMember
-      data.merge!(
-        group_name: model.group.name,
-        group_path: model.group.path,
-        group_id: model.group.id,
-        user_name: model.user.name,
-        user_email: model.user.email,
-        user_id: model.user.id,
-        group_access: model.human_access,
-      )
+      data.merge!(group_member_data(model))
     end
   end
 
@@ -96,4 +69,43 @@ class SystemHooksService
       "#{model.class.name.downcase}_#{event.to_s}"
     end
   end
+
+  def project_data(model)
+    owner = model.owner
+
+    {
+      name: model.name,
+      path: model.path,
+      path_with_namespace: model.path_with_namespace,
+      project_id: model.id,
+      owner_name: owner.name,
+      owner_email: owner.respond_to?(:email) ?  owner.email : "",
+      project_visibility: Project.visibility_levels.key(model.visibility_level_field).downcase
+    }
+  end
+
+  def project_member_data(model)
+    {
+      project_name: model.project.name,
+      project_path: model.project.path,
+      project_path_with_namespace: model.project.path_with_namespace,
+      project_id: model.project.id,
+      user_name: model.user.name,
+      user_email: model.user.email,
+      access_level: model.human_access,
+      project_visibility: Project.visibility_levels.key(model.project.visibility_level_field).downcase
+    }
+  end
+
+  def group_member_data(model)
+    {
+      group_name: model.group.name,
+      group_path: model.group.path,
+      group_id: model.group.id,
+      user_name: model.user.name,
+      user_email: model.user.email,
+      user_id: model.user.id,
+      group_access: model.human_access,
+    }
+  end
 end

lib/gitlab/google_code_import/importer.rb

@@ -76,18 +76,7 @@ module Gitlab
           attachments = format_attachments(raw_issue["id"], 0, issue_comment["attachments"])
 
           body = format_issue_body(author, date, content, attachments)
-
-          labels = []
-          raw_issue["labels"].each do |label|
-            name = nice_label_name(label)
-            labels << name
-
-            unless @known_labels.include?(name)
-              create_label(name)
-              @known_labels << name
-            end
-          end
-          labels << nice_status_name(raw_issue["status"])
+          labels = import_issue_labels(raw_issue)
 
           assignee_id = nil
           if raw_issue.has_key?("owner")
@@ -110,6 +99,7 @@ module Gitlab
             assignee_id:  assignee_id,
             state:        raw_issue["state"] == "closed" ? "closed" : "opened"
           )
+
           issue.add_labels_by_names(labels)
 
           if issue.iid != raw_issue["id"]
@@ -120,6 +110,23 @@ module Gitlab
         end
       end
 
+      def import_issue_labels(raw_issue)
+        labels = []
+
+        raw_issue["labels"].each do |label|
+          name = nice_label_name(label)
+          labels << name
+
+          unless @known_labels.include?(name)
+            create_label(name)
+            @known_labels << name
+          end
+        end
+
+        labels << nice_status_name(raw_issue["status"])
+        labels
+      end
+
       def import_issue_comments(issue, comments)
         Note.transaction do
           while raw_comment = comments.shift
@@ -278,25 +285,24 @@ module Gitlab
 
         if raw_updates.has_key?("blockedOn")
           blocked_ons = raw_updates["blockedOn"].map do |raw_blocked_on|
-            name, id = raw_blocked_on.split(":", 2)
-
-            deleted = name.start_with?("-") 
-            name = name[1..-1] if deleted
-
-            text =
-              if name == project.import_source
-                "##{id}"
-              else
-                "#{project.namespace.path}/#{name}##{id}"
-              end
-            text = "~~#{text}~~" if deleted
-            text
+            format_blocking_updates(raw_blocked_on)
           end
+
           updates << "*Blocked on: #{blocked_ons.join(", ")}*"
         end
 
         if raw_updates.has_key?("blocking")
           blockings = raw_updates["blocking"].map do |raw_blocked_on|
+            format_blocking_updates(raw_blocked_on)
+          end
+
+          updates << "*Blocking: #{blockings.join(", ")}*"
+        end
+
+        updates
+      end
+
+      def format_blocking_updates(raw_blocked_on)
         name, id = raw_blocked_on.split(":", 2)
 
         deleted = name.start_with?("-")
@@ -311,11 +317,6 @@ module Gitlab
         text = "~~#{text}~~" if deleted
         text
       end
-          updates << "*Blocking: #{blockings.join(", ")}*"
-        end
-
-        updates
-      end
 
       def format_attachments(issue_id, comment_id, raw_attachments)
         return [] unless raw_attachments

lib/gitlab/inline_diff.rb

@@ -11,20 +11,21 @@ module Gitlab
         indexes.each do |index|
           first_line = diff_arr[index+1]
           second_line = diff_arr[index+2]
-          max_length = [first_line.size, second_line.size].max
 
           # Skip inline diff if empty line was replaced with content
           next if first_line == "-\n"
 
-          first_the_same_symbols = 0
-          (0..max_length + 1).each do |i|
-            first_the_same_symbols = i - 1
-            if first_line[i] != second_line[i] && i > 0
-              break
+          first_token = find_first_token(first_line, second_line)
+          apply_first_token(diff_arr, index, first_token)
+
+          last_token = find_last_token(first_line, second_line, first_token)
+          apply_last_token(diff_arr, index, last_token)
         end
+
+        diff_arr
       end
 
-          first_token = first_line[0..first_the_same_symbols][1..-1]
+      def apply_first_token(diff_arr, index, first_token)
         start = first_token + START
 
         if first_token.empty?
@@ -35,24 +36,46 @@ module Gitlab
           diff_arr[index+1].sub!(first_token, first_token => start)
           diff_arr[index+2].sub!(first_token, first_token => start)
         end
+      end
 
+      def apply_last_token(diff_arr, index, last_token)
+        # This is tricky: escape backslashes so that `sub` doesn't interpret them
+        # as backreferences. Regexp.escape does NOT do the right thing.
+        replace_token = FINISH + last_token.gsub(/\\/, '\&\&')
+        diff_arr[index+1].sub!(/#{Regexp.escape(last_token)}$/, replace_token)
+        diff_arr[index+2].sub!(/#{Regexp.escape(last_token)}$/, replace_token)
+      end
+
+      def find_first_token(first_line, second_line)
+        max_length = [first_line.size, second_line.size].max
+        first_the_same_symbols = 0
+
+        (0..max_length + 1).each do |i|
+          first_the_same_symbols = i - 1
+
+          if first_line[i] != second_line[i] && i > 0
+            break
+          end
+        end
+
+        first_line[0..first_the_same_symbols][1..-1]
+      end
+
+      def find_last_token(first_line, second_line, first_token)
+        max_length = [first_line.size, second_line.size].max
         last_the_same_symbols = 0
+
         (1..max_length + 1).each do |i|
           last_the_same_symbols = -i
           shortest_line = second_line.size > first_line.size ? first_line : second_line
-            if ( first_line[-i] != second_line[-i] ) || "#{first_token}#{START}".size == shortest_line[1..-i].size
+
+          if (first_line[-i] != second_line[-i]) || "#{first_token}#{START}".size == shortest_line[1..-i].size
             break
           end
         end
+
         last_the_same_symbols += 1
-          last_token = first_line[last_the_same_symbols..-1]
-          # This is tricky: escape backslashes so that `sub` doesn't interpret them
-          # as backreferences. Regexp.escape does NOT do the right thing.
-          replace_token = FINISH + last_token.gsub(/\\/, '\&\&')
-          diff_arr[index+1].sub!(/#{Regexp.escape(last_token)}$/, replace_token)
-          diff_arr[index+2].sub!(/#{Regexp.escape(last_token)}$/, replace_token)
-        end
-        diff_arr
+        first_line[last_the_same_symbols..-1]
       end
 
       def _indexes_of_changed_lines(diff_arr)