Refactor AutocompleteController

This refactors the AutocompleteController according to the guidelines
and boundaries discussed in
https://gitlab.com/gitlab-org/gitlab-ce/issues/49653. Specifically,
ActiveRecord logic is moved to different finders, which are then used in
the controller. View logic in turn is moved to presenters, instead of
directly using ActiveRecord's "to_json" method.

The finder MoveToProjectFinder is also adjusted according to the
abstraction guidelines and boundaries, resulting in a much more simple
finder.

By using finders (and other abstractions) more actively, we can push a
lot of logic out of the controller. We also remove the need for various
"before_action" hooks, though this could be achieved without using
finders as well.

The various finders related to AutcompleteController have also been
moved into a namespace. This removes the need for calling everything
"AutocompleteSmurfFinder", instead you can use
"Autocomplete::SmurfFinder".

spec/finders/autocomplete/group_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Autocomplete::GroupFinder do
+  let(:user) { create(:user) }
+
+  describe '#execute' do
+    context 'with a project' do
+      it 'returns nil' do
+        project = create(:project)
+
+        expect(described_class.new(user, project).execute).to be_nil
+      end
+    end
+
+    context 'without a group ID' do
+      it 'returns nil' do
+        expect(described_class.new(user).execute).to be_nil
+      end
+    end
+
+    context 'with an empty String as the group ID' do
+      it 'returns nil' do
+        expect(described_class.new(user, nil, group_id: '').execute).to be_nil
+      end
+    end
+
+    context 'without a project and with a group ID' do
+      it 'raises ActiveRecord::RecordNotFound if the group does not exist' do
+        finder = described_class.new(user, nil, group_id: 1)
+
+        expect { finder.execute }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+
+      it 'raises ActiveRecord::RecordNotFound if the user can not read the group' do
+        group = create(:group, :private)
+        finder = described_class.new(user, nil, group_id: group.id)
+
+        expect { finder.execute }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+
+      it 'raises ActiveRecord::RecordNotFound if an anonymous user can not read the group' do
+        group = create(:group, :private)
+        finder = described_class.new(nil, nil, group_id: group.id)
+
+        expect { finder.execute }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+
+      it 'returns the group if it exists and is readable' do
+        group = create(:group)
+        finder = described_class.new(user, nil, group_id: group.id)
+
+        expect(finder.execute).to eq(group)
+      end
+    end
+  end
+end

spec/finders/move_to_project_finder_spec.rb → spec/finders/autocomplete/move_to_project_finder_spec.rb

 require 'spec_helper'
-describe MoveToProjectFinder do
+describe Autocomplete::MoveToProjectFinder do
   let(:user) { create(:user) }
   let(:project) { create(:project) }
@@ -10,14 +10,14 @@ describe MoveToProjectFinder do
   let(:developer_project) { create(:project) }
   let(:maintainer_project) { create(:project) }
-  subject { described_class.new(user) }
-
   describe '#execute' do
     context 'filter' do
       it 'does not return projects under Gitlab::Access::REPORTER' do
         guest_project.add_guest(user)
-        expect(subject.execute(project)).to be_empty
+        finder = described_class.new(user, project_id: project.id)
+
+        expect(finder.execute).to be_empty
       end
       it 'returns projects equal or above Gitlab::Access::REPORTER ordered by id in descending order' do
@@ -25,13 +25,17 @@ describe MoveToProjectFinder do
         developer_project.add_developer(user)
         maintainer_project.add_maintainer(user)
-        expect(subject.execute(project).to_a).to eq([maintainer_project, developer_project, reporter_project])
+        finder = described_class.new(user, project_id: project.id)
+
+        expect(finder.execute.to_a).to eq([maintainer_project, developer_project, reporter_project])
       end
       it 'does not include the source project' do
         project.add_reporter(user)
-        expect(subject.execute(project).to_a).to be_empty
+        finder = described_class.new(user, project_id: project.id)
+
+        expect(finder.execute.to_a).to be_empty
       end
       it 'does not return archived projects' do
