Merge branch '28812-refactor-search-controller-show' into 'master'

Refactor SearchController#show

Closes #28812

See merge request !9655

7 files changed, 408 insertions, 72 deletions

diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb
index 612d69cf557..4a579601785 100644
--- a/app/controllers/search_controller.rb
+++ b/app/controllers/search_controller.rb
@@ -6,45 +6,19 @@ class SearchController < ApplicationController
   layout 'search'
 
   def show
-    if params[:project_id].present?
-      @project = Project.find_by(id: params[:project_id])
-      @project = nil unless can?(current_user, :download_code, @project)
-    end
+    search_service = SearchService.new(current_user, params)
 
-    if params[:group_id].present?
-      @group = Group.find_by(id: params[:group_id])
-      @group = nil unless can?(current_user, :read_group, @group)
-    end
+    @project = search_service.project
+    @group = search_service.group
 
     return if params[:search].blank?
 
     @search_term = params[:search]
 
-    @scope = params[:scope]
-    @show_snippets = params[:snippets].eql? 'true'
-
-    @search_results =
-      if @project
-        unless %w(blobs notes issues merge_requests milestones wiki_blobs
-                  commits).include?(@scope)
-          @scope = 'blobs'
-        end
-
-        Search::ProjectService.new(@project, current_user, params).execute
-      elsif @show_snippets
-        unless %w(snippet_blobs snippet_titles).include?(@scope)
-          @scope = 'snippet_blobs'
-        end
-
-        Search::SnippetService.new(current_user, params).execute
-      else
-        unless %w(projects issues merge_requests milestones).include?(@scope)
-          @scope = 'projects'
-        end
-        Search::GlobalService.new(current_user, params).execute
-      end
-
-    @search_objects = @search_results.objects(@scope, params[:page])
+    @scope = search_service.scope
+    @show_snippets = search_service.show_snippets?
+    @search_results = search_service.search_results
+    @search_objects = search_service.search_objects
 
     check_single_commit_result
   end
diff --git a/app/services/search/global_service.rb b/app/services/search/global_service.rb
index 781cd13b44b..a3c655493a5 100644
--- a/app/services/search/global_service.rb
+++ b/app/services/search/global_service.rb
@@ -16,5 +16,9 @@ module Search
 
       Gitlab::SearchResults.new(current_user, projects, params[:search])
     end
+
+    def scope
+      @scope ||= %w[issues merge_requests milestones].delete(params[:scope]) { 'projects' }
+    end
   end
 end
diff --git a/app/services/search/project_service.rb b/app/services/search/project_service.rb
index 4b500914cfb..9a22abae635 100644
--- a/app/services/search/project_service.rb
+++ b/app/services/search/project_service.rb
@@ -12,5 +12,9 @@ module Search
                                        params[:search],
                                        params[:repository_ref])
     end
+
+    def scope
+      @scope ||= %w[notes issues merge_requests milestones wiki_blobs commits].delete(params[:scope]) { 'blobs' }
+    end
   end
 end
diff --git a/app/services/search/snippet_service.rb b/app/services/search/snippet_service.rb
index 0b3e713e220..4f161beea4d 100644
--- a/app/services/search/snippet_service.rb
+++ b/app/services/search/snippet_service.rb
@@ -11,5 +11,9 @@ module Search
 
       Gitlab::SnippetSearchResults.new(snippets, params[:search])
     end
+
+    def scope
+      @scope ||= %w[snippet_titles].delete(params[:scope]) { 'snippet_blobs' }
+    end
   end
 end
