Move services for collecting items to Finders

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

app/controllers/dashboard_controller.rb

@@ -53,13 +53,13 @@ class DashboardController < ApplicationController
   end
 
   def merge_requests
-    @merge_requests = FilteringService.new.execute(MergeRequest, current_user, params)
+    @merge_requests = MergeRequestsFinder.new.execute(current_user, params)
     @merge_requests = @merge_requests.page(params[:page]).per(20)
     @merge_requests = @merge_requests.preload(:author, :target_project)
   end
 
   def issues
-    @issues = FilteringService.new.execute(Issue, current_user, params)
+    @issues = IssuesFinder.new.execute(current_user, params)
     @issues = @issues.page(params[:page]).per(20)
     @issues = @issues.preload(:author, :project)
 

app/controllers/groups_controller.rb

@@ -47,13 +47,13 @@ class GroupsController < ApplicationController
   end
 
   def merge_requests
-    @merge_requests = FilteringService.new.execute(MergeRequest, current_user, params)
+    @merge_requests = MergeRequestsFinder.new.execute(current_user, params)
     @merge_requests = @merge_requests.page(params[:page]).per(20)
     @merge_requests = @merge_requests.preload(:author, :target_project)
   end
 
   def issues
-    @issues = FilteringService.new.execute(Issue, current_user, params)
+    @issues = IssuesFinder.new.execute(current_user, params)
     @issues = @issues.page(params[:page]).per(20)
     @issues = @issues.preload(:author, :project)
 
@@ -100,7 +100,7 @@ class GroupsController < ApplicationController
   end
 
   def projects
-    @projects ||= Projects::CollectService.new.execute(current_user, group: group)
+    @projects ||= ProjectsFinder.new.execute(current_user, group: group)
   end
 
   def project_ids

app/controllers/projects/issues_controller.rb

@@ -121,7 +121,7 @@ class Projects::IssuesController < Projects::ApplicationController
   def issues_filtered
     params[:scope] = 'all' if params[:scope].blank?
     params[:state] = 'opened' if params[:state].blank?
-    @issues = FilteringService.new.execute(Issue, current_user, params.merge(project_id: @project.id))
+    @issues = IssuesFinder.new.execute(current_user, params.merge(project_id: @project.id))
   end
 
   # Since iids are implemented only in 6.1

app/controllers/projects/merge_requests_controller.rb

@@ -21,7 +21,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     params[:scope] = 'all' if params[:scope].blank?
     params[:state] = 'opened' if params[:state].blank?
 
-    @merge_requests = FilteringService.new.execute(MergeRequest, current_user, params.merge(project_id: @project.id))
+    @merge_requests = MergeRequestsFinder.new.execute(current_user, params.merge(project_id: @project.id))
     @merge_requests = @merge_requests.page(params[:page]).per(20)
 
     @sort = params[:sort].humanize

app/controllers/projects/notes_controller.rb

@@ -5,7 +5,7 @@ class Projects::NotesController < Projects::ApplicationController
   before_filter :authorize_admin_note!, only: [:update, :destroy]
 
   def index
-    @notes = Notes::LoadService.new(project, current_user, params).execute
+    @notes = NotesFinder.new.execute(project, current_user, params)
 
     notes_json = { notes: [] }
 

app/finders/README.md

+# Finders
+
+This type of classes responsible for collectiong items based on different conditions.
+To prevent lookup methods in models like this: 
+
+```
+class Project
+  def issues_for_user_filtered_by(user, filter)
+    # A lot of logic not related to project model itself
+  end
+end
+
+issues = project.issues_for_user_filtered_by(user, params)
+```
+
+Better use this: 
+
+```
+selector = Finders::Issues.new
+
+issues = selector.execute(project, user, filter)
+```
+
+It will help keep models thiner

app/services/filtering_service.rb → app/finders/base_finder.rb

-# FilteringService class
+# BaseFinder
 #
 # Used to filter Issues and MergeRequests collections by set of params
 #
@@ -16,11 +16,10 @@
 #     label_name: string
 #     sort: string
 #
-class FilteringService
-  attr_accessor :klass, :current_user, :params
+class BaseFinder
+  attr_accessor :current_user, :params
 
-  def execute(klass, current_user, params)
-    @klass = klass
+  def execute(current_user, params)
     @current_user = current_user
     @params = params
 

app/finders/issues_finder.rb

+# Finders::Issues class
+#
+# Used to filter Issues collections by set of params
+#
+# Arguments:
+#   current_user - which user use
+#   params:
+#     scope: 'created-by-me' or 'assigned-to-me' or 'all'
+#     state: 'open' or 'closed' or 'all'
+#     group_id: integer
+#     project_id: integer
+#     milestone_id: integer
+#     assignee_id: integer
+#     search: string
+#     label_name: string
+#     sort: string
+#
+class IssuesFinder < BaseFinder
+  def klass
+    Issue
+  end
+end

app/finders/merge_requests_finder.rb

+# Finders::MergeRequest class
+#
+# Used to filter MergeRequests collections by set of params
+#
+# Arguments:
+#   current_user - which user use
+#   params:
+#     scope: 'created-by-me' or 'assigned-to-me' or 'all'
+#     state: 'open' or 'closed' or 'all'
+#     group_id: integer
+#     project_id: integer
+#     milestone_id: integer
+#     assignee_id: integer
+#     search: string
+#     label_name: string
+#     sort: string
+#
+class MergeRequestsFinder < BaseFinder
+  def klass
+    MergeRequest
+  end
+end