@@ -40,7 +44,9 @@ describe MoveToProjectFinder do
         other_reporter_project = create(:project)
         other_reporter_project.add_reporter(user)
-        expect(subject.execute(project).to_a).to eq([other_reporter_project])
+        finder = described_class.new(user, project_id: project.id)
+
+        expect(finder.execute.to_a).to eq([other_reporter_project])
       end
       it 'does not return projects for which issues are disabled' do
@@ -49,39 +55,42 @@ describe MoveToProjectFinder do
         other_reporter_project = create(:project)
         other_reporter_project.add_reporter(user)
-        expect(subject.execute(project).to_a).to eq([other_reporter_project])
+        finder = described_class.new(user, project_id: project.id)
+
+        expect(finder.execute.to_a).to eq([other_reporter_project])
       end
       it 'returns a page of projects ordered by id in descending order' do
-        stub_const 'MoveToProjectFinder::PAGE_SIZE', 2
-        reporter_project.add_reporter(user)
-        developer_project.add_developer(user)
-        maintainer_project.add_maintainer(user)
-        expect(subject.execute(project).to_a).to eq([maintainer_project, developer_project])
+        allow(Kaminari.config).to receive(:default_per_page).and_return(2)
+        projects = create_list(:project, 2) do |project|
+          project.add_developer(user)
+        end
+        finder = described_class.new(user, project_id: project.id)
+        page = finder.execute.to_a
+
+        expect(page.length).to eq(Kaminari.config.default_per_page)
+        expect(page[0]).to eq(projects.last)
       end
       it 'returns projects after the given offset id' do
-        stub_const 'MoveToProjectFinder::PAGE_SIZE', 2
-
         reporter_project.add_reporter(user)
         developer_project.add_developer(user)
         maintainer_project.add_maintainer(user)
-        expect(subject.execute(project, search: nil, offset_id: maintainer_project.id).to_a).to eq([developer_project, reporter_project])
-        expect(subject.execute(project, search: nil, offset_id: developer_project.id).to_a).to eq([reporter_project])
-        expect(subject.execute(project, search: nil, offset_id: reporter_project.id).to_a).to be_empty
-      end
-    end
-    context 'search' do
-      it 'uses Project#search' do
-        expect(user).to receive_message_chain(:projects_where_can_admin_issues, :search) { Project.all }
-        subject.execute(project, search: 'wadus')
+        expect(described_class.new(user, project_id: project.id, offset_id: maintainer_project.id).execute.to_a)
+          .to eq([developer_project, reporter_project])
+        expect(described_class.new(user, project_id: project.id, offset_id: developer_project.id).execute.to_a)
+          .to eq([reporter_project])
+        expect(described_class.new(user, project_id: project.id, offset_id: reporter_project.id).execute.to_a)
+          .to be_empty
       end
+    end
+    context 'search' do
       it 'returns projects matching a search query' do
         foo_project = create(:project)
         foo_project.add_maintainer(user)
@@ -89,8 +98,11 @@ describe MoveToProjectFinder do
         wadus_project = create(:project, name: 'wadus')
         wadus_project.add_maintainer(user)
-        expect(subject.execute(project).to_a).to eq([wadus_project, foo_project])
-        expect(subject.execute(project, search: 'wadus').to_a).to eq([wadus_project])
+        expect(described_class.new(user, project_id: project.id).execute.to_a)
+          .to eq([wadus_project, foo_project])
+
+        expect(described_class.new(user, project_id: project.id, search: 'wadus').execute.to_a)
+          .to eq([wadus_project])
       end
     end
   end