diff --git a/app/services/search_service.rb b/app/services/search_service.rb
new file mode 100644
index 00000000000..8d46a8dab3e
--- /dev/null
+++ b/app/services/search_service.rb
@@ -0,0 +1,63 @@
+class SearchService
+  include Gitlab::Allowable
+
+  def initialize(current_user, params = {})
+    @current_user = current_user
+    @params = params.dup
+  end
+
+  def project
+    return @project if defined?(@project)
+
+    @project =
+      if params[:project_id].present?
+        the_project = Project.find_by(id: params[:project_id])
+        can?(current_user, :download_code, the_project) ? the_project : nil
+      else
+        nil
+      end
+  end
+
+  def group
+    return @group if defined?(@group)
+
+    @group =
+      if params[:group_id].present?
+        the_group = Group.find_by(id: params[:group_id])
+        can?(current_user, :read_group, the_group) ? the_group : nil
+      else
+        nil
+      end
+  end
+
+  def show_snippets?
+    return @show_snippets if defined?(@show_snippets)
+
+    @show_snippets = params[:snippets] == 'true'
+  end
+
+  delegate :scope, to: :search_service
+
+  def search_results
+    @search_results ||= search_service.execute
+  end
+
+  def search_objects
+    @search_objects ||= search_results.objects(scope, params[:page])
+  end
+
+  private
+
+  def search_service
+    @search_service ||=
+      if project
+        Search::ProjectService.new(project, current_user, params)
+      elsif show_snippets?
+        Search::SnippetService.new(current_user, params)
+      else
+        Search::GlobalService.new(current_user, params)
+      end
+  end
+
+  attr_reader :current_user, :params
+end
diff --git a/spec/services/search/global_service_spec.rb b/spec/services/search/global_service_spec.rb
new file mode 100644
index 00000000000..2531607acad
--- /dev/null
+++ b/spec/services/search/global_service_spec.rb
@@ -0,0 +1,66 @@
+require 'spec_helper'
+
+describe Search::GlobalService, services: true do
+  let(:user) { create(:user) }
+  let(:internal_user) { create(:user) }
+
+  let!(:found_project)    { create(:empty_project, :private, name: 'searchable_project') }
+  let!(:unfound_project)  { create(:empty_project, :private, name: 'unfound_project') }
+  let!(:internal_project) { create(:empty_project, :internal, name: 'searchable_internal_project') }
+  let!(:public_project)   { create(:empty_project, :public, name: 'searchable_public_project') }
+
+  before do
+    found_project.add_master(user)
+  end
+
+  describe '#execute' do
+    context 'unauthenticated' do
+      it 'returns public projects only' do
+        results = Search::GlobalService.new(nil, search: "searchable").execute
+
+        expect(results.objects('projects')).to match_array [public_project]
+      end
+    end
+
+    context 'authenticated' do
+      it 'returns public, internal and private projects' do
+        results = Search::GlobalService.new(user, search: "searchable").execute
+
+        expect(results.objects('projects')).to match_array [public_project, found_project, internal_project]
+      end
+
+      it 'returns only public & internal projects' do
+        results = Search::GlobalService.new(internal_user, search: "searchable").execute
+
+        expect(results.objects('projects')).to match_array [internal_project, public_project]
+      end
+
+      it 'namespace name is searchable' do
+        results = Search::GlobalService.new(user, search: found_project.namespace.path).execute
+
+        expect(results.objects('projects')).to match_array [found_project]
+      end
+
+      context 'nested group' do
+        let!(:nested_group) { create(:group, :nested) }
+        let!(:project) { create(:empty_project, namespace: nested_group) }
+
+        before do
+          project.add_master(user)
+        end
+
+        it 'returns result from nested group' do
+          results = Search::GlobalService.new(user, search: project.path).execute
+
+          expect(results.objects('projects')).to match_array [project]
+        end
+
+        it 'returns result from descendants when search inside group' do
+          results = Search::GlobalService.new(user, search: project.path, group_id: nested_group.parent).execute
+
+          expect(results.objects('projects')).to match_array [project]
+        end
+      end
+    end
+  end
+end
diff --git a/spec/services/search_service_spec.rb b/spec/services/search_service_spec.rb
index 6ef5fa008aa..2112f1cf9ea 100644
--- a/spec/services/search_service_spec.rb
+++ b/spec/services/search_service_spec.rb
@@ -1,65 +1,286 @@
 require 'spec_helper'
 
-describe 'Search::GlobalService', services: true do
+describe SearchService, services: true do
   let(:user) { create(:user) }
-  let(:public_user) { create(:user) }
-  let(:internal_user) { create(:user) }
 
-  let!(:found_project)    { create(:empty_project, :private, name: 'searchable_project') }
-  let!(:unfound_project)  { create(:empty_project, :private, name: 'unfound_project') }
-  let!(:internal_project) { create(:empty_project, :internal, name: 'searchable_internal_project') }
-  let!(:public_project)   { create(:empty_project, :public, name: 'searchable_public_project') }
+  let(:accessible_group) { create(:group, :private) }
+  let(:inaccessible_group) { create(:group, :private) }
+  let!(:group_member) { create(:group_member, group: accessible_group, user: user) }
+
+  let!(:accessible_project) { create(:empty_project, :private, name: 'accessible_project') }
+  let!(:inaccessible_project) { create(:empty_project, :private, name: 'inaccessible_project') }
+  let(:note) { create(:note_on_issue, project: accessible_project) }
+
+  let(:snippet) { create(:snippet, author: user) }
+  let(:group_project) { create(:empty_project, group: accessible_group, name: 'group_project') }
+  let(:public_project) { create(:empty_project, :public, name: 'public_project') }
 
   before do
