diff options
| -rw-r--r-- | app/controllers/dashboard_controller.rb | 4 | ||||
| -rw-r--r-- | app/controllers/groups_controller.rb | 6 | ||||
| -rw-r--r-- | app/controllers/projects/issues_controller.rb | 2 | ||||
| -rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 2 | ||||
| -rw-r--r-- | app/controllers/projects/notes_controller.rb | 2 | ||||
| -rw-r--r-- | app/finders/README.md | 24 | ||||
| -rw-r--r-- | app/finders/base_finder.rb (renamed from app/services/filtering_service.rb) | 9 | ||||
| -rw-r--r-- | app/finders/issues_finder.rb | 22 | ||||
| -rw-r--r-- | app/finders/merge_requests_finder.rb | 22 | ||||
| -rw-r--r-- | app/finders/notes_finder.rb | 17 | ||||
| -rw-r--r-- | app/finders/projects_finder.rb | 63 | ||||
| -rw-r--r-- | app/models/ability.rb | 2 | ||||
| -rw-r--r-- | app/services/notes/load_service.rb | 20 | ||||
| -rw-r--r-- | app/services/projects/collect_service.rb | 69 | ||||
| -rw-r--r-- | config/application.rb | 4 |
15 files changed, 163 insertions, 105 deletions
diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index e7edaed1272..a74e97ac253 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 a9dce2f6b13..ebddf36de97 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 dd11948edd1..9d97c820f38 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 2b410c5a610..6f7ea9c96a4 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 9c9c2decc78..85d042a89b5 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 00000000000..6fbf0e2f4c1 --- /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 index cbbb04ccba9..d20716fb170 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 00000000000..8e0c606249e --- /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 00000000000..3727149c8fb --- /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 00000000000..384316e14b7 --- /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 00000000000..bfaba758788 --- /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 7b7a46bce32..69ada753d02 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 f7ad7d60a3a..00000000000 --- 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 0c26231f6ec..00000000000 --- 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 c53257090cf..4d7c1415c8e 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' |