spec/finders/autocomplete/project_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Autocomplete::ProjectFinder do
+  let(:user) { create(:user) }
+
+  describe '#execute' do
+    context 'without a project ID' do
+      it 'returns nil' do
+        expect(described_class.new(user).execute).to be_nil
+      end
+    end
+
+    context 'with an empty String as the project ID' do
+      it 'returns nil' do
+        expect(described_class.new(user, project_id: '').execute).to be_nil
+      end
+    end
+
+    context 'with a project ID' do
+      it 'raises ActiveRecord::RecordNotFound if the project does not exist' do
+        finder = described_class.new(user, project_id: 1)
+
+        expect { finder.execute }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+
+      it 'raises ActiveRecord::RecordNotFound if the user can not read the project' do
+        project = create(:project, :private)
+
+        finder = described_class.new(user, project_id: project.id)
+
+        expect { finder.execute }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+
+      it 'raises ActiveRecord::RecordNotFound if an anonymous user can not read the project' do
+        project = create(:project, :private)
+
+        finder = described_class.new(nil, project_id: project.id)
+
+        expect { finder.execute }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+
+      it 'returns the project if it exists and is readable' do
+        project = create(:project, :private)
+
+        project.add_maintainer(user)
+
+        finder = described_class.new(user, project_id: project.id)
+
+        expect(finder.execute).to eq(project)
+      end
+    end
+  end
+end

spec/finders/autocomplete_users_finder_spec.rb → spec/finders/autocomplete/users_finder_spec.rb

 require 'spec_helper'
-describe AutocompleteUsersFinder do
+describe Autocomplete::UsersFinder do
   describe '#execute' do
     let!(:user1) { create(:user, username: 'johndoe') }
     let!(:user2) { create(:user, :blocked, username: 'notsorandom') }

spec/finders/awarded_emoji_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe AwardedEmojiFinder do
+  describe '#execute' do
+    it 'returns an Array containing the awarded emoji names' do
+      user = create(:user)
+
+      create(:award_emoji, user: user, name: 'thumbsup')
+      create(:award_emoji, user: user, name: 'thumbsup')
+      create(:award_emoji, user: user, name: 'thumbsdown')
+
+      awarded = described_class.new(user).execute
+
+      expect(awarded).to eq([{ name: 'thumbsup' }, { name: 'thumbsdown' }])
+    end
+
+    it 'returns an empty Array when no user is given' do
+      awarded = described_class.new.execute
+
+      expect(awarded).to be_empty
+    end
+  end
+end

spec/finders/user_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe UserFinder do
+  describe '#execute' do
+    context 'when the user exists' do
+      it 'returns the user' do
+        user = create(:user)
+        found = described_class.new(id: user.id).execute
+
+        expect(found).to eq(user)
+      end
+    end
+
+    context 'when the user does not exist' do
+      it 'returns nil' do
+        found = described_class.new(id: 1).execute
+
+        expect(found).to be_nil
+      end
+    end
+  end
+
+  describe '#execute!' do
+    context 'when the user exists' do
+      it 'returns the user' do
+        user = create(:user)
+        found = described_class.new(id: user.id).execute!
+
+        expect(found).to eq(user)
+      end
+    end
+
+    context 'when the user does not exist' do
+      it 'raises ActiveRecord::RecordNotFound' do
+        finder = described_class.new(id: 1)
+
+        expect { finder.execute! }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+    end
+  end
+end

spec/models/award_emoji_spec.rb

@@ -77,4 +77,27 @@ describe AwardEmoji do
       end
     end
   end
+
+  describe '.award_counts_for_user' do
+    let(:user) { create(:user) }
+
+    before do
+      create(:award_emoji, user: user, name: 'thumbsup')
+      create(:award_emoji, user: user, name: 'thumbsup')
+      create(:award_emoji, user: user, name: 'thumbsdown')
+      create(:award_emoji, user: user, name: '+1')
+    end
+
+    it 'returns the awarded emoji in descending order' do
+      awards = described_class.award_counts_for_user(user)
+
+      expect(awards).to eq('thumbsup' => 2, 'thumbsdown' => 1, '+1' => 1)
+    end
+
+    it 'limits the returned number of rows' do
+      awards = described_class.award_counts_for_user(user, 1)
+
+      expect(awards).to eq('thumbsup' => 2)
+    end
+  end
 end