-    found_project.team << [user, :master]
+    accessible_project.add_master(user)
+  end
+
+  describe '#project' do
+    context 'when the project is accessible' do
+      it 'returns the project' do
+        project = SearchService.new(user, project_id: accessible_project.id).project
+
+        expect(project).to eq accessible_project
+      end
+    end
+
+    context 'when the project is not accessible' do
+      it 'returns nil' do
+        project = SearchService.new(user, project_id: inaccessible_project.id).project
+
+        expect(project).to be_nil
+      end
+    end
+
+    context 'when there is no project_id' do
+      it 'returns nil' do
+        project = SearchService.new(user).project
+
+        expect(project).to be_nil
+      end
+    end
   end
 
-  describe '#execute' do
-    context 'unauthenticated' do
-      it 'returns public projects only' do
-        context = Search::GlobalService.new(nil, search: "searchable")
-        results = context.execute
-        expect(results.objects('projects')).to match_array [public_project]
+  describe '#group' do
+    context 'when the group is accessible' do
+      it 'returns the group' do
+        group = SearchService.new(user, group_id: accessible_group.id).group
+
+        expect(group).to eq accessible_group
       end
     end
 
-    context 'authenticated' do
-      it 'returns public, internal and private projects' do
-        context = Search::GlobalService.new(user, search: "searchable")
-        results = context.execute
-        expect(results.objects('projects')).to match_array [public_project, found_project, internal_project]
+    context 'when the group is not accessible' do
+      it 'returns nil' do
+        group = SearchService.new(user, group_id: inaccessible_group.id).group
+
+        expect(group).to be_nil
       end
+    end
+
+    context 'when there is no group_id' do
+      it 'returns nil' do
+        group = SearchService.new(user).group
 
-      it 'returns only public & internal projects' do
-        context = Search::GlobalService.new(internal_user, search: "searchable")
-        results = context.execute
-        expect(results.objects('projects')).to match_array [internal_project, public_project]
+        expect(group).to be_nil
       end
+    end
+  end
+
+  describe '#show_snippets?' do
+    context 'when :snippets is \'true\'' do
+      it 'returns true' do
+        show_snippets = SearchService.new(user, snippets: 'true').show_snippets?
 
-      it 'namespace name is searchable' do
-        context = Search::GlobalService.new(user, search: found_project.namespace.path)
-        results = context.execute
-        expect(results.objects('projects')).to match_array [found_project]
+        expect(show_snippets).to be_truthy
       end
+    end
 
-      context 'nested group' do
-        let!(:nested_group) { create(:group, :nested) }
-        let!(:project) { create(:empty_project, namespace: nested_group) }
+    context 'when :snippets is not \'true\'' do
+      it 'returns false' do
+        show_snippets = SearchService.new(user, snippets: 'tru').show_snippets?
+
+        expect(show_snippets).to be_falsey
+      end
+    end
 
-        before { project.add_master(user) }
+    context 'when :snippets is missing' do
+      it 'returns false' do
+        show_snippets = SearchService.new(user).show_snippets?
 
-        it 'returns result from nested group' do
-          context = Search::GlobalService.new(user, search: project.path)
-          results = context.execute
-          expect(results.objects('projects')).to match_array [project]
+        expect(show_snippets).to be_falsey
+      end
+    end
+  end
+
+  describe '#scope' do
+    context 'with accessible project_id' do
+      context 'and allowed scope' do
+        it 'returns the specified scope' do
+          scope = SearchService.new(user, project_id: accessible_project.id, scope: 'notes').scope
+
+          expect(scope).to eq 'notes'
         end
+      end
+
+      context 'and disallowed scope' do
+        it 'returns the default scope' do
+          scope = SearchService.new(user, project_id: accessible_project.id, scope: 'projects').scope
 
-        it 'returns result from descendants when search inside group' do
-          context = Search::GlobalService.new(user, search: project.path, group_id: nested_group.parent)
-          results = context.execute
-          expect(results.objects('projects')).to match_array [project]
+          expect(scope).to eq 'blobs'
         end
       end
