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

3 files changed, 83 insertions, 65 deletions

diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index 551325e4cab..aa1fd79d22d 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -22,24 +22,27 @@ module Issues
             issue, issue.labels - old_labels, old_labels - issue.labels)
         end
 
-        if issue.previous_changes.include?('milestone_id')
-          create_milestone_note(issue)
-        end
-
-        if issue.previous_changes.include?('assignee_id')
-          create_assignee_note(issue)
-          notification_service.reassigned_issue(issue, current_user)
-        end
-
-        if issue.previous_changes.include?('title')
-          create_title_change_note(issue, issue.previous_changes['title'].first)
-        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
+
+      if issue.previous_changes.include?('assignee_id')
+        create_assignee_note(issue)
+        notification_service.reassigned_issue(issue, current_user)
+      end
+
+      if issue.previous_changes.include?('title')
+        create_title_change_note(issue, issue.previous_changes['title'].first)
+      end
+    end
   end
 end
diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb
index 61f7d2bbe89..d2849e5193f 100644
--- a/app/services/merge_requests/update_service.rb
+++ b/app/services/merge_requests/update_service.rb
@@ -35,35 +35,38 @@ module MergeRequests
           )
         end
 
-        if merge_request.previous_changes.include?('target_branch')
-          create_branch_change_note(merge_request, 'target',
-                                    merge_request.previous_changes['target_branch'].first,
-                                    merge_request.target_branch)
-        end
+        handle_changes(merge_request)
+        merge_request.create_new_cross_references!(current_user)
+        execute_hooks(merge_request, 'update')
+      end
 
-        if merge_request.previous_changes.include?('milestone_id')
-          create_milestone_note(merge_request)
-        end
+      merge_request
+    end
 
-        if merge_request.previous_changes.include?('assignee_id')
-          create_assignee_note(merge_request)
-          notification_service.reassigned_merge_request(merge_request, current_user)
-        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,
+                                  merge_request.target_branch)
+      end
 
-        if merge_request.previous_changes.include?('title')
-          create_title_change_note(merge_request, merge_request.previous_changes['title'].first)
-        end
+      if merge_request.previous_changes.include?('milestone_id')
+        create_milestone_note(merge_request)
+      end
 
-        if merge_request.previous_changes.include?('target_branch') ||
-            merge_request.previous_changes.include?('source_branch')
-          merge_request.mark_as_unchecked
-        end
+      if merge_request.previous_changes.include?('assignee_id')
+        create_assignee_note(merge_request)
+        notification_service.reassigned_merge_request(merge_request, current_user)
+      end
 
-        merge_request.create_new_cross_references!(current_user)
-        execute_hooks(merge_request, 'update')
+      if merge_request.previous_changes.include?('title')
+        create_title_change_note(merge_request, merge_request.previous_changes['title'].first)
       end
 
-      merge_request
+      if merge_request.previous_changes.include?('target_branch') ||
+          merge_request.previous_changes.include?('source_branch')
+        merge_request.mark_as_unchecked
+      end
     end
   end
 end
diff --git a/app/services/system_hooks_service.rb b/app/services/system_hooks_service.rb
index 9a5fe4af9dd..8b5143e1eb7 100644
--- a/app/services/system_hooks_service.rb
+++ b/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