spec/models/concerns/optionally_search_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe OptionallySearch do
+  let(:model) do
+    Class.new(ActiveRecord::Base) do
+      self.table_name = 'users'
+
+      include OptionallySearch
+    end
+  end
+
+  describe '.search' do
+    it 'raises NotImplementedError' do
+      expect { model.search('foo') }.to raise_error(NotImplementedError)
+    end
+  end
+
+  describe '.optionally_search' do
+    context 'when a query is given' do
+      it 'delegates to the search method' do
+        expect(model)
+          .to receive(:search)
+          .with('foo')
+
+        model.optionally_search('foo')
+      end
+    end
+
+    context 'when no query is given' do
+      it 'returns the current relation' do
+        expect(model.optionally_search).to be_a_kind_of(ActiveRecord::Relation)
+      end
+    end
+
+    context 'when an empty query is given' do
+      it 'returns the current relation' do
+        expect(model.optionally_search(''))
+          .to be_a_kind_of(ActiveRecord::Relation)
+      end
+    end
+  end
+end

spec/models/project_spec.rb

@@ -1478,6 +1478,53 @@ describe Project do
     end
   end
+  describe '.optionally_search' do
+    let(:project) { create(:project) }
+
+    it 'searches for projects matching the query if one is given' do
+      relation = described_class.optionally_search(project.name)
+
+      expect(relation).to eq([project])
+    end
+
+    it 'returns the current relation if no search query is given' do
+      relation = described_class.where(id: project.id)
+
+      expect(relation.optionally_search).to eq(relation)
+    end
+  end
+
+  describe '.paginate_in_descending_order_using_id' do
+    let!(:project1) { create(:project) }
+    let!(:project2) { create(:project) }
+
+    it 'orders the relation in descending order' do
+      expect(described_class.paginate_in_descending_order_using_id)
+        .to eq([project2, project1])
+    end
+
+    it 'applies a limit to the relation' do
+      expect(described_class.paginate_in_descending_order_using_id(limit: 1))
+        .to eq([project2])
+    end
+
+    it 'limits projects by and ID when given' do
+      expect(described_class.paginate_in_descending_order_using_id(before: project2.id))
+        .to eq([project1])
+    end
+  end
+
+  describe '.including_namespace_and_owner' do
+    it 'eager loads the namespace and namespace owner' do
+      create(:project)
+
+      row = described_class.eager_load_namespace_and_owner.to_a.first
+      recorder = ActiveRecord::QueryRecorder.new { row.namespace.owner }
+
+      expect(recorder.count).to be_zero
+    end
+  end
+
   describe '#expire_caches_before_rename' do
     let(:project) { create(:project, :repository) }
     let(:repo)    { double(:repo, exists?: true) }

spec/models/user_spec.rb

@@ -346,17 +346,55 @@ describe User do
       end
     end
-    describe '.todo_authors' do
-      it 'filters users' do
-        create :user
-        user_2 = create :user
-        user_3 = create :user
-        current_user = create :user
-        create(:todo, user: current_user, author: user_2, state: :done)
-        create(:todo, user: current_user, author: user_3, state: :pending)
-        expect(described_class.todo_authors(current_user.id, 'pending')).to eq [user_3]
-        expect(described_class.todo_authors(current_user.id, 'done')).to eq [user_2]
+    describe '.limit_to_todo_authors' do
+      context 'when filtering by todo authors' do
+        let(:user1) { create(:user) }
+        let(:user2) { create(:user) }
+        before do
+          create(:todo, user: user1, author: user1, state: :done)
+          create(:todo, user: user2, author: user2, state: :pending)
+        end
+
+        it 'only returns users that have authored todos' do
+          users = described_class.limit_to_todo_authors(
+            user: user2,
+            with_todos: true,
+            todo_state: :pending
+          )
+
+          expect(users).to eq([user2])
+        end
+
+        it 'ignores users that do not have a todo in the matching state' do
+          users = described_class.limit_to_todo_authors(
+            user: user1,
+            with_todos: true,
+            todo_state: :pending
+          )
+
+          expect(users).to be_empty
+        end
+      end
+
+      context 'when not filtering by todo authors' do
+        it 'returns the input relation' do
+          user1 = create(:user)
+          user2 = create(:user)
+          rel = described_class.limit_to_todo_authors(user: user1)
+
+          expect(rel).to include(user1, user2)
+        end
+      end
+
+      context 'when no user is provided' do
+        it 'returns the input relation' do
+          user1 = create(:user)
+          user2 = create(:user)
+          rel = described_class.limit_to_todo_authors
+
+          expect(rel).to include(user1, user2)
+        end
       end
     end
   end
