diff options
Diffstat (limited to 'spec/services/merge_requests')
36 files changed, 294 insertions, 185 deletions
diff --git a/spec/services/merge_requests/add_context_service_spec.rb b/spec/services/merge_requests/add_context_service_spec.rb index 27b46a9023c..448be27efe8 100644 --- a/spec/services/merge_requests/add_context_service_spec.rb +++ b/spec/services/merge_requests/add_context_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe MergeRequests::AddContextService do let(:commits) { ["874797c3a73b60d2187ed6e2fcabd289ff75171e"] } let(:raw_repository) { project.repository.raw } - subject(:service) { described_class.new(project, admin, merge_request: merge_request, commits: commits) } + subject(:service) { described_class.new(project: project, current_user: admin, params: { merge_request: merge_request, commits: commits }) } describe "#execute" do context "when admin mode is enabled", :enable_admin_mode do @@ -32,7 +32,7 @@ RSpec.describe MergeRequests::AddContextService do let(:user) { create(:user) } let(:merge_request1) { create(:merge_request, source_project: project, author: user) } - subject(:service) { described_class.new(project, user, merge_request: merge_request, commits: commits) } + subject(:service) { described_class.new(project: project, current_user: user, params: { merge_request: merge_request, commits: commits }) } it "doesn't add context commit" do subject.execute @@ -42,7 +42,7 @@ RSpec.describe MergeRequests::AddContextService do end context "when the commits array is empty" do - subject(:service) { described_class.new(project, admin, merge_request: merge_request, commits: []) } + subject(:service) { described_class.new(project: project, current_user: admin, params: { merge_request: merge_request, commits: [] }) } it "doesn't add context commit" do subject.execute diff --git a/spec/services/merge_requests/add_spent_time_service_spec.rb b/spec/services/merge_requests/add_spent_time_service_spec.rb new file mode 100644 index 00000000000..db3380e9582 --- /dev/null +++ b/spec/services/merge_requests/add_spent_time_service_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::AddSpentTimeService do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :public, :repository) } + let_it_be_with_reload(:merge_request) { create(:merge_request, :simple, :unique_branches, source_project: project) } + + let(:duration) { 1500 } + let(:params) { { spend_time: { duration: duration, user_id: user.id } } } + let(:service) { described_class.new(project: project, current_user: user, params: params) } + + describe '#execute' do + before do + project.add_developer(user) + end + + it 'creates a new timelog with the specified duration' do + expect { service.execute(merge_request) }.to change { Timelog.count }.from(0).to(1) + + timelog = merge_request.timelogs.last + + expect(timelog).not_to be_nil + expect(timelog.time_spent).to eq(1500) + end + + it 'creates a system note with the time added' do + expect { service.execute(merge_request) }.to change { Note.count }.from(0).to(1) + + system_note = merge_request.notes.last + + expect(system_note).not_to be_nil + expect(system_note.note_html).to include('added 25m of time spent') + end + + it 'saves usage data' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_time_spent_changed_action).once.with(user: user) + + service.execute(merge_request) + end + + it 'is more efficient than using the full update-service' do + other_mr = create(:merge_request, :simple, :unique_branches, source_project: project) + + update_service = ::MergeRequests::UpdateService.new(project: project, current_user: user, params: params) + other_mr.reload + + expect { service.execute(merge_request) } + .to issue_fewer_queries_than { update_service.execute(other_mr) } + end + + context 'when duration is nil' do + let(:duration) { nil } + + it 'does not create a timelog with the specified duration' do + expect { service.execute(merge_request) }.not_to change { Timelog.count } + expect(merge_request).not_to be_valid + end + end + end +end diff --git a/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb b/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb index 6edaa91b8b2..8d1abe5ea89 100644 --- a/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb +++ b/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe ::MergeRequests::AddTodoWhenBuildFailsService do let(:ref) { merge_request.source_branch } let(:service) do - described_class.new(project, user, commit_message: 'Awesome message') + described_class.new(project: project, current_user: user, params: { commit_message: 'Awesome message' }) end let(:todo_service) { spy('todo service') } diff --git a/spec/services/merge_requests/after_create_service_spec.rb b/spec/services/merge_requests/after_create_service_spec.rb index e1f28e32164..cbbd193a411 100644 --- a/spec/services/merge_requests/after_create_service_spec.rb +++ b/spec/services/merge_requests/after_create_service_spec.rb @@ -6,7 +6,7 @@ RSpec.describe MergeRequests::AfterCreateService do let_it_be(:merge_request) { create(:merge_request) } subject(:after_create_service) do - described_class.new(merge_request.target_project, merge_request.author) + described_class.new(project: merge_request.target_project, current_user: merge_request.author) end describe '#execute' do @@ -191,7 +191,7 @@ RSpec.describe MergeRequests::AfterCreateService do it 'calls MergeRequests::LinkLfsObjectsService#execute' do service = instance_spy(MergeRequests::LinkLfsObjectsService) - allow(MergeRequests::LinkLfsObjectsService).to receive(:new).with(merge_request.target_project).and_return(service) + allow(MergeRequests::LinkLfsObjectsService).to receive(:new).with(project: merge_request.target_project).and_return(service) execute_service diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb index df9a98c5540..d30b2721a36 100644 --- a/spec/services/merge_requests/approval_service_spec.rb +++ b/spec/services/merge_requests/approval_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe MergeRequests::ApprovalService do let(:project) { merge_request.project } let!(:todo) { create(:todo, user: user, project: project, target: merge_request) } - subject(:service) { described_class.new(project, user) } + subject(:service) { described_class.new(project: project, current_user: user) } before do project.add_developer(user) diff --git a/spec/services/merge_requests/assign_issues_service_spec.rb b/spec/services/merge_requests/assign_issues_service_spec.rb index 6398e8c533e..b857f26c052 100644 --- a/spec/services/merge_requests/assign_issues_service_spec.rb +++ b/spec/services/merge_requests/assign_issues_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe MergeRequests::AssignIssuesService do let(:project) { create(:project, :public, :repository) } let(:issue) { create(:issue, project: project) } let(:merge_request) { create(:merge_request, :simple, source_project: project, author: user, description: "fixes #{issue.to_reference}") } - let(:service) { described_class.new(project, user, merge_request: merge_request) } + let(:service) { described_class.new(project: project, current_user: user, params: { merge_request: merge_request }) } before do project.add_developer(user) @@ -37,10 +37,12 @@ RSpec.describe MergeRequests::AssignIssuesService do it 'accepts precomputed data for closes_issues' do issue2 = create(:issue, project: project) - service2 = described_class.new(project, - user, - merge_request: merge_request, - closes_issues: [issue, issue2]) + service2 = described_class.new(project: project, + current_user: user, + params: { + merge_request: merge_request, + closes_issues: [issue, issue2] + }) expect(service2.assignable_issues.count).to eq 2 end @@ -52,10 +54,12 @@ RSpec.describe MergeRequests::AssignIssuesService do it 'ignores external issues' do external_issue = ExternalIssue.new('JIRA-123', project) service = described_class.new( - project, - user, - merge_request: merge_request, - closes_issues: [external_issue] + project: project, + current_user: user, + params: { + merge_request: merge_request, + closes_issues: [external_issue] + } ) expect(service.assignable_issues.count).to eq 0 diff --git a/spec/services/merge_requests/base_service_spec.rb b/spec/services/merge_requests/base_service_spec.rb index d8ba2bc43fb..7911392ef19 100644 --- a/spec/services/merge_requests/base_service_spec.rb +++ b/spec/services/merge_requests/base_service_spec.rb @@ -17,7 +17,7 @@ RSpec.describe MergeRequests::BaseService do } end - subject { MergeRequests::CreateService.new(project, project.owner, params) } + subject { MergeRequests::CreateService.new(project: project, current_user: project.owner, params: params) } describe '#execute_hooks' do shared_examples 'enqueues Jira sync worker' do diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index 8adf6d69f73..5a6a9df3f44 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -49,7 +49,7 @@ RSpec.describe MergeRequests::BuildService do end let(:service) do - described_class.new(project, user, params) + described_class.new(project: project, current_user: user, params: params) end before do @@ -100,7 +100,7 @@ RSpec.describe MergeRequests::BuildService do context 'with force_remove_source_branch parameter when the user is authorized' do let(:mr_params) { params.merge(force_remove_source_branch: '1') } let(:source_project) { fork_project(project, user) } - let(:merge_request) { described_class.new(project, user, mr_params).execute } + let(:merge_request) { described_class.new(project: project, current_user: user, params: mr_params).execute } before do project.add_reporter(user) diff --git a/spec/services/merge_requests/cleanup_refs_service_spec.rb b/spec/services/merge_requests/cleanup_refs_service_spec.rb index a1822a4d5ba..e8690ae5bf2 100644 --- a/spec/services/merge_requests/cleanup_refs_service_spec.rb +++ b/spec/services/merge_requests/cleanup_refs_service_spec.rb @@ -54,7 +54,7 @@ RSpec.describe MergeRequests::CleanupRefsService do context 'when merge request has merge ref' do before do MergeRequests::MergeToRefService - .new(merge_request.project, merge_request.author) + .new(project: merge_request.project, current_user: merge_request.author) .execute(merge_request) end diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 48f56b3ec68..f6336a85a25 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -21,7 +21,7 @@ RSpec.describe MergeRequests::CloseService do it_behaves_like 'merge request reviewers cache counters invalidator' context 'valid params' do - let(:service) { described_class.new(project, user, {}) } + let(:service) { described_class.new(project: project, current_user: user) } before do allow(service).to receive(:execute_hooks) @@ -73,7 +73,7 @@ RSpec.describe MergeRequests::CloseService do expect(metrics_service).to receive(:close) - described_class.new(project, user, {}).execute(merge_request) + described_class.new(project: project, current_user: user).execute(merge_request) end it 'calls the merge request activity counter' do @@ -81,11 +81,11 @@ RSpec.describe MergeRequests::CloseService do .to receive(:track_close_mr_action) .with(user: user) - described_class.new(project, user, {}).execute(merge_request) + described_class.new(project: project, current_user: user).execute(merge_request) end it 'refreshes the number of open merge requests for a valid MR', :use_clean_rails_memory_store_caching do - service = described_class.new(project, user, {}) + service = described_class.new(project: project, current_user: user) expect { service.execute(merge_request) } .to change { project.open_merge_requests_count }.from(1).to(0) @@ -96,19 +96,19 @@ RSpec.describe MergeRequests::CloseService do expect(service).to receive(:execute_for_merge_request).with(merge_request) end - described_class.new(project, user).execute(merge_request) + described_class.new(project: project, current_user: user).execute(merge_request) end it 'schedules CleanupRefsService' do expect(MergeRequests::CleanupRefsService).to receive(:schedule).with(merge_request) - described_class.new(project, user).execute(merge_request) + described_class.new(project: project, current_user: user).execute(merge_request) end context 'current user is not authorized to close merge request' do before do perform_enqueued_jobs do - @merge_request = described_class.new(project, guest).execute(merge_request) + @merge_request = described_class.new(project: project, current_user: guest).execute(merge_request) end end diff --git a/spec/services/merge_requests/create_from_issue_service_spec.rb b/spec/services/merge_requests/create_from_issue_service_spec.rb index 6528edfc8b7..749b30bff5f 100644 --- a/spec/services/merge_requests/create_from_issue_service_spec.rb +++ b/spec/services/merge_requests/create_from_issue_service_spec.rb @@ -11,8 +11,8 @@ RSpec.describe MergeRequests::CreateFromIssueService do let(:milestone_id) { create(:milestone, project: project).id } let(:issue) { create(:issue, project: project, milestone_id: milestone_id) } let(:custom_source_branch) { 'custom-source-branch' } - let(:service) { described_class.new(project, user, service_params) } - let(:service_with_custom_source_branch) { described_class.new(project, user, branch_name: custom_source_branch, **service_params) } + let(:service) { described_class.new(project: project, current_user: user, mr_params: service_params) } + let(:service_with_custom_source_branch) { described_class.new(project: project, current_user: user, mr_params: { branch_name: custom_source_branch, **service_params }) } before do project.add_developer(user) @@ -21,14 +21,14 @@ RSpec.describe MergeRequests::CreateFromIssueService do describe '#execute' do shared_examples_for 'a service that creates a merge request from an issue' do it 'returns an error when user can not create merge request on target project' do - result = described_class.new(project, create(:user), service_params).execute + result = described_class.new(project: project, current_user: create(:user), mr_params: service_params).execute expect(result[:status]).to eq(:error) expect(result[:message]).to eq('Not allowed to create merge request') end it 'returns an error with invalid issue iid' do - result = described_class.new(project, user, issue_iid: -1).execute + result = described_class.new(project: project, current_user: user, mr_params: { issue_iid: -1 }).execute expect(result[:status]).to eq(:error) expect(result[:message]).to eq('Invalid issue iid') @@ -123,7 +123,7 @@ RSpec.describe MergeRequests::CreateFromIssueService do end context 'when ref branch is set', :sidekiq_might_not_need_inline do - subject { described_class.new(project, user, ref: 'feature', **service_params).execute } + subject { described_class.new(project: project, current_user: user, mr_params: { ref: 'feature', **service_params }).execute } it 'sets the merge request source branch to the new issue branch' do expect(subject[:merge_request].source_branch).to eq(issue.to_branch_name) @@ -134,7 +134,7 @@ RSpec.describe MergeRequests::CreateFromIssueService do end context 'when the ref is a tag' do - subject { described_class.new(project, user, ref: 'v1.0.0', **service_params).execute } + subject { described_class.new(project: project, current_user: user, mr_params: { ref: 'v1.0.0', **service_params }).execute } it 'sets the merge request source branch to the new issue branch' do expect(subject[:merge_request].source_branch).to eq(issue.to_branch_name) @@ -150,7 +150,7 @@ RSpec.describe MergeRequests::CreateFromIssueService do end context 'when ref branch does not exist' do - subject { described_class.new(project, user, ref: 'no-such-branch', **service_params).execute } + subject { described_class.new(project: project, current_user: user, mr_params: { ref: 'no-such-branch', **service_params }).execute } it 'creates a merge request' do expect { subject }.to change(target_project.merge_requests, :count).by(1) diff --git a/spec/services/merge_requests/create_pipeline_service_spec.rb b/spec/services/merge_requests/create_pipeline_service_spec.rb index 3e2e940dc24..a0ac168f3d7 100644 --- a/spec/services/merge_requests/create_pipeline_service_spec.rb +++ b/spec/services/merge_requests/create_pipeline_service_spec.rb @@ -8,7 +8,7 @@ RSpec.describe MergeRequests::CreatePipelineService do let_it_be(:project, reload: true) { create(:project, :repository) } let_it_be(:user) { create(:user) } - let(:service) { described_class.new(project, actor, params) } + let(:service) { described_class.new(project: project, current_user: actor, params: params) } let(:actor) { user } let(:params) { {} } diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index f2bc55103f0..b2351ab53bd 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -21,7 +21,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do } end - let(:service) { described_class.new(project, user, opts) } + let(:service) { described_class.new(project: project, current_user: user, params: opts) } let(:merge_request) { service.execute } before do @@ -347,12 +347,12 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do } end - let(:issuable) { described_class.new(project, user, params).execute } + let(:issuable) { described_class.new(project: project, current_user: user, params: params).execute } end context 'Quick actions' do context 'with assignee and milestone in params and command' do - let(:merge_request) { described_class.new(project, user, opts).execute } + let(:merge_request) { described_class.new(project: project, current_user: user, params: opts).execute } let(:milestone) { create(:milestone, project: project) } let(:opts) do @@ -390,7 +390,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do it 'removes assignee_id when user id is invalid' do opts = { title: 'Title', description: 'Description', assignee_ids: [-1] } - merge_request = described_class.new(project, user, opts).execute + merge_request = described_class.new(project: project, current_user: user, params: opts).execute expect(merge_request.assignee_ids).to be_empty end @@ -398,7 +398,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do it 'removes assignee_id when user id is 0' do opts = { title: 'Title', description: 'Description', assignee_ids: [0] } - merge_request = described_class.new(project, user, opts).execute + merge_request = described_class.new(project: project, current_user: user, params: opts).execute expect(merge_request.assignee_ids).to be_empty end @@ -407,7 +407,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do project.add_maintainer(user2) opts = { title: 'Title', description: 'Description', assignee_ids: [user2.id] } - merge_request = described_class.new(project, user, opts).execute + merge_request = described_class.new(project: project, current_user: user, params: opts).execute expect(merge_request.assignees).to eq([user2]) end @@ -426,7 +426,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do it 'invalidates open merge request counter for assignees when merge request is assigned' do project.add_maintainer(user2) - described_class.new(project, user, opts).execute + described_class.new(project: project, current_user: user, params: opts).execute expect(user2.assigned_open_merge_requests_count).to eq 1 end @@ -445,7 +445,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do project.update!(visibility_level: level) opts = { title: 'Title', description: 'Description', assignee_ids: [user2.id] } - merge_request = described_class.new(project, user, opts).execute + merge_request = described_class.new(project: project, current_user: user, params: opts).execute expect(merge_request.assignee_id).to be_nil end @@ -473,7 +473,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end it 'raises an error' do - expect { described_class.new(project, user, opts).execute } + expect { described_class.new(project: project, current_user: user, params: opts).execute } .to raise_error Gitlab::Access::AccessDeniedError end end @@ -485,7 +485,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end it 'raises an error' do - expect { described_class.new(project, user, opts).execute } + expect { described_class.new(project: project, current_user: user, params: opts).execute } .to raise_error Gitlab::Access::AccessDeniedError end end @@ -497,7 +497,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end it 'creates the merge request', :sidekiq_might_not_need_inline do - merge_request = described_class.new(project, user, opts).execute + merge_request = described_class.new(project: project, current_user: user, params: opts).execute expect(merge_request).to be_persisted end @@ -505,7 +505,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do it 'does not create the merge request when the target project is archived' do target_project.update!(archived: true) - expect { described_class.new(project, user, opts).execute } + expect { described_class.new(project: project, current_user: user, params: opts).execute } .to raise_error Gitlab::Access::AccessDeniedError end end @@ -529,7 +529,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end it 'ignores source_project_id' do - merge_request = described_class.new(project, user, opts).execute + merge_request = described_class.new(project: project, current_user: user, params: opts).execute expect(merge_request.source_project_id).to eq(project.id) end diff --git a/spec/services/merge_requests/ff_merge_service_spec.rb b/spec/services/merge_requests/ff_merge_service_spec.rb index aec5a3b3fa3..24a1a8b3113 100644 --- a/spec/services/merge_requests/ff_merge_service_spec.rb +++ b/spec/services/merge_requests/ff_merge_service_spec.rb @@ -23,7 +23,7 @@ RSpec.describe MergeRequests::FfMergeService do describe '#execute' do context 'valid params' do - let(:service) { described_class.new(project, user, valid_merge_params) } + let(:service) { described_class.new(project: project, current_user: user, params: valid_merge_params) } def execute_ff_merge perform_enqueued_jobs do @@ -92,7 +92,7 @@ RSpec.describe MergeRequests::FfMergeService do end context 'error handling' do - let(:service) { described_class.new(project, user, valid_merge_params.merge(commit_message: 'Awesome message')) } + let(:service) { described_class.new(project: project, current_user: user, params: valid_merge_params.merge(commit_message: 'Awesome message')) } before do allow(Gitlab::AppLogger).to receive(:error) diff --git a/spec/services/merge_requests/get_urls_service_spec.rb b/spec/services/merge_requests/get_urls_service_spec.rb index 053752626dc..5f81e1728fa 100644 --- a/spec/services/merge_requests/get_urls_service_spec.rb +++ b/spec/services/merge_requests/get_urls_service_spec.rb @@ -6,7 +6,7 @@ RSpec.describe MergeRequests::GetUrlsService do include ProjectForksHelper let(:project) { create(:project, :public, :repository) } - let(:service) { described_class.new(project) } + let(:service) { described_class.new(project: project) } let(:source_branch) { "merge-test" } let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/-/merge_requests/new?merge_request%5Bsource_branch%5D=#{source_branch}" } let(:show_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/-/merge_requests/#{merge_request.iid}" } @@ -106,7 +106,7 @@ RSpec.describe MergeRequests::GetUrlsService do let!(:merge_request) { create(:merge_request, source_project: forked_project, target_project: project, source_branch: source_branch) } let(:changes) { existing_branch_changes } # Source project is now the forked one - let(:service) { described_class.new(forked_project) } + let(:service) { described_class.new(project: forked_project) } before do allow(forked_project).to receive(:empty_repo?).and_return(false) diff --git a/spec/services/merge_requests/handle_assignees_change_service_spec.rb b/spec/services/merge_requests/handle_assignees_change_service_spec.rb index cc595aab04b..0bf18f16abb 100644 --- a/spec/services/merge_requests/handle_assignees_change_service_spec.rb +++ b/spec/services/merge_requests/handle_assignees_change_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe MergeRequests::HandleAssigneesChangeService do let_it_be(:old_assignees) { create_list(:user, 3) } let(:options) { {} } - let(:service) { described_class.new(project, user) } + let(:service) { described_class.new(project: project, current_user: user) } before_all do project.add_maintainer(user) @@ -38,18 +38,6 @@ RSpec.describe MergeRequests::HandleAssigneesChangeService do async_execute end - - context 'when async_handle_merge_request_assignees_change feature is disabled' do - before do - stub_feature_flags(async_handle_merge_request_assignees_change: false) - end - - it 'calls #execute' do - expect(service).to receive(:execute).with(merge_request, old_assignees, options) - - async_execute - end - end end describe '#execute' do diff --git a/spec/services/merge_requests/link_lfs_objects_service_spec.rb b/spec/services/merge_requests/link_lfs_objects_service_spec.rb index c1765e3a2ab..2fb6bbaf02f 100644 --- a/spec/services/merge_requests/link_lfs_objects_service_spec.rb +++ b/spec/services/merge_requests/link_lfs_objects_service_spec.rb @@ -18,7 +18,7 @@ RSpec.describe MergeRequests::LinkLfsObjectsService, :sidekiq_inline do ) end - subject { described_class.new(target_project) } + subject { described_class.new(project: target_project) } shared_examples_for 'linking LFS objects' do context 'when source project is the same as target project' do diff --git a/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb b/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb index 1075f6f9034..4d7bd3d8800 100644 --- a/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb +++ b/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe MergeRequests::MarkReviewerReviewedService do let(:merge_request) { create(:merge_request, reviewers: [current_user]) } let(:reviewer) { merge_request.merge_request_reviewers.find_by(user_id: current_user.id) } let(:project) { merge_request.project } - let(:service) { described_class.new(project, current_user) } + let(:service) { described_class.new(project: project, current_user: current_user) } let(:result) { service.execute(merge_request) } before do @@ -16,7 +16,7 @@ RSpec.describe MergeRequests::MarkReviewerReviewedService do describe '#execute' do describe 'invalid permissions' do - let(:service) { described_class.new(project, create(:user)) } + let(:service) { described_class.new(project: project, current_user: create(:user)) } it 'returns an error' do expect(result[:status]).to eq :error @@ -24,7 +24,7 @@ RSpec.describe MergeRequests::MarkReviewerReviewedService do end describe 'reviewer does not exist' do - let(:service) { described_class.new(project, create(:user)) } + let(:service) { described_class.new(project: project, current_user: create(:user)) } it 'returns an error' do expect(result[:status]).to eq :error diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index c73cbad9d2f..ac39fb59c62 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe MergeRequests::MergeService do + include ExclusiveLeaseHelpers + let_it_be(:user) { create(:user) } let_it_be(:user2) { create(:user) } @@ -15,11 +17,14 @@ RSpec.describe MergeRequests::MergeService do end describe '#execute' do - let(:service) { described_class.new(project, user, merge_params) } + let(:service) { described_class.new(project: project, current_user: user, params: merge_params) } let(:merge_params) do { commit_message: 'Awesome message', sha: merge_request.diff_head_sha } end + let(:lease_key) { "merge_requests_merge_service:#{merge_request.id}" } + let!(:lease) { stub_exclusive_lease(lease_key) } + context 'valid params' do before do allow(service).to receive(:execute_hooks) @@ -90,6 +95,20 @@ RSpec.describe MergeRequests::MergeService do end end + context 'running the service multiple time' do + it 'is idempotent' do + 2.times { service.execute(merge_request) } + + expect(merge_request.merge_error).to be_falsey + expect(merge_request).to be_valid + expect(merge_request).to be_merged + + commit_messages = project.repository.commits('master', limit: 2).map(&:message) + expect(commit_messages.uniq.size).to eq(2) + expect(merge_request.in_progress_merge_commit_sha).to be_nil + end + end + context 'when an invalid sha is passed' do let(:merge_request) do create(:merge_request, :simple, @@ -209,7 +228,7 @@ RSpec.describe MergeRequests::MergeService do context 'source branch removal' do context 'when the source branch is protected' do let(:service) do - described_class.new(project, user, merge_params.merge('should_remove_source_branch' => true)) + described_class.new(project: project, current_user: user, params: merge_params.merge('should_remove_source_branch' => true)) end before do @@ -225,7 +244,7 @@ RSpec.describe MergeRequests::MergeService do context 'when the source branch is the default branch' do let(:service) do - described_class.new(project, user, merge_params.merge('should_remove_source_branch' => true)) + described_class.new(project: project, current_user: user, params: merge_params.merge('should_remove_source_branch' => true)) end before do @@ -251,7 +270,7 @@ RSpec.describe MergeRequests::MergeService do end context 'when the merger set the source branch not to be removed' do - let(:service) { described_class.new(project, user, merge_params.merge('should_remove_source_branch' => false)) } + let(:service) { described_class.new(project: project, current_user: user, params: merge_params.merge('should_remove_source_branch' => false)) } it 'does not delete the source branch' do expect(::MergeRequests::DeleteSourceBranchWorker).not_to receive(:perform_async) @@ -263,7 +282,7 @@ RSpec.describe MergeRequests::MergeService do context 'when MR merger set the source branch to be removed' do let(:service) do - described_class.new(project, user, merge_params.merge('should_remove_source_branch' => true)) + described_class.new(project: project, current_user: user, params: merge_params.merge('should_remove_source_branch' => true)) end it 'removes the source branch using the current user' do @@ -306,10 +325,12 @@ RSpec.describe MergeRequests::MergeService do end it 'logs and saves error if user is not authorized' do + stub_exclusive_lease + unauthorized_user = create(:user) project.add_reporter(unauthorized_user) - service = described_class.new(project, unauthorized_user) + service = described_class.new(project: project, current_user: unauthorized_user) service.execute(merge_request) @@ -423,6 +444,7 @@ RSpec.describe MergeRequests::MergeService do merge_request.project.update!(merge_method: merge_method) error_message = 'Only fast-forward merge is allowed for your project. Please update your source branch' allow(service).to receive(:execute_hooks) + expect(lease).to receive(:cancel) service.execute(merge_request) @@ -473,5 +495,17 @@ RSpec.describe MergeRequests::MergeService do end end end + + context 'when the other sidekiq worker has already been running' do + before do + stub_exclusive_lease_taken(lease_key) + end + + it 'does not execute service' do + expect(service).not_to receive(:commit) + + service.execute(merge_request) + end + end end end diff --git a/spec/services/merge_requests/merge_to_ref_service_spec.rb b/spec/services/merge_requests/merge_to_ref_service_spec.rb index 938165a807c..bb764ff5672 100644 --- a/spec/services/merge_requests/merge_to_ref_service_spec.rb +++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb @@ -74,7 +74,7 @@ RSpec.describe MergeRequests::MergeToRefService do describe '#execute' do let(:service) do - described_class.new(project, user, **params) + described_class.new(project: project, current_user: user, params: params) end let(:params) { { commit_message: 'Awesome message', should_remove_source_branch: true, sha: merge_request.diff_head_sha } } @@ -94,7 +94,7 @@ RSpec.describe MergeRequests::MergeToRefService do it 'returns an error when Gitlab::Git::CommandError is raised during merge' do allow(project.repository).to receive(:merge_to_ref) do - raise Gitlab::Git::CommandError.new('Failed to create merge commit') + raise Gitlab::Git::CommandError, 'Failed to create merge commit' end result = service.execute(merge_request) @@ -111,11 +111,11 @@ RSpec.describe MergeRequests::MergeToRefService do end let(:merge_ref_service) do - described_class.new(project, user, {}) + described_class.new(project: project, current_user: user) end let(:merge_service) do - MergeRequests::MergeService.new(project, user, { sha: merge_request.diff_head_sha }) + MergeRequests::MergeService.new(project: project, current_user: user, params: { sha: merge_request.diff_head_sha }) end context 'when merge commit' do diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb index e0baf5af8b4..65599b7e046 100644 --- a/spec/services/merge_requests/mergeability_check_service_spec.rb +++ b/spec/services/merge_requests/mergeability_check_service_spec.rb @@ -87,7 +87,7 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar described_class.new(merge_request).async_execute end - context 'when read only DB' do + context 'when read-only DB' do before do allow(Gitlab::Database).to receive(:read_only?) { true } end @@ -232,7 +232,7 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar context 'when MR cannot be merged and has outdated merge ref' do before do - MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request) + MergeRequests::MergeToRefService.new(project: project, current_user: merge_request.author).execute(merge_request) merge_request.mark_as_unmergeable! end @@ -258,7 +258,7 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar end end - context 'when read only DB' do + context 'when read-only DB' do it 'returns ServiceResponse.error' do allow(Gitlab::Database).to receive(:read_only?) { true } @@ -332,7 +332,7 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar context 'when MR is mergeable but merge-ref is already updated' do before do - MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request) + MergeRequests::MergeToRefService.new(project: project, current_user: merge_request.author).execute(merge_request) merge_request.mark_as_mergeable! end @@ -361,7 +361,7 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar context 'merge with conflicts' do it 'calls MergeToRefService with true allow_conflicts param' do expect(MergeRequests::MergeToRefService).to receive(:new) - .with(project, merge_request.author, { allow_conflicts: true }).and_call_original + .with(project: project, current_user: merge_request.author, params: { allow_conflicts: true }).and_call_original subject end @@ -373,7 +373,7 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar it 'calls MergeToRefService with false allow_conflicts param' do expect(MergeRequests::MergeToRefService).to receive(:new) - .with(project, merge_request.author, { allow_conflicts: false }).and_call_original + .with(project: project, current_user: merge_request.author, params: { allow_conflicts: false }).and_call_original subject end diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index 247b053e729..14804aa33d4 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe MergeRequests::PostMergeService do let_it_be(:merge_request, reload: true) { create(:merge_request, assignees: [user]) } let_it_be(:project) { merge_request.project } - subject { described_class.new(project, user).execute(merge_request) } + subject { described_class.new(project: project, current_user: user).execute(merge_request) } before do project.add_maintainer(user) @@ -22,7 +22,6 @@ RSpec.describe MergeRequests::PostMergeService do it 'refreshes the number of open merge requests for a valid MR', :use_clean_rails_memory_store_caching do # Cache the counter before the MR changed state. project.open_merge_requests_count - merge_request.update!(state: 'merged') expect { subject }.to change { project.open_merge_requests_count }.from(1).to(0) end diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index b5086ea3a82..87c3fc6a2d8 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -11,7 +11,7 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do let_it_be(:user3) { create(:user, developer_projects: [project]) } let_it_be(:forked_project) { fork_project(project, user1, repository: true) } - let(:service) { described_class.new(project, user1, changes, push_options) } + let(:service) { described_class.new(project: project, current_user: user1, changes: changes, push_options: push_options) } let(:source_branch) { 'fix' } let(:target_branch) { 'feature' } let(:title) { 'my title' } diff --git a/spec/services/merge_requests/pushed_branches_service_spec.rb b/spec/services/merge_requests/pushed_branches_service_spec.rb index cd6af4c275e..59424263ec5 100644 --- a/spec/services/merge_requests/pushed_branches_service_spec.rb +++ b/spec/services/merge_requests/pushed_branches_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe MergeRequests::PushedBranchesService do let(:project) { create(:project) } - let!(:service) { described_class.new(project, nil, changes: pushed_branches) } + let!(:service) { described_class.new(project: project, current_user: nil, params: { changes: pushed_branches }) } context 'when branches pushed' do let(:pushed_branches) do diff --git a/spec/services/merge_requests/rebase_service_spec.rb b/spec/services/merge_requests/rebase_service_spec.rb index 653fcf12a76..a46f3cf6148 100644 --- a/spec/services/merge_requests/rebase_service_spec.rb +++ b/spec/services/merge_requests/rebase_service_spec.rb @@ -18,7 +18,7 @@ RSpec.describe MergeRequests::RebaseService do let(:repository) { project.repository.raw } let(:skip_ci) { false } - subject(:service) { described_class.new(project, user, {}) } + subject(:service) { described_class.new(project: project, current_user: user) } before do project.add_maintainer(user) diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index f9b76db877b..6e6b4a91e0d 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -64,7 +64,7 @@ RSpec.describe MergeRequests::RefreshService do end context 'push to origin repo source branch' do - let(:refresh_service) { service.new(@project, @user) } + let(:refresh_service) { service.new(project: @project, current_user: @user) } let(:notification_service) { spy('notification_service') } before do @@ -187,7 +187,7 @@ RSpec.describe MergeRequests::RefreshService do context 'when pipeline exists for the source branch' do let!(:pipeline) { create(:ci_empty_pipeline, ref: @merge_request.source_branch, project: @project, sha: @commits.first.sha)} - subject { service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master') } + subject { service.new(project: @project, current_user: @user).execute(@oldrev, @newrev, 'refs/heads/master') } it 'updates the head_pipeline_id for @merge_request', :sidekiq_might_not_need_inline do expect { subject }.to change { @merge_request.reload.head_pipeline_id }.from(nil).to(pipeline.id) @@ -198,12 +198,12 @@ RSpec.describe MergeRequests::RefreshService do end end - shared_examples 'Pipelines for merge requests' do + context 'Pipelines for merge requests', :sidekiq_inline do before do stub_ci_pipeline_yaml_file(config) end - subject { service.new(project, @user).execute(@oldrev, @newrev, ref) } + subject { service.new(project: project, current_user: @user).execute(@oldrev, @newrev, ref) } let(:ref) { 'refs/heads/master' } let(:project) { @project } @@ -291,11 +291,11 @@ RSpec.describe MergeRequests::RefreshService do context "when MergeRequestUpdateWorker is retried by an exception" do it 'does not re-create a duplicate detached merge request pipeline' do expect do - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master') + service.new(project: @project, current_user: @user).execute(@oldrev, @newrev, 'refs/heads/master') end.to change { @merge_request.pipelines_for_merge_request.count }.by(1) expect do - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master') + service.new(project: @project, current_user: @user).execute(@oldrev, @newrev, 'refs/heads/master') end.not_to change { @merge_request.pipelines_for_merge_request.count } end end @@ -364,20 +364,8 @@ RSpec.describe MergeRequests::RefreshService do end end - context 'when the code_review_async_pipeline_creation feature flag is on', :sidekiq_inline do - it_behaves_like 'Pipelines for merge requests' - end - - context 'when the code_review_async_pipeline_creation feature flag is off', :sidekiq_inline do - before do - stub_feature_flags(code_review_async_pipeline_creation: false) - end - - it_behaves_like 'Pipelines for merge requests' - end - context 'push to origin repo source branch' do - let(:refresh_service) { service.new(@project, @user) } + let(:refresh_service) { service.new(project: @project, current_user: @user) } let(:notification_service) { spy('notification_service') } before do @@ -409,7 +397,7 @@ RSpec.describe MergeRequests::RefreshService do context 'push to origin repo target branch', :sidekiq_might_not_need_inline do context 'when all MRs to the target branch had diffs' do before do - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + service.new(project: @project, current_user: @user).execute(@oldrev, @newrev, 'refs/heads/feature') reload_mrs end @@ -438,7 +426,7 @@ RSpec.describe MergeRequests::RefreshService do # feature all along. empty_fork_merge_request.update_columns(target_branch: 'feature') - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + service.new(project: @project, current_user: @user).execute(@oldrev, @newrev, 'refs/heads/feature') reload_mrs empty_fork_merge_request.reload end @@ -461,7 +449,7 @@ RSpec.describe MergeRequests::RefreshService do # Merge master -> feature branch @project.repository.merge(@user, @merge_request.diff_head_sha, @merge_request, 'Test message') commit = @project.repository.commit('feature') - service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature') + service.new(project: @project, current_user: @user).execute(@oldrev, commit.id, 'refs/heads/feature') reload_mrs end @@ -479,7 +467,7 @@ RSpec.describe MergeRequests::RefreshService do end context 'push to fork repo source branch', :sidekiq_might_not_need_inline do - let(:refresh_service) { service.new(@fork_project, @user) } + let(:refresh_service) { service.new(project: @fork_project, current_user: @user) } def refresh allow(refresh_service).to receive(:execute_hooks) @@ -546,7 +534,7 @@ RSpec.describe MergeRequests::RefreshService do context 'push to fork repo target branch', :sidekiq_might_not_need_inline do describe 'changes to merge requests' do before do - service.new(@fork_project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + service.new(project: @fork_project, current_user: @user).execute(@oldrev, @newrev, 'refs/heads/feature') reload_mrs end @@ -563,7 +551,7 @@ RSpec.describe MergeRequests::RefreshService do describe 'merge request diff' do it 'does not reload the diff of the merge request made from fork' do expect do - service.new(@fork_project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + service.new(project: @fork_project, current_user: @user).execute(@oldrev, @newrev, 'refs/heads/feature') end.not_to change { @fork_merge_request.reload.merge_request_diff } end end @@ -594,28 +582,28 @@ RSpec.describe MergeRequests::RefreshService do it 'reloads a new diff for a push to the forked project' do expect do - service.new(@fork_project, @user).execute(@oldrev, first_commit, 'refs/heads/master') + service.new(project: @fork_project, current_user: @user).execute(@oldrev, first_commit, 'refs/heads/master') reload_mrs end.to change { forked_master_mr.merge_request_diffs.count }.by(1) end it 'reloads a new diff for a force push to the source branch' do expect do - service.new(@fork_project, @user).execute(@oldrev, force_push_commit, 'refs/heads/master') + service.new(project: @fork_project, current_user: @user).execute(@oldrev, force_push_commit, 'refs/heads/master') reload_mrs end.to change { forked_master_mr.merge_request_diffs.count }.by(1) end it 'reloads a new diff for a force push to the target branch' do expect do - service.new(@project, @user).execute(@oldrev, force_push_commit, 'refs/heads/master') + service.new(project: @project, current_user: @user).execute(@oldrev, force_push_commit, 'refs/heads/master') reload_mrs end.to change { forked_master_mr.merge_request_diffs.count }.by(1) end it 'reloads a new diff for a push to the target project that contains a commit in the MR' do expect do - service.new(@project, @user).execute(@oldrev, first_commit, 'refs/heads/master') + service.new(project: @project, current_user: @user).execute(@oldrev, first_commit, 'refs/heads/master') reload_mrs end.to change { forked_master_mr.merge_request_diffs.count }.by(1) end @@ -626,7 +614,7 @@ RSpec.describe MergeRequests::RefreshService do branch_name: 'master') expect do - service.new(@project, @user).execute(@newrev, new_commit, 'refs/heads/master') + service.new(project: @project, current_user: @user).execute(@newrev, new_commit, 'refs/heads/master') reload_mrs end.not_to change { forked_master_mr.merge_request_diffs.count } end @@ -635,7 +623,7 @@ RSpec.describe MergeRequests::RefreshService do context 'push to origin repo target branch after fork project was removed' do before do @fork_project.destroy! - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + service.new(project: @project, current_user: @user).execute(@oldrev, @newrev, 'refs/heads/feature') reload_mrs end @@ -651,7 +639,7 @@ RSpec.describe MergeRequests::RefreshService do end context 'push new branch that exists in a merge request' do - let(:refresh_service) { service.new(@fork_project, @user) } + let(:refresh_service) { service.new(project: @fork_project, current_user: @user) } it 'refreshes the merge request', :sidekiq_might_not_need_inline do expect(refresh_service).to receive(:execute_hooks) @@ -700,7 +688,7 @@ RSpec.describe MergeRequests::RefreshService do source_branch: 'close-by-commit', source_project: project) - refresh_service = service.new(project, user) + refresh_service = service.new(project: project, current_user: user) allow(refresh_service).to receive(:execute_hooks) refresh_service.execute(@oldrev, @newrev, 'refs/heads/close-by-commit') @@ -723,7 +711,7 @@ RSpec.describe MergeRequests::RefreshService do source_branch: 'close-by-commit', source_project: forked_project) - refresh_service = service.new(forked_project, user) + refresh_service = service.new(project: forked_project, current_user: user) allow(refresh_service).to receive(:execute_hooks) refresh_service.execute(@oldrev, @newrev, 'refs/heads/close-by-commit') @@ -734,7 +722,7 @@ RSpec.describe MergeRequests::RefreshService do end context 'marking the merge request as draft' do - let(:refresh_service) { service.new(@project, @user) } + let(:refresh_service) { service.new(project: @project, current_user: @user) } before do allow(refresh_service).to receive(:execute_hooks) @@ -814,7 +802,7 @@ RSpec.describe MergeRequests::RefreshService do end describe 'updating merge_commit' do - let(:service) { described_class.new(project, user) } + let(:service) { described_class.new(project: project, current_user: user) } let(:user) { create(:user) } let(:project) { create(:project, :repository) } @@ -902,7 +890,7 @@ RSpec.describe MergeRequests::RefreshService do end let(:auto_merge_strategy) { AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS } - let(:refresh_service) { service.new(project, user) } + let(:refresh_service) { service.new(project: project, current_user: user) } before do target_project.merge_method = merge_method diff --git a/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb b/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb index 3152a4e3861..b333d4af6cf 100644 --- a/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb +++ b/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe MergeRequests::ReloadMergeHeadDiffService do describe '#execute' do before do MergeRequests::MergeToRefService - .new(merge_request.project, merge_request.author) + .new(project: merge_request.project, current_user: merge_request.author) .execute(merge_request) end diff --git a/spec/services/merge_requests/remove_approval_service_spec.rb b/spec/services/merge_requests/remove_approval_service_spec.rb index 4ef2da290e1..ef6a0ec69bd 100644 --- a/spec/services/merge_requests/remove_approval_service_spec.rb +++ b/spec/services/merge_requests/remove_approval_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe MergeRequests::RemoveApprovalService do let(:merge_request) { create(:merge_request, source_project: project) } let!(:existing_approval) { create(:approval, merge_request: merge_request) } - subject(:service) { described_class.new(project, user) } + subject(:service) { described_class.new(project: project, current_user: user) } def execute! service.execute(merge_request) diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb index 8541d597581..b9df31b6727 100644 --- a/spec/services/merge_requests/reopen_service_spec.rb +++ b/spec/services/merge_requests/reopen_service_spec.rb @@ -20,7 +20,7 @@ RSpec.describe MergeRequests::ReopenService do it_behaves_like 'merge request reviewers cache counters invalidator' context 'valid params' do - let(:service) { described_class.new(project, user, {}) } + let(:service) { described_class.new(project: project, current_user: user) } before do allow(service).to receive(:execute_hooks) @@ -65,7 +65,7 @@ RSpec.describe MergeRequests::ReopenService do it 'caches merge request closing issues' do expect(merge_request).to receive(:cache_merge_request_closes_issues!) - described_class.new(project, user, {}).execute(merge_request) + described_class.new(project: project, current_user: user).execute(merge_request) end it 'updates metrics' do @@ -78,7 +78,7 @@ RSpec.describe MergeRequests::ReopenService do expect(service).to receive(:reopen) - described_class.new(project, user, {}).execute(merge_request) + described_class.new(project: project, current_user: user).execute(merge_request) end it 'calls the merge request activity counter' do @@ -86,11 +86,11 @@ RSpec.describe MergeRequests::ReopenService do .to receive(:track_reopen_mr_action) .with(user: user) - described_class.new(project, user, {}).execute(merge_request) + described_class.new(project: project, current_user: user).execute(merge_request) end it 'refreshes the number of open merge requests for a valid MR' do - service = described_class.new(project, user, {}) + service = described_class.new(project: project, current_user: user) expect { service.execute(merge_request) } .to change { project.open_merge_requests_count }.from(0).to(1) @@ -99,7 +99,7 @@ RSpec.describe MergeRequests::ReopenService do context 'current user is not authorized to reopen merge request' do before do perform_enqueued_jobs do - @merge_request = described_class.new(project, guest).execute(merge_request) + @merge_request = described_class.new(project: project, current_user: guest).execute(merge_request) end end diff --git a/spec/services/merge_requests/request_review_service_spec.rb b/spec/services/merge_requests/request_review_service_spec.rb index 5cb4120852a..8bc31df605c 100644 --- a/spec/services/merge_requests/request_review_service_spec.rb +++ b/spec/services/merge_requests/request_review_service_spec.rb @@ -8,7 +8,7 @@ RSpec.describe MergeRequests::RequestReviewService do let(:merge_request) { create(:merge_request, reviewers: [user]) } let(:reviewer) { merge_request.find_reviewer(user) } let(:project) { merge_request.project } - let(:service) { described_class.new(project, current_user) } + let(:service) { described_class.new(project: project, current_user: current_user) } let(:result) { service.execute(merge_request, user) } let(:todo_service) { spy('todo service') } let(:notification_service) { spy('notification service') } @@ -26,7 +26,7 @@ RSpec.describe MergeRequests::RequestReviewService do describe '#execute' do describe 'invalid permissions' do - let(:service) { described_class.new(project, create(:user)) } + let(:service) { described_class.new(project: project, current_user: create(:user)) } it 'returns an error' do expect(result[:status]).to eq :error diff --git a/spec/services/merge_requests/resolve_todos_service_spec.rb b/spec/services/merge_requests/resolve_todos_service_spec.rb index 3e6f2ea3f5d..53bd259f0f4 100644 --- a/spec/services/merge_requests/resolve_todos_service_spec.rb +++ b/spec/services/merge_requests/resolve_todos_service_spec.rb @@ -23,18 +23,6 @@ RSpec.describe MergeRequests::ResolveTodosService do async_execute end - - context 'when resolve_merge_request_todos_async feature is disabled' do - before do - stub_feature_flags(resolve_merge_request_todos_async: false) - end - - it 'calls #execute' do - expect(service).to receive(:execute) - - async_execute - end - end end describe '#execute' do diff --git a/spec/services/merge_requests/resolved_discussion_notification_service_spec.rb b/spec/services/merge_requests/resolved_discussion_notification_service_spec.rb index 874cf66659a..74f3a1b06fc 100644 --- a/spec/services/merge_requests/resolved_discussion_notification_service_spec.rb +++ b/spec/services/merge_requests/resolved_discussion_notification_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe MergeRequests::ResolvedDiscussionNotificationService do let(:user) { create(:user) } let(:project) { merge_request.project } - subject { described_class.new(project, user) } + subject { described_class.new(project: project, current_user: user) } describe "#execute" do context "when not all discussions are resolved" do diff --git a/spec/services/merge_requests/retarget_chain_service_spec.rb b/spec/services/merge_requests/retarget_chain_service_spec.rb index 3937fbe58c3..87bde4a1400 100644 --- a/spec/services/merge_requests/retarget_chain_service_spec.rb +++ b/spec/services/merge_requests/retarget_chain_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe MergeRequests::RetargetChainService do let_it_be(:merge_request, reload: true) { create(:merge_request, assignees: [user]) } let_it_be(:project) { merge_request.project } - subject { described_class.new(project, user).execute(merge_request) } + subject { described_class.new(project: project, current_user: user).execute(merge_request) } before do project.add_maintainer(user) diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb index acbd0a42fcd..149748cdabc 100644 --- a/spec/services/merge_requests/squash_service_spec.rb +++ b/spec/services/merge_requests/squash_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe MergeRequests::SquashService do include GitHelpers - let(:service) { described_class.new(project, user, { merge_request: merge_request }) } + let(:service) { described_class.new(project: project, current_user: user, params: { merge_request: merge_request }) } let(:user) { project.owner } let(:project) { create(:project, :repository) } let(:repository) { project.repository.raw } @@ -62,7 +62,7 @@ RSpec.describe MergeRequests::SquashService do end it 'will still perform the squash when a custom squash commit message has been provided' do - service = described_class.new(project, user, { merge_request: merge_request, squash_commit_message: 'A custom commit message' }) + service = described_class.new(project: project, current_user: user, params: { merge_request: merge_request, squash_commit_message: 'A custom commit message' }) expect(merge_request.target_project.repository).to receive(:squash).and_return('sha') @@ -98,7 +98,7 @@ RSpec.describe MergeRequests::SquashService do end context 'if a message was provided' do - let(:service) { described_class.new(project, user, { merge_request: merge_request, squash_commit_message: message }) } + let(:service) { described_class.new(project: project, current_user: user, params: { merge_request: merge_request, squash_commit_message: message }) } let(:message) { 'My custom message' } let(:squash_sha) { service.execute[:squash_sha] } diff --git a/spec/services/merge_requests/update_assignees_service_spec.rb b/spec/services/merge_requests/update_assignees_service_spec.rb index de03aab5418..076161c9029 100644 --- a/spec/services/merge_requests/update_assignees_service_spec.rb +++ b/spec/services/merge_requests/update_assignees_service_spec.rb @@ -26,7 +26,7 @@ RSpec.describe MergeRequests::UpdateAssigneesService do project.add_developer(user3) end - let(:service) { described_class.new(project, user, opts) } + let(:service) { described_class.new(project: project, current_user: user, params: opts) } let(:opts) { { assignee_ids: [user2.id] } } describe 'execute' do @@ -36,8 +36,24 @@ RSpec.describe MergeRequests::UpdateAssigneesService do end context 'when the parameters are valid' do + context 'when using sentinel values' do + let(:opts) { { assignee_ids: [0] } } + + it 'removes all assignees' do + expect { update_merge_request }.to change(merge_request, :assignees).to([]) + end + end + + context 'the assignee_ids parameter is the empty list' do + let(:opts) { { assignee_ids: [] } } + + it 'removes all assignees' do + expect { update_merge_request }.to change(merge_request, :assignees).to([]) + end + end + it 'updates the MR, and queues the more expensive work for later' do - expect_next(MergeRequests::HandleAssigneesChangeService, project, user) do |service| + expect_next(MergeRequests::HandleAssigneesChangeService, project: project, current_user: user) do |service| expect(service) .to receive(:async_execute) .with(merge_request, [user3], execute_hooks: true) @@ -56,7 +72,7 @@ RSpec.describe MergeRequests::UpdateAssigneesService do end it 'is more efficient than using the full update-service' do - allow_next(MergeRequests::HandleAssigneesChangeService, project, user) do |service| + allow_next(MergeRequests::HandleAssigneesChangeService, project: project, current_user: user) do |service| expect(service) .to receive(:async_execute) .with(merge_request, [user3], execute_hooks: true) @@ -69,7 +85,7 @@ RSpec.describe MergeRequests::UpdateAssigneesService do source_project: merge_request.project, author: merge_request.author) - update_service = ::MergeRequests::UpdateService.new(project, user, opts) + update_service = ::MergeRequests::UpdateService.new(project: project, current_user: user, params: opts) expect { service.execute(merge_request) } .to issue_fewer_queries_than { update_service.execute(other_mr) } diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 8c010855eb2..a85fbd77d70 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -43,7 +43,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end def update_merge_request(opts) - @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + @merge_request = MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) @merge_request.reload end @@ -64,7 +64,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do } end - let(:service) { described_class.new(project, current_user, opts) } + let(:service) { described_class.new(project: project, current_user: current_user, params: opts) } let(:current_user) { user } before do @@ -99,7 +99,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) .to receive(:track_description_edit_action).once.with(user: user) - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request2) + MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request2) end it 'tracks Draft/WIP marking' do @@ -108,7 +108,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts[:title] = "WIP: #{opts[:title]}" - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request2) + MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request2) end it 'tracks Draft/WIP un-marking' do @@ -117,7 +117,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts[:title] = "Non-draft/wip title string" - MergeRequests::UpdateService.new(project, user, opts).execute(draft_merge_request) + MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(draft_merge_request) end context 'when MR is locked' do @@ -128,7 +128,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts[:discussion_locked] = true - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) end end @@ -139,7 +139,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts[:discussion_locked] = false - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) end end end @@ -154,7 +154,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts[:discussion_locked] = false - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) end end @@ -165,7 +165,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts[:discussion_locked] = true - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) end end end @@ -184,7 +184,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do spent_at: Date.parse('2021-02-24') } - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) end it 'tracks milestone change' do @@ -193,7 +193,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts[:milestone] = milestone - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) end it 'track labels change' do @@ -202,7 +202,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts[:label_ids] = [label2.id] - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) end context 'reviewers' do @@ -213,7 +213,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts[:reviewers] = [user2] - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) end end @@ -224,7 +224,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts[:reviewers] = merge_request.reviewers - MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) end end end @@ -439,7 +439,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do let(:milestone) { create(:milestone, project: project) } let(:req_opts) { { source_branch: 'feature', target_branch: 'master' } } - subject { MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) } + subject { MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) } context 'when mentionable attributes change' do let(:opts) { { description: "Description with #{user.to_reference}" }.merge(req_opts) } @@ -486,7 +486,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do } end - let(:service) { described_class.new(project, user, opts) } + let(:service) { described_class.new(project: project, current_user: user, params: opts) } context 'without pipeline' do before do @@ -547,7 +547,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do context 'with a non-authorised user' do let(:visitor) { create(:user) } - let(:service) { described_class.new(project, visitor, opts) } + let(:service) { described_class.new(project: project, current_user: visitor, params: opts) } before do merge_request.update_attribute(:merge_error, 'Error') @@ -805,7 +805,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts = { title: 'New title' } perform_enqueued_jobs do - @merge_request = described_class.new(project, user, opts).execute(merge_request) + @merge_request = described_class.new(project: project, current_user: user, params: opts).execute(merge_request) end should_email(subscriber) @@ -818,7 +818,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts = { title: 'Draft: New title' } perform_enqueued_jobs do - @merge_request = described_class.new(project, user, opts).execute(merge_request) + @merge_request = described_class.new(project: project, current_user: user, params: opts).execute(merge_request) end should_not_email(subscriber) @@ -840,7 +840,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts = { label_ids: [label.id] } perform_enqueued_jobs do - @merge_request = described_class.new(project, user, opts).execute(merge_request) + @merge_request = described_class.new(project: project, current_user: user, params: opts).execute(merge_request) end should_email(subscriber) @@ -856,7 +856,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts = { label_ids: [label.id, label2.id] } perform_enqueued_jobs do - @merge_request = described_class.new(project, user, opts).execute(merge_request) + @merge_request = described_class.new(project: project, current_user: user, params: opts).execute(merge_request) end should_not_email(subscriber) @@ -867,7 +867,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do opts = { label_ids: [label2.id] } perform_enqueued_jobs do - @merge_request = described_class.new(project, user, opts).execute(merge_request) + @merge_request = described_class.new(project: project, current_user: user, params: opts).execute(merge_request) end should_not_email(subscriber) @@ -933,7 +933,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do it 'creates a `MergeRequestsClosingIssues` record for each issue' do issue_closing_opts = { description: "Closes #{first_issue.to_reference} and #{second_issue.to_reference}" } - service = described_class.new(project, user, issue_closing_opts) + service = described_class.new(project: project, current_user: user, params: issue_closing_opts) allow(service).to receive(:execute_hooks) service.execute(merge_request) @@ -945,7 +945,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do create(:merge_requests_closing_issues, issue: first_issue, merge_request: merge_request) create(:merge_requests_closing_issues, issue: second_issue, merge_request: merge_request) - service = described_class.new(project, user, description: "not closing any issues") + service = described_class.new(project: project, current_user: user, params: { description: "not closing any issues" }) allow(service).to receive(:execute_hooks) service.execute(merge_request.reload) @@ -1002,7 +1002,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do it 'unassigns assignee when user id is 0' do merge_request.update!(assignee_ids: [user.id]) - expect_next_instance_of(MergeRequests::HandleAssigneesChangeService, project, user) do |service| + expect_next_instance_of(MergeRequests::HandleAssigneesChangeService, project: project, current_user: user) do |service| expect(service) .to receive(:async_execute) .with(merge_request, [user]) @@ -1014,7 +1014,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end it 'saves assignee when user id is valid' do - expect_next_instance_of(MergeRequests::HandleAssigneesChangeService, project, user) do |service| + expect_next_instance_of(MergeRequests::HandleAssigneesChangeService, project: project, current_user: user) do |service| expect(service) .to receive(:async_execute) .with(merge_request, [user3]) @@ -1052,6 +1052,35 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end end + context 'when adding time spent' do + let(:spend_time) { { duration: 1800, user_id: user3.id } } + + context ':use_specialized_service' do + context 'when true' do + it 'passes the update action to ::MergeRequests::AddSpentTimeService' do + expect(::MergeRequests::AddSpentTimeService) + .to receive(:new).and_call_original + + update_merge_request(spend_time: spend_time, use_specialized_service: true) + end + end + + context 'when false or nil' do + before do + expect(::MergeRequests::AddSpentTimeService).not_to receive(:new) + end + + it 'does not pass the update action to ::MergeRequests::UpdateAssigneesService when false' do + update_merge_request(spend_time: spend_time, use_specialized_service: false) + end + + it 'does not pass the update action to ::MergeRequests::UpdateAssigneesService when nil' do + update_merge_request(spend_time: spend_time, use_specialized_service: nil) + end + end + end + end + include_examples 'issuable update service' do let(:open_issuable) { merge_request } let(:closed_issuable) { create(:closed_merge_request, source_project: project) } @@ -1145,7 +1174,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do it_behaves_like 'issuable record that supports quick actions' do let(:existing_merge_request) { create(:merge_request, source_project: project) } - let(:issuable) { described_class.new(project, user, params).execute(existing_merge_request) } + let(:issuable) { described_class.new(project: project, current_user: user, params: params).execute(existing_merge_request) } end end end |