diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-09-19 01:45:44 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-09-19 01:45:44 +0000 |
commit | 85dc423f7090da0a52c73eb66faf22ddb20efff9 (patch) | |
tree | 9160f299afd8c80c038f08e1545be119f5e3f1e1 /spec/services/merge_requests | |
parent | 15c2c8c66dbe422588e5411eee7e68f1fa440bb8 (diff) | |
download | gitlab-ce-85dc423f7090da0a52c73eb66faf22ddb20efff9.tar.gz |
Add latest changes from gitlab-org/gitlab@13-4-stable-ee
Diffstat (limited to 'spec/services/merge_requests')
12 files changed, 402 insertions, 52 deletions
diff --git a/spec/services/merge_requests/base_service_spec.rb b/spec/services/merge_requests/base_service_spec.rb new file mode 100644 index 00000000000..bb7b70f1ba2 --- /dev/null +++ b/spec/services/merge_requests/base_service_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::BaseService do + include ProjectForksHelper + + let_it_be(:project) { create(:project, :repository) } + let(:title) { 'Awesome merge_request' } + let(:params) do + { + title: title, + description: 'please fix', + source_branch: 'feature', + target_branch: 'master' + } + end + + subject { MergeRequests::CreateService.new(project, project.owner, params) } + + describe '#execute_hooks' do + shared_examples 'enqueues Jira sync worker' do + it do + Sidekiq::Testing.fake! do + expect { subject.execute }.to change(JiraConnect::SyncMergeRequestWorker.jobs, :size).by(1) + end + end + end + + shared_examples 'does not enqueue Jira sync worker' do + it do + Sidekiq::Testing.fake! do + expect { subject.execute }.not_to change(JiraConnect::SyncMergeRequestWorker.jobs, :size) + end + end + end + + context 'with a Jira subscription' do + before do + create(:jira_connect_subscription, namespace: project.namespace) + end + + context 'MR contains Jira issue key' do + let(:title) { 'Awesome merge_request with issue JIRA-123' } + + it_behaves_like 'enqueues Jira sync worker' + end + + context 'MR does not contain Jira issue key' do + it_behaves_like 'does not enqueue Jira sync worker' + end + end + + context 'without a Jira subscription' do + it_behaves_like 'does not enqueue Jira sync worker' + end + end +end diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index f99be26927d..f83b8d98ce8 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -88,6 +88,10 @@ RSpec.describe MergeRequests::BuildService do let(:source_project) { fork_project(project, user) } let(:merge_request) { described_class.new(project, user, mr_params).execute } + before do + project.add_reporter(user) + end + it 'assigns force_remove_source_branch' do expect(merge_request.force_remove_source_branch?).to be_truthy end @@ -510,7 +514,7 @@ RSpec.describe MergeRequests::BuildService do let(:target_project) { create(:project, :public, :repository) } before do - target_project.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + target_project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) end it 'sets the target_project correctly' do diff --git a/spec/services/merge_requests/cleanup_refs_service_spec.rb b/spec/services/merge_requests/cleanup_refs_service_spec.rb new file mode 100644 index 00000000000..b38ccee4aa0 --- /dev/null +++ b/spec/services/merge_requests/cleanup_refs_service_spec.rb @@ -0,0 +1,146 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::CleanupRefsService do + describe '.schedule' do + let(:merge_request) { build(:merge_request) } + + it 'schedules MergeRequestCleanupRefsWorker' do + expect(MergeRequestCleanupRefsWorker) + .to receive(:perform_in) + .with(described_class::TIME_THRESHOLD, merge_request.id) + + described_class.schedule(merge_request) + end + end + + describe '#execute' do + before do + # Need to re-enable this as it's being stubbed in spec_helper for + # performance reasons but is needed to run for this test. + allow(Gitlab::Git::KeepAround).to receive(:execute).and_call_original + end + + subject(:result) { described_class.new(merge_request).execute } + + shared_examples_for 'service that cleans up merge request refs' do + it 'creates keep around ref and deletes merge request refs' do + old_ref_head = ref_head + + aggregate_failures do + expect(result[:status]).to eq(:success) + expect(kept_around?(old_ref_head)).to be_truthy + expect(ref_head).to be_nil + end + end + + context 'when merge request has merge ref' do + before do + MergeRequests::MergeToRefService + .new(merge_request.project, merge_request.author) + .execute(merge_request) + end + + it 'caches merge ref sha and deletes merge ref' do + old_merge_ref_head = merge_request.merge_ref_head + + aggregate_failures do + expect(result[:status]).to eq(:success) + expect(kept_around?(old_merge_ref_head)).to be_truthy + expect(merge_request.reload.merge_ref_sha).to eq(old_merge_ref_head.id) + expect(ref_exists?(merge_request.merge_ref_path)).to be_falsy + end + end + + context 'when merge ref sha cannot be cached' do + before do + allow(merge_request) + .to receive(:update_column) + .with(:merge_ref_sha, merge_request.merge_ref_head.id) + .and_return(false) + end + + it_behaves_like 'service that does not clean up merge request refs' + end + end + + context 'when keep around ref cannot be created' do + before do + allow_next_instance_of(Gitlab::Git::KeepAround) do |keep_around| + expect(keep_around).to receive(:kept_around?).and_return(false) + end + end + + it_behaves_like 'service that does not clean up merge request refs' + end + end + + shared_examples_for 'service that does not clean up merge request refs' do + it 'does not delete merge request refs' do + aggregate_failures do + expect(result[:status]).to eq(:error) + expect(ref_head).to be_present + end + end + end + + context 'when merge request is closed' do + let(:merge_request) { create(:merge_request, :closed) } + + context "when closed #{described_class::TIME_THRESHOLD.inspect} ago" do + before do + merge_request.metrics.update!(latest_closed_at: described_class::TIME_THRESHOLD.ago) + end + + it_behaves_like 'service that cleans up merge request refs' + end + + context "when closed later than #{described_class::TIME_THRESHOLD.inspect} ago" do + before do + merge_request.metrics.update!(latest_closed_at: (described_class::TIME_THRESHOLD - 1.day).ago) + end + + it_behaves_like 'service that does not clean up merge request refs' + end + end + + context 'when merge request is merged' do + let(:merge_request) { create(:merge_request, :merged) } + + context "when merged #{described_class::TIME_THRESHOLD.inspect} ago" do + before do + merge_request.metrics.update!(merged_at: described_class::TIME_THRESHOLD.ago) + end + + it_behaves_like 'service that cleans up merge request refs' + end + + context "when merged later than #{described_class::TIME_THRESHOLD.inspect} ago" do + before do + merge_request.metrics.update!(merged_at: (described_class::TIME_THRESHOLD - 1.day).ago) + end + + it_behaves_like 'service that does not clean up merge request refs' + end + end + + context 'when merge request is not closed nor merged' do + let(:merge_request) { create(:merge_request, :opened) } + + it_behaves_like 'service that does not clean up merge request refs' + end + end + + def kept_around?(commit) + Gitlab::Git::KeepAround.new(merge_request.project.repository).kept_around?(commit.id) + end + + def ref_head + merge_request.project.repository.commit(merge_request.ref_path) + end + + def ref_exists?(ref) + merge_request.project.repository.ref_exists?(ref) + end +end diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index e518e439a84..e7ac286f48b 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -99,6 +99,12 @@ RSpec.describe MergeRequests::CloseService do described_class.new(project, 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) + end + context 'current user is not authorized to close merge request' do before do perform_enqueued_jobs do diff --git a/spec/services/merge_requests/conflicts/list_service_spec.rb b/spec/services/merge_requests/conflicts/list_service_spec.rb index 14133731e37..5132eac0158 100644 --- a/spec/services/merge_requests/conflicts/list_service_spec.rb +++ b/spec/services/merge_requests/conflicts/list_service_spec.rb @@ -30,14 +30,13 @@ RSpec.describe MergeRequests::Conflicts::ListService do it 'returns a falsey value when one of the MR branches is missing' do merge_request = create_merge_request('conflict-resolvable') merge_request.project.repository.rm_branch(merge_request.author, 'conflict-resolvable') - merge_request.clear_memoized_source_branch_exists expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey end it 'returns a falsey value when the MR does not support new diff notes' do merge_request = create_merge_request('conflict-resolvable') - merge_request.merge_request_diff.update(start_commit_sha: nil) + merge_request.merge_request_diff.update!(start_commit_sha: nil) expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey end diff --git a/spec/services/merge_requests/create_pipeline_service_spec.rb b/spec/services/merge_requests/create_pipeline_service_spec.rb index db46bd37eea..4dd70627977 100644 --- a/spec/services/merge_requests/create_pipeline_service_spec.rb +++ b/spec/services/merge_requests/create_pipeline_service_spec.rb @@ -5,13 +5,14 @@ require 'spec_helper' RSpec.describe MergeRequests::CreatePipelineService do include ProjectForksHelper - let_it_be(:project) { create(:project, :repository) } + let_it_be(:project, reload: true) { create(:project, :repository) } let_it_be(:user) { create(:user) } let(:service) { described_class.new(project, actor, params) } let(:actor) { user } let(:params) { {} } before do + stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false) project.add_developer(user) end @@ -58,9 +59,27 @@ RSpec.describe MergeRequests::CreatePipelineService do expect(subject.project).to eq(project) end - context 'when ci_allow_to_create_merge_request_pipelines_in_target_project feature flag is disabled' do + context 'when source branch is protected' do + context 'when actor does not have permission to update the protected branch in target project' do + let!(:protected_branch) { create(:protected_branch, name: '*', project: project) } + + it 'creates a pipeline in the source project' do + expect(subject.project).to eq(source_project) + end + end + + context 'when actor has permission to update the protected branch in target project' do + let!(:protected_branch) { create(:protected_branch, :developers_can_merge, name: '*', project: project) } + + it 'creates a pipeline in the target project' do + expect(subject.project).to eq(project) + end + end + end + + context 'when ci_disallow_to_create_merge_request_pipelines_in_target_project feature flag is enabled' do before do - stub_feature_flags(ci_allow_to_create_merge_request_pipelines_in_target_project: false) + stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: true) end it 'creates a pipeline in the source project' do diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index bb62e594e7a..d042b318d02 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do let(:project) { create(:project, :repository) } let(:user) { create(:user) } - let(:assignee) { create(:user) } + let(:user2) { create(:user) } describe '#execute' do context 'valid params' do @@ -26,7 +26,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do before do project.add_maintainer(user) - project.add_developer(assignee) + project.add_developer(user2) allow(service).to receive(:execute_hooks) end @@ -75,7 +75,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do description: "well this is not done yet\n/wip", source_branch: 'feature', target_branch: 'master', - assignees: [assignee] + assignees: [user2] } end @@ -91,7 +91,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do description: "well this is not done yet\n/wip", source_branch: 'feature', target_branch: 'master', - assignees: [assignee] + assignees: [user2] } end @@ -108,17 +108,17 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do description: 'please fix', source_branch: 'feature', target_branch: 'master', - assignees: [assignee] + assignees: [user2] } end - it { expect(merge_request.assignees).to eq([assignee]) } + it { expect(merge_request.assignees).to eq([user2]) } it 'creates a todo for new assignee' do attributes = { project: project, author: user, - user: assignee, + user: user2, target_id: merge_request.id, target_type: merge_request.class.name, action: Todo::ASSIGNED, @@ -129,6 +129,34 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end end + context 'when reviewer is assigned' do + let(:opts) do + { + title: 'Awesome merge_request', + description: 'please fix', + source_branch: 'feature', + target_branch: 'master', + reviewers: [user2] + } + end + + it { expect(merge_request.reviewers).to eq([user2]) } + + it 'creates a todo for new reviewer' do + attributes = { + project: project, + author: user, + user: user2, + target_id: merge_request.id, + target_type: merge_request.class.name, + action: Todo::REVIEW_REQUESTED, + state: :pending + } + + expect(Todo.where(attributes).count).to eq 1 + end + end + context 'when head pipelines already exist for merge request source branch', :sidekiq_inline do let(:shas) { project.repository.commits(opts[:source_branch], limit: 2).map(&:id) } let!(:pipeline_1) { create(:ci_pipeline, project: project, ref: opts[:source_branch], project_id: project.id, sha: shas[1]) } @@ -212,7 +240,8 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end before do - target_project.add_developer(assignee) + stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false) + target_project.add_developer(user2) target_project.add_maintainer(user) end @@ -338,6 +367,10 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end end end + + it_behaves_like 'reviewer_ids filter' do + let(:execute) { service.execute } + end end it_behaves_like 'issuable record that supports quick actions' do @@ -361,7 +394,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do assignee_ids: create(:user).id, milestone_id: 1, title: 'Title', - description: %(/assign @#{assignee.username}\n/milestone %"#{milestone.name}"), + description: %(/assign @#{user2.username}\n/milestone %"#{milestone.name}"), source_branch: 'feature', target_branch: 'master' } @@ -369,12 +402,12 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do before do project.add_maintainer(user) - project.add_maintainer(assignee) + project.add_maintainer(user2) end it 'assigns and sets milestone to issuable from command' do expect(merge_request).to be_persisted - expect(merge_request.assignees).to eq([assignee]) + expect(merge_request.assignees).to eq([user2]) expect(merge_request.milestone).to eq(milestone) end end @@ -382,7 +415,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do context 'merge request create service' do context 'asssignee_id' do - let(:assignee) { create(:user) } + let(:user2) { create(:user) } before do project.add_maintainer(user) @@ -405,12 +438,12 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end it 'saves assignee when user id is valid' do - project.add_maintainer(assignee) - opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } + project.add_maintainer(user2) + opts = { title: 'Title', description: 'Description', assignee_ids: [user2.id] } merge_request = described_class.new(project, user, opts).execute - expect(merge_request.assignees).to eq([assignee]) + expect(merge_request.assignees).to eq([user2]) end context 'when assignee is set' do @@ -418,24 +451,24 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do { title: 'Title', description: 'Description', - assignee_ids: [assignee.id], + assignee_ids: [user2.id], source_branch: 'feature', target_branch: 'master' } end it 'invalidates open merge request counter for assignees when merge request is assigned' do - project.add_maintainer(assignee) + project.add_maintainer(user2) described_class.new(project, user, opts).execute - expect(assignee.assigned_open_merge_requests_count).to eq 1 + expect(user2.assigned_open_merge_requests_count).to eq 1 end end context "when issuable feature is private" do before do - project.project_feature.update(issues_access_level: ProjectFeature::PRIVATE, + project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE, merge_requests_access_level: ProjectFeature::PRIVATE) end @@ -443,8 +476,8 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do levels.each do |level| it "removes not authorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do - project.update(visibility_level: level) - opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } + project.update!(visibility_level: level) + opts = { title: 'Title', description: 'Description', assignee_ids: [user2.id] } merge_request = described_class.new(project, user, opts).execute @@ -470,7 +503,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do before do project.add_maintainer(user) - project.add_developer(assignee) + project.add_developer(user2) end it 'creates a `MergeRequestsClosingIssues` record for each issue' do @@ -498,7 +531,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do context 'when user can not access source project' do before do - target_project.add_developer(assignee) + target_project.add_developer(user2) target_project.add_maintainer(user) end @@ -510,7 +543,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do context 'when user can not access target project' do before do - target_project.add_developer(assignee) + target_project.add_developer(user2) target_project.add_maintainer(user) end @@ -562,7 +595,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end before do - project.add_developer(assignee) + project.add_developer(user2) project.add_maintainer(user) 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 377615bbc6f..cdaacaf5fca 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 @@ -19,7 +19,7 @@ RSpec.describe MergeRequests::DeleteNonLatestDiffsService, :clean_gitlab_redis_s expect(diffs.count).to eq(4) - Timecop.freeze do + freeze_time do expect(DeleteDiffFilesWorker) .to receive(:bulk_perform_in) .with(5.minutes, [[diffs.first.id], [diffs.second.id]]) diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 11e341994f7..8328f461029 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -152,6 +152,7 @@ RSpec.describe MergeRequests::MergeService do let(:commit) { double('commit', safe_message: "Fixes #{jira_issue.to_reference}") } before do + stub_jira_service_test project.update!(has_external_issue_tracker: true) jira_service_settings stub_jira_urls(jira_issue.id) @@ -175,7 +176,7 @@ RSpec.describe MergeRequests::MergeService do end it 'does not close issue' do - jira_tracker.update(jira_issue_transition_id: nil) + jira_tracker.update!(jira_issue_transition_id: nil) expect_any_instance_of(JiraService).not_to receive(:transition_issue) @@ -388,7 +389,7 @@ RSpec.describe MergeRequests::MergeService do error_message = 'Failed to squash. Should be done manually' allow_any_instance_of(MergeRequests::SquashService).to receive(:squash!).and_return(nil) - merge_request.update(squash: true) + merge_request.update!(squash: true) service.execute(merge_request) @@ -402,7 +403,7 @@ RSpec.describe MergeRequests::MergeService do error_message = 'another squash is already in progress' allow_any_instance_of(MergeRequest).to receive(:squash_in_progress?).and_return(true) - merge_request.update(squash: true) + merge_request.update!(squash: true) service.execute(merge_request) @@ -420,7 +421,7 @@ RSpec.describe MergeRequests::MergeService do %w(semi-linear ff).each do |merge_method| it "logs and saves error if merge is #{merge_method} only" do merge_method = 'rebase_merge' if merge_method == 'semi-linear' - merge_request.project.update(merge_method: merge_method) + 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) @@ -434,6 +435,43 @@ RSpec.describe MergeRequests::MergeService do end end end + + context 'when not mergeable' do + let!(:error_message) { 'Merge request is not mergeable' } + + context 'with failing CI' do + before do + allow(merge_request).to receive(:mergeable_ci_state?) { false } + end + + it 'logs and saves error' do + service.execute(merge_request) + + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + end + end + + context 'with unresolved discussions' do + before do + allow(merge_request).to receive(:mergeable_discussions_state?) { false } + end + + it 'logs and saves error' do + service.execute(merge_request) + + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + end + + context 'when passing `skip_discussions_check: true` as `options` parameter' do + it 'merges the merge request' do + service.execute(merge_request, skip_discussions_check: true) + + expect(merge_request).to be_valid + expect(merge_request).to be_merged + end + end + end + end 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 a51a896ca96..402f753c0af 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -50,7 +50,7 @@ RSpec.describe MergeRequests::PostMergeService do end it 'marks MR as merged regardless of errors when closing issues' do - merge_request.update(target_branch: 'foo') + merge_request.update!(target_branch: 'foo') allow(project).to receive(:default_branch).and_return('foo') issue = create(:issue, project: project) @@ -72,6 +72,12 @@ RSpec.describe MergeRequests::PostMergeService do subject end + it 'schedules CleanupRefsService' do + expect(MergeRequests::CleanupRefsService).to receive(:schedule).with(merge_request) + + subject + end + context 'when the merge request has review apps' do it 'cancels all review app deployments' do pipeline = create(:ci_pipeline, diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 0696e8a247f..cace1e0bf09 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -225,6 +225,10 @@ RSpec.describe MergeRequests::RefreshService do context 'when service runs on forked project' do let(:project) { @fork_project } + before do + stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false) + end + it 'creates detached merge request pipeline for fork merge request', :sidekiq_inline do expect { subject } .to change { @fork_merge_request.pipelines_for_merge_request.count }.by(1) @@ -617,7 +621,7 @@ RSpec.describe MergeRequests::RefreshService do before do stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - @fork_project.destroy + @fork_project.destroy! service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') reload_mrs end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index c3433c8c9d2..6b7463d4996 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -52,6 +52,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do title: 'New title', description: 'Also please fix', assignee_ids: [user.id], + reviewer_ids: [user.id], state_event: 'close', label_ids: [label.id], target_branch: 'target', @@ -75,6 +76,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(@merge_request).to be_valid expect(@merge_request.title).to eq('New title') expect(@merge_request.assignees).to match_array([user]) + expect(@merge_request.reviewers).to match_array([user]) expect(@merge_request).to be_closed expect(@merge_request.labels.count).to eq(1) expect(@merge_request.labels.first.title).to eq(label.name) @@ -161,6 +163,29 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(@merge_request.merge_params["force_remove_source_branch"]).to eq("1") end end + + it_behaves_like 'reviewer_ids filter' do + let(:opts) { {} } + let(:execute) { update_merge_request(opts) } + end + + context 'with an existing reviewer' do + let(:merge_request) do + create(:merge_request, :simple, source_project: project, reviewer_ids: [user2.id]) + end + + context 'when merge_request_reviewer feature is enabled' do + before do + stub_feature_flags(merge_request_reviewer: true) + end + + let(:opts) { { reviewer_ids: [IssuableFinder::Params::NONE] } } + + it 'removes reviewers' do + expect(update_merge_request(opts).reviewers).to eq [] + end + end + end end context 'after_save callback to store_mentions' do @@ -379,11 +404,31 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end end - context 'when the milestone is removed' do + context 'when reviewers gets changed' do before do - stub_feature_flags(track_resource_milestone_change_events: false) + update_merge_request({ reviewer_ids: [user2.id] }) + end + + it 'marks pending todo as done' do + expect(pending_todo.reload).to be_done + end + + it 'creates a pending todo for new review request' do + attributes = { + project: project, + author: user, + user: user2, + target_id: merge_request.id, + target_type: merge_request.class.name, + action: Todo::REVIEW_REQUESTED, + state: :pending + } + + expect(Todo.where(attributes).count).to eq 1 end + end + context 'when the milestone is removed' do let!(:non_subscriber) { create(:user) } let!(:subscriber) do @@ -393,12 +438,10 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end end - it_behaves_like 'system notes for milestones' - it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do merge_request.milestone = create(:milestone, project: project) - merge_request.save + merge_request.save! perform_enqueued_jobs do update_merge_request(milestone_id: "") @@ -410,10 +453,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end context 'when the milestone is changed' do - before do - stub_feature_flags(track_resource_milestone_change_events: false) - end - let!(:non_subscriber) { create(:user) } let!(:subscriber) do @@ -429,8 +468,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(pending_todo.reload).to be_done end - it_behaves_like 'system notes for milestones' - it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do perform_enqueued_jobs do update_merge_request(milestone: create(:milestone, project: project)) @@ -628,7 +665,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do context 'updating asssignee_ids' do it 'does not update assignee when assignee_id is invalid' do - merge_request.update(assignee_ids: [user.id]) + merge_request.update!(assignee_ids: [user.id]) update_merge_request(assignee_ids: [-1]) @@ -636,7 +673,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end it 'unassigns assignee when user id is 0' do - merge_request.update(assignee_ids: [user.id]) + merge_request.update!(assignee_ids: [user.id]) update_merge_request(assignee_ids: [0]) @@ -664,7 +701,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do levels.each do |level| it "does not update with unauthorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do assignee = create(:user) - project.update(visibility_level: level) + project.update!(visibility_level: level) feature_visibility_attr = :"#{merge_request.model_name.plural}_access_level" project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE) |