@@ -2901,4 +2939,86 @@ describe User do
       let(:uploader_class) { AttachmentUploader }
     end
   end
+
+  describe '.union_with_user' do
+    context 'when no user ID is provided' do
+      it 'returns the input relation' do
+        user = create(:user)
+
+        expect(described_class.union_with_user).to eq([user])
+      end
+    end
+
+    context 'when a user ID is provided' do
+      it 'includes the user object in the returned relation' do
+        user1 = create(:user)
+        user2 = create(:user)
+        users = described_class.where(id: user1.id).union_with_user(user2.id)
+
+        expect(users).to include(user1)
+        expect(users).to include(user2)
+      end
+
+      it 'does not re-apply any WHERE conditions on the outer query' do
+        relation = described_class.where(id: 1).union_with_user(2)
+
+        expect(relation.arel.where_sql).to be_nil
+      end
+    end
+  end
+
+  describe '.optionally_search' do
+    context 'using nil as the argument' do
+      it 'returns the current relation' do
+        user = create(:user)
+
+        expect(described_class.optionally_search).to eq([user])
+      end
+    end
+
+    context 'using an empty String as the argument' do
+      it 'returns the current relation' do
+        user = create(:user)
+
+        expect(described_class.optionally_search('')).to eq([user])
+      end
+    end
+
+    context 'using a non-empty String' do
+      it 'returns users matching the search query' do
+        user1 = create(:user)
+        create(:user)
+
+        expect(described_class.optionally_search(user1.name)).to eq([user1])
+      end
+    end
+  end
+
+  describe '.where_not_in' do
+    context 'without an argument' do
+      it 'returns the current relation' do
+        user = create(:user)
+
+        expect(described_class.where_not_in).to eq([user])
+      end
+    end
+
+    context 'using a list of user IDs' do
+      it 'excludes the users from the returned relation' do
+        user1 = create(:user)
+        user2 = create(:user)
+
+        expect(described_class.where_not_in([user2.id])).to eq([user1])
+      end
+    end
+  end
+
+  describe '.reorder_by_name' do
+    it 'reorders the input relation' do
+      user1 = create(:user, name: 'A')
+      user2 = create(:user, name: 'B')
+
+      expect(described_class.reorder_by_name).to eq([user1, user2])
+    end
+  end
 end

spec/serializers/move_to_project_entity_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MoveToProjectEntity do
+  describe '#as_json' do
+    let(:project) { build(:project, id: 1) }
+
+    subject { described_class.new(project).as_json }
+
+    it 'includes the project ID' do
+      expect(subject[:id]).to eq(project.id)
+    end
+
+    it 'includes the full path' do
+      expect(subject[:name_with_namespace]).to eq(project.name_with_namespace)
+    end
+  end
+end

spec/serializers/move_to_project_serializer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MoveToProjectSerializer do
+  describe '#represent' do
+    it 'includes the name and name with namespace' do
+      project = build(:project, id: 1)
+      output = described_class.new.represent(project)
+
+      expect(output).to include(:id, :name_with_namespace)
+    end
+  end
+end