diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb
index e7edaed127216b67bff03ee5d17386f38952a997..a74e97ac25370f7f39102f50aaa9903b100bc397 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 a9dce2f6b132cedcfea76476b85ce873e9e8b792..ebddf36de973c21f4ae9492cb06a0d8f53cc23fe 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 dd11948edd11b931d6aa0eb322a8c0578e470274..9d97c820f38437b69f6a140fabec860230fce1b1 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 2b410c5a610ca72a5697e128bbdd958c9176e3af..6f7ea9c96a4581c0671a9cea21a591db4f92828a 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 9c9c2decc788e09d1d11755dfdc9e7c4fb03b3d9..85d042a89b5736c5baa051460978cfd0cb06c9f6 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 0000000000000000000000000000000000000000..6fbf0e2f4c12f7c396f5c4d4409b6f0111caabc3
--- /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/services/filtering_service.rb b/app/finders/base_finder.rb
similarity index 94%
rename from app/services/filtering_service.rb
rename to app/finders/base_finder.rb
index cbbb04ccba92be0b4637c642ba8b24cf214044b6..d20716fb170bc8082018a9fb860a16581dbd045e 100644
--- a/app/services/filtering_service.rb
+++ b/app/finders/base_finder.rb
@@ -1,4 +1,4 @@
-# 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
diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb
new file mode 100644
index 0000000000000000000000000000000000000000..8e0c606249e2a3296f0914430e798bdcb52809c5
--- /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 0000000000000000000000000000000000000000..3727149c8fb4c5f35e6194e0e1bfeab955847cf8
--- /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 0000000000000000000000000000000000000000..384316e14b7d2308a3edc6fc37527e5925df3adb
--- /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 0000000000000000000000000000000000000000..bfaba7587884bb6aea7b979fa1641edd4e8e9f2e
--- /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 7b7a46bce32413ebc137ee29028555f0b1578e57..69ada753d026d22d57a2cb337852a35a2696292a 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/notes/load_service.rb b/app/services/notes/load_service.rb
deleted file mode 100644
index f7ad7d60a3a7fc3ea0e7c98879a5d4aaafa11353..0000000000000000000000000000000000000000
--- 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 0c26231f6ecf816c72aa1f627da0f78575c54f31..0000000000000000000000000000000000000000
--- 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 c53257090cf93851b18f71f2aa96b86397de1070..4d7c1415c8eabb09d9e2f23565991a0fc3dd5500 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'