Permalink
Browse files

Move services for collecting items to Finders

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
  • Loading branch information...
1 parent 0f47367 commit 645e8d470559b07a22164c55b76195a60fb8b37b @dzaporozhets dzaporozhets committed Feb 25, 2014
@@ -53,13 +53,13 @@ def projects
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)
@@ -47,13 +47,13 @@ def show
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 @@ def group
end
def projects
- @projects ||= Projects::CollectService.new.execute(current_user, group: group)
+ @projects ||= ProjectsFinder.new.execute(current_user, group: group)
end
def project_ids
@@ -121,7 +121,7 @@ def module_enabled
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
@@ -21,7 +21,7 @@ def index
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
@@ -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: [] }
View
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)
@dzaporozhets

dzaporozhets Feb 26, 2014

Owner

@jhollingsworth the idea is to simplify things. All this scopes + associations caused PGError for me in some cases because of rails context magic. So I decided move it to separate class with simple if..else conditions and simple sql queries(which gives us performance improvement too).

+ 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
View
@@ -184,7 +184,7 @@ def project_admin_rules
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
@@ -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
@@ -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
View
@@ -12,7 +12,7 @@ class Application < Rails::Application
# -- 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 @@ class Application < Rails::Application
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'

0 comments on commit 645e8d4

Please sign in to comment.