diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2018-07-30 17:45:49 +0200 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2018-08-20 13:53:00 +0200 |
commit | 6f3c490107f5fa7dd00bce0bbd89e4a0fa4d6389 (patch) | |
tree | 574fb9d604b8269846876430b9761f397e391d2b /spec | |
parent | 0a73c1c5833ac5a86289418e93b5d50aa8e89cbd (diff) | |
download | gitlab-ce-6f3c490107f5fa7dd00bce0bbd89e4a0fa4d6389.tar.gz |
Refactor AutocompleteControllerdefine-abstraction-levels
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".
Diffstat (limited to 'spec')
-rw-r--r-- | spec/controllers/autocomplete_controller_spec.rb | 13 | ||||
-rw-r--r-- | spec/finders/autocomplete/group_finder_spec.rb | 58 | ||||
-rw-r--r-- | spec/finders/autocomplete/move_to_project_finder_spec.rb (renamed from spec/finders/move_to_project_finder_spec.rb) | 64 | ||||
-rw-r--r-- | spec/finders/autocomplete/project_finder_spec.rb | 55 | ||||
-rw-r--r-- | spec/finders/autocomplete/users_finder_spec.rb (renamed from spec/finders/autocomplete_users_finder_spec.rb) | 2 | ||||
-rw-r--r-- | spec/finders/awarded_emoji_finder_spec.rb | 25 | ||||
-rw-r--r-- | spec/finders/user_finder_spec.rb | 43 | ||||
-rw-r--r-- | spec/models/award_emoji_spec.rb | 23 | ||||
-rw-r--r-- | spec/models/concerns/optionally_search_spec.rb | 44 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 47 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 140 | ||||
-rw-r--r-- | spec/serializers/move_to_project_entity_spec.rb | 19 | ||||
-rw-r--r-- | spec/serializers/move_to_project_serializer_spec.rb | 14 |
13 files changed, 502 insertions, 45 deletions
diff --git a/spec/controllers/autocomplete_controller_spec.rb b/spec/controllers/autocomplete_controller_spec.rb index 2c59d1929a1..883bb35f396 100644 --- a/spec/controllers/autocomplete_controller_spec.rb +++ b/spec/controllers/autocomplete_controller_spec.rb @@ -274,14 +274,11 @@ describe AutocompleteController do context 'authorized projects apply limit' do before do - authorized_project2 = create(:project) - authorized_project3 = create(:project) - - authorized_project.add_maintainer(user) - authorized_project2.add_maintainer(user) - authorized_project3.add_maintainer(user) + allow(Kaminari.config).to receive(:default_per_page).and_return(2) - stub_const 'MoveToProjectFinder::PAGE_SIZE', 2 + create_list(:project, 2) do |project| + project.add_maintainer(user) + end end describe 'GET #projects with project ID' do @@ -291,7 +288,7 @@ describe AutocompleteController do it 'returns projects' do expect(json_response).to be_kind_of(Array) - expect(json_response.size).to eq 2 # Of a total of 3 + expect(json_response.size).to eq(Kaminari.config.default_per_page) end end end diff --git a/spec/finders/autocomplete/group_finder_spec.rb b/spec/finders/autocomplete/group_finder_spec.rb new file mode 100644 index 00000000000..d7cb2c3bbe2 --- /dev/null +++ b/spec/finders/autocomplete/group_finder_spec.rb @@ -0,0 +1,58 @@ +# 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 diff --git a/spec/finders/move_to_project_finder_spec.rb b/spec/finders/autocomplete/move_to_project_finder_spec.rb index 1b3f44cced1..c3bc410a7f6 100644 --- a/spec/finders/move_to_project_finder_spec.rb +++ b/spec/finders/autocomplete/move_to_project_finder_spec.rb @@ -1,6 +1,6 @@ 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 + allow(Kaminari.config).to receive(:default_per_page).and_return(2) - reporter_project.add_reporter(user) - developer_project.add_developer(user) - maintainer_project.add_maintainer(user) + projects = create_list(:project, 2) do |project| + project.add_developer(user) + end - expect(subject.execute(project).to_a).to eq([maintainer_project, developer_project]) + 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 + expect(described_class.new(user, project_id: project.id, offset_id: maintainer_project.id).execute.to_a) + .to eq([developer_project, reporter_project]) - context 'search' do - it 'uses Project#search' do - expect(user).to receive_message_chain(:projects_where_can_admin_issues, :search) { Project.all } + expect(described_class.new(user, project_id: project.id, offset_id: developer_project.id).execute.to_a) + .to eq([reporter_project]) - subject.execute(project, search: 'wadus') + 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 diff --git a/spec/finders/autocomplete/project_finder_spec.rb b/spec/finders/autocomplete/project_finder_spec.rb new file mode 100644 index 00000000000..207d0598c28 --- /dev/null +++ b/spec/finders/autocomplete/project_finder_spec.rb @@ -0,0 +1,55 @@ +# 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 diff --git a/spec/finders/autocomplete_users_finder_spec.rb b/spec/finders/autocomplete/users_finder_spec.rb index dcf9111776e..abd0d6b5185 100644 --- a/spec/finders/autocomplete_users_finder_spec.rb +++ b/spec/finders/autocomplete/users_finder_spec.rb @@ -1,6 +1,6 @@ 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') } diff --git a/spec/finders/awarded_emoji_finder_spec.rb b/spec/finders/awarded_emoji_finder_spec.rb new file mode 100644 index 00000000000..d4479df7418 --- /dev/null +++ b/spec/finders/awarded_emoji_finder_spec.rb @@ -0,0 +1,25 @@ +# 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 diff --git a/spec/finders/user_finder_spec.rb b/spec/finders/user_finder_spec.rb new file mode 100644 index 00000000000..e53aa50dd33 --- /dev/null +++ b/spec/finders/user_finder_spec.rb @@ -0,0 +1,43 @@ +# 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 diff --git a/spec/models/award_emoji_spec.rb b/spec/models/award_emoji_spec.rb index b909e04dfc3..3f52091698c 100644 --- a/spec/models/award_emoji_spec.rb +++ b/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 diff --git a/spec/models/concerns/optionally_search_spec.rb b/spec/models/concerns/optionally_search_spec.rb new file mode 100644 index 00000000000..ff4212ddf18 --- /dev/null +++ b/spec/models/concerns/optionally_search_spec.rb @@ -0,0 +1,44 @@ +# 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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index d8a5e5f6869..56c07f5793b 100644 --- a/spec/models/project_spec.rb +++ b/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) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index f5e2c977104..9763477a711 100644 --- a/spec/models/user_spec.rb +++ b/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) + describe '.limit_to_todo_authors' do + context 'when filtering by todo authors' do + let(:user1) { create(:user) } + let(:user2) { create(:user) } - 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] + 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 diff --git a/spec/serializers/move_to_project_entity_spec.rb b/spec/serializers/move_to_project_entity_spec.rb new file mode 100644 index 00000000000..ac495eadb68 --- /dev/null +++ b/spec/serializers/move_to_project_entity_spec.rb @@ -0,0 +1,19 @@ +# 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 diff --git a/spec/serializers/move_to_project_serializer_spec.rb b/spec/serializers/move_to_project_serializer_spec.rb new file mode 100644 index 00000000000..841ac969eeb --- /dev/null +++ b/spec/serializers/move_to_project_serializer_spec.rb @@ -0,0 +1,14 @@ +# 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 |