From 645e8d470559b07a22164c55b76195a60fb8b37b Mon Sep 17 00:00:00 2001
From: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Date: Tue, 25 Feb 2014 19:15:08 +0200
Subject: [PATCH] Move services for collecting items to Finders

---
 app/controllers/dashboard_controller.rb               |   4 ++--
 app/controllers/groups_controller.rb                  |   6 +++---
 app/controllers/projects/issues_controller.rb         |   2 +-
 app/controllers/projects/merge_requests_controller.rb |   2 +-
 app/controllers/projects/notes_controller.rb          |   2 +-
 app/finders/README.md                                 |  24 ++++++++++++++++++++++++
 app/finders/base_finder.rb                            | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 app/finders/issues_finder.rb                          |  22 ++++++++++++++++++++++
 app/finders/merge_requests_finder.rb                  |  22 ++++++++++++++++++++++
 app/finders/notes_finder.rb                           |  17 +++++++++++++++++
 app/finders/projects_finder.rb                        |  63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 app/models/ability.rb                                 |   2 +-
 app/services/filtering_service.rb                     | 138 ------------------------------------------------------------------------------------------------------------------------------------------
 app/services/notes/load_service.rb                    |  20 --------------------
 app/services/projects/collect_service.rb              |  69 ---------------------------------------------------------------------
 config/application.rb                                 |   4 ++--
 16 files changed, 296 insertions(+), 238 deletions(-)
 create mode 100644 app/finders/README.md
 create mode 100644 app/finders/base_finder.rb
 create mode 100644 app/finders/issues_finder.rb
 create mode 100644 app/finders/merge_requests_finder.rb
 create mode 100644 app/finders/notes_finder.rb
 create mode 100644 app/finders/projects_finder.rb
 delete mode 100644 app/services/filtering_service.rb
 delete mode 100644 app/services/notes/load_service.rb
 delete mode 100644 app/services/projects/collect_service.rb

diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb
index e7edaed..a74e97a 100644
--- a/app/controllers/dashboard_controller.rb
+++ b/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)
 
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index a9dce2f..ebddf36 100644
--- a/app/controllers/groups_controller.rb
+++ b/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
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index dd11948..9d97c82 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/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
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 2b410c5..6f7ea9c 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/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
diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb
index 9c9c2de..85d042a 100644
--- a/app/controllers/projects/notes_controller.rb
+++ b/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: [] }
 
diff --git a/app/finders/README.md b/app/finders/README.md
new file mode 100644
index 0000000..6fbf0e2
--- /dev/null
+++ b/app/finders/README.md
@@ -0,0 +1,24 @@
+# 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
diff --git a/app/finders/base_finder.rb b/app/finders/base_finder.rb
new file mode 100644
index 0000000..d20716f
--- /dev/null
+++ b/app/finders/base_finder.rb
@@ -0,0 +1,137 @@
+# BaseFinder
+#
+# Used to filter Issues and MergeRequests collections by set of params
+#
+# Arguments:
+#   klass - actual class like Issue or MergeRequest
+#   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 BaseFinder
+  attr_accessor :current_user, :params
+
+  def execute(current_user, params)
+    @current_user = current_user
+    @params = params
+
+    items = init_collection
+    items = by_scope(items)
+    items = by_state(items)
+    items = by_group(items)
+    items = by_project(items)
+    items = by_search(items)
+    items = by_milestone(items)
+    items = by_assignee(items)
+    items = by_label(items)
+    items = sort(items)
+  end
+
+  private
+
+  def init_collection
+    table_name = klass.table_name
+
+    if project
+      if project.public? || (current_user && current_user.can?(:read_project, project))
+        project.send(table_name)
+      else
+        []
+      end
+    elsif current_user && params[:authorized_only].presence
+      klass.of_projects(current_user.authorized_projects)
+    else
+      klass.of_projects(Project.accessible_to(current_user))
+    end
+  end
+
+  def by_scope(items)
+    case params[:scope]
+    when 'created-by-me', 'authored' then
+      items.where(author_id: current_user.id)
+    when 'all' then
+      items
+    when 'assigned-to-me' then
+      items.where(assignee_id: current_user.id)
+    else
+      raise 'You must specify default scope'
+    end
+  end
+
+  def by_state(items)
+    case params[:state]
+    when 'closed'
+      items.closed
+    when 'all'
+      items
+    when 'opened'
+      items.opened
+    else
+      raise 'You must specify default state'
+    end
+  end
+
+  def by_group(items)
+    if params[:group_id].present?
+      items = items.of_group(Group.find(params[:group_id]))
+    end
+
+    items
+  end
+
+  def by_project(items)
+    if params[:project_id].present?
+      items = items.of_projects(params[:project_id])
+    end
+
+    items
+  end
+
+  def by_search(items)
+    if params[:search].present?
+      items = items.search(params[:search])
+    end
+
+    items
+  end
+
+  def sort(items)
+    items.sort(params[:sort])
+  end
+
+  def by_milestone(items)
+    if params[:milestone_id].present?
+      items = items.where(milestone_id: (params[:milestone_id] == '0' ? nil : params[:milestone_id]))
+    end
+
+    items
+  end
+
+  def by_assignee(items)
+    if params[:assignee_id].present?
+      items = items.where(assignee_id: (params[:assignee_id] == '0' ? nil : params[:assignee_id]))
+    end
+
+    items
+  end
+
+  def by_label(items)
+    if params[:label_name].present?
+      items = items.tagged_with(params[:label_name])
+    end
+
+    items
+  end
+
+  def project
+    Project.where(id: params[:project_id]).first if params[:project_id].present?
+  end
+end
diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb
new file mode 100644
index 0000000..8e0c606
--- /dev/null
+++ b/app/finders/issues_finder.rb
@@ -0,0 +1,22 @@
+# 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
diff --git a/app/finders/merge_requests_finder.rb b/app/finders/merge_requests_finder.rb
new file mode 100644
index 0000000..3727149
--- /dev/null
+++ b/app/finders/merge_requests_finder.rb
@@ -0,0 +1,22 @@
+# 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
diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb
new file mode 100644
index 0000000..384316e
--- /dev/null
+++ b/app/finders/notes_finder.rb
@@ -0,0 +1,17 @@
+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
diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb
new file mode 100644
index 0000000..bfaba75
--- /dev/null
+++ b/app/finders/projects_finder.rb
@@ -0,0 +1,63 @@
+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
diff --git a/app/models/ability.rb b/app/models/ability.rb
index 7b7a46b..69ada75 100644
--- a/app/models/ability.rb
+++ b/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
 
