diff options
Diffstat (limited to 'spec/services/merge_requests')
15 files changed, 514 insertions, 90 deletions
diff --git a/spec/services/merge_requests/after_create_service_spec.rb b/spec/services/merge_requests/after_create_service_spec.rb index 9ae310d8cee..f21feb70bc5 100644 --- a/spec/services/merge_requests/after_create_service_spec.rb +++ b/spec/services/merge_requests/after_create_service_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' RSpec.describe MergeRequests::AfterCreateService do - include AfterNextHelpers - let_it_be(:merge_request) { create(:merge_request) } subject(:after_create_service) do @@ -66,15 +64,8 @@ RSpec.describe MergeRequests::AfterCreateService do execute_service end - it 'registers an onboarding progress action' do - OnboardingProgress.onboard(merge_request.target_project.namespace) - - expect_next(OnboardingProgressService, merge_request.target_project.namespace) - .to receive(:execute).with(action: :merge_request_created).and_call_original - - execute_service - - expect(OnboardingProgress.completed?(merge_request.target_project.namespace, :merge_request_created)).to be(true) + it_behaves_like 'records an onboarding progress action', :merge_request_created do + let(:namespace) { merge_request.target_project.namespace } end end end diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb index 124501f17d5..df9a98c5540 100644 --- a/spec/services/merge_requests/approval_service_spec.rb +++ b/spec/services/merge_requests/approval_service_spec.rb @@ -31,6 +31,13 @@ RSpec.describe MergeRequests::ApprovalService do expect(todo.reload).to be_pending end + + it 'does not track merge request approve action' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .not_to receive(:track_approve_mr_action).with(user: user) + + service.execute(merge_request) + end end context 'with valid approval' do @@ -59,6 +66,13 @@ RSpec.describe MergeRequests::ApprovalService do service.execute(merge_request) end end + + it 'tracks merge request approve action' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_approve_mr_action).with(user: user) + + service.execute(merge_request) + end end context 'user cannot update the merge request' do diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index f83b8d98ce8..22b3456708f 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -252,6 +252,7 @@ RSpec.describe MergeRequests::BuildService do issue.update!(iid: 123) else create(:"#{issue_tracker}_service", project: project) + project.reload end end @@ -351,6 +352,7 @@ RSpec.describe MergeRequests::BuildService do issue.update!(iid: 123) else create(:"#{issue_tracker}_service", project: project) + project.reload 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 be8c41bc4a1..6528edfc8b7 100644 --- a/spec/services/merge_requests/create_from_issue_service_spec.rb +++ b/spec/services/merge_requests/create_from_issue_service_spec.rb @@ -52,6 +52,14 @@ RSpec.describe MergeRequests::CreateFromIssueService do service.execute end + it 'tracks the mr creation when the mr is valid' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_mr_create_from_issue) + .with(user: user) + + service.execute + end + it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created', :sidekiq_might_not_need_inline do expect_next_instance_of(MergeRequest) do |instance| expect(instance).to receive(:valid?).at_least(:once).and_return(false) @@ -62,6 +70,17 @@ RSpec.describe MergeRequests::CreateFromIssueService do service.execute end + it 'does not track the mr creation when the Mr is invalid' do + expect_next_instance_of(MergeRequest) do |instance| + expect(instance).to receive(:valid?).at_least(:once).and_return(false) + end + + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .not_to receive(:track_mr_create_from_issue) + + service.execute + end + it 'creates a merge request', :sidekiq_might_not_need_inline do expect { service.execute }.to change(target_project.merge_requests, :count).by(1) end diff --git a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb index cdaacaf5fca..d2070a466b1 100644 --- a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb +++ b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb @@ -12,6 +12,8 @@ RSpec.describe MergeRequests::DeleteNonLatestDiffsService, :clean_gitlab_redis_s stub_const("#{described_class.name}::BATCH_SIZE", 2) 3.times { merge_request.create_merge_request_diff } + merge_request.create_merge_head_diff + merge_request.reset end it 'schedules non-latest merge request diffs removal' 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 new file mode 100644 index 00000000000..1075f6f9034 --- /dev/null +++ b/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::MarkReviewerReviewedService do + let(:current_user) { create(:user) } + 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(:result) { service.execute(merge_request) } + + before do + project.add_developer(current_user) + end + + describe '#execute' do + describe 'invalid permissions' do + let(:service) { described_class.new(project, create(:user)) } + + it 'returns an error' do + expect(result[:status]).to eq :error + end + end + + describe 'reviewer does not exist' do + let(:service) { described_class.new(project, create(:user)) } + + it 'returns an error' do + expect(result[:status]).to eq :error + end + end + + describe 'reviewer exists' do + it 'returns success' do + expect(result[:status]).to eq :success + end + + it 'updates reviewers state' do + expect(result[:status]).to eq :success + expect(reviewer.state).to eq 'reviewed' + end + end + end +end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index dd37d87e3f5..611f12c8146 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -161,7 +161,7 @@ RSpec.describe MergeRequests::MergeService do commit = double('commit', safe_message: "Fixes #{jira_issue.to_reference}") allow(merge_request).to receive(:commits).and_return([commit]) - expect_any_instance_of(JiraService).to receive(:close_issue).with(merge_request, jira_issue).once + expect_any_instance_of(JiraService).to receive(:close_issue).with(merge_request, jira_issue, user).once service.execute(merge_request) end @@ -310,12 +310,12 @@ RSpec.describe MergeRequests::MergeService do it 'logs and saves error if there is an exception' do error_message = 'error message' - allow(service).to receive(:repository).and_raise('error message') + allow(service).to receive(:repository).and_raise(error_message) allow(service).to receive(:execute_hooks) service.execute(merge_request) - expect(merge_request.merge_error).to include('Something went wrong during merge') + expect(merge_request.merge_error).to eq(described_class::GENERIC_ERROR_MESSAGE) expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end @@ -343,9 +343,7 @@ RSpec.describe MergeRequests::MergeService do expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end - it 'logs and saves error if there is a merge conflict' do - error_message = 'Conflicts detected during merge' - + it 'logs and saves error if commit is not created' do allow_any_instance_of(Repository).to receive(:merge).and_return(false) allow(service).to receive(:execute_hooks) @@ -353,8 +351,8 @@ RSpec.describe MergeRequests::MergeService do expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil - expect(merge_request.merge_error).to include(error_message) - expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + expect(merge_request.merge_error).to include(described_class::GENERIC_ERROR_MESSAGE) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(described_class::GENERIC_ERROR_MESSAGE)) end context 'when squashing is required' do diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb index 17bfa9d7368..e0baf5af8b4 100644 --- a/spec/services/merge_requests/mergeability_check_service_spec.rb +++ b/spec/services/merge_requests/mergeability_check_service_spec.rb @@ -33,6 +33,14 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar expect(merge_request.merge_status).to eq('can_be_merged') end + it 'reloads merge head diff' do + expect_next_instance_of(MergeRequests::ReloadMergeHeadDiffService) do |service| + expect(service).to receive(:execute) + end + + subject + end + it 'update diff discussion positions' do expect_next_instance_of(Discussions::CaptureDiffNotePositionsService) do |service| expect(service).to receive(:execute) @@ -142,7 +150,11 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar end it 'resets one merge request upon execution' do - expect_any_instance_of(MergeRequest).to receive(:reset).once + expect_next_instance_of(MergeRequests::ReloadMergeHeadDiffService) do |svc| + expect(svc).to receive(:execute).and_return(status: :success) + end + + expect_any_instance_of(MergeRequest).to receive(:reset).once.and_call_original execute_within_threads(amount: 2) end @@ -266,6 +278,14 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar it_behaves_like 'unmergeable merge request' + it 'reloads merge head diff' do + expect_next_instance_of(MergeRequests::ReloadMergeHeadDiffService) do |service| + expect(service).to receive(:execute) + end + + subject + end + it 'returns ServiceResponse.error' do result = subject @@ -329,6 +349,12 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar subject end + + it 'does not reload merge head diff' do + expect(MergeRequests::ReloadMergeHeadDiffService).not_to receive(:new) + + subject + end end end diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index 6523b5a158c..71329905558 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -3,9 +3,11 @@ require 'spec_helper' RSpec.describe MergeRequests::PostMergeService do - let(:user) { create(:user) } - let(:merge_request) { create(:merge_request, assignees: [user]) } - let(:project) { merge_request.project } + include ProjectForksHelper + + let_it_be(:user) { create(:user) } + 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) } @@ -128,5 +130,139 @@ RSpec.describe MergeRequests::PostMergeService do expect(deploy_job.reload.canceled?).to be false end end + + context 'for a merge request chain' do + before do + ::MergeRequests::UpdateService + .new(project, user, force_remove_source_branch: '1') + .execute(merge_request) + end + + context 'when there is another MR' do + let!(:another_merge_request) do + create(:merge_request, + source_project: source_project, + source_branch: 'my-awesome-feature', + target_project: merge_request.source_project, + target_branch: merge_request.source_branch + ) + end + + shared_examples 'does not retarget merge request' do + it 'another merge request is unchanged' do + expect { subject }.not_to change { another_merge_request.reload.target_branch } + .from(merge_request.source_branch) + end + end + + shared_examples 'retargets merge request' do + it 'another merge request is retargeted' do + expect(SystemNoteService) + .to receive(:change_branch).once + .with(another_merge_request, another_merge_request.project, user, + 'target', 'delete', + merge_request.source_branch, merge_request.target_branch) + + expect { subject }.to change { another_merge_request.reload.target_branch } + .from(merge_request.source_branch) + .to(merge_request.target_branch) + end + + context 'when FF retarget_merge_requests is disabled' do + before do + stub_feature_flags(retarget_merge_requests: false) + end + + include_examples 'does not retarget merge request' + end + + context 'when source branch is to be kept' do + before do + ::MergeRequests::UpdateService + .new(project, user, force_remove_source_branch: false) + .execute(merge_request) + end + + include_examples 'does not retarget merge request' + end + end + + context 'in the same project' do + let(:source_project) { project } + + it_behaves_like 'retargets merge request' + + context 'and is closed' do + before do + another_merge_request.close + end + + it_behaves_like 'does not retarget merge request' + end + + context 'and is merged' do + before do + another_merge_request.mark_as_merged + end + + it_behaves_like 'does not retarget merge request' + end + end + + context 'in forked project' do + let!(:source_project) { fork_project(project) } + + context 'when user has access to source project' do + before do + source_project.add_developer(user) + end + + it_behaves_like 'retargets merge request' + end + + context 'when user does not have access to source project' do + it_behaves_like 'does not retarget merge request' + end + end + + context 'and current and another MR is from a fork' do + let(:project) { create(:project) } + let(:source_project) { fork_project(project) } + + let(:merge_request) do + create(:merge_request, + source_project: source_project, + target_project: project + ) + end + + before do + source_project.add_developer(user) + end + + it_behaves_like 'does not retarget merge request' + end + end + + context 'when many merge requests are to be retargeted' do + let!(:many_merge_requests) do + create_list(:merge_request, 10, :unique_branches, + source_project: merge_request.source_project, + target_project: merge_request.source_project, + target_branch: merge_request.source_branch + ) + end + + it 'retargets only 4 of them' do + subject + + expect(many_merge_requests.each(&:reload).pluck(:target_branch).tally) + .to eq( + merge_request.source_branch => 6, + merge_request.target_branch => 4 + ) + end + end + end end 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 85bcf4562b1..c2769d4fa88 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -21,6 +21,7 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } let(:deleted_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 #{Gitlab::Git::BLANK_SHA} refs/heads/#{source_branch}" } let(:default_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{project.default_branch}" } + let(:error_mr_required) { "A merge_request.create push option is required to create a merge request for branch #{source_branch}" } shared_examples_for 'a service that can create a merge request' do subject(:last_mr) { MergeRequest.last } @@ -176,11 +177,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -197,11 +196,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -263,11 +260,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -308,11 +303,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -329,11 +322,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -374,11 +365,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -395,11 +384,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -440,11 +427,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -461,11 +446,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -506,11 +489,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -527,11 +508,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 3ccf02fcdfb..747ecbf4fa4 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -633,31 +633,37 @@ RSpec.describe MergeRequests::RefreshService do end context 'merge request metrics' do - let(:issue) { create :issue, project: @project } - let(:commit_author) { create :user } + let(:user) { create(:user) } + let(:project) { create(:project, :repository) } + let(:issue) { create(:issue, project: project) } let(:commit) { project.commit } before do - project.add_developer(commit_author) project.add_developer(user) allow(commit).to receive_messages( safe_message: "Closes #{issue.to_reference}", references: [issue], - author_name: commit_author.name, - author_email: commit_author.email, + author_name: user.name, + author_email: user.email, committed_date: Time.current ) - - allow_any_instance_of(MergeRequest).to receive(:commits).and_return(CommitCollection.new(@project, [commit], 'feature')) end context 'when the merge request is sourced from the same project' do it 'creates a `MergeRequestsClosingIssues` record for each issue closed by a commit' do - merge_request = create(:merge_request, target_branch: 'master', source_branch: 'feature', source_project: @project) - refresh_service = service.new(@project, @user) + allow_any_instance_of(MergeRequest).to receive(:commits).and_return( + CommitCollection.new(project, [commit], 'close-by-commit') + ) + + merge_request = create(:merge_request, + target_branch: 'master', + source_branch: 'close-by-commit', + source_project: project) + + refresh_service = service.new(project, user) allow(refresh_service).to receive(:execute_hooks) - refresh_service.execute(@oldrev, @newrev, 'refs/heads/feature') + refresh_service.execute(@oldrev, @newrev, 'refs/heads/close-by-commit') issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id) expect(issue_ids).to eq([issue.id]) @@ -666,16 +672,21 @@ RSpec.describe MergeRequests::RefreshService do context 'when the merge request is sourced from a different project' do it 'creates a `MergeRequestsClosingIssues` record for each issue closed by a commit' do - forked_project = fork_project(@project, @user, repository: true) + forked_project = fork_project(project, user, repository: true) + + allow_any_instance_of(MergeRequest).to receive(:commits).and_return( + CommitCollection.new(forked_project, [commit], 'close-by-commit') + ) merge_request = create(:merge_request, target_branch: 'master', - source_branch: 'feature', - target_project: @project, + target_project: project, + source_branch: 'close-by-commit', source_project: forked_project) - refresh_service = service.new(@project, @user) + + refresh_service = service.new(forked_project, user) allow(refresh_service).to receive(:execute_hooks) - refresh_service.execute(@oldrev, @newrev, 'refs/heads/feature') + refresh_service.execute(@oldrev, @newrev, 'refs/heads/close-by-commit') issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id) expect(issue_ids).to eq([issue.id]) 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 new file mode 100644 index 00000000000..3152a4e3861 --- /dev/null +++ b/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::ReloadMergeHeadDiffService do + let(:merge_request) { create(:merge_request) } + + subject { described_class.new(merge_request).execute } + + describe '#execute' do + before do + MergeRequests::MergeToRefService + .new(merge_request.project, merge_request.author) + .execute(merge_request) + end + + it 'creates a merge head diff' do + expect(subject[:status]).to eq(:success) + expect(merge_request.reload.merge_head_diff).to be_present + end + + context 'when merge ref head is not present' do + before do + allow(merge_request).to receive(:merge_ref_head).and_return(nil) + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + end + end + + context 'when failed to create merge head diff' do + before do + allow(merge_request).to receive(:create_merge_head_diff!).and_raise('fail') + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + end + end + + context 'when there is existing merge head diff' do + let!(:existing_merge_head_diff) { create(:merge_request_diff, :merge_head, merge_request: merge_request) } + + it 'recreates merge head diff' do + expect(subject[:status]).to eq(:success) + expect(merge_request.reload.merge_head_diff).not_to eq(existing_merge_head_diff) + end + end + + context 'when default_merge_ref_for_diffs feature flag is disabled' do + before do + stub_feature_flags(default_merge_ref_for_diffs: false) + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + end + end + end +end diff --git a/spec/services/merge_requests/remove_approval_service_spec.rb b/spec/services/merge_requests/remove_approval_service_spec.rb index 40da928e832..4ef2da290e1 100644 --- a/spec/services/merge_requests/remove_approval_service_spec.rb +++ b/spec/services/merge_requests/remove_approval_service_spec.rb @@ -32,6 +32,13 @@ RSpec.describe MergeRequests::RemoveApprovalService do execute! end + + it 'tracks merge request unapprove action' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_unapprove_mr_action).with(user: user) + + execute! + end end context 'with a user who has not approved' do @@ -41,6 +48,13 @@ RSpec.describe MergeRequests::RemoveApprovalService do execute! end + + it 'does not track merge request unapprove action' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .not_to receive(:track_unapprove_mr_action).with(user: user) + + execute! + end end end end diff --git a/spec/services/merge_requests/request_review_service_spec.rb b/spec/services/merge_requests/request_review_service_spec.rb new file mode 100644 index 00000000000..5cb4120852a --- /dev/null +++ b/spec/services/merge_requests/request_review_service_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::RequestReviewService do + let(:current_user) { create(:user) } + let(:user) { create(:user) } + 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(:result) { service.execute(merge_request, user) } + let(:todo_service) { spy('todo service') } + let(:notification_service) { spy('notification service') } + + before do + allow(NotificationService).to receive(:new) { notification_service } + allow(service).to receive(:todo_service).and_return(todo_service) + allow(service).to receive(:notification_service).and_return(notification_service) + + reviewer.update!(state: MergeRequestReviewer.states[:reviewed]) + + project.add_developer(current_user) + project.add_developer(user) + end + + describe '#execute' do + describe 'invalid permissions' do + let(:service) { described_class.new(project, create(:user)) } + + it 'returns an error' do + expect(result[:status]).to eq :error + end + end + + describe 'reviewer does not exist' do + let(:result) { service.execute(merge_request, create(:user)) } + + it 'returns an error' do + expect(result[:status]).to eq :error + end + end + + describe 'reviewer exists' do + it 'returns success' do + expect(result[:status]).to eq :success + end + + it 'updates reviewers state' do + service.execute(merge_request, user) + reviewer.reload + + expect(reviewer.state).to eq 'unreviewed' + end + + it 'sends email to reviewer' do + expect(notification_service).to receive_message_chain(:async, :review_requested_of_merge_request).with(merge_request, current_user, user) + + service.execute(merge_request, user) + end + + it 'creates a new todo for the reviewer' do + expect(todo_service).to receive(:create_request_review_todo).with(merge_request, current_user, user) + + service.execute(merge_request, user) + end + end + end +end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 9eb82dcd0ad..edb95840604 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -87,6 +87,38 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(@merge_request.discussion_locked).to be_truthy end + context 'usage counters' do + let(:merge_request2) { create(:merge_request) } + let(:draft_merge_request) { create(:merge_request, :draft_merge_request)} + + it 'update as expected' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_title_edit_action).once.with(user: user) + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_description_edit_action).once.with(user: user) + + MergeRequests::UpdateService.new(project, user, opts).execute(merge_request2) + end + + it 'tracks Draft/WIP marking' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_marked_as_draft_action).once.with(user: user) + + opts[:title] = "WIP: #{opts[:title]}" + + MergeRequests::UpdateService.new(project, user, opts).execute(merge_request2) + end + + it 'tracks Draft/WIP un-marking' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_unmarked_as_draft_action).once.with(user: user) + + opts[:title] = "Non-draft/wip title string" + + MergeRequests::UpdateService.new(project, user, opts).execute(draft_merge_request) + end + end + context 'updating milestone' do RSpec.shared_examples 'updates milestone' do it 'sets milestone' do @@ -142,29 +174,19 @@ RSpec.describe MergeRequests::UpdateService, :mailer do context 'with reviewers' do let(:opts) { { reviewer_ids: [user2.id] } } - context 'when merge_request_reviewers feature is disabled' do - before(:context) do - stub_feature_flags(merge_request_reviewers: false) - end - - it 'does not create a system note about merge_request review request' do - note = find_note('review requested from') + it 'creates system note about merge_request review request' do + note = find_note('requested review from') - expect(note).to be_nil - end + expect(note).not_to be_nil + expect(note.note).to include "requested review from #{user2.to_reference}" end - context 'when merge_request_reviewers feature is enabled' do - before(:context) do - stub_feature_flags(merge_request_reviewers: true) - end - - it 'creates system note about merge_request review request' do - note = find_note('requested review from') + it 'updates the tracking' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_users_review_requested) + .with(users: [user]) - expect(note).not_to be_nil - expect(note.note).to include "requested review from #{user2.to_reference}" - end + update_merge_request(reviewer_ids: [user.id]) end end @@ -794,6 +816,14 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(merge_request.assignee_ids).to eq([user.id]) end + it 'updates the tracking when user ids are valid' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_users_assigned_to_mr) + .with(users: [user]) + + update_merge_request(assignee_ids: [user.id]) + end + it 'does not update assignee_id when user cannot read issue' do non_member = create(:user) original_assignees = merge_request.assignees @@ -883,6 +913,33 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end end + context 'updating `target_branch`' do + let(:merge_request) do + create(:merge_request, + source_project: project, + source_branch: 'mr-b', + target_branch: 'mr-a') + end + + it 'updates to master' do + expect(SystemNoteService).to receive(:change_branch).with( + merge_request, project, user, 'target', 'update', 'mr-a', 'master' + ) + + expect { update_merge_request(target_branch: 'master') } + .to change { merge_request.reload.target_branch }.from('mr-a').to('master') + end + + it 'updates to master because of branch deletion' do + expect(SystemNoteService).to receive(:change_branch).with( + merge_request, project, user, 'target', 'delete', 'mr-a', 'master' + ) + + expect { update_merge_request(target_branch: 'master', target_branch_was_deleted: true) } + .to change { merge_request.reload.target_branch }.from('mr-a').to('master') + end + end + 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) } |