+
+      context 'and no scope' do
+        it 'returns the default scope' do
+          scope = SearchService.new(user, project_id: accessible_project.id).scope
+
+          expect(scope).to eq 'blobs'
+        end
+      end
+    end
+
+    context 'with \'true\' snippets' do
+      context 'and allowed scope' do
+        it 'returns the specified scope' do
+          scope = SearchService.new(user, snippets: 'true', scope: 'snippet_titles').scope
+
+          expect(scope).to eq 'snippet_titles'
+        end
+      end
+
+      context 'and disallowed scope' do
+        it 'returns the default scope' do
+          scope = SearchService.new(user, snippets: 'true', scope: 'projects').scope
+
+          expect(scope).to eq 'snippet_blobs'
+        end
+      end
+
+      context 'and no scope' do
+        it 'returns the default scope' do
+          scope = SearchService.new(user, snippets: 'true').scope
+
+          expect(scope).to eq 'snippet_blobs'
+        end
+      end
+    end
+
+    context 'with no project_id, no snippets' do
+      context 'and allowed scope' do
+        it 'returns the specified scope' do
+          scope = SearchService.new(user, scope: 'issues').scope
+
+          expect(scope).to eq 'issues'
+        end
+      end
+
+      context 'and disallowed scope' do
+        it 'returns the default scope' do
+          scope = SearchService.new(user, scope: 'blobs').scope
+
+          expect(scope).to eq 'projects'
+        end
+      end
+
+      context 'and no scope' do
+        it 'returns the default scope' do
+          scope = SearchService.new(user).scope
+
+          expect(scope).to eq 'projects'
+        end
+      end
+    end
+  end
+
+  describe '#search_results' do
+    context 'with accessible project_id' do
+      it 'returns an instance of Gitlab::ProjectSearchResults' do
+        search_results = SearchService.new(
+          user,
+          project_id: accessible_project.id,
+          scope: 'notes',
+          search: note.note).search_results
+
+        expect(search_results).to be_a Gitlab::ProjectSearchResults
+      end
+    end
+
+    context 'with accessible project_id and \'true\' snippets' do
+      it 'returns an instance of Gitlab::ProjectSearchResults' do
+        search_results = SearchService.new(
+          user,
+          project_id: accessible_project.id,
+          snippets: 'true',
+          scope: 'notes',
+          search: note.note).search_results
+
+        expect(search_results).to be_a Gitlab::ProjectSearchResults
+      end
+    end
+
+    context 'with \'true\' snippets' do
+      it 'returns an instance of Gitlab::SnippetSearchResults' do
+        search_results = SearchService.new(
+          user,
+          snippets: 'true',
+          search: snippet.content).search_results
+
+        expect(search_results).to be_a Gitlab::SnippetSearchResults
+      end
+    end
+
+    context 'with no project_id and no snippets' do
+      it 'returns an instance of Gitlab::SearchResults' do
+        search_results = SearchService.new(
+          user,
+          search: public_project.name).search_results
+
+        expect(search_results).to be_a Gitlab::SearchResults
+      end
+    end
+  end
+
+  describe '#search_objects' do
+    context 'with accessible project_id' do
+      it 'returns objects in the project' do
+        search_objects = SearchService.new(
+          user,
+          project_id: accessible_project.id,
+          scope: 'notes',
+          search: note.note).search_objects
+
+        expect(search_objects.first).to eq note
+      end
+    end
+
+    context 'with accessible project_id and \'true\' snippets' do
+      it 'returns objects in the project' do
+        search_objects = SearchService.new(
+          user,
+          project_id: accessible_project.id,
+          snippets: 'true',
+          scope: 'notes',
+          search: note.note).search_objects
+
+        expect(search_objects.first).to eq note
+      end
+    end
+
+    context 'with \'true\' snippets' do
+      it 'returns objects in snippets' do
+        search_objects = SearchService.new(
+          user,
+          snippets: 'true',
+          search: snippet.content).search_objects
+
+        expect(search_objects.first).to eq snippet
+      end
+    end
+
+    context 'with accessible group_id' do
+      it 'returns objects in the group' do
+        search_objects = SearchService.new(
+          user,
+          group_id: accessible_group.id,
+          search: group_project.name).search_objects
+
+        expect(search_objects.first).to eq group_project
+      end
+    end
+
+    context 'with no project_id, group_id or snippets' do
+      it 'returns objects in global' do
+        search_objects = SearchService.new(
+          user,
+          search: public_project.name).search_objects
+
+        expect(search_objects.first).to eq public_project
+      end
     end
   end
 end