Merge branch 'refacoring/selectors' into 'master'

Refacoring: Finders

Services -> change data
Finders -> select data

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: 
+
+```ruby
+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: 
+
+```ruby
+issues = IssuesFinder.new.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'
 

spec/services/filtering_service_spec.rb → spec/finders/issues_finder_spec.rb

 require 'spec_helper'
 
-describe FilteringService do
+describe IssuesFinder do
   let(:user) { create :user }
   let(:user2) { create :user }
   let(:project1) { create(:project) }
   let(:project2) { create(:project) }
-  let(:merge_request1) { create(:merge_request, author: user, source_project: project1, target_project: project2) }
-  let(:merge_request2) { create(:merge_request, author: user, source_project: project2, target_project: project1) }
-  let(:merge_request3) { create(:merge_request, author: user, source_project: project2, target_project: project2) }
   let(:issue1) { create(:issue, assignee: user, project: project1) }
   let(:issue2) { create(:issue, assignee: user, project: project2) }
   let(:issue3) { create(:issue, assignee: user2, project: project2) }
@@ -18,27 +15,7 @@ describe FilteringService do
     project2.team << [user2, :developer]
   end
 
-  describe 'merge requests' do
-    before :each do
-      merge_request1
-      merge_request2
-      merge_request3
-    end
-
-    it 'should filter by scope' do
-      params = { scope: 'authored', state: 'opened' }
-      merge_requests = FilteringService.new.execute(MergeRequest, user, params)
-      merge_requests.size.should == 3
-    end
-
-    it 'should filter by project' do
-      params = { project_id: project1.id, scope: 'authored', state: 'opened' }
-      merge_requests = FilteringService.new.execute(MergeRequest, user, params)
-      merge_requests.size.should == 1
-    end
-  end
-
-  describe 'issues' do
+  describe :execute do
     before :each do
       issue1
       issue2
@@ -47,31 +24,31 @@ describe FilteringService do
 
     it 'should filter by all' do
       params = { scope: "all", state: 'opened' }
-      issues = FilteringService.new.execute(Issue, user, params)
+      issues = IssuesFinder.new.execute(user, params)
       issues.size.should == 3
     end
 
     it 'should filter by assignee' do
       params = { scope: "assigned-to-me", state: 'opened' }
-      issues = FilteringService.new.execute(Issue, user, params)
+      issues = IssuesFinder.new.execute(user, params)
       issues.size.should == 2
     end
 
     it 'should filter by project' do
       params = { scope: "assigned-to-me", state: 'opened', project_id: project1.id }
-      issues = FilteringService.new.execute(Issue, user, params)
+      issues = IssuesFinder.new.execute(user, params)
       issues.size.should == 1
     end
 
     it 'should be empty for unauthorized user' do
       params = { scope: "all", state: 'opened' }
-      issues = FilteringService.new.execute(Issue, nil, params)
+      issues = IssuesFinder.new.execute(nil, params)
       issues.size.should be_zero
     end
 
     it 'should not include unauthorized issues' do
       params = { scope: "all", state: 'opened' }
-      issues = FilteringService.new.execute(Issue, user2, params)
+      issues = IssuesFinder.new.execute(user2, params)
       issues.size.should == 2
       issues.should_not include(issue1)
       issues.should include(issue2)

spec/finders/merge_requests_finder_spec.rb

+require 'spec_helper'
+
+describe MergeRequestsFinder do
+  let(:user) { create :user }
+  let(:user2) { create :user }
+  let(:project1) { create(:project) }
+  let(:project2) { create(:project) }
+  let(:merge_request1) { create(:merge_request, author: user, source_project: project1, target_project: project2) }
+  let(:merge_request2) { create(:merge_request, author: user, source_project: project2, target_project: project1) }
+  let(:merge_request3) { create(:merge_request, author: user, source_project: project2, target_project: project2) }
+
+  before do
+    project1.team << [user, :master]
+    project2.team << [user, :developer]
+    project2.team << [user2, :developer]
+  end
+
+  describe :execute do
+    before :each do
+      merge_request1
+      merge_request2
+      merge_request3
+    end
+
+    it 'should filter by scope' do
+      params = { scope: 'authored', state: 'opened' }
+      merge_requests = MergeRequestsFinder.new.execute(user, params)
+      merge_requests.size.should == 3
+    end
+
+    it 'should filter by project' do
+      params = { project_id: project1.id, scope: 'authored', state: 'opened' }
+      merge_requests = MergeRequestsFinder.new.execute(user, params)
+      merge_requests.size.should == 1
+    end
+  end
+end

spec/services/projects_collect_service_spec.rb → spec/finders/projects_finder_spec.rb

 require 'spec_helper'
 
-describe Projects::CollectService do
+describe ProjectsFinder do
   let(:user) { create :user }
   let(:group) { create :group }
 
@@ -10,7 +10,7 @@ describe Projects::CollectService do
   let(:project4) { create(:empty_project, group: group, visibility_level: Project::PRIVATE) }
 
   context 'non authenticated' do
-    subject { Projects::CollectService.new.execute(nil, group: group) }
+    subject { ProjectsFinder.new.execute(nil, group: group) }
 
     it { should include(project1) }
     it { should_not include(project2) }
@@ -19,7 +19,7 @@ describe Projects::CollectService do
   end
 
   context 'authenticated' do
-    subject { Projects::CollectService.new.execute(user, group: group) }
+    subject { ProjectsFinder.new.execute(user, group: group) }
 
     it { should include(project1) }
     it { should include(project2) }
@@ -30,7 +30,7 @@ describe Projects::CollectService do
   context 'authenticated, project member' do
     before { project3.team << [user, :developer] }
 
-    subject { Projects::CollectService.new.execute(user, group: group) }
+    subject { ProjectsFinder.new.execute(user, group: group) }
 
     it { should include(project1) }
     it { should include(project2) }
@@ -41,7 +41,7 @@ describe Projects::CollectService do
   context 'authenticated, group member' do
     before { group.add_user(user, Gitlab::Access::DEVELOPER) }
 
-    subject { Projects::CollectService.new.execute(user, group: group) }
+    subject { ProjectsFinder.new.execute(user, group: group) }
 
     it { should include(project1) }
     it { should include(project2) }