diff --git a/app/services/filtering_service.rb b/app/services/filtering_service.rb
deleted file mode 100644
index cbbb04c..0000000
--- a/app/services/filtering_service.rb
+++ /dev/null
@@ -1,138 +0,0 @@
-# FilteringService class
-#
-# Used to filter Issues and MergeRequests collections by set of params
-#
-# Arguments:
-#   klass - actual class like Issue or MergeRequest
-#   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 FilteringService
-  attr_accessor :klass, :current_user, :params
-
-  def execute(klass, current_user, params)
-    @klass = klass
-    @current_user = current_user
-    @params = params
-
-    items = init_collection
-    items = by_scope(items)
-    items = by_state(items)
-    items = by_group(items)
-    items = by_project(items)
-    items = by_search(items)
-    items = by_milestone(items)
-    items = by_assignee(items)
-    items = by_label(items)
-    items = sort(items)
-  end
-
-  private
-
-  def init_collection
-    table_name = klass.table_name
-
-    if project
-      if project.public? || (current_user && current_user.can?(:read_project, project))
-        project.send(table_name)
-      else
-        []
-      end
-    elsif current_user && params[:authorized_only].presence
-      klass.of_projects(current_user.authorized_projects)
-    else
-      klass.of_projects(Project.accessible_to(current_user))
-    end
-  end
-
-  def by_scope(items)
-    case params[:scope]
-    when 'created-by-me', 'authored' then
-      items.where(author_id: current_user.id)
-    when 'all' then
-      items
-    when 'assigned-to-me' then
-      items.where(assignee_id: current_user.id)
-    else
-      raise 'You must specify default scope'
-    end
-  end
-
-  def by_state(items)
-    case params[:state]
-    when 'closed'
-      items.closed
-    when 'all'
-      items
-    when 'opened'
-      items.opened
-    else
-      raise 'You must specify default state'
-    end
-  end
-
-  def by_group(items)
-    if params[:group_id].present?
-      items = items.of_group(Group.find(params[:group_id]))
-    end
-
-    items
-  end
-
-  def by_project(items)
-    if params[:project_id].present?
-      items = items.of_projects(params[:project_id])
-    end
-
-    items
-  end
-
-  def by_search(items)
-    if params[:search].present?
-      items = items.search(params[:search])
-    end
-
-    items
-  end
-
-  def sort(items)
-    items.sort(params[:sort])
-  end
-
-  def by_milestone(items)
-    if params[:milestone_id].present?
-      items = items.where(milestone_id: (params[:milestone_id] == '0' ? nil : params[:milestone_id]))
-    end
-
-    items
-  end
-
-  def by_assignee(items)
-    if params[:assignee_id].present?
-      items = items.where(assignee_id: (params[:assignee_id] == '0' ? nil : params[:assignee_id]))
-    end
-
-    items
-  end
-
-  def by_label(items)
-    if params[:label_name].present?
-      items = items.tagged_with(params[:label_name])
-    end
-
-    items
-  end
-
-  def project
-    Project.where(id: params[:project_id]).first if params[:project_id].present?
-  end
-end
diff --git a/app/services/notes/load_service.rb b/app/services/notes/load_service.rb
deleted file mode 100644
index f7ad7d6..0000000
--- a/app/services/notes/load_service.rb
+++ /dev/null
@@ -1,20 +0,0 @@
-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
diff --git a/app/services/projects/collect_service.rb b/app/services/projects/collect_service.rb
deleted file mode 100644
index 0c26231..0000000
--- a/app/services/projects/collect_service.rb
+++ /dev/null
@@ -1,69 +0,0 @@
-# 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
diff --git a/config/application.rb b/config/application.rb
index c532570..4d7c141 100644
--- a/config/application.rb
+++ b/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'
 
--
libgit2 0.21.2