app/finders/notes_finder.rb

+class NotesFinder
+  def execute(project, current_user, params)
+    target_type = params[:target_type]
+    target_id   = params[:target_id]
+
+    case target_type
+    when "commit"
+      project.notes.for_commit_id(target_id).not_inline.fresh
+    when "issue"
+      project.issues.find(target_id).notes.inc_author.fresh
+    when "merge_request"
+      project.merge_requests.find(target_id).mr_and_commit_notes.inc_author.fresh
+    when "snippet"
+      project.snippets.find(target_id).notes.fresh
+    end
+  end
+end

app/finders/projects_finder.rb

+class ProjectsFinder
+  def execute(current_user, options)
+    group = options[:group]
+
+    if group
+      group_projects(current_user, group)
+    else
+      all_projects(current_user)
+    end
+  end
+
+  private
+
+  def group_projects(current_user, group)
+    if current_user
+      if group.users.include?(current_user)
+        # User is group member
+        #
+        # Return ALL group projects
+        group.projects
+      else
+        projects_members = UsersProject.where(
+          project_id: group.projects,
+          user_id: current_user
+        )
+
+        if projects_members.any?
+          # User is a project member
+          #
+          # Return only:
+          #   public projects
+          #   internal projects
+          #   joined projects
+          #
+          group.projects.where(
+            "projects.id IN (?) OR projects.visibility_level IN (?)",
+            projects_members.pluck(:project_id),
+            Project.public_and_internal_levels
+          )
+        else
+          # User has no access to group or group projects
+          #
+          # Return only:
+          #   public projects
+          #   internal projects
+          #
+          group.projects.public_and_internal_only
+        end
+      end
+    else
+      # Not authenticated
+      #
+      # Return only:
+      #   public projects
+      group.projects.public_only
+    end
+  end
+
+  def all_projects
+    # TODO: implement
+    raise 'Not implemented yet'
+  end
+end

app/models/ability.rb

@@ -184,7 +184,7 @@ class Ability
     def group_abilities user, group
       rules = []
 
-      if user.admin? || group.users.include?(user) || Projects::CollectService.new.execute(user, group: group).any?
+      if user.admin? || group.users.include?(user) || ProjectsFinder.new.execute(user, group: group).any?
         rules << :read_group
       end
 

app/services/notes/load_service.rb

-module Notes
-  class LoadService < BaseService
-    def execute
-      target_type = params[:target_type]
-      target_id   = params[:target_id]
-
-
-      @notes = case target_type
-               when "commit"
-                 project.notes.for_commit_id(target_id).not_inline.fresh
-               when "issue"
-                 project.issues.find(target_id).notes.inc_author.fresh
-               when "merge_request"
-                 project.merge_requests.find(target_id).mr_and_commit_notes.inc_author.fresh
-               when "snippet"
-                 project.snippets.find(target_id).notes.fresh
-               end
-    end
-  end
-end

app/services/projects/collect_service.rb

-# Projects::CollectService class
-#
-# Used to collect projects user has access to
-#
-module Projects
-  class CollectService
-    def execute(current_user, options)
-      group = options[:group]
-
-      if group
-        group_projects(current_user, group)
-      else
-        all_projects(current_user)
-      end
-    end
-
-    private
-
-    def group_projects(current_user, group)
-      if current_user
-        if group.users.include?(current_user)
-          # User is group member
-          #
-          # Return ALL group projects
-          group.projects
-        else
-          projects_members = UsersProject.where(
-            project_id: group.projects,
-            user_id: current_user
-          )
-
-          if projects_members.any?
-            # User is a project member
-            #
-            # Return only:
-            #   public projects
-            #   internal projects
-            #   joined projects
-            #
-            group.projects.where(
-              "projects.id IN (?) OR projects.visibility_level IN (?)",
-              projects_members.pluck(:project_id),
-              Project.public_and_internal_levels
-            )
-          else
-            # User has no access to group or group projects
-            #
-            # Return only:
-            #   public projects
-            #   internal projects
-            #
-            group.projects.public_and_internal_only
-          end
-        end
-      else
-        # Not authenticated
-        #
-        # Return only:
-        #   public projects
-        group.projects.public_only
-      end
-    end
-  end
-
-  def all_projects
-    # TODO: implement
-    raise 'Not implemented yet'
-  end
-end

config/application.rb

@@ -12,7 +12,7 @@ module Gitlab
     # -- all .rb files in that directory are automatically loaded.
 
     # Custom directories with classes and modules you want to be autoloadable.
-    config.autoload_paths += %W(#{config.root}/lib #{config.root}/app/models/concerns #{config.root}/app/models/project_services)
+    config.autoload_paths += %W(#{config.root}/lib #{config.root}/app/finders #{config.root}/app/models/concerns #{config.root}/app/models/project_services)
 
     # Only load the plugins named here, in the order given (default is alphabetical).
     # :all can be used as a placeholder for all plugins not explicitly named.
@@ -64,7 +64,7 @@ module Gitlab
     config.assets.enabled = true
     config.assets.paths << Emoji.images_path
     config.assets.precompile << "emoji/*.png"
-    
+
     # Version of your assets, change this if you want to expire all your assets
     config.assets.version = '1.0'