diff options
Diffstat (limited to 'spec/services')
88 files changed, 4409 insertions, 933 deletions
diff --git a/spec/services/application_settings/update_service_spec.rb b/spec/services/application_settings/update_service_spec.rb index a641828faa5..adb5219d691 100644 --- a/spec/services/application_settings/update_service_spec.rb +++ b/spec/services/application_settings/update_service_spec.rb @@ -62,6 +62,54 @@ describe ApplicationSettings::UpdateService do end end + describe 'updating outbound_local_requests_whitelist' do + context 'when params is blank' do + let(:params) { {} } + + it 'does not add to whitelist' do + expect { subject.execute }.not_to change { + application_settings.outbound_local_requests_whitelist + } + end + end + + context 'when param add_to_outbound_local_requests_whitelist contains values' do + before do + application_settings.outbound_local_requests_whitelist = ['127.0.0.1'] + end + + let(:params) { { add_to_outbound_local_requests_whitelist: ['example.com', ''] } } + + it 'adds to whitelist' do + expect { subject.execute }.to change { + application_settings.outbound_local_requests_whitelist + } + + expect(application_settings.outbound_local_requests_whitelist).to contain_exactly( + '127.0.0.1', 'example.com' + ) + end + end + + context 'when param outbound_local_requests_whitelist_raw is passed' do + before do + application_settings.outbound_local_requests_whitelist = ['127.0.0.1'] + end + + let(:params) { { outbound_local_requests_whitelist_raw: 'example.com;gitlab.com' } } + + it 'overwrites the existing whitelist' do + expect { subject.execute }.to change { + application_settings.outbound_local_requests_whitelist + } + + expect(application_settings.outbound_local_requests_whitelist).to contain_exactly( + 'example.com', 'gitlab.com' + ) + end + end + end + describe 'performance bar settings' do using RSpec::Parameterized::TableSyntax @@ -180,4 +228,20 @@ describe ApplicationSettings::UpdateService do described_class.new(application_settings, admin, { home_page_url: 'http://foo.bar' }).execute end end + + context 'when raw_blob_request_limit is passsed' do + let(:params) do + { + raw_blob_request_limit: 600 + } + end + + it 'updates raw_blob_request_limit value' do + subject.execute + + application_settings.reload + + expect(application_settings.raw_blob_request_limit).to eq(600) + end + end end diff --git a/spec/services/audit_event_service_spec.rb b/spec/services/audit_event_service_spec.rb index 32fd98e6ef9..e42bff607b2 100644 --- a/spec/services/audit_event_service_spec.rb +++ b/spec/services/audit_event_service_spec.rb @@ -10,11 +10,8 @@ describe AuditEventService do let(:logger) { instance_double(Gitlab::AuditJsonLogger) } describe '#security_event' do - before do - expect(service).to receive(:file_logger).and_return(logger) - end - it 'creates an event and logs to a file' do + expect(service).to receive(:file_logger).and_return(logger) expect(logger).to receive(:info).with(author_id: user.id, entity_id: project.id, entity_type: "Project", @@ -22,5 +19,32 @@ describe AuditEventService do expect { service.security_event }.to change(SecurityEvent, :count).by(1) end + + it 'formats from and to fields' do + service = described_class.new( + user, project, + { + from: true, + to: false, + action: :create, + target_id: 1 + }) + expect(service).to receive(:file_logger).and_return(logger) + expect(logger).to receive(:info).with(author_id: user.id, + entity_type: 'Project', + entity_id: project.id, + from: 'true', + to: 'false', + action: :create, + target_id: 1) + + expect { service.security_event }.to change(SecurityEvent, :count).by(1) + + details = SecurityEvent.last.details + expect(details[:from]).to be true + expect(details[:to]).to be false + expect(details[:action]).to eq(:create) + expect(details[:target_id]).to eq(1) + end end end diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb index 4f4776bbb27..3ca389ba25b 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -145,6 +145,19 @@ describe Auth::ContainerRegistryAuthenticationService do it_behaves_like 'not a container repository factory' end + describe '#pull_access_token' do + let(:project) { create(:project) } + let(:token) { described_class.pull_access_token(project.full_path) } + + subject { { token: token } } + + it_behaves_like 'an accessible' do + let(:actions) { ['pull'] } + end + + it_behaves_like 'not a container repository factory' + end + context 'user authorization' do let(:current_user) { create(:user) } diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb index cd08e0b6f32..a409f21a7e4 100644 --- a/spec/services/auto_merge/base_service_spec.rb +++ b/spec/services/auto_merge/base_service_spec.rb @@ -59,6 +59,11 @@ describe AutoMerge::BaseService do context 'when strategy is merge when pipeline succeeds' do let(:service) { AutoMerge::MergeWhenPipelineSucceedsService.new(project, user) } + before do + pipeline = build(:ci_pipeline) + allow(merge_request).to receive(:actual_head_pipeline) { pipeline } + end + it 'sets the auto merge strategy' do subject @@ -116,11 +121,7 @@ describe AutoMerge::BaseService do end end - describe '#cancel' do - subject { service.cancel(merge_request) } - - let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } - + shared_examples_for 'Canceled or Dropped' do it 'removes properies from the merge request' do subject @@ -168,6 +169,20 @@ describe AutoMerge::BaseService do it 'does not yield block' do expect { |b| service.execute(merge_request, &b) }.not_to yield_control end + end + end + + describe '#cancel' do + subject { service.cancel(merge_request) } + + let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } + + it_behaves_like 'Canceled or Dropped' + + context 'when failed to save' do + before do + allow(merge_request).to receive(:save) { false } + end it 'returns error status' do expect(subject[:status]).to eq(:error) @@ -175,4 +190,24 @@ describe AutoMerge::BaseService do end end end + + describe '#abort' do + subject { service.abort(merge_request, reason) } + + let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } + let(:reason) { 'an error'} + + it_behaves_like 'Canceled or Dropped' + + context 'when failed to save' do + before do + allow(merge_request).to receive(:save) { false } + end + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq("Can't abort the automatic merge") + end + end + end end diff --git a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb index a20bf8e17e4..931b52470c4 100644 --- a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb @@ -64,8 +64,11 @@ describe AutoMerge::MergeWhenPipelineSucceedsService do end it 'creates a system note' do + pipeline = build(:ci_pipeline) + allow(merge_request).to receive(:actual_head_pipeline) { pipeline } + note = merge_request.notes.last - expect(note.note).to match %r{enabled an automatic merge when the pipeline for (\w+/\w+@)?\h{8}} + expect(note.note).to match "enabled an automatic merge when the pipeline for #{pipeline.sha}" end end @@ -174,6 +177,17 @@ describe AutoMerge::MergeWhenPipelineSucceedsService do end end + describe "#abort" do + before do + service.abort(mr_merge_if_green_enabled, 'an error') + end + + it 'posts a system note' do + note = mr_merge_if_green_enabled.notes.last + expect(note.note).to include 'aborted the automatic merge' + end + end + describe 'pipeline integration' do context 'when there are multiple stages in the pipeline' do let(:ref) { mr_merge_if_green_enabled.source_branch } diff --git a/spec/services/auto_merge_service_spec.rb b/spec/services/auto_merge_service_spec.rb index 93a22e60498..50dfc49a59c 100644 --- a/spec/services/auto_merge_service_spec.rb +++ b/spec/services/auto_merge_service_spec.rb @@ -161,4 +161,29 @@ describe AutoMergeService do end end end + + describe '#abort' do + subject { service.abort(merge_request, error) } + + let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } + let(:error) { 'an error' } + + it 'delegates to a relevant service instance' do + expect_next_instance_of(AutoMerge::MergeWhenPipelineSucceedsService) do |service| + expect(service).to receive(:abort).with(merge_request, error) + end + + subject + end + + context 'when auto merge is not enabled' do + let(:merge_request) { create(:merge_request) } + + it 'returns error' do + expect(subject[:message]).to eq("Can't abort the automatic merge") + expect(subject[:status]).to eq(:error) + expect(subject[:http_status]).to eq(406) + end + end + end end diff --git a/spec/services/boards/issues/list_service_spec.rb b/spec/services/boards/issues/list_service_spec.rb index 40878e24cb4..931b67b2950 100644 --- a/spec/services/boards/issues/list_service_spec.rb +++ b/spec/services/boards/issues/list_service_spec.rb @@ -112,7 +112,7 @@ describe Boards::Issues::ListService do it_behaves_like 'issues list service' end - context 'and group is an ancestor', :nested_groups do + context 'and group is an ancestor' do let(:parent) { create(:group) } let(:group) { create(:group, parent: parent) } let!(:backlog) { create(:backlog_list, board: board) } diff --git a/spec/services/boards/issues/move_service_spec.rb b/spec/services/boards/issues/move_service_spec.rb index 16e2a2fba6b..cf84ec8fd4c 100644 --- a/spec/services/boards/issues/move_service_spec.rb +++ b/spec/services/boards/issues/move_service_spec.rb @@ -52,5 +52,91 @@ describe Boards::Issues::MoveService do it_behaves_like 'issues move service', true end + + describe '#execute_multiple' do + set(:group) { create(:group) } + set(:user) { create(:user) } + set(:project) { create(:project, namespace: group) } + set(:board1) { create(:board, group: group) } + set(:development) { create(:group_label, group: group, name: 'Development') } + set(:testing) { create(:group_label, group: group, name: 'Testing') } + set(:list1) { create(:list, board: board1, label: development, position: 0) } + set(:list2) { create(:list, board: board1, label: testing, position: 1) } + let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list2.id } } + + before do + project.add_developer(user) + end + + it 'returns the expected result if list of issues is empty' do + expect(described_class.new(group, user, params).execute_multiple([])).to eq({ count: 0, success: false, issues: [] }) + end + + context 'moving multiple issues' do + let(:issue1) { create(:labeled_issue, project: project, labels: [development]) } + let(:issue2) { create(:labeled_issue, project: project, labels: [development]) } + + it 'moves multiple issues from one list to another' do + expect(described_class.new(group, user, params).execute_multiple([issue1, issue2])).to be_truthy + + expect(issue1.labels).to eq([testing]) + expect(issue2.labels).to eq([testing]) + end + end + + context 'moving a single issue' do + let(:issue1) { create(:labeled_issue, project: project, labels: [development]) } + + it 'moves one issue' do + expect(described_class.new(group, user, params).execute_multiple([issue1])).to be_truthy + + expect(issue1.labels).to eq([testing]) + end + end + + context 'moving issues visually after an existing issue' do + let(:existing_issue) { create(:labeled_issue, project: project, labels: [testing], relative_position: 10) } + let(:issue1) { create(:labeled_issue, project: project, labels: [development]) } + let(:issue2) { create(:labeled_issue, project: project, labels: [development]) } + + let(:move_params) do + params.dup.tap do |hash| + hash[:move_before_id] = existing_issue.id + end + end + + it 'moves one issue' do + expect(described_class.new(group, user, move_params).execute_multiple([issue1, issue2])).to be_truthy + + expect(issue1.labels).to eq([testing]) + expect(issue2.labels).to eq([testing]) + + expect(issue1.relative_position > existing_issue.relative_position).to eq(true) + expect(issue2.relative_position > issue1.relative_position).to eq(true) + end + end + + context 'moving issues visually before an existing issue' do + let(:existing_issue) { create(:labeled_issue, project: project, labels: [testing], relative_position: 10) } + let(:issue1) { create(:labeled_issue, project: project, labels: [development]) } + let(:issue2) { create(:labeled_issue, project: project, labels: [development]) } + + let(:move_params) do + params.dup.tap do |hash| + hash[:move_after_id] = existing_issue.id + end + end + + it 'moves one issue' do + expect(described_class.new(group, user, move_params).execute_multiple([issue1, issue2])).to be_truthy + + expect(issue1.labels).to eq([testing]) + expect(issue2.labels).to eq([testing]) + + expect(issue2.relative_position < existing_issue.relative_position).to eq(true) + expect(issue1.relative_position < issue2.relative_position).to eq(true) + end + end + end end end diff --git a/spec/services/boards/visits/latest_service_spec.rb b/spec/services/boards/visits/latest_service_spec.rb deleted file mode 100644 index c8a0a5e4243..00000000000 --- a/spec/services/boards/visits/latest_service_spec.rb +++ /dev/null @@ -1,59 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Boards::Visits::LatestService do - describe '#execute' do - let(:user) { create(:user) } - - context 'when a project board' do - let(:project) { create(:project) } - let(:project_board) { create(:board, project: project) } - - subject(:service) { described_class.new(project_board.parent, user) } - - it 'returns nil when there is no user' do - service.current_user = nil - - expect(service.execute).to eq nil - end - - it 'queries for most recent visit' do - expect(BoardProjectRecentVisit).to receive(:latest).once - - service.execute - end - - it 'queries for last N visits' do - expect(BoardProjectRecentVisit).to receive(:latest).with(user, project, count: 5).once - - described_class.new(project_board.parent, user, count: 5).execute - end - end - - context 'when a group board' do - let(:group) { create(:group) } - let(:group_board) { create(:board, group: group) } - - subject(:service) { described_class.new(group_board.parent, user) } - - it 'returns nil when there is no user' do - service.current_user = nil - - expect(service.execute).to eq nil - end - - it 'queries for most recent visit' do - expect(BoardGroupRecentVisit).to receive(:latest).once - - service.execute - end - - it 'queries for last N visits' do - expect(BoardGroupRecentVisit).to receive(:latest).with(user, group, count: 5).once - - described_class.new(group_board.parent, user, count: 5).execute - end - end - end -end diff --git a/spec/services/branches/diverging_commit_counts_service_spec.rb b/spec/services/branches/diverging_commit_counts_service_spec.rb new file mode 100644 index 00000000000..370da773ab2 --- /dev/null +++ b/spec/services/branches/diverging_commit_counts_service_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Branches::DivergingCommitCountsService do + let(:project) { create(:project, :repository) } + let(:repository) { project.repository } + + describe '#call' do + let(:diverged_branch) { repository.find_branch('fix') } + let(:root_ref_sha) { repository.raw_repository.commit(repository.root_ref).id } + let(:diverged_branch_sha) { diverged_branch.dereferenced_target.sha } + + let(:service) { described_class.new(repository) } + + it 'returns the commit counts behind and ahead of default branch' do + result = service.call(diverged_branch) + + expect(result).to eq(behind: 29, ahead: 2) + end + + it 'calls diverging_commit_count without max count' do + expect(repository.raw_repository) + .to receive(:diverging_commit_count) + .with(root_ref_sha, diverged_branch_sha) + .and_return([29, 2]) + + service.call(diverged_branch) + end + end +end diff --git a/spec/services/ci/archive_trace_service_spec.rb b/spec/services/ci/archive_trace_service_spec.rb index 44a77c29086..454db3d5a48 100644 --- a/spec/services/ci/archive_trace_service_spec.rb +++ b/spec/services/ci/archive_trace_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Ci::ArchiveTraceService, '#execute' do - subject { described_class.new.execute(job) } + subject { described_class.new.execute(job, worker_name: ArchiveTraceWorker.name) } context 'when job is finished' do let(:job) { create(:ci_build, :success, :trace_live) } @@ -25,6 +25,34 @@ describe Ci::ArchiveTraceService, '#execute' do expect { subject }.not_to change { Ci::JobArtifact.trace.count } end end + + context 'when job does not have trace' do + let(:job) { create(:ci_build, :success) } + + it 'leaves a warning message in sidekiq log' do + expect(Sidekiq.logger).to receive(:warn).with( + class: ArchiveTraceWorker.name, + message: 'The job does not have live trace but going to be archived.', + job_id: job.id) + + subject + end + end + + context 'when job failed to archive trace but did not raise an exception' do + before do + allow_any_instance_of(Gitlab::Ci::Trace).to receive(:archive!) {} + end + + it 'leaves a warning message in sidekiq log' do + expect(Sidekiq.logger).to receive(:warn).with( + class: ArchiveTraceWorker.name, + message: 'The job does not have archived trace after archiving.', + job_id: job.id) + + subject + end + end end context 'when job is running' do @@ -37,10 +65,10 @@ describe Ci::ArchiveTraceService, '#execute' do issue_url: 'https://gitlab.com/gitlab-org/gitlab-ce/issues/51502', extra: { job_id: job.id } ).once - expect(Rails.logger) - .to receive(:error) - .with("Failed to archive trace. id: #{job.id} message: Job is not finished yet") - .and_call_original + expect(Sidekiq.logger).to receive(:warn).with( + class: ArchiveTraceWorker.name, + message: "Failed to archive trace. message: Job is not finished yet.", + job_id: job.id).and_call_original expect(Gitlab::Metrics) .to receive(:counter) diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 867692d4d64..deb68899309 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -1099,6 +1099,63 @@ describe Ci::CreatePipelineService do end end end + + context 'when needs is used' do + let(:pipeline) { execute_service } + + let(:config) do + { + build_a: { + stage: "build", + script: "ls", + only: %w[master] + }, + test_a: { + stage: "test", + script: "ls", + only: %w[master feature], + needs: %w[build_a] + }, + deploy: { + stage: "deploy", + script: "ls", + only: %w[tags] + } + } + end + + before do + stub_ci_pipeline_yaml_file(YAML.dump(config)) + end + + context 'when pipeline on master is created' do + let(:ref_name) { 'refs/heads/master' } + + it 'creates a pipeline with build_a and test_a' do + expect(pipeline).to be_persisted + expect(pipeline.builds.map(&:name)).to contain_exactly("build_a", "test_a") + end + end + + context 'when pipeline on feature is created' do + let(:ref_name) { 'refs/heads/feature' } + + it 'does not create a pipeline as test_a depends on build_a' do + expect(pipeline).not_to be_persisted + expect(pipeline.builds).to be_empty + expect(pipeline.errors[:base]).to contain_exactly("test_a: needs 'build_a'") + end + end + + context 'when pipeline on v1.0.0 is created' do + let(:ref_name) { 'refs/tags/v1.0.0' } + + it 'does create a pipeline only with deploy' do + expect(pipeline).to be_persisted + expect(pipeline.builds.map(&:name)).to contain_exactly("deploy") + end + end + end end describe '#execute!' do @@ -1132,5 +1189,17 @@ describe Ci::CreatePipelineService do .with_message('Insufficient permissions to create a new pipeline') end end + + context 'when a user with permissions has been blocked' do + before do + user.block! + end + + it 'raises an error' do + expect { subject } + .to raise_error(described_class::CreateError) + .with_message('Insufficient permissions to create a new pipeline') + end + end end end diff --git a/spec/services/ci/pipeline_schedule_service_spec.rb b/spec/services/ci/pipeline_schedule_service_spec.rb index 867ed0acc0d..f7590720f66 100644 --- a/spec/services/ci/pipeline_schedule_service_spec.rb +++ b/spec/services/ci/pipeline_schedule_service_spec.rb @@ -25,6 +25,38 @@ describe Ci::PipelineScheduleService do subject end + context 'when ci_pipeline_schedule_async feature flag is disabled' do + before do + stub_feature_flags(ci_pipeline_schedule_async: false) + end + + it 'runs RunPipelineScheduleWorker synchronously' do + expect_next_instance_of(RunPipelineScheduleWorker) do |worker| + expect(worker).to receive(:perform).with(schedule.id, schedule.owner.id) + end + + subject + end + + it 'calls Garbage Collection manually' do + expect(GC).to receive(:start) + + subject + end + + context 'when ci_pipeline_schedule_force_gc feature flag is disabled' do + before do + stub_feature_flags(ci_pipeline_schedule_force_gc: false) + end + + it 'does not call Garbage Collection manually' do + expect(GC).not_to receive(:start) + + subject + end + end + end + context 'when owner is nil' do let(:schedule) { create(:ci_pipeline_schedule, project: project, owner: nil) } diff --git a/spec/services/ci/play_build_service_spec.rb b/spec/services/ci/play_build_service_spec.rb index 634f865e2d8..cf39f3da4fe 100644 --- a/spec/services/ci/play_build_service_spec.rb +++ b/spec/services/ci/play_build_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Ci::PlayBuildService, '#execute' do - let(:user) { create(:user) } + let(:user) { create(:user, developer_projects: [project]) } let(:project) { create(:project) } let(:pipeline) { create(:ci_pipeline, project: project) } let(:build) { create(:ci_build, :manual, pipeline: pipeline) } @@ -16,8 +16,6 @@ describe Ci::PlayBuildService, '#execute' do let(:project) { create(:project) } it 'allows user to play build if protected branch rules are met' do - project.add_developer(user) - create(:protected_branch, :developers_can_merge, name: build.ref, project: project) @@ -27,8 +25,6 @@ describe Ci::PlayBuildService, '#execute' do end it 'does not allow user with developer role to play build' do - project.add_developer(user) - expect { service.execute(build) } .to raise_error Gitlab::Access::AccessDeniedError end @@ -38,23 +34,21 @@ describe Ci::PlayBuildService, '#execute' do let(:project) { create(:project, :repository) } it 'allows user with developer role to play a build' do - project.add_developer(user) - service.execute(build) expect(build.reload).to be_pending end + + it 'prevents a blocked developer from playing a build' do + user.block! + + expect { service.execute(build) }.to raise_error(Gitlab::Access::AccessDeniedError) + end end context 'when build is a playable manual action' do let(:build) { create(:ci_build, :manual, pipeline: pipeline) } - - before do - project.add_developer(user) - - create(:protected_branch, :developers_can_merge, - name: build.ref, project: project) - end + let!(:branch) { create(:protected_branch, :developers_can_merge, name: build.ref, project: project) } it 'enqueues the build' do expect(service.execute(build)).to eq build @@ -66,17 +60,24 @@ describe Ci::PlayBuildService, '#execute' do expect(build.reload.user).to eq user end - end - context 'when build is not a playable manual action' do - let(:build) { create(:ci_build, when: :manual, pipeline: pipeline) } + context 'when variables are supplied' do + let(:job_variables) do + [{ key: 'first', secret_value: 'first' }, + { key: 'second', secret_value: 'second' }] + end - before do - project.add_developer(user) + it 'assigns the variables to the build' do + service.execute(build, job_variables) - create(:protected_branch, :developers_can_merge, - name: build.ref, project: project) + expect(build.reload.job_variables.map(&:key)).to contain_exactly('first', 'second') + end end + end + + context 'when build is not a playable manual action' do + let(:build) { create(:ci_build, when: :manual, pipeline: pipeline) } + let!(:branch) { create(:protected_branch, :developers_can_merge, name: build.ref, project: project) } it 'duplicates the build' do duplicate = service.execute(build) @@ -94,6 +95,7 @@ describe Ci::PlayBuildService, '#execute' do end context 'when build is not action' do + let(:user) { create(:user) } let(:build) { create(:ci_build, :success, pipeline: pipeline) } it 'raises an error' do @@ -103,10 +105,8 @@ describe Ci::PlayBuildService, '#execute' do end context 'when user does not have ability to trigger action' do - before do - create(:protected_branch, :no_one_can_push, - name: build.ref, project: project) - end + let(:user) { create(:user) } + let!(:branch) { create(:protected_branch, :developers_can_merge, name: build.ref, project: project) } it 'raises an error' do expect { service.execute(build) } diff --git a/spec/services/ci/process_pipeline_service_spec.rb b/spec/services/ci/process_pipeline_service_spec.rb index cadb519ccee..1b28d2d4d02 100644 --- a/spec/services/ci/process_pipeline_service_spec.rb +++ b/spec/services/ci/process_pipeline_service_spec.rb @@ -700,6 +700,138 @@ describe Ci::ProcessPipelineService, '#execute' do end end + context 'when pipeline with needs is created' do + let!(:linux_build) { create_build('linux:build', stage: 'build', stage_idx: 0) } + let!(:mac_build) { create_build('mac:build', stage: 'build', stage_idx: 0) } + let!(:linux_rspec) { create_build('linux:rspec', stage: 'test', stage_idx: 1) } + let!(:linux_rubocop) { create_build('linux:rubocop', stage: 'test', stage_idx: 1) } + let!(:mac_rspec) { create_build('mac:rspec', stage: 'test', stage_idx: 1) } + let!(:mac_rubocop) { create_build('mac:rubocop', stage: 'test', stage_idx: 1) } + let!(:deploy) { create_build('deploy', stage: 'deploy', stage_idx: 2) } + + let!(:linux_rspec_on_build) { create(:ci_build_need, build: linux_rspec, name: 'linux:build') } + let!(:linux_rubocop_on_build) { create(:ci_build_need, build: linux_rubocop, name: 'linux:build') } + + let!(:mac_rspec_on_build) { create(:ci_build_need, build: mac_rspec, name: 'mac:build') } + let!(:mac_rubocop_on_build) { create(:ci_build_need, build: mac_rubocop, name: 'mac:build') } + + it 'when linux:* finishes first it runs it out of order' do + expect(process_pipeline).to be_truthy + + expect(stages).to eq(%w(pending created created)) + expect(builds.pending).to contain_exactly(linux_build, mac_build) + + # we follow the single path of linux + linux_build.reset.success! + + expect(stages).to eq(%w(running pending created)) + expect(builds.success).to contain_exactly(linux_build) + expect(builds.pending).to contain_exactly(mac_build, linux_rspec, linux_rubocop) + + linux_rspec.reset.success! + + expect(stages).to eq(%w(running running created)) + expect(builds.success).to contain_exactly(linux_build, linux_rspec) + expect(builds.pending).to contain_exactly(mac_build, linux_rubocop) + + linux_rubocop.reset.success! + + expect(stages).to eq(%w(running running created)) + expect(builds.success).to contain_exactly(linux_build, linux_rspec, linux_rubocop) + expect(builds.pending).to contain_exactly(mac_build) + + mac_build.reset.success! + mac_rspec.reset.success! + mac_rubocop.reset.success! + + expect(stages).to eq(%w(success success pending)) + expect(builds.success).to contain_exactly( + linux_build, linux_rspec, linux_rubocop, mac_build, mac_rspec, mac_rubocop) + expect(builds.pending).to contain_exactly(deploy) + end + + context 'when feature ci_dag_support is disabled' do + before do + stub_feature_flags(ci_dag_support: false) + end + + it 'when linux:build finishes first it follows stages' do + expect(process_pipeline).to be_truthy + + expect(stages).to eq(%w(pending created created)) + expect(builds.pending).to contain_exactly(linux_build, mac_build) + + # we follow the single path of linux + linux_build.reset.success! + + expect(stages).to eq(%w(running created created)) + expect(builds.success).to contain_exactly(linux_build) + expect(builds.pending).to contain_exactly(mac_build) + + mac_build.reset.success! + + expect(stages).to eq(%w(success pending created)) + expect(builds.success).to contain_exactly(linux_build, mac_build) + expect(builds.pending).to contain_exactly( + linux_rspec, linux_rubocop, mac_rspec, mac_rubocop) + + linux_rspec.reset.success! + linux_rubocop.reset.success! + mac_rspec.reset.success! + mac_rubocop.reset.success! + + expect(stages).to eq(%w(success success pending)) + expect(builds.success).to contain_exactly( + linux_build, linux_rspec, linux_rubocop, mac_build, mac_rspec, mac_rubocop) + expect(builds.pending).to contain_exactly(deploy) + end + end + + context 'when one of the jobs is run on a failure' do + let!(:linux_notify) { create_build('linux:notify', stage: 'deploy', stage_idx: 2, when: 'on_failure') } + + let!(:linux_notify_on_build) { create(:ci_build_need, build: linux_notify, name: 'linux:build') } + + context 'when another job in build phase fails first' do + context 'when ci_dag_support is enabled' do + it 'does skip linux:notify' do + expect(process_pipeline).to be_truthy + + mac_build.reset.drop! + linux_build.reset.success! + + expect(linux_notify.reset).to be_skipped + end + end + + context 'when ci_dag_support is disabled' do + before do + stub_feature_flags(ci_dag_support: false) + end + + it 'does run linux:notify' do + expect(process_pipeline).to be_truthy + + mac_build.reset.drop! + linux_build.reset.success! + + expect(linux_notify.reset).to be_pending + end + end + end + + context 'when linux:build job fails first' do + it 'does run linux:notify' do + expect(process_pipeline).to be_truthy + + linux_build.reset.drop! + + expect(linux_notify.reset).to be_pending + end + end + end + end + def process_pipeline described_class.new(pipeline.project, user).execute(pipeline) end @@ -712,6 +844,10 @@ describe Ci::ProcessPipelineService, '#execute' do all_builds.where.not(status: [:created, :skipped]) end + def stages + pipeline.reset.stages.map(&:status) + end + def builds_names builds.pluck(:name) end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index e9a26400723..fe7c6fe4700 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -30,7 +30,8 @@ describe Ci::RetryBuildService do job_artifacts_sast job_artifacts_dependency_scanning job_artifacts_container_scanning job_artifacts_dast job_artifacts_license_management job_artifacts_performance - job_artifacts_codequality job_artifacts_metrics scheduled_at].freeze + job_artifacts_codequality job_artifacts_metrics scheduled_at + job_variables].freeze IGNORE_ACCESSORS = %i[type lock_version target_url base_tags trace_sections @@ -38,7 +39,7 @@ describe Ci::RetryBuildService do runner_id tag_taggings taggings tags trigger_request_id user_id auto_canceled_by_id retried failure_reason sourced_pipelines artifacts_file_store artifacts_metadata_store - metadata runner_session trace_chunks + metadata runner_session trace_chunks upstream_pipeline_id artifacts_file artifacts_metadata artifacts_size].freeze shared_examples 'build duplication' do @@ -65,6 +66,9 @@ describe Ci::RetryBuildService do file_type: file_type, job: build, expire_at: build.artifacts_expire_at) end + create(:ci_job_variable, job: build) + create(:ci_build_need, build: build) + build.reload end diff --git a/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb b/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb index 9ab83d913f5..a948b442441 100644 --- a/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb +++ b/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb @@ -41,7 +41,7 @@ describe Clusters::Applications::CheckUninstallProgressService do end end - context 'when application is installing' do + context 'when application is uninstalling' do RESCHEDULE_PHASES.each { |phase| it_behaves_like 'a not yet terminated installation', phase } context 'when installation POD succeeded' do @@ -56,6 +56,12 @@ describe Clusters::Applications::CheckUninstallProgressService do service.execute end + it 'runs application post_uninstall' do + expect(application).to receive(:post_uninstall).and_call_original + + service.execute + end + it 'destroys the application' do expect(worker_class).not_to receive(:perform_in) diff --git a/spec/services/clusters/build_kubernetes_namespace_service_spec.rb b/spec/services/clusters/build_kubernetes_namespace_service_spec.rb new file mode 100644 index 00000000000..36c05469542 --- /dev/null +++ b/spec/services/clusters/build_kubernetes_namespace_service_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Clusters::BuildKubernetesNamespaceService do + let(:cluster) { create(:cluster, :project, :provided_by_gcp) } + let(:environment) { create(:environment) } + let(:project) { environment.project } + + let(:namespace_generator) { double(from_environment_slug: namespace) } + let(:namespace) { 'namespace' } + + subject { described_class.new(cluster, environment: environment).execute } + + before do + allow(Gitlab::Kubernetes::DefaultNamespace).to receive(:new).and_return(namespace_generator) + end + + shared_examples 'shared attributes' do + it 'initializes a new namespace and sets default values' do + expect(subject).to be_new_record + expect(subject.cluster).to eq cluster + expect(subject.project).to eq project + expect(subject.namespace).to eq namespace + expect(subject.service_account_name).to eq "#{namespace}-service-account" + end + end + + include_examples 'shared attributes' + + it 'sets cluster_project and environment' do + expect(subject.cluster_project).to eq cluster.cluster_project + expect(subject.environment).to eq environment + end + + context 'namespace per environment is disabled' do + let(:cluster) { create(:cluster, :project, :provided_by_gcp, :namespace_per_environment_disabled) } + + include_examples 'shared attributes' + + it 'does not set environment' do + expect(subject.cluster_project).to eq cluster.cluster_project + expect(subject.environment).to be_nil + end + end + + context 'group cluster' do + let(:cluster) { create(:cluster, :group, :provided_by_gcp) } + + include_examples 'shared attributes' + + it 'does not set cluster_project' do + expect(subject.cluster_project).to be_nil + expect(subject.environment).to eq environment + end + end +end diff --git a/spec/services/clusters/gcp/finalize_creation_service_spec.rb b/spec/services/clusters/gcp/finalize_creation_service_spec.rb index 2664649df47..5f91acb8e84 100644 --- a/spec/services/clusters/gcp/finalize_creation_service_spec.rb +++ b/spec/services/clusters/gcp/finalize_creation_service_spec.rb @@ -19,10 +19,6 @@ describe Clusters::Gcp::FinalizeCreationService, '#execute' do subject { described_class.new.execute(provider) } - before do - allow(ClusterConfigureWorker).to receive(:perform_async) - end - shared_examples 'success' do it 'configures provider and kubernetes' do subject @@ -42,12 +38,6 @@ describe Clusters::Gcp::FinalizeCreationService, '#execute' do expect(platform.password).to eq(password) expect(platform.token).to eq(token) end - - it 'calls ClusterConfigureWorker in a ascync fashion' do - expect(ClusterConfigureWorker).to receive(:perform_async).with(cluster.id) - - subject - end end shared_examples 'error' do diff --git a/spec/services/clusters/gcp/kubernetes/create_or_update_namespace_service_spec.rb b/spec/services/clusters/gcp/kubernetes/create_or_update_namespace_service_spec.rb index be052a07da7..e44cc3f5a78 100644 --- a/spec/services/clusters/gcp/kubernetes/create_or_update_namespace_service_spec.rb +++ b/spec/services/clusters/gcp/kubernetes/create_or_update_namespace_service_spec.rb @@ -9,8 +9,9 @@ describe Clusters::Gcp::Kubernetes::CreateOrUpdateNamespaceService, '#execute' d let(:platform) { cluster.platform } let(:api_url) { 'https://kubernetes.example.com' } let(:project) { cluster.project } + let(:environment) { create(:environment, project: project) } let(:cluster_project) { cluster.cluster_project } - let(:namespace) { "#{project.path}-#{project.id}" } + let(:namespace) { "#{project.name}-#{project.id}-#{environment.slug}" } subject do described_class.new( @@ -34,6 +35,8 @@ describe Clusters::Gcp::Kubernetes::CreateOrUpdateNamespaceService, '#execute' d stub_kubeclient_create_service_account(api_url, namespace: namespace) stub_kubeclient_create_secret(api_url, namespace: namespace) stub_kubeclient_put_secret(api_url, "#{namespace}-token", namespace: namespace) + stub_kubeclient_put_role(api_url, Clusters::Gcp::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_NAME, namespace: namespace) + stub_kubeclient_put_role_binding(api_url, Clusters::Gcp::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_BINDING_NAME, namespace: namespace) stub_kubeclient_get_secret( api_url, @@ -77,7 +80,8 @@ describe Clusters::Gcp::Kubernetes::CreateOrUpdateNamespaceService, '#execute' d let(:kubernetes_namespace) do build(:cluster_kubernetes_namespace, cluster: cluster, - project: project) + project: project, + environment: environment) end it_behaves_like 'successful creation of kubernetes namespace' @@ -90,20 +94,22 @@ describe Clusters::Gcp::Kubernetes::CreateOrUpdateNamespaceService, '#execute' d build(:cluster_kubernetes_namespace, cluster: cluster, project: cluster_project.project, - cluster_project: cluster_project) + cluster_project: cluster_project, + environment: environment) end it_behaves_like 'successful creation of kubernetes namespace' end context 'when there is a Kubernetes Namespace associated' do - let(:namespace) { 'new-namespace' } + let(:namespace) { "new-namespace-#{environment.slug}" } let(:kubernetes_namespace) do create(:cluster_kubernetes_namespace, cluster: cluster, project: cluster_project.project, - cluster_project: cluster_project) + cluster_project: cluster_project, + environment: environment) end before do diff --git a/spec/services/clusters/gcp/kubernetes/create_or_update_service_account_service_spec.rb b/spec/services/clusters/gcp/kubernetes/create_or_update_service_account_service_spec.rb index 382b9043566..8b874989758 100644 --- a/spec/services/clusters/gcp/kubernetes/create_or_update_service_account_service_spec.rb +++ b/spec/services/clusters/gcp/kubernetes/create_or_update_service_account_service_spec.rb @@ -143,6 +143,8 @@ describe Clusters::Gcp::Kubernetes::CreateOrUpdateServiceAccountService do stub_kubeclient_get_role_binding_error(api_url, role_binding_name, namespace: namespace) stub_kubeclient_create_role_binding(api_url, namespace: namespace) + stub_kubeclient_put_role(api_url, Clusters::Gcp::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_NAME, namespace: namespace) + stub_kubeclient_put_role_binding(api_url, Clusters::Gcp::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_BINDING_NAME, namespace: namespace) end it_behaves_like 'creates service account and token' @@ -169,6 +171,24 @@ describe Clusters::Gcp::Kubernetes::CreateOrUpdateServiceAccountService do ) ) end + + it 'creates a role and role binding granting knative serving permissions to the service account' do + subject + + expect(WebMock).to have_requested(:put, api_url + "/apis/rbac.authorization.k8s.io/v1/namespaces/#{namespace}/roles/#{Clusters::Gcp::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_NAME}").with( + body: hash_including( + metadata: { + name: Clusters::Gcp::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_NAME, + namespace: namespace + }, + rules: [{ + apiGroups: %w(serving.knative.dev), + resources: %w(configurations configurationgenerations routes revisions revisionuids autoscalers services), + verbs: %w(get list create update delete patch watch) + }] + ) + ) + end end end end diff --git a/spec/services/clusters/gcp/kubernetes/fetch_kubernetes_token_service_spec.rb b/spec/services/clusters/gcp/kubernetes/fetch_kubernetes_token_service_spec.rb index a5806559b14..93c0dc37ade 100644 --- a/spec/services/clusters/gcp/kubernetes/fetch_kubernetes_token_service_spec.rb +++ b/spec/services/clusters/gcp/kubernetes/fetch_kubernetes_token_service_spec.rb @@ -17,7 +17,7 @@ describe Clusters::Gcp::Kubernetes::FetchKubernetesTokenService do ) end - subject { described_class.new(kubeclient, service_account_token_name, namespace).execute } + subject { described_class.new(kubeclient, service_account_token_name, namespace, token_retry_delay: 0).execute } before do stub_kubeclient_discover(api_url) @@ -26,8 +26,7 @@ describe Clusters::Gcp::Kubernetes::FetchKubernetesTokenService do context 'when params correct' do let(:decoded_token) { 'xxx.token.xxx' } let(:token) { Base64.encode64(decoded_token) } - - context 'when gitlab-token exists' do + context 'when the secret exists' do before do stub_kubeclient_get_secret( api_url, @@ -50,13 +49,62 @@ describe Clusters::Gcp::Kubernetes::FetchKubernetesTokenService do it { expect { subject }.to raise_error(Kubeclient::HttpError) } end - context 'when gitlab-token does not exist' do + context 'when the secret does not exist on the first try' do + before do + stub_kubeclient_get_secret_not_found_then_found( + api_url, + { + metadata_name: service_account_token_name, + namespace: namespace, + token: token + } + ) + end + + it 'retries and finds the token' do + expect(subject).to eq(decoded_token) + end + end + + context 'when the secret permanently does not exist' do before do stub_kubeclient_get_secret_error(api_url, service_account_token_name, namespace: namespace, status: 404) end it { is_expected.to be_nil } end + + context 'when the secret is missing a token on the first try' do + before do + stub_kubeclient_get_secret_missing_token_then_with_token( + api_url, + { + metadata_name: service_account_token_name, + namespace: namespace, + token: token + } + ) + end + + it 'retries and finds the token' do + expect(subject).to eq(decoded_token) + end + end + + context 'when the secret is permanently missing a token' do + before do + stub_kubeclient_get_secret( + api_url, + { + metadata_name: service_account_token_name, + namespace: namespace, + token: nil + } + ) + end + + it { is_expected.to be_nil } + end end end end diff --git a/spec/services/clusters/refresh_service_spec.rb b/spec/services/clusters/refresh_service_spec.rb deleted file mode 100644 index 5bc8a709941..00000000000 --- a/spec/services/clusters/refresh_service_spec.rb +++ /dev/null @@ -1,113 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Clusters::RefreshService do - shared_examples 'creates a kubernetes namespace' do - let(:token) { 'aaaaaa' } - let(:service_account_creator) { double(Clusters::Gcp::Kubernetes::CreateOrUpdateServiceAccountService, execute: true) } - let(:secrets_fetcher) { double(Clusters::Gcp::Kubernetes::FetchKubernetesTokenService, execute: token) } - - it 'creates a kubernetes namespace' do - expect(Clusters::Gcp::Kubernetes::CreateOrUpdateServiceAccountService).to receive(:namespace_creator).and_return(service_account_creator) - expect(Clusters::Gcp::Kubernetes::FetchKubernetesTokenService).to receive(:new).and_return(secrets_fetcher) - - expect { subject }.to change(project.kubernetes_namespaces, :count) - - kubernetes_namespace = cluster.kubernetes_namespaces.first - expect(kubernetes_namespace).to be_present - expect(kubernetes_namespace.project).to eq(project) - end - end - - shared_examples 'does not create a kubernetes namespace' do - it 'does not create a new kubernetes namespace' do - expect(Clusters::Gcp::Kubernetes::CreateOrUpdateServiceAccountService).not_to receive(:namespace_creator) - expect(Clusters::Gcp::Kubernetes::FetchKubernetesTokenService).not_to receive(:new) - - expect { subject }.not_to change(Clusters::KubernetesNamespace, :count) - end - end - - describe '.create_or_update_namespaces_for_cluster' do - let(:cluster) { create(:cluster, :provided_by_user, :project) } - let(:project) { cluster.project } - - subject { described_class.create_or_update_namespaces_for_cluster(cluster) } - - context 'cluster is project level' do - include_examples 'creates a kubernetes namespace' - - context 'when project already has kubernetes namespace' do - before do - create(:cluster_kubernetes_namespace, project: project, cluster: cluster) - end - - include_examples 'does not create a kubernetes namespace' - end - end - - context 'cluster is group level' do - let(:cluster) { create(:cluster, :provided_by_user, :group) } - let(:group) { cluster.group } - let(:project) { create(:project, group: group) } - - include_examples 'creates a kubernetes namespace' - - context 'when project already has kubernetes namespace' do - before do - create(:cluster_kubernetes_namespace, project: project, cluster: cluster) - end - - include_examples 'does not create a kubernetes namespace' - end - end - end - - describe '.create_or_update_namespaces_for_project' do - let(:project) { create(:project) } - - subject { described_class.create_or_update_namespaces_for_project(project) } - - it 'creates no kubernetes namespaces' do - expect { subject }.not_to change(project.kubernetes_namespaces, :count) - end - - context 'project has a project cluster' do - let!(:cluster) { create(:cluster, :provided_by_gcp, cluster_type: :project_type, projects: [project]) } - - include_examples 'creates a kubernetes namespace' - - context 'when project already has kubernetes namespace' do - before do - create(:cluster_kubernetes_namespace, project: project, cluster: cluster) - end - - include_examples 'does not create a kubernetes namespace' - end - end - - context 'project belongs to a group cluster' do - let!(:cluster) { create(:cluster, :provided_by_gcp, :group) } - - let(:group) { cluster.group } - let(:project) { create(:project, group: group) } - - include_examples 'does not create a kubernetes namespace' - - context 'when project already has kubernetes namespace' do - before do - create(:cluster_kubernetes_namespace, project: project, cluster: cluster) - end - - include_examples 'does not create a kubernetes namespace' - end - end - - context 'cluster is not managed' do - let!(:cluster) { create(:cluster, :project, :not_managed, projects: [project]) } - - include_examples 'does not create a kubernetes namespace' - end - end -end diff --git a/spec/services/clusters/update_service_spec.rb b/spec/services/clusters/update_service_spec.rb index 21b37f88fd8..3ee45375dca 100644 --- a/spec/services/clusters/update_service_spec.rb +++ b/spec/services/clusters/update_service_spec.rb @@ -39,7 +39,6 @@ describe Clusters::UpdateService do end before do - allow(ClusterConfigureWorker).to receive(:perform_async) stub_kubeclient_get_namespace('https://kubernetes.example.com', namespace: 'my-namespace') end diff --git a/spec/services/create_snippet_service_spec.rb b/spec/services/create_snippet_service_spec.rb index f6b6989b955..9b83f65a17e 100644 --- a/spec/services/create_snippet_service_spec.rb +++ b/spec/services/create_snippet_service_spec.rb @@ -36,6 +36,22 @@ describe CreateSnippetService do end end + describe 'usage counter' do + let(:counter) { Gitlab::UsageDataCounters::SnippetCounter } + + it 'increments count' do + expect do + create_snippet(nil, @admin, @opts) + end.to change { counter.read(:create) }.by 1 + end + + it 'does not increment count if create fails' do + expect do + create_snippet(nil, @admin, {}) + end.not_to change { counter.read(:create) } + end + end + def create_snippet(project, user, opts) CreateSnippetService.new(project, user, opts).execute end diff --git a/spec/services/deploy_tokens/create_service_spec.rb b/spec/services/deploy_tokens/create_service_spec.rb index 886ffd4593d..fbb66fe4cb7 100644 --- a/spec/services/deploy_tokens/create_service_spec.rb +++ b/spec/services/deploy_tokens/create_service_spec.rb @@ -32,6 +32,22 @@ describe DeployTokens::CreateService do end end + context 'when username is empty string' do + let(:deploy_token_params) { attributes_for(:deploy_token, username: '') } + + it 'converts it to nil' do + expect(subject.read_attribute(:username)).to be_nil + end + end + + context 'when username is provided' do + let(:deploy_token_params) { attributes_for(:deploy_token, username: 'deployer') } + + it 'keeps the provided username' do + expect(subject.read_attribute(:username)).to eq('deployer') + end + end + context 'when the deploy token is invalid' do let(:deploy_token_params) { attributes_for(:deploy_token, read_repository: false, read_registry: false) } diff --git a/spec/services/git/base_hooks_service_spec.rb b/spec/services/git/base_hooks_service_spec.rb index 4a2ec769116..874df9a68cd 100644 --- a/spec/services/git/base_hooks_service_spec.rb +++ b/spec/services/git/base_hooks_service_spec.rb @@ -14,6 +14,78 @@ describe Git::BaseHooksService do let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0 let(:ref) { 'refs/tags/v1.1.0' } + describe '#execute_project_hooks' do + class TestService < described_class + def hook_name + :push_hooks + end + + def commits + [] + end + end + + let(:project) { create(:project, :repository) } + + subject { TestService.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) } + + context '#execute_hooks' do + before do + expect(project).to receive(:has_active_hooks?).and_return(active) + end + + context 'active hooks' do + let(:active) { true } + + it 'executes the hooks' do + expect(subject).to receive(:push_data).at_least(:once).and_call_original + expect(project).to receive(:execute_hooks) + + subject.execute + end + end + + context 'inactive hooks' do + let(:active) { false } + + it 'does not execute the hooks' do + expect(subject).not_to receive(:push_data) + expect(project).not_to receive(:execute_hooks) + + subject.execute + end + end + end + + context '#execute_services' do + before do + expect(project).to receive(:has_active_services?).and_return(active) + end + + context 'active services' do + let(:active) { true } + + it 'executes the services' do + expect(subject).to receive(:push_data).at_least(:once).and_call_original + expect(project).to receive(:execute_services) + + subject.execute + end + end + + context 'inactive services' do + let(:active) { false } + + it 'does not execute the services' do + expect(subject).not_to receive(:push_data) + expect(project).not_to receive(:execute_services) + + subject.execute + end + end + end + end + describe 'with remote mirrors' do class TestService < described_class def commits diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb index b5694628269..2bf7dc32436 100644 --- a/spec/services/git/branch_hooks_service_spec.rb +++ b/spec/services/git/branch_hooks_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe Git::BranchHooksService do include RepoHelpers + include ProjectForksHelper let(:project) { create(:project, :repository) } let(:user) { project.creator } @@ -25,7 +26,7 @@ describe Git::BranchHooksService do end describe "Git Push Data" do - subject(:push_data) { service.execute } + subject(:push_data) { service.send(:push_data) } it 'has expected push data attributes' do is_expected.to match a_hash_including( @@ -109,6 +110,7 @@ describe Git::BranchHooksService do expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) expect(event.push_event_payload.commit_from).to eq(oldrev) expect(event.push_event_payload.commit_to).to eq(newrev) + expect(event.push_event_payload.commit_title).to eq('Change some files') expect(event.push_event_payload.ref).to eq('master') expect(event.push_event_payload.commit_count).to eq(1) end @@ -124,6 +126,7 @@ describe Git::BranchHooksService do expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) expect(event.push_event_payload.commit_from).to be_nil expect(event.push_event_payload.commit_to).to eq(newrev) + expect(event.push_event_payload.commit_title).to eq('Initial commit') expect(event.push_event_payload.ref).to eq('master') expect(event.push_event_payload.commit_count).to be > 1 end @@ -156,9 +159,13 @@ describe Git::BranchHooksService do let(:blank_sha) { Gitlab::Git::BLANK_SHA } def clears_cache(extended: []) - expect(ProjectCacheWorker) - .to receive(:perform_async) - .with(project.id, extended, %i[commit_count repository_size]) + expect(service).to receive(:invalidated_file_types).and_return(extended) + + if extended.present? + expect(ProjectCacheWorker) + .to receive(:perform_async) + .with(project.id, extended, [], false) + end service.execute end @@ -266,10 +273,10 @@ describe Git::BranchHooksService do end describe 'Processing commit messages' do - # Create 4 commits, 2 of which have references. Limiting to 2 commits, we - # expect to see one commit message processor enqueued. - let(:commit_ids) do - Array.new(4) do |i| + # Create 6 commits, 3 of which have references. Limiting to 4 commits, we + # expect to see two commit message processors enqueued. + let!(:commit_ids) do + Array.new(6) do |i| message = "Issue #{'#' if i.even?}#{i}" project.repository.update_file( user, 'README.md', '', message: message, branch_name: branch @@ -277,18 +284,18 @@ describe Git::BranchHooksService do end end - let(:oldrev) { commit_ids.first } + let(:oldrev) { project.commit(commit_ids.first).parent_id } let(:newrev) { commit_ids.last } before do - stub_const("::Git::BaseHooksService::PROCESS_COMMIT_LIMIT", 2) + stub_const("::Git::BaseHooksService::PROCESS_COMMIT_LIMIT", 4) end context 'creating the default branch' do let(:oldrev) { Gitlab::Git::BLANK_SHA } it 'processes a limited number of commit messages' do - expect(ProcessCommitWorker).to receive(:perform_async).once + expect(ProcessCommitWorker).to receive(:perform_async).twice service.execute end @@ -296,7 +303,7 @@ describe Git::BranchHooksService do context 'updating the default branch' do it 'processes a limited number of commit messages' do - expect(ProcessCommitWorker).to receive(:perform_async).once + expect(ProcessCommitWorker).to receive(:perform_async).twice service.execute end @@ -317,7 +324,7 @@ describe Git::BranchHooksService do let(:oldrev) { Gitlab::Git::BLANK_SHA } it 'processes a limited number of commit messages' do - expect(ProcessCommitWorker).to receive(:perform_async).once + expect(ProcessCommitWorker).to receive(:perform_async).twice service.execute end @@ -327,7 +334,7 @@ describe Git::BranchHooksService do let(:branch) { 'fix' } it 'processes a limited number of commit messages' do - expect(ProcessCommitWorker).to receive(:perform_async).once + expect(ProcessCommitWorker).to receive(:perform_async).twice service.execute end @@ -343,5 +350,88 @@ describe Git::BranchHooksService do service.execute end end + + context 'when the project is forked' do + let(:upstream_project) { project } + let(:forked_project) { fork_project(upstream_project, user, repository: true) } + + let!(:forked_service) do + described_class.new(forked_project, user, oldrev: oldrev, newrev: newrev, ref: ref) + end + + context 'when commits already exists in the upstream project' do + it 'does not process commit messages' do + expect(ProcessCommitWorker).not_to receive(:perform_async) + + forked_service.execute + end + end + + context 'when a commit does not exist in the upstream repo' do + # On top of the existing 6 commits, 3 of which have references, + # create 2 more, 1 of which has a reference. Limiting to 4 commits, we + # expect to see one commit message processor enqueued. + let!(:forked_commit_ids) do + Array.new(2) do |i| + message = "Issue #{'#' if i.even?}#{i}" + forked_project.repository.update_file( + user, 'README.md', '', message: message, branch_name: branch + ) + end + end + + let(:newrev) { forked_commit_ids.last } + + it 'processes the commit message' do + expect(ProcessCommitWorker).to receive(:perform_async).once + + forked_service.execute + end + end + + context 'when the upstream project no longer exists' do + it 'processes the commit messages' do + upstream_project.destroy! + + expect(ProcessCommitWorker).to receive(:perform_async).twice + + forked_service.execute + end + end + end + end + + describe 'New branch detection' do + let(:branch) { 'fix' } + + context 'oldrev is the blank SHA' do + let(:oldrev) { Gitlab::Git::BLANK_SHA } + + it 'is treated as a new branch' do + expect(service).to receive(:branch_create_hooks) + + service.execute + end + end + + context 'oldrev is set' do + context 'Gitaly does not know about the branch' do + it 'is treated as a new branch' do + allow(project.repository).to receive(:branch_names) { [] } + + expect(service).to receive(:branch_create_hooks) + + service.execute + end + end + + context 'Gitaly knows about the branch' do + it 'is not treated as a new branch' do + expect(service).not_to receive(:branch_create_hooks) + + service.execute + end + end + end end end diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index 6e39fa6b3c0..d9e607cd251 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -76,9 +76,28 @@ describe Git::BranchPushService, services: true do stub_ci_pipeline_to_return_yaml_file end + it 'creates a pipeline with the right parameters' do + expect(Ci::CreatePipelineService) + .to receive(:new) + .with(project, + user, + { + before: oldrev, + after: newrev, + ref: ref, + checkout_sha: SeedRepo::Commit::ID, + push_options: {} + }).and_call_original + + subject + end + it "creates a new pipeline" do expect { subject }.to change { Ci::Pipeline.count } - expect(Ci::Pipeline.last).to be_push + + pipeline = Ci::Pipeline.last + expect(pipeline).to be_push + expect(Gitlab::Git::BRANCH_REF_PREFIX + pipeline.ref).to eq(ref) end end @@ -123,6 +142,10 @@ describe Git::BranchPushService, services: true do describe "Webhooks" do context "execute webhooks" do + before do + create(:project_hook, push_events: true, project: project) + end + it "when pushing a branch for the first time" do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") diff --git a/spec/services/git/tag_hooks_service_spec.rb b/spec/services/git/tag_hooks_service_spec.rb index f5938a5c708..e362577d289 100644 --- a/spec/services/git/tag_hooks_service_spec.rb +++ b/spec/services/git/tag_hooks_service_spec.rb @@ -26,7 +26,8 @@ describe Git::TagHooksService, :service do describe 'System hooks' do it 'Executes system hooks' do - push_data = service.execute + push_data = service.send(:push_data) + expect(project).to receive(:has_active_hooks?).and_return(true) expect_next_instance_of(SystemHooksService) do |system_hooks_service| expect(system_hooks_service) @@ -40,6 +41,7 @@ describe Git::TagHooksService, :service do describe "Webhooks" do it "executes hooks on the project" do + expect(project).to receive(:has_active_hooks?).and_return(true) expect(project).to receive(:execute_hooks) service.execute @@ -61,7 +63,7 @@ describe Git::TagHooksService, :service do describe 'Push data' do shared_examples_for 'tag push data expectations' do - subject(:push_data) { service.execute } + subject(:push_data) { service.send(:push_data) } it 'has expected push data attributes' do is_expected.to match a_hash_including( object_kind: 'tag_push', diff --git a/spec/services/git/tag_push_service_spec.rb b/spec/services/git/tag_push_service_spec.rb index 418952b52da..7e008637182 100644 --- a/spec/services/git/tag_push_service_spec.rb +++ b/spec/services/git/tag_push_service_spec.rb @@ -26,8 +26,8 @@ describe Git::TagPushService do subject end - it 'flushes the tags cache' do - expect(project.repository).to receive(:expire_tags_cache) + it 'does not flush the tags cache' do + expect(project.repository).not_to receive(:expire_tags_cache) subject end diff --git a/spec/services/groups/auto_devops_service_spec.rb b/spec/services/groups/auto_devops_service_spec.rb index 7f8ab517cef..7591b2f6f12 100644 --- a/spec/services/groups/auto_devops_service_spec.rb +++ b/spec/services/groups/auto_devops_service_spec.rb @@ -47,7 +47,7 @@ describe Groups::AutoDevopsService, '#execute' do expect(subgroup_1.auto_devops_enabled?).to eq(false) end - context 'when subgroups have projects', :nested_groups do + context 'when subgroups have projects' do it 'reflects changes on projects' do subgroup_1 = create(:group, parent: group) project_1 = create(:project, namespace: subgroup_1) diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index c5ff6cdbacd..0f9f20de586 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -44,7 +44,7 @@ describe Groups::CreateService, '#execute' do end end - describe 'creating subgroup', :nested_groups do + describe 'creating subgroup' do let!(:group) { create(:group) } let!(:service) { described_class.new(user, group_params.merge(parent_id: group.id)) } @@ -54,39 +54,31 @@ describe Groups::CreateService, '#execute' do end it { is_expected.to be_persisted } + end - context 'when nested groups feature is disabled' do - it 'does not save group and returns an error' do - allow(Group).to receive(:supports_nested_objects?).and_return(false) + context 'as guest' do + it 'does not save group and returns an error' do + is_expected.not_to be_persisted - is_expected.not_to be_persisted - expect(subject.errors[:parent_id]).to include('You don’t have permission to create a subgroup in this group.') - expect(subject.parent_id).to be_nil - end + expect(subject.errors[:parent_id].first).to eq('You don’t have permission to create a subgroup in this group.') + expect(subject.parent_id).to be_nil end end - context 'when nested groups feature is enabled' do + context 'as owner' do before do - allow(Group).to receive(:supports_nested_objects?).and_return(true) + group.add_owner(user) end - context 'as guest' do - it 'does not save group and returns an error' do - is_expected.not_to be_persisted + it { is_expected.to be_persisted } + end - expect(subject.errors[:parent_id].first).to eq('You don’t have permission to create a subgroup in this group.') - expect(subject.parent_id).to be_nil - end + context 'as maintainer' do + before do + group.add_maintainer(user) end - context 'as owner' do - before do - group.add_owner(user) - end - - it { is_expected.to be_persisted } - end + it { is_expected.to be_persisted } end end diff --git a/spec/services/groups/nested_create_service_spec.rb b/spec/services/groups/nested_create_service_spec.rb index 13acf9e055b..b30392c1b12 100644 --- a/spec/services/groups/nested_create_service_spec.rb +++ b/spec/services/groups/nested_create_service_spec.rb @@ -28,35 +28,7 @@ describe Groups::NestedCreateService do end end - describe 'without subgroups' do - let(:params) { { group_path: 'a-group' } } - - before do - allow(Group).to receive(:supports_nested_objects?) { false } - end - - it 'creates the group' do - group = service.execute - - expect(group).to be_persisted - end - - it 'returns the group if it already existed' do - existing_group = create(:group, path: 'a-group') - - expect(service.execute).to eq(existing_group) - end - - it 'raises an error when tring to create a subgroup' do - service = described_class.new(user, group_path: 'a-group/a-sub-group') - - expect { service.execute }.to raise_error('Nested groups are not supported on MySQL') - end - - it_behaves_like 'with a visibility level' - end - - describe 'with subgroups', :nested_groups do + describe 'with subgroups' do let(:params) { { group_path: 'a-group/a-sub-group' } } describe "#execute" do diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index b5708ebba76..f3af8cf5f3b 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -2,28 +2,13 @@ require 'rails_helper' -describe Groups::TransferService, :postgresql do +describe Groups::TransferService do let(:user) { create(:user) } let(:new_parent_group) { create(:group, :public) } let!(:group_member) { create(:group_member, :owner, group: group, user: user) } let(:transfer_service) { described_class.new(group, user) } shared_examples 'ensuring allowed transfer for a group' do - context 'with other database than PostgreSQL' do - before do - allow(Group).to receive(:supports_nested_objects?).and_return(false) - end - - it 'returns false' do - expect(transfer_service.execute(new_parent_group)).to be_falsy - end - - it 'adds an error on group' do - transfer_service.execute(new_parent_group) - expect(transfer_service.error).to eq('Transfer failed: Database is not supported.') - end - end - context "when there's an exception on GitLab shell directories" do let(:new_parent_group) { create(:group, :public) } diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index d081c20f669..12e9c2b2f3a 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -86,6 +86,7 @@ describe Groups::UpdateService do context "unauthorized visibility_level validation" do let!(:service) { described_class.new(internal_group, user, visibility_level: 99) } + before do internal_group.add_user(user, Gitlab::Access::MAINTAINER) end @@ -96,6 +97,20 @@ describe Groups::UpdateService do end end + context 'when updating #emails_disabled' do + let(:service) { described_class.new(internal_group, user, emails_disabled: true) } + + it 'updates the attribute' do + internal_group.add_user(user, Gitlab::Access::OWNER) + + expect { service.execute }.to change { internal_group.emails_disabled }.to(true) + end + + it 'does not update when not group owner' do + expect { service.execute }.not_to change { internal_group.emails_disabled } + end + end + context 'rename group' do let!(:service) { described_class.new(internal_group, user, path: SecureRandom.hex) } @@ -133,7 +148,7 @@ describe Groups::UpdateService do end end - context 'for a subgroup', :nested_groups do + context 'for a subgroup' do let(:subgroup) { create(:group, :private, parent: private_group) } context 'when the parent group share_with_group_lock is enabled' do diff --git a/spec/services/issuable/bulk_update_service_spec.rb b/spec/services/issuable/bulk_update_service_spec.rb index aebc5ba2874..e3a728f2566 100644 --- a/spec/services/issuable/bulk_update_service_spec.rb +++ b/spec/services/issuable/bulk_update_service_spec.rb @@ -11,173 +11,31 @@ describe Issuable::BulkUpdateService do .reverse_merge(issuable_ids: Array(issuables).map(&:id).join(',')) type = Array(issuables).first.model_name.param_key - Issuable::BulkUpdateService.new(project, user, bulk_update_params).execute(type) + Issuable::BulkUpdateService.new(user, bulk_update_params).execute(type) end - describe 'close issues' do - let(:issues) { create_list(:issue, 2, project: project) } - - it 'succeeds and returns the correct number of issues updated' do - result = bulk_update(issues, state_event: 'close') + shared_examples 'updates milestones' do + it 'succeeds' do + result = bulk_update(issuables, milestone_id: milestone.id) expect(result[:success]).to be_truthy - expect(result[:count]).to eq(issues.count) - end - - it 'closes all the issues passed' do - bulk_update(issues, state_event: 'close') - - expect(project.issues.opened).to be_empty - expect(project.issues.closed).not_to be_empty + expect(result[:count]).to eq(issuables.count) end - context 'when issue for a different project is created' do - let(:private_project) { create(:project, :private) } - let(:issue) { create(:issue, project: private_project, author: user) } + it 'updates the issuables milestone' do + bulk_update(issuables, milestone_id: milestone.id) - context 'when user has access to the project' do - it 'closes all issues passed' do - private_project.add_maintainer(user) - - bulk_update(issues + [issue], state_event: 'close') - - expect(project.issues.opened).to be_empty - expect(project.issues.closed).not_to be_empty - expect(private_project.issues.closed).not_to be_empty - end - end - - context 'when user does not have access to project' do - it 'only closes all issues that the user has access to' do - bulk_update(issues + [issue], state_event: 'close') - - expect(project.issues.opened).to be_empty - expect(project.issues.closed).not_to be_empty - expect(private_project.issues.closed).to be_empty - end + issuables.each do |issuable| + expect(issuable.reload.milestone).to eq(milestone) end end end - describe 'reopen issues' do - let(:issues) { create_list(:closed_issue, 2, project: project) } - - it 'succeeds and returns the correct number of issues updated' do - result = bulk_update(issues, state_event: 'reopen') - - expect(result[:success]).to be_truthy - expect(result[:count]).to eq(issues.count) - end - - it 'reopens all the issues passed' do - bulk_update(issues, state_event: 'reopen') - - expect(project.issues.closed).to be_empty - expect(project.issues.opened).not_to be_empty - end - end - - describe 'updating merge request assignee' do - let(:merge_request) { create(:merge_request, target_project: project, source_project: project, assignees: [user]) } - - context 'when the new assignee ID is a valid user' do - it 'succeeds' do - new_assignee = create(:user) - project.add_developer(new_assignee) - - result = bulk_update(merge_request, assignee_ids: [user.id, new_assignee.id]) - - expect(result[:success]).to be_truthy - expect(result[:count]).to eq(1) - end - - it 'updates the assignee to the user ID passed' do - assignee = create(:user) - project.add_developer(assignee) - - expect { bulk_update(merge_request, assignee_ids: [assignee.id]) } - .to change { merge_request.reload.assignee_ids }.from([user.id]).to([assignee.id]) - end - end - - context "when the new assignee ID is #{IssuableFinder::NONE}" do - it 'unassigns the issues' do - expect { bulk_update(merge_request, assignee_ids: [IssuableFinder::NONE]) } - .to change { merge_request.reload.assignee_ids }.to([]) - end - end - - context 'when the new assignee ID is not present' do - it 'does not unassign' do - expect { bulk_update(merge_request, assignee_ids: []) } - .not_to change { merge_request.reload.assignee_ids } - end - end - end - - describe 'updating issue assignee' do - let(:issue) { create(:issue, project: project, assignees: [user]) } - - context 'when the new assignee ID is a valid user' do - it 'succeeds' do - new_assignee = create(:user) - project.add_developer(new_assignee) - - result = bulk_update(issue, assignee_ids: [new_assignee.id]) - - expect(result[:success]).to be_truthy - expect(result[:count]).to eq(1) - end - - it 'updates the assignee to the user ID passed' do - assignee = create(:user) - project.add_developer(assignee) - expect { bulk_update(issue, assignee_ids: [assignee.id]) } - .to change { issue.reload.assignees.first }.from(user).to(assignee) - end - end - - context "when the new assignee ID is #{IssuableFinder::NONE}" do - it "unassigns the issues" do - expect { bulk_update(issue, assignee_ids: [IssuableFinder::NONE.to_s]) } - .to change { issue.reload.assignees.count }.from(1).to(0) - end - end - - context 'when the new assignee ID is not present' do - it 'does not unassign' do - expect { bulk_update(issue, assignee_ids: []) } - .not_to change { issue.reload.assignees } - end - end - end - - describe 'updating milestones' do - let(:issue) { create(:issue, project: project) } - let(:milestone) { create(:milestone, project: project) } - - it 'succeeds' do - result = bulk_update(issue, milestone_id: milestone.id) - - expect(result[:success]).to be_truthy - expect(result[:count]).to eq(1) - end - - it 'updates the issue milestone' do - expect { bulk_update(issue, milestone_id: milestone.id) } - .to change { issue.reload.milestone }.from(nil).to(milestone) - end - end - - describe 'updating labels' do + shared_examples 'updating labels' do def create_issue_with_labels(labels) create(:labeled_issue, project: project, labels: labels) end - let(:bug) { create(:label, project: project) } - let(:regression) { create(:label, project: project) } - let(:merge_requests) { create(:label, project: project) } - let(:issue_all_labels) { create_issue_with_labels([bug, regression, merge_requests]) } let(:issue_bug_and_regression) { create_issue_with_labels([bug, regression]) } let(:issue_bug_and_merge_requests) { create_issue_with_labels([bug, merge_requests]) } @@ -257,7 +115,7 @@ describe Issuable::BulkUpdateService do end it 'removes the label IDs from all issues passed' do - expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(merge_requests.id) + expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(merge_requests.id) end it 'does not update issues not passed in' do @@ -289,11 +147,11 @@ describe Issuable::BulkUpdateService do let(:remove_labels) { [regression] } it 'removes the label IDs from all issues passed' do - expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(regression.id) + expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(regression.id) end it 'ignores the label IDs parameter' do - expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(merge_requests.id) + expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(merge_requests.id) end it 'does not update issues not passed in' do @@ -312,11 +170,11 @@ describe Issuable::BulkUpdateService do end it 'removes the label IDs from all issues passed' do - expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(merge_requests.id) + expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(merge_requests.id) end it 'ignores the label IDs parameter' do - expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(regression.id) + expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(regression.id) end it 'does not update issues not passed in' do @@ -325,29 +183,226 @@ describe Issuable::BulkUpdateService do end end - describe 'subscribe to issues' do - let(:issues) { create_list(:issue, 2, project: project) } + context 'with issuables at a project level' do + describe 'close issues' do + let(:issues) { create_list(:issue, 2, project: project) } + + it 'succeeds and returns the correct number of issues updated' do + result = bulk_update(issues, state_event: 'close') + + expect(result[:success]).to be_truthy + expect(result[:count]).to eq(issues.count) + end + + it 'closes all the issues passed' do + bulk_update(issues, state_event: 'close') + + expect(project.issues.opened).to be_empty + expect(project.issues.closed).not_to be_empty + end + + context 'when issue for a different project is created' do + let(:private_project) { create(:project, :private) } + let(:issue) { create(:issue, project: private_project, author: user) } + + context 'when user has access to the project' do + it 'closes all issues passed' do + private_project.add_maintainer(user) - it 'subscribes the given user' do - bulk_update(issues, subscription_event: 'subscribe') + bulk_update(issues + [issue], state_event: 'close') - expect(issues).to all(be_subscribed(user, project)) + expect(project.issues.opened).to be_empty + expect(project.issues.closed).not_to be_empty + expect(private_project.issues.closed).not_to be_empty + end + end + + context 'when user does not have access to project' do + it 'only closes all issues that the user has access to' do + bulk_update(issues + [issue], state_event: 'close') + + expect(project.issues.opened).to be_empty + expect(project.issues.closed).not_to be_empty + expect(private_project.issues.closed).to be_empty + end + end + end + end + + describe 'reopen issues' do + let(:issues) { create_list(:closed_issue, 2, project: project) } + + it 'succeeds and returns the correct number of issues updated' do + result = bulk_update(issues, state_event: 'reopen') + + expect(result[:success]).to be_truthy + expect(result[:count]).to eq(issues.count) + end + + it 'reopens all the issues passed' do + bulk_update(issues, state_event: 'reopen') + + expect(project.issues.closed).to be_empty + expect(project.issues.opened).not_to be_empty + end + end + + describe 'updating merge request assignee' do + let(:merge_request) { create(:merge_request, target_project: project, source_project: project, assignees: [user]) } + + context 'when the new assignee ID is a valid user' do + it 'succeeds' do + new_assignee = create(:user) + project.add_developer(new_assignee) + + result = bulk_update(merge_request, assignee_ids: [user.id, new_assignee.id]) + + expect(result[:success]).to be_truthy + expect(result[:count]).to eq(1) + end + + it 'updates the assignee to the user ID passed' do + assignee = create(:user) + project.add_developer(assignee) + + expect { bulk_update(merge_request, assignee_ids: [assignee.id]) } + .to change { merge_request.reload.assignee_ids }.from([user.id]).to([assignee.id]) + end + end + + context "when the new assignee ID is #{IssuableFinder::NONE}" do + it 'unassigns the issues' do + expect { bulk_update(merge_request, assignee_ids: [IssuableFinder::NONE]) } + .to change { merge_request.reload.assignee_ids }.to([]) + end + end + + context 'when the new assignee ID is not present' do + it 'does not unassign' do + expect { bulk_update(merge_request, assignee_ids: []) } + .not_to change { merge_request.reload.assignee_ids } + end + end + end + + describe 'updating issue assignee' do + let(:issue) { create(:issue, project: project, assignees: [user]) } + + context 'when the new assignee ID is a valid user' do + it 'succeeds' do + new_assignee = create(:user) + project.add_developer(new_assignee) + + result = bulk_update(issue, assignee_ids: [new_assignee.id]) + + expect(result[:success]).to be_truthy + expect(result[:count]).to eq(1) + end + + it 'updates the assignee to the user ID passed' do + assignee = create(:user) + project.add_developer(assignee) + expect { bulk_update(issue, assignee_ids: [assignee.id]) } + .to change { issue.reload.assignees.first }.from(user).to(assignee) + end + end + + context "when the new assignee ID is #{IssuableFinder::NONE}" do + it "unassigns the issues" do + expect { bulk_update(issue, assignee_ids: [IssuableFinder::NONE.to_s]) } + .to change { issue.reload.assignees.count }.from(1).to(0) + end + end + + context 'when the new assignee ID is not present' do + it 'does not unassign' do + expect { bulk_update(issue, assignee_ids: []) } + .not_to change(issue.assignees, :count) + end + end + end + + describe 'updating milestones' do + let(:issuables) { [create(:issue, project: project)] } + let(:milestone) { create(:milestone, project: project) } + + it_behaves_like 'updates milestones' + end + + describe 'updating labels' do + let(:bug) { create(:label, project: project) } + let(:regression) { create(:label, project: project) } + let(:merge_requests) { create(:label, project: project) } + + it_behaves_like 'updating labels' + end + + describe 'subscribe to issues' do + let(:issues) { create_list(:issue, 2, project: project) } + + it 'subscribes the given user' do + bulk_update(issues, subscription_event: 'subscribe') + + expect(issues).to all(be_subscribed(user, project)) + end + end + + describe 'unsubscribe from issues' do + let(:issues) do + create_list(:closed_issue, 2, project: project) do |issue| + issue.subscriptions.create(user: user, project: project, subscribed: true) + end + end + + it 'unsubscribes the given user' do + bulk_update(issues, subscription_event: 'unsubscribe') + + issues.each do |issue| + expect(issue).not_to be_subscribed(user, project) + end + end end end - describe 'unsubscribe from issues' do - let(:issues) do - create_list(:closed_issue, 2, project: project) do |issue| - issue.subscriptions.create(user: user, project: project, subscribed: true) + context 'with issuables at a group level' do + let(:group) { create(:group) } + + describe 'updating milestones' do + let(:milestone) { create(:milestone, group: group) } + let(:project) { create(:project, :repository, group: group) } + + before do + group.add_maintainer(user) + end + + context 'when issues' do + let(:issue1) { create(:issue, project: project) } + let(:issue2) { create(:issue, project: project) } + let(:issuables) { [issue1, issue2] } + + it_behaves_like 'updates milestones' + end + + context 'when merge requests' do + let(:merge_request1) { create(:merge_request, source_project: project, source_branch: 'branch-1') } + let(:merge_request2) { create(:merge_request, source_project: project, source_branch: 'branch-2') } + let(:issuables) { [merge_request1, merge_request2] } + + it_behaves_like 'updates milestones' end end - it 'unsubscribes the given user' do - bulk_update(issues, subscription_event: 'unsubscribe') + describe 'updating labels' do + let(:project) { create(:project, :repository, group: group) } + let(:bug) { create(:group_label, group: group) } + let(:regression) { create(:group_label, group: group) } + let(:merge_requests) { create(:group_label, group: group) } - issues.each do |issue| - expect(issue).not_to be_subscribed(user, project) + before do + group.add_reporter(user) end + + it_behaves_like 'updating labels' end end end diff --git a/spec/services/issuable/clone/content_rewriter_spec.rb b/spec/services/issuable/clone/content_rewriter_spec.rb index 230e1123280..3479c20862a 100644 --- a/spec/services/issuable/clone/content_rewriter_spec.rb +++ b/spec/services/issuable/clone/content_rewriter_spec.rb @@ -165,5 +165,18 @@ describe Issuable::Clone::ContentRewriter do expect(note.note_html).not_to eq(new_note.note_html) end end + + context "discussion notes" do + let(:note) { create(:note, noteable: original_issue, note: "sample note", project: project1) } + let!(:discussion) { create(:discussion_note_on_issue, in_reply_to: note, note: "reply to sample note") } + + it 'rewrites discussion correctly' do + subject.execute + + expect(new_issue.notes.count).to eq(original_issue.notes.count) + expect(new_issue.notes.where(discussion_id: discussion.discussion_id).count).to eq(0) + expect(original_issue.notes.where(discussion_id: discussion.discussion_id).count).to eq(1) + end + end end end diff --git a/spec/services/issues/reorder_service_spec.rb b/spec/services/issues/reorder_service_spec.rb new file mode 100644 index 00000000000..b147cdf4e64 --- /dev/null +++ b/spec/services/issues/reorder_service_spec.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Issues::ReorderService do + set(:user) { create(:user) } + set(:project) { create(:project) } + set(:group) { create(:group) } + + shared_examples 'issues reorder service' do + context 'when reordering issues' do + it 'returns false with no params' do + expect(service({}).execute(issue1)).to be_falsey + end + + it 'returns false with both invalid params' do + params = { move_after_id: nil, move_before_id: 1 } + + expect(service(params).execute(issue1)).to be_falsey + end + + it 'sorts issues' do + params = { move_after_id: issue2.id, move_before_id: issue3.id } + + service(params).execute(issue1) + + expect(issue1.relative_position) + .to be_between(issue2.relative_position, issue3.relative_position) + end + end + end + + describe '#execute' do + let(:issue1) { create(:issue, project: project, relative_position: 10) } + let(:issue2) { create(:issue, project: project, relative_position: 20) } + let(:issue3) { create(:issue, project: project, relative_position: 30) } + + context 'when ordering issues in a project' do + let(:parent) { project } + + before do + parent.add_developer(user) + end + + it_behaves_like 'issues reorder service' + end + + context 'when ordering issues in a group' do + let(:project) { create(:project, namespace: group) } + + before do + group.add_developer(user) + end + + it_behaves_like 'issues reorder service' + + context 'when ordering in a group issue list' do + let(:params) { { move_after_id: issue2.id, move_before_id: issue3.id, group_full_path: group.full_path } } + + subject { service(params) } + + it 'sends the board_group_id parameter' do + match_params = { move_between_ids: [issue2.id, issue3.id], board_group_id: group.id } + + expect(Issues::UpdateService) + .to receive(:new).with(project, user, match_params) + .and_return(double(execute: build(:issue))) + + subject.execute(issue1) + end + + it 'sorts issues' do + project2 = create(:project, namespace: group) + issue4 = create(:issue, project: project2) + + subject.execute(issue4) + + expect(issue4.relative_position) + .to be_between(issue2.relative_position, issue3.relative_position) + end + end + end + end + + def service(params) + described_class.new(project, user, params) + end +end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 22f5607cb9c..d9f35afee06 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -116,7 +116,7 @@ describe Issues::UpdateService, :mailer do expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position) end - context 'when moving issue between issues from different projects', :nested_groups do + context 'when moving issue between issues from different projects' do let(:group) { create(:group) } let(:subgroup) { create(:group, parent: group) } @@ -179,7 +179,7 @@ describe Issues::UpdateService, :mailer do it 'sends email to user2 about assign of new issue and email to user3 about issue unassignment' do deliveries = ActionMailer::Base.deliveries email = deliveries.last - recipients = deliveries.last(2).map(&:to).flatten + recipients = deliveries.last(2).flat_map(&:to) expect(recipients).to include(user2.email, user3.email) expect(email.subject).to include(issue.title) end @@ -226,6 +226,15 @@ describe Issues::UpdateService, :mailer do end end + it 'creates zoom_link_added system note when a zoom link is added to the description' do + update_issue(description: 'Changed description https://zoom.us/j/5873603787') + + note = find_note('a Zoom call was added') + + expect(note).not_to be_nil + expect(note.note).to eq('a Zoom call was added to this issue') + end + context 'when issue turns confidential' do let(:opts) do { @@ -480,6 +489,22 @@ describe Issues::UpdateService, :mailer do update_issue(description: "- [x] Task 1\n- [X] Task 2") end + it 'does not check for spam on task status change' do + params = { + update_task: { + index: 1, + checked: false, + line_source: '- [x] Task 1', + line_number: 1 + } + } + service = described_class.new(project, user, params) + + expect(service).not_to receive(:spam_check) + + service.execute(issue) + end + it 'creates system note about task status change' do note1 = find_note('marked the task **Task 1** as completed') note2 = find_note('marked the task **Task 2** as completed') @@ -687,6 +712,22 @@ describe Issues::UpdateService, :mailer do end end + context 'when moving an issue ' do + it 'raises an error for invalid move ids within a project' do + opts = { move_between_ids: [9000, 9999] } + + expect { described_class.new(issue.project, user, opts).execute(issue) } + .to raise_error(ActiveRecord::RecordNotFound) + end + + it 'raises an error for invalid move ids within a group' do + opts = { move_between_ids: [9000, 9999], board_group_id: create(:group).id } + + expect { described_class.new(issue.project, user, opts).execute(issue) } + .to raise_error(ActiveRecord::RecordNotFound) + end + end + include_examples 'issuable update service' do let(:open_issuable) { issue } let(:closed_issuable) { create(:closed_issue, project: project) } diff --git a/spec/services/lfs/file_transformer_spec.rb b/spec/services/lfs/file_transformer_spec.rb index 888eea6e91e..9973d64930b 100644 --- a/spec/services/lfs/file_transformer_spec.rb +++ b/spec/services/lfs/file_transformer_spec.rb @@ -3,13 +3,13 @@ require "spec_helper" describe Lfs::FileTransformer do - let(:project) { create(:project, :repository) } + let(:project) { create(:project, :repository, :wiki_repo) } let(:repository) { project.repository } let(:file_content) { 'Test file content' } let(:branch_name) { 'lfs' } let(:file_path) { 'test_file.lfs' } - subject { described_class.new(project, branch_name) } + subject { described_class.new(project, repository, branch_name) } describe '#new_file' do context 'with lfs disabled' do @@ -100,6 +100,12 @@ describe Lfs::FileTransformer do end.to change { project.lfs_objects.count }.by(1) end + it 'saves the repository_type to LfsObjectsProject' do + subject.new_file(file_path, file_content) + + expect(project.lfs_objects_projects.first.repository_type).to eq('project') + end + context 'when LfsObject already exists' do let(:lfs_pointer) { Gitlab::Git::LfsPointerFile.new(file_content) } @@ -113,6 +119,56 @@ describe Lfs::FileTransformer do end.to change { project.lfs_objects.count }.by(1) end end + + context 'when the LfsObject is already linked to project' do + before do + subject.new_file(file_path, file_content) + end + + shared_examples 'a new LfsObject is not created' do + it do + expect do + second_service.new_file(file_path, file_content) + end.not_to change { project.lfs_objects.count } + end + end + + context 'and the service is called again with the same repository type' do + let(:second_service) { described_class.new(project, repository, branch_name) } + + include_examples 'a new LfsObject is not created' + + it 'does not create a new LfsObjectsProject record' do + expect do + second_service.new_file(file_path, file_content) + end.not_to change { project.lfs_objects_projects.count } + end + end + + context 'and the service is called again with a different repository type' do + let(:second_service) { described_class.new(project, project.wiki.repository, branch_name) } + + before do + expect(second_service).to receive(:lfs_file?).and_return(true) + end + + include_examples 'a new LfsObject is not created' + + it 'creates a new LfsObjectsProject record' do + expect do + second_service.new_file(file_path, file_content) + end.to change { project.lfs_objects_projects.count }.by(1) + end + + it 'sets the correct repository_type on the new LfsObjectsProject record' do + second_service.new_file(file_path, file_content) + + repository_types = project.lfs_objects_projects.order(:id).pluck(:repository_type) + + expect(repository_types).to eq(%w(project wiki)) + end + end + end end end end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 52f9a305d8f..7dce7f035d4 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -267,15 +267,15 @@ describe Members::DestroyService do expect(group.members.map(&:user)).not_to include(member_user) end - it 'removes the subgroup membership', :postgresql do + it 'removes the subgroup membership' do expect(subgroup.members.map(&:user)).not_to include(member_user) end - it 'removes the subsubgroup membership', :postgresql do + it 'removes the subsubgroup membership' do expect(subsubgroup.members.map(&:user)).not_to include(member_user) end - it 'removes the subsubproject membership', :postgresql do + it 'removes the subsubproject membership' do expect(subsubproject.members.map(&:user)).not_to include(member_user) end diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index 5c3b209086c..f18239f6d39 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -1,5 +1,4 @@ # frozen_string_literal: true - require 'spec_helper' describe MergeRequests::BuildService do @@ -225,6 +224,11 @@ describe MergeRequests::BuildService do let(:label_ids) { [label2.id] } let(:milestone_id) { milestone2.id } + before do + # Guests are not able to assign labels or milestones to an issue + project.add_developer(user) + end + it 'assigns milestone_id and label_ids instead of issue labels and milestone' do expect(merge_request.milestone).to eq(milestone2) expect(merge_request.labels).to match_array([label2]) @@ -479,4 +483,35 @@ describe MergeRequests::BuildService do end end end + + context 'when assigning labels' do + let(:label_ids) { [create(:label, project: project).id] } + + context 'for members with less than developer access' do + it 'is not allowed' do + expect(merge_request.label_ids).to be_empty + end + end + + context 'for users allowed to assign labels' do + before do + project.add_developer(user) + end + + context 'for labels in the project' do + it 'is allowed for developers' do + expect(merge_request.label_ids).to contain_exactly(*label_ids) + end + end + + context 'for unrelated labels' do + let(:project_label) { create(:label, project: project) } + let(:label_ids) { [create(:label).id, project_label.id] } + + it 'only assigns related labels' do + expect(merge_request.label_ids).to contain_exactly(project_label.id) + end + end + end + 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 a0ac7dba89d..0e0da6a13ab 100644 --- a/spec/services/merge_requests/create_from_issue_service_spec.rb +++ b/spec/services/merge_requests/create_from_issue_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe MergeRequests::CreateFromIssueService do + include ProjectForksHelper + let(:project) { create(:project, :repository) } let(:user) { create(:user) } let(:label_ids) { create_pair(:label, project: project).map(&:id) } @@ -10,139 +12,174 @@ describe MergeRequests::CreateFromIssueService do let(:issue) { create(:issue, project: project, milestone_id: milestone_id) } let(:custom_source_branch) { 'custom-source-branch' } - subject(:service) { described_class.new(project, user, issue_iid: issue.iid) } - subject(:service_with_custom_source_branch) { described_class.new(project, user, issue_iid: issue.iid, branch_name: custom_source_branch) } + subject(:service) { described_class.new(project, user, service_params) } + subject(:service_with_custom_source_branch) { described_class.new(project, user, branch_name: custom_source_branch, **service_params) } before do project.add_developer(user) end describe '#execute' do - it 'returns an error with invalid issue iid' do - result = described_class.new(project, user, issue_iid: -1).execute + 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 - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Invalid issue iid') - end + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('Not allowed to create merge request') + end - it 'delegates issue search to IssuesFinder' do - expect_any_instance_of(IssuesFinder).to receive(:find_by).once.and_call_original + it 'returns an error with invalid issue iid' do + result = described_class.new(project, user, issue_iid: -1).execute - described_class.new(project, user, issue_iid: -1).execute - end - - it "inherits labels" do - issue.assign_attributes(label_ids: label_ids) + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('Invalid issue iid') + end - result = service.execute + it 'creates a branch based on issue title' do + service.execute - expect(result[:merge_request].label_ids).to eq(label_ids) - end + expect(target_project.repository.branch_exists?(issue.to_branch_name)).to be_truthy + end - it "inherits milestones" do - result = service.execute + it 'creates a branch using passed name' do + service_with_custom_source_branch.execute - expect(result[:merge_request].milestone_id).to eq(milestone_id) - end + expect(target_project.repository.branch_exists?(custom_source_branch)).to be_truthy + end - it 'delegates the branch creation to CreateBranchService' do - expect_any_instance_of(CreateBranchService).to receive(:execute).once.and_call_original + it 'creates the new_merge_request system note' do + expect(SystemNoteService).to receive(:new_merge_request).with(issue, project, user, instance_of(MergeRequest)) - service.execute - end + service.execute + end - it 'creates a branch based on issue title' do - service.execute + it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created' do + expect_any_instance_of(MergeRequest).to receive(:valid?).at_least(:once).and_return(false) - expect(project.repository.branch_exists?(issue.to_branch_name)).to be_truthy - end + expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, issue.to_branch_name, branch_project: target_project) - it 'creates a branch using passed name' do - service_with_custom_source_branch.execute + service.execute + end - expect(project.repository.branch_exists?(custom_source_branch)).to be_truthy - end + it 'creates a merge request' do + expect { service.execute }.to change(target_project.merge_requests, :count).by(1) + end - it 'creates the new_merge_request system note' do - expect(SystemNoteService).to receive(:new_merge_request).with(issue, project, user, instance_of(MergeRequest)) + it 'sets the merge request author to current user' do + result = service.execute - service.execute - end + expect(result[:merge_request].author).to eq(user) + end - it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created' do - project.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED) + it 'sets the merge request source branch to the new issue branch' do + result = service.execute - expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, issue.to_branch_name) + expect(result[:merge_request].source_branch).to eq(issue.to_branch_name) + end - service.execute - end + it 'sets the merge request source branch to the passed branch name' do + result = service_with_custom_source_branch.execute - it 'creates a merge request' do - expect { service.execute }.to change(project.merge_requests, :count).by(1) - end + expect(result[:merge_request].source_branch).to eq(custom_source_branch) + end - it 'sets the merge request title to: "WIP: Resolves "$issue-title"' do - result = service.execute + it 'sets the merge request target branch to the project default branch' do + result = service.execute - expect(result[:merge_request].title).to eq("WIP: Resolve \"#{issue.title}\"") - end + expect(result[:merge_request].target_branch).to eq(target_project.default_branch) + end - it 'sets the merge request author to current user' do - result = service.execute + it 'executes quick actions if the build service sets them in the description' do + allow(service).to receive(:merge_request).and_wrap_original do |m, *args| + m.call(*args).tap do |merge_request| + merge_request.description = "/assign #{user.to_reference}" + end + end - expect(result[:merge_request].author).to eq(user) - end + result = service.execute - it 'sets the merge request source branch to the new issue branch' do - result = service.execute + expect(result[:merge_request].assignees).to eq([user]) + end - expect(result[:merge_request].source_branch).to eq(issue.to_branch_name) - end + context 'when ref branch is set' do + subject { described_class.new(project, user, ref: 'feature', **service_params).execute } - it 'sets the merge request source branch to the passed branch name' do - result = service_with_custom_source_branch.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) + end - expect(result[:merge_request].source_branch).to eq(custom_source_branch) - end + it 'sets the merge request target branch to the ref branch' do + expect(subject[:merge_request].target_branch).to eq('feature') + end - it 'sets the merge request target branch to the project default branch' do - result = service.execute + context 'when ref branch does not exist' do + subject { described_class.new(project, user, ref: 'no-such-branch', **service_params).execute } - expect(result[:merge_request].target_branch).to eq(project.default_branch) - end + it 'creates a merge request' do + expect { subject }.to change(target_project.merge_requests, :count).by(1) + end - it 'executes quick actions if the build service sets them in the description' do - allow(service).to receive(:merge_request).and_wrap_original do |m, *args| - m.call(*args).tap do |merge_request| - merge_request.description = "/assign #{user.to_reference}" + it 'sets the merge request target branch to the project default branch' do + expect(subject[:merge_request].target_branch).to eq(target_project.default_branch) + end end end + end - result = service.execute + context 'no target_project_id specified' do + let(:service_params) { { issue_iid: issue.iid } } + let(:target_project) { project } - expect(result[:merge_request].assignees).to eq([user]) - end + it_behaves_like 'a service that creates a merge request from an issue' - context 'when ref branch is set' do - subject { described_class.new(project, user, issue_iid: issue.iid, ref: 'feature').execute } + it "inherits labels" do + issue.assign_attributes(label_ids: label_ids) - 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) + result = service.execute + + expect(result[:merge_request].label_ids).to eq(label_ids) end - it 'sets the merge request target branch to the ref branch' do - expect(subject[:merge_request].target_branch).to eq('feature') + it "inherits milestones" do + result = service.execute + + expect(result[:merge_request].milestone_id).to eq(milestone_id) end - context 'when ref branch does not exist' do - subject { described_class.new(project, user, issue_iid: issue.iid, ref: 'no-such-branch').execute } + it 'sets the merge request title to: "WIP: Resolves "$issue-title"' do + result = service.execute - it 'creates a merge request' do - expect { subject }.to change(project.merge_requests, :count).by(1) + expect(result[:merge_request].title).to eq("WIP: Resolve \"#{issue.title}\"") + end + end + + context 'target_project_id is specified' do + let(:service_params) { { issue_iid: issue.iid, target_project_id: target_project.id } } + + context 'target project is not a fork of the project' do + let(:target_project) { create(:project, :repository) } + + it 'returns an error about not finding the project' do + result = service.execute + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('Project not found') end - it 'sets the merge request target branch to the project default branch' do - expect(subject[:merge_request].target_branch).to eq(project.default_branch) + it 'does not create merge request' do + expect { service.execute }.to change(target_project.merge_requests, :count).by(0) + end + end + + context 'target project is a fork of project project' do + let(:target_project) { fork_project(project, user, repository: true) } + + it_behaves_like 'a service that creates a merge request from an issue' + + it 'sets the merge request title to: "WIP: $issue-branch-name' do + result = service.execute + + expect(result[:merge_request].title).to eq("WIP: #{issue.to_branch_name.titleize.humanize}") end end end diff --git a/spec/services/merge_requests/get_urls_service_spec.rb b/spec/services/merge_requests/get_urls_service_spec.rb index 0933c6d4336..9e7a5260ca4 100644 --- a/spec/services/merge_requests/get_urls_service_spec.rb +++ b/spec/services/merge_requests/get_urls_service_spec.rb @@ -8,8 +8,8 @@ describe MergeRequests::GetUrlsService do let(:project) { create(:project, :public, :repository) } let(:service) { described_class.new(project) } let(:source_branch) { "merge-test" } - let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{source_branch}" } - let(:show_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/#{merge_request.iid}" } + 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}" } let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } let(:deleted_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 #{Gitlab::Git::BLANK_SHA} refs/heads/#{source_branch}" } let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } @@ -119,7 +119,7 @@ describe MergeRequests::GetUrlsService do let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch" } let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/markdown" } let(:changes) { "#{new_branch_changes}\n#{existing_branch_changes}" } - let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch" } + let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch" } it 'returns 2 urls for both creating new and showing merge request' do result = service.execute(changes) diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 2fbe5468b21..22578436c18 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -58,7 +58,7 @@ describe MergeRequests::MergeService do expect(issue.reload.closed?).to be_truthy end - context 'with JIRA integration' do + context 'with Jira integration' do include JiraServiceHelper let(:jira_tracker) { project.create_jira_service } @@ -72,7 +72,7 @@ describe MergeRequests::MergeService do allow(merge_request).to receive(:commits).and_return([commit]) end - it 'closes issues on JIRA issue tracker' do + it 'closes issues on Jira issue tracker' do jira_issue = ExternalIssue.new('JIRA-123', project) stub_jira_urls(jira_issue) commit = double('commit', safe_message: "Fixes #{jira_issue.to_reference}") @@ -98,7 +98,7 @@ describe MergeRequests::MergeService do end context "wrong issue markdown" do - it 'does not close issues on JIRA issue tracker' do + it 'does not close issues on Jira issue tracker' do jira_issue = ExternalIssue.new('#JIRA-123', project) stub_jira_urls(jira_issue) commit = double('commit', safe_message: "Fixes #{jira_issue.to_reference}") @@ -214,6 +214,19 @@ describe MergeRequests::MergeService do allow(Rails.logger).to receive(:error) end + context 'when source is missing' do + it 'logs and saves error' do + allow(merge_request).to receive(:diff_head_sha) { nil } + + error_message = 'No source for merge' + + service.execute(merge_request) + + expect(merge_request.merge_error).to eq(error_message) + expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + end + end + it 'logs and saves error if there is an exception' do error_message = 'error message' 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 0ac23050caf..758679edc45 100644 --- a/spec/services/merge_requests/merge_to_ref_service_spec.rb +++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb @@ -22,7 +22,6 @@ describe MergeRequests::MergeToRefService do shared_examples_for 'successfully merges to ref with merge method' do it 'writes commit to merge ref' do repository = project.repository - target_ref = merge_request.merge_ref_path expect(repository.ref_exists?(target_ref)).to be(false) @@ -33,7 +32,7 @@ describe MergeRequests::MergeToRefService do expect(result[:status]).to eq(:success) expect(result[:commit_id]).to be_present expect(result[:source_id]).to eq(merge_request.source_branch_sha) - expect(result[:target_id]).to eq(merge_request.target_branch_sha) + expect(result[:target_id]).to eq(repository.commit(first_parent_ref).sha) expect(repository.ref_exists?(target_ref)).to be(true) expect(ref_head.id).to eq(result[:commit_id]) end @@ -72,26 +71,33 @@ describe MergeRequests::MergeToRefService do let(:merge_request) { create(:merge_request, :simple) } let(:project) { merge_request.project } - before do - project.add_maintainer(user) - end - describe '#execute' do let(:service) do - described_class.new(project, user, commit_message: 'Awesome message', - should_remove_source_branch: true) + described_class.new(project, user, **params) end + let(:params) { { commit_message: 'Awesome message', should_remove_source_branch: true } } + def process_merge_to_ref perform_enqueued_jobs do service.execute(merge_request) end end - it_behaves_like 'successfully merges to ref with merge method' + it_behaves_like 'successfully merges to ref with merge method' do + let(:first_parent_ref) { 'refs/heads/master' } + let(:target_ref) { merge_request.merge_ref_path } + end + it_behaves_like 'successfully evaluates pre-condition checks' context 'commit history comparison with regular MergeService' do + before do + # The merge service needs an authorized user while merge-to-ref + # doesn't. + project.add_maintainer(user) + end + let(:merge_ref_service) do described_class.new(project, user, {}) end @@ -104,12 +110,18 @@ describe MergeRequests::MergeToRefService do it_behaves_like 'MergeService for target ref' end - context 'when merge commit with squash', :quarantine do + context 'when merge commit with squash' do before do - merge_request.update!(squash: true, source_branch: 'master', target_branch: 'feature') + merge_request.update!(squash: true) end it_behaves_like 'MergeService for target ref' + + it 'does not squash before merging' do + expect(MergeRequests::SquashService).not_to receive(:new) + + process_merge_to_ref + end end end @@ -121,14 +133,22 @@ describe MergeRequests::MergeToRefService do context 'when semi-linear merge method' do let(:merge_method) { :rebase_merge } - it_behaves_like 'successfully merges to ref with merge method' + it_behaves_like 'successfully merges to ref with merge method' do + let(:first_parent_ref) { 'refs/heads/master' } + let(:target_ref) { merge_request.merge_ref_path } + end + it_behaves_like 'successfully evaluates pre-condition checks' end context 'when fast-forward merge method' do let(:merge_method) { :ff } - it_behaves_like 'successfully merges to ref with merge method' + it_behaves_like 'successfully merges to ref with merge method' do + let(:first_parent_ref) { 'refs/heads/master' } + let(:target_ref) { merge_request.merge_ref_path } + end + it_behaves_like 'successfully evaluates pre-condition checks' end @@ -136,9 +156,9 @@ describe MergeRequests::MergeToRefService do let(:merge_method) { :merge } it 'returns error' do - allow(merge_request).to receive(:mergeable_to_ref?) { false } + allow(project).to receive_message_chain(:repository, :merge_to_ref) { nil } - error_message = "Merge request is not mergeable to #{merge_request.merge_ref_path}" + error_message = 'Conflicts detected during merge' result = service.execute(merge_request) @@ -171,27 +191,55 @@ describe MergeRequests::MergeToRefService do it { expect(todo).not_to be_done } end - context 'when merge request is WIP state' do - it 'fails to merge' do - merge_request = create(:merge_request, title: 'WIP: The feature') + context 'when source is missing' do + it 'returns error' do + allow(merge_request).to receive(:diff_head_sha) { nil } + + error_message = 'No source for merge' result = service.execute(merge_request) expect(result[:status]).to eq(:error) - expect(result[:message]).to eq("Merge request is not mergeable to #{merge_request.merge_ref_path}") + expect(result[:message]).to eq(error_message) end end - it 'returns error when user has no authorization to admin the merge request' do - unauthorized_user = create(:user) - project.add_reporter(unauthorized_user) + context 'when target ref is passed as a parameter' do + let(:params) { { commit_message: 'merge train', target_ref: target_ref } } - service = described_class.new(project, unauthorized_user) + it_behaves_like 'successfully merges to ref with merge method' do + let(:first_parent_ref) { 'refs/heads/master' } + let(:target_ref) { 'refs/merge-requests/1/train' } + end + end - result = service.execute(merge_request) + describe 'cascading merge refs' do + set(:project) { create(:project, :repository) } + let(:params) { { commit_message: 'Cascading merge', first_parent_ref: first_parent_ref, target_ref: target_ref } } - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('You are not allowed to merge to this ref') + context 'when first merge happens' do + let(:merge_request) do + create(:merge_request, source_project: project, source_branch: 'feature', + target_project: project, target_branch: 'master') + end + + it_behaves_like 'successfully merges to ref with merge method' do + let(:first_parent_ref) { 'refs/heads/master' } + let(:target_ref) { 'refs/merge-requests/1/train' } + end + + context 'when second merge happens' do + let(:merge_request) do + create(:merge_request, source_project: project, source_branch: 'improve/awesome', + target_project: project, target_branch: 'master') + end + + it_behaves_like 'successfully merges to ref with merge method' do + let(:first_parent_ref) { 'refs/merge-requests/1/train' } + let(:target_ref) { 'refs/merge-requests/2/train' } + end + end + end end end end diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb new file mode 100644 index 00000000000..a864da0a6fb --- /dev/null +++ b/spec/services/merge_requests/mergeability_check_service_spec.rb @@ -0,0 +1,336 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shared_state do + shared_examples_for 'unmergeable merge request' do + it 'updates or keeps merge status as cannot_be_merged' do + subject + + expect(merge_request.merge_status).to eq('cannot_be_merged') + end + + it 'does not change the merge ref HEAD' do + merge_ref_head = merge_request.merge_ref_head + + subject + + expect(merge_request.reload.merge_ref_head).to eq merge_ref_head + end + + it 'returns ServiceResponse.error' do + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result).to be_error + end + end + + shared_examples_for 'mergeable merge request' do + it 'updates or keeps merge status as can_be_merged' do + subject + + expect(merge_request.merge_status).to eq('can_be_merged') + end + + it 'updates the merge ref' do + expect { subject }.to change(merge_request, :merge_ref_head).from(nil) + end + + it 'returns ServiceResponse.success' do + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result).to be_success + end + + it 'ServiceResponse has merge_ref_head payload' do + result = subject + + expect(result.payload.keys).to contain_exactly(:merge_ref_head) + expect(result.payload[:merge_ref_head].keys) + .to contain_exactly(:commit_id, :target_id, :source_id) + end + end + + describe '#execute' do + let(:project) { create(:project, :repository) } + let(:merge_request) { create(:merge_request, merge_status: :unchecked, source_project: project, target_project: project) } + let(:repo) { project.repository } + + subject { described_class.new(merge_request).execute } + + def execute_within_threads(amount:, retry_lease: true) + threads = [] + + amount.times do + # Let's use a different object for each thread to get closer + # to a real world scenario. + mr = MergeRequest.find(merge_request.id) + + threads << Thread.new do + described_class.new(mr).execute(retry_lease: retry_lease) + end + end + + threads.each(&:join) + threads + end + + before do + project.add_developer(merge_request.author) + end + + it_behaves_like 'mergeable merge request' + + context 'when lock is disabled' do + before do + stub_feature_flags(merge_ref_auto_sync_lock: false) + end + + it_behaves_like 'mergeable merge request' + end + + context 'when concurrent calls' do + it 'waits first lock and returns "cached" result in subsequent calls' do + threads = execute_within_threads(amount: 3) + results = threads.map { |t| t.value.status } + + expect(results).to contain_exactly(:success, :success, :success) + end + + it 'writes the merge-ref once' do + service = instance_double(MergeRequests::MergeToRefService) + + expect(MergeRequests::MergeToRefService).to receive(:new).once { service } + expect(service).to receive(:execute).once.and_return(success: true) + + execute_within_threads(amount: 3) + end + + it 'resets one merge request upon execution' do + expect_any_instance_of(MergeRequest).to receive(:reset).once + + execute_within_threads(amount: 2) + end + + context 'when retry_lease flag is false' do + it 'the first call succeeds, subsequent concurrent calls get a lock error response' do + threads = execute_within_threads(amount: 3, retry_lease: false) + results = threads.map { |t| [t.value.status, t.value.message] } + + expect(results).to contain_exactly([:error, 'Failed to obtain a lock'], + [:error, 'Failed to obtain a lock'], + [:success, nil]) + end + end + end + + context 'disabled merge ref sync feature flag' do + before do + stub_feature_flags(merge_ref_auto_sync: false) + end + + it 'returns error and no payload' do + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result.error?).to be(true) + expect(result.message).to eq('Merge ref is outdated due to disabled feature') + expect(result.payload).to be_empty + end + + it 'ignores merge-ref and updates merge status' do + expect { subject }.to change(merge_request, :merge_status).from('unchecked').to('can_be_merged') + end + end + + context 'when broken' do + before do + expect(merge_request).to receive(:broken?) { true } + end + + it_behaves_like 'unmergeable merge request' + + it 'returns ServiceResponse.error' do + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result.error?).to be(true) + expect(result.message).to eq('Merge request is not mergeable') + end + end + + context 'when it cannot be merged on git' do + let(:merge_request) do + create(:merge_request, + merge_status: :unchecked, + source_branch: 'conflict-resolvable', + source_project: project, + target_branch: 'conflict-start') + end + + it_behaves_like 'unmergeable merge request' + + it 'returns ServiceResponse.error' do + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result.error?).to be(true) + expect(result.message).to eq('Merge request is not mergeable') + end + end + + context 'when MR cannot be merged and has no merge ref' do + before do + merge_request.mark_as_unmergeable! + end + + it_behaves_like 'unmergeable merge request' + + it 'returns ServiceResponse.error' do + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result.error?).to be(true) + expect(result.message).to eq('Merge request is not mergeable') + end + end + + context 'when MR cannot be merged and has outdated merge ref' do + before do + MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request) + merge_request.mark_as_unmergeable! + end + + it_behaves_like 'unmergeable merge request' + + it 'returns ServiceResponse.error' do + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result.error?).to be(true) + expect(result.message).to eq('Merge request is not mergeable') + end + end + + context 'when merge request is not given' do + subject { described_class.new(nil).execute } + + it 'returns ServiceResponse.error' do + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result.message).to eq('Invalid argument') + end + end + + context 'when read only DB' do + it 'returns ServiceResponse.error' do + allow(Gitlab::Database).to receive(:read_only?) { true } + + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result.message).to eq('Unsupported operation') + end + end + + context 'when fails to update the merge-ref' do + before do + expect_next_instance_of(MergeRequests::MergeToRefService) do |merge_to_ref| + expect(merge_to_ref).to receive(:execute).and_return(status: :failed) + end + end + + it_behaves_like 'unmergeable merge request' + + it 'returns ServiceResponse.error' do + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result.error?).to be(true) + expect(result.message).to eq('Merge request is not mergeable') + end + end + + context 'recheck enforced' do + subject { described_class.new(merge_request).execute(recheck: true) } + + context 'when MR is mergeable and merge-ref auto-sync is disabled' do + before do + stub_feature_flags(merge_ref_auto_sync: false) + merge_request.mark_as_mergeable! + end + + it 'returns ServiceResponse.error' do + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result.error?).to be(true) + expect(result.message).to eq('Merge ref is outdated due to disabled feature') + expect(result.payload).to be_empty + end + + it 'merge status is not changed' do + subject + + expect(merge_request.merge_status).to eq('can_be_merged') + end + end + + context 'when MR is marked as mergeable, but repo is not mergeable and MR is not opened' do + before do + # Making sure that we don't touch the merge-status after + # the MR is not opened any longer. Source branch might + # have been removed, etc. + allow(merge_request).to receive(:broken?) { true } + merge_request.mark_as_mergeable! + merge_request.close! + end + + it 'returns ServiceResponse.error' do + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result.error?).to be(true) + expect(result.message).to eq('Merge ref cannot be updated') + expect(result.payload).to be_empty + end + + it 'does not change the merge status' do + expect { subject }.not_to change(merge_request, :merge_status).from('can_be_merged') + end + end + + context 'when MR is mergeable but merge-ref does not exists' do + before do + merge_request.mark_as_mergeable! + end + + it_behaves_like 'mergeable merge request' + end + + context 'when MR is mergeable but merge-ref is already updated' do + before do + MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request) + merge_request.mark_as_mergeable! + end + + it 'returns ServiceResponse.success' do + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result).to be_success + expect(result.payload[:merge_ref_head]).to be_present + end + + it 'does not recreate the merge-ref' do + expect(MergeRequests::MergeToRefService).not_to receive(:new) + + subject + 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 54b9c6dae38..a27fea0c90f 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -11,6 +11,8 @@ describe MergeRequests::PushOptionsHandlerService do let(:service) { described_class.new(project, user, changes, push_options) } let(:source_branch) { 'fix' } let(:target_branch) { 'feature' } + let(:title) { 'my title' } + let(:description) { 'my description' } let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } let(:deleted_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 #{Gitlab::Git::BLANK_SHA} refs/heads/#{source_branch}" } @@ -73,6 +75,26 @@ describe MergeRequests::PushOptionsHandlerService do end end + shared_examples_for 'a service that can set the title of a merge request' do + subject(:last_mr) { MergeRequest.last } + + it 'sets the title' do + service.execute + + expect(last_mr.title).to eq(title) + end + end + + shared_examples_for 'a service that can set the description of a merge request' do + subject(:last_mr) { MergeRequest.last } + + it 'sets the description' do + service.execute + + expect(last_mr.description).to eq(description) + end + end + shared_examples_for 'a service that can set the merge request to merge when pipeline succeeds' do subject(:last_mr) { MergeRequest.last } @@ -90,6 +112,16 @@ describe MergeRequests::PushOptionsHandlerService do end end + shared_examples_for 'a service that can remove the source branch when it is merged' do + subject(:last_mr) { MergeRequest.last } + + it 'returns true to force_remove_source_branch?' do + service.execute + + expect(last_mr.force_remove_source_branch?).to eq(true) + end + end + shared_examples_for 'a service that does not create a merge request' do it do expect { service.execute }.not_to change { MergeRequest.count } @@ -208,6 +240,72 @@ describe MergeRequests::PushOptionsHandlerService do end end + describe '`remove_source_branch` push option' do + let(:push_options) { { remove_source_branch: true } } + + context 'with a new branch' do + let(:changes) { new_branch_changes } + + 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) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, remove_source_branch: true } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can remove the source branch when it is merged' + end + end + + context 'with an existing branch but no open MR' do + let(:changes) { existing_branch_changes } + + 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) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, remove_source_branch: true } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can remove the source branch when it is merged' + end + end + + context 'with an existing branch that has a merge request open' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can remove the source branch when it is merged' + end + + context 'with a deleted branch' do + let(:changes) { deleted_branch_changes } + + it_behaves_like 'a service that does nothing' + end + + context 'with the project default branch' do + let(:changes) { default_branch_changes } + + it_behaves_like 'a service that does nothing' + end + end + describe '`target` push option' do let(:push_options) { { target: target_branch } } @@ -274,6 +372,138 @@ describe MergeRequests::PushOptionsHandlerService do end end + describe '`title` push option' do + let(:push_options) { { title: title } } + + context 'with a new branch' do + let(:changes) { new_branch_changes } + + 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) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, title: title } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the title of a merge request' + end + end + + context 'with an existing branch but no open MR' do + let(:changes) { existing_branch_changes } + + 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) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, title: title } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the title of a merge request' + end + end + + context 'with an existing branch that has a merge request open' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can set the title of a merge request' + end + + context 'with a deleted branch' do + let(:changes) { deleted_branch_changes } + + it_behaves_like 'a service that does nothing' + end + + context 'with the project default branch' do + let(:changes) { default_branch_changes } + + it_behaves_like 'a service that does nothing' + end + end + + describe '`description` push option' do + let(:push_options) { { description: description } } + + context 'with a new branch' do + let(:changes) { new_branch_changes } + + 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) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, description: description } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the description of a merge request' + end + end + + context 'with an existing branch but no open MR' do + let(:changes) { existing_branch_changes } + + 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) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, description: description } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the description of a merge request' + end + end + + context 'with an existing branch that has a merge request open' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can set the description of a merge request' + end + + context 'with a deleted branch' do + let(:changes) { deleted_branch_changes } + + it_behaves_like 'a service that does nothing' + end + + context 'with the project default branch' do + let(:changes) { default_branch_changes } + + it_behaves_like 'a service that does nothing' + end + end + describe 'multiple pushed branches' do let(:push_options) { { create: true } } let(:changes) do diff --git a/spec/services/merge_requests/rebase_service_spec.rb b/spec/services/merge_requests/rebase_service_spec.rb index 7e2f03d1097..7b8c94c86fe 100644 --- a/spec/services/merge_requests/rebase_service_spec.rb +++ b/spec/services/merge_requests/rebase_service_spec.rb @@ -6,10 +6,12 @@ describe MergeRequests::RebaseService do include ProjectForksHelper let(:user) { create(:user) } + let(:rebase_jid) { 'fake-rebase-jid' } let(:merge_request) do - create(:merge_request, + create :merge_request, source_branch: 'feature_conflict', - target_branch: 'master') + target_branch: 'master', + rebase_jid: rebase_jid end let(:project) { merge_request.project } let(:repository) { project.repository.raw } @@ -23,11 +25,11 @@ describe MergeRequests::RebaseService do describe '#execute' do context 'when another rebase is already in progress' do before do - allow(merge_request).to receive(:rebase_in_progress?).and_return(true) + allow(repository).to receive(:rebase_in_progress?).with(merge_request.id).and_return(true) end it 'saves the error message' do - subject.execute(merge_request) + service.execute(merge_request) expect(merge_request.reload.merge_error).to eq 'Rebase task canceled: Another rebase is already in progress' end @@ -36,6 +38,13 @@ describe MergeRequests::RebaseService do expect(service.execute(merge_request)).to match(status: :error, message: described_class::REBASE_ERROR) end + + it 'clears rebase_jid' do + expect { service.execute(merge_request) } + .to change { merge_request.rebase_jid } + .from(rebase_jid) + .to(nil) + end end shared_examples 'sequence of failure and success' do @@ -43,14 +52,19 @@ describe MergeRequests::RebaseService do allow(repository).to receive(:gitaly_operation_client).and_raise('Something went wrong') service.execute(merge_request) + merge_request.reload - expect(merge_request.reload.merge_error).to eq described_class::REBASE_ERROR + expect(merge_request.reload.merge_error).to eq(described_class::REBASE_ERROR) + expect(merge_request.rebase_jid).to eq(nil) allow(repository).to receive(:gitaly_operation_client).and_call_original + merge_request.update!(rebase_jid: rebase_jid) service.execute(merge_request) + merge_request.reload - expect(merge_request.reload.merge_error).to eq nil + expect(merge_request.merge_error).to eq(nil) + expect(merge_request.rebase_jid).to eq(nil) end end @@ -72,7 +86,7 @@ describe MergeRequests::RebaseService do it 'saves a generic error message' do subject.execute(merge_request) - expect(merge_request.reload.merge_error).to eq described_class::REBASE_ERROR + expect(merge_request.reload.merge_error).to eq(described_class::REBASE_ERROR) end it 'returns an error' do diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index f566d235787..9688e02d6ac 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -91,7 +91,8 @@ describe MergeRequests::UpdateService, :mailer do labels: [], mentioned_users: [user2], assignees: [user3], - total_time_spent: 0 + total_time_spent: 0, + description: "FYI #{user2.to_reference}" } ) end @@ -99,7 +100,7 @@ describe MergeRequests::UpdateService, :mailer do it 'sends email to user2 about assign of new merge request and email to user3 about merge request unassignment' do deliveries = ActionMailer::Base.deliveries email = deliveries.last - recipients = deliveries.last(2).map(&:to).flatten + recipients = deliveries.last(2).flat_map(&:to) expect(recipients).to include(user2.email, user3.email) expect(email.subject).to include(merge_request.title) end @@ -598,7 +599,7 @@ describe MergeRequests::UpdateService, :mailer do feature_visibility_attr = :"#{merge_request.model_name.plural}_access_level" project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE) - expect { update_merge_request(assignee_ids: [assignee]) }.not_to change { merge_request.reload.assignees } + expect { update_merge_request(assignee_ids: [assignee]) }.not_to change(merge_request.assignees, :count) end end end diff --git a/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb b/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb new file mode 100644 index 00000000000..53b7497ae21 --- /dev/null +++ b/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb @@ -0,0 +1,145 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Metrics::Dashboard::CustomMetricEmbedService do + include MetricsDashboardHelpers + + set(:project) { build(:project) } + set(:user) { create(:user) } + set(:environment) { create(:environment, project: project) } + + before do + project.add_maintainer(user) + end + + let(:dashboard_path) { system_dashboard_path } + let(:group) { business_metric_title } + let(:title) { 'title' } + let(:y_label) { 'y_label' } + + describe '.valid_params?' do + let(:valid_params) do + { + embedded: true, + dashboard_path: dashboard_path, + group: group, + title: title, + y_label: y_label + } + end + + subject { described_class.valid_params?(params) } + + let(:params) { valid_params } + + it { is_expected.to be_truthy } + + context 'not embedded' do + let(:params) { valid_params.except(:embedded) } + + it { is_expected.to be_falsey } + end + + context 'non-system dashboard' do + let(:dashboard_path) { '.gitlab/dashboards/test.yml' } + + it { is_expected.to be_falsey } + end + + context 'undefined dashboard' do + let(:params) { valid_params.except(:dashboard_path) } + + it { is_expected.to be_truthy } + end + + context 'non-custom metric group' do + let(:group) { 'Different Group' } + + it { is_expected.to be_falsey } + end + + context 'missing group' do + let(:group) { nil } + + it { is_expected.to be_falsey } + end + + context 'missing title' do + let(:title) { nil } + + it { is_expected.to be_falsey } + end + + context 'undefined y-axis label' do + let(:params) { valid_params.except(:y_label) } + + it { is_expected.to be_falsey } + end + end + + describe '#get_dashboard' do + let(:service_params) do + [ + project, + user, + { + embedded: true, + environment: environment, + dashboard_path: dashboard_path, + group: group, + title: title, + y_label: y_label + } + ] + end + + let(:service_call) { described_class.new(*service_params).get_dashboard } + + it_behaves_like 'misconfigured dashboard service response', :not_found + it_behaves_like 'raises error for users with insufficient permissions' + + context 'the custom metric exists' do + let!(:metric) { create(:prometheus_metric, project: project) } + + it_behaves_like 'valid embedded dashboard service response' + + it 'does not cache the unprocessed dashboard' do + expect(Gitlab::Metrics::Dashboard::Cache).not_to receive(:fetch) + + described_class.new(*service_params).get_dashboard + end + + context 'multiple metrics meet criteria' do + let!(:metric_2) { create(:prometheus_metric, project: project, query: 'avg(metric_2)') } + + it_behaves_like 'valid embedded dashboard service response' + + it 'includes both metrics' do + result = service_call + included_queries = all_queries(result[:dashboard]) + + expect(included_queries).to include('avg(metric_2)', 'avg(metric)') + end + end + end + + context 'when the metric exists in another project' do + let!(:metric) { create(:prometheus_metric, project: create(:project)) } + + it_behaves_like 'misconfigured dashboard service response', :not_found + end + end + + private + + def all_queries(dashboard) + dashboard[:panel_groups].flat_map do |group| + group[:panels].flat_map do |panel| + panel[:metrics].map do |metric| + metric[:query_range] + end + end + end + end +end diff --git a/spec/services/metrics/dashboard/default_embed_service_spec.rb b/spec/services/metrics/dashboard/default_embed_service_spec.rb new file mode 100644 index 00000000000..803b9a93be7 --- /dev/null +++ b/spec/services/metrics/dashboard/default_embed_service_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Metrics::Dashboard::DefaultEmbedService, :use_clean_rails_memory_store_caching do + include MetricsDashboardHelpers + + set(:project) { build(:project) } + set(:user) { create(:user) } + set(:environment) { create(:environment, project: project) } + + before do + project.add_maintainer(user) + end + + describe '#get_dashboard' do + let(:service_params) { [project, user, { environment: environment }] } + let(:service_call) { described_class.new(*service_params).get_dashboard } + + it_behaves_like 'valid embedded dashboard service response' + it_behaves_like 'raises error for users with insufficient permissions' + + it 'caches the unprocessed dashboard for subsequent calls' do + system_service = Metrics::Dashboard::SystemDashboardService + + expect(system_service).to receive(:new).once.and_call_original + + described_class.new(*service_params).get_dashboard + described_class.new(*service_params).get_dashboard + end + + context 'when called with a non-system dashboard' do + let(:dashboard_path) { 'garbage/dashboard/path' } + + it_behaves_like 'valid embedded dashboard service response' + end + end +end diff --git a/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb b/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb new file mode 100644 index 00000000000..a0f7315f750 --- /dev/null +++ b/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb @@ -0,0 +1,151 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Metrics::Dashboard::DynamicEmbedService, :use_clean_rails_memory_store_caching do + include MetricsDashboardHelpers + + set(:project) { build(:project) } + set(:user) { create(:user) } + set(:environment) { create(:environment, project: project) } + + before do + project.add_maintainer(user) + end + + let(:dashboard_path) { '.gitlab/dashboards/test.yml' } + let(:group) { 'Group A' } + let(:title) { 'Super Chart A1' } + let(:y_label) { 'y_label' } + + describe '.valid_params?' do + let(:valid_params) do + { + embedded: true, + dashboard_path: dashboard_path, + group: group, + title: title, + y_label: y_label + } + end + + subject { described_class.valid_params?(params) } + + let(:params) { valid_params } + + it { is_expected.to be_truthy } + + context 'not embedded' do + let(:params) { valid_params.except(:embedded) } + + it { is_expected.to be_falsey } + end + + context 'undefined dashboard' do + let(:params) { valid_params.except(:dashboard_path) } + + it { is_expected.to be_truthy } + end + + context 'missing dashboard' do + let(:dashboard) { '' } + + it { is_expected.to be_truthy } + end + + context 'missing group' do + let(:group) { '' } + + it { is_expected.to be_falsey } + end + + context 'missing title' do + let(:title) { '' } + + it { is_expected.to be_falsey } + end + + context 'undefined y-axis label' do + let(:params) { valid_params.except(:y_label) } + + it { is_expected.to be_falsey } + end + end + + describe '#get_dashboard' do + let(:service_params) do + [ + project, + user, + { + environment: environment, + dashboard_path: dashboard_path, + group: group, + title: title, + y_label: y_label + } + ] + end + + let(:service_call) { described_class.new(*service_params).get_dashboard } + + context 'when the dashboard does not exist' do + it_behaves_like 'misconfigured dashboard service response', :not_found + end + + context 'when the dashboard is exists' do + let(:project) { project_with_dashboard(dashboard_path) } + + it_behaves_like 'valid embedded dashboard service response' + it_behaves_like 'raises error for users with insufficient permissions' + + it 'caches the unprocessed dashboard for subsequent calls' do + expect(YAML).to receive(:safe_load).once.and_call_original + + described_class.new(*service_params).get_dashboard + described_class.new(*service_params).get_dashboard + end + + context 'when the specified group is not present on the dashboard' do + let(:group) { 'Group Not Found' } + + it_behaves_like 'misconfigured dashboard service response', :not_found + end + + context 'when the specified title is not present on the dashboard' do + let(:title) { 'Title Not Found' } + + it_behaves_like 'misconfigured dashboard service response', :not_found + end + + context 'when the specified y-axis label is not present on the dashboard' do + let(:y_label) { 'Y-Axis Not Found' } + + it_behaves_like 'misconfigured dashboard service response', :not_found + end + end + + shared_examples 'uses system dashboard' do + it 'uses the default dashboard' do + expect(Gitlab::Metrics::Dashboard::Finder) + .to receive(:find_raw) + .with(project, dashboard_path: system_dashboard_path) + .once + + service_call + end + end + + context 'when the dashboard is nil' do + let(:dashboard_path) { nil } + + it_behaves_like 'uses system dashboard' + end + + context 'when the dashboard is not present' do + let(:dashboard_path) { '' } + + it_behaves_like 'uses system dashboard' + end + end +end diff --git a/spec/services/metrics/dashboard/project_dashboard_service_spec.rb b/spec/services/metrics/dashboard/project_dashboard_service_spec.rb new file mode 100644 index 00000000000..1357914be2a --- /dev/null +++ b/spec/services/metrics/dashboard/project_dashboard_service_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Metrics::Dashboard::ProjectDashboardService, :use_clean_rails_memory_store_caching do + include MetricsDashboardHelpers + + set(:user) { create(:user) } + set(:project) { create(:project) } + set(:environment) { create(:environment, project: project) } + + before do + project.add_maintainer(user) + end + + describe 'get_dashboard' do + let(:dashboard_path) { '.gitlab/dashboards/test.yml' } + let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] } + let(:service_call) { described_class.new(*service_params).get_dashboard } + + context 'when the dashboard does not exist' do + it_behaves_like 'misconfigured dashboard service response', :not_found + end + + it_behaves_like 'raises error for users with insufficient permissions' + + context 'when the dashboard exists' do + let(:project) { project_with_dashboard(dashboard_path) } + + it_behaves_like 'valid dashboard service response' + + it 'caches the unprocessed dashboard for subsequent calls' do + expect_any_instance_of(described_class) + .to receive(:get_raw_dashboard) + .once + .and_call_original + + described_class.new(*service_params).get_dashboard + described_class.new(*service_params).get_dashboard + end + + context 'and the dashboard is then deleted' do + it 'does not return the previously cached dashboard' do + described_class.new(*service_params).get_dashboard + + delete_project_dashboard(project, user, dashboard_path) + + expect_any_instance_of(described_class) + .to receive(:get_raw_dashboard) + .once + .and_call_original + + described_class.new(*service_params).get_dashboard + end + end + end + + context 'when the dashboard is configured incorrectly' do + let(:project) { project_with_dashboard(dashboard_path, {}) } + + it_behaves_like 'misconfigured dashboard service response', :unprocessable_entity + end + end + + describe '::all_dashboard_paths' do + let(:all_dashboards) { described_class.all_dashboard_paths(project) } + + context 'when there are no project dashboards' do + it 'returns an empty array' do + expect(all_dashboards).to be_empty + end + end + + context 'when there are project dashboards available' do + let(:dashboard_path) { '.gitlab/dashboards/test.yml' } + let(:project) { project_with_dashboard(dashboard_path) } + + it 'returns the dashboard attributes' do + expect(all_dashboards).to eq( + [{ + path: dashboard_path, + display_name: 'test.yml', + default: false + }] + ) + end + end + end +end diff --git a/spec/services/metrics/dashboard/system_dashboard_service_spec.rb b/spec/services/metrics/dashboard/system_dashboard_service_spec.rb new file mode 100644 index 00000000000..8be3e7f6064 --- /dev/null +++ b/spec/services/metrics/dashboard/system_dashboard_service_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Metrics::Dashboard::SystemDashboardService, :use_clean_rails_memory_store_caching do + include MetricsDashboardHelpers + + set(:user) { create(:user) } + set(:project) { create(:project) } + set(:environment) { create(:environment, project: project) } + + before do + project.add_maintainer(user) + end + + describe 'get_dashboard' do + let(:dashboard_path) { described_class::SYSTEM_DASHBOARD_PATH } + let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] } + let(:service_call) { described_class.new(*service_params).get_dashboard } + + it_behaves_like 'valid dashboard service response' + it_behaves_like 'raises error for users with insufficient permissions' + + it 'caches the unprocessed dashboard for subsequent calls' do + expect(YAML).to receive(:safe_load).once.and_call_original + + described_class.new(*service_params).get_dashboard + described_class.new(*service_params).get_dashboard + end + + context 'when called with a non-system dashboard' do + let(:dashboard_path) { 'garbage/dashboard/path' } + + # We want to alwaus return the system dashboard. + it_behaves_like 'valid dashboard service response' + end + end + + describe '::all_dashboard_paths' do + it 'returns the dashboard attributes' do + all_dashboards = described_class.all_dashboard_paths(project) + + expect(all_dashboards).to eq( + [{ + path: described_class::SYSTEM_DASHBOARD_PATH, + display_name: described_class::SYSTEM_DASHBOARD_NAME, + default: true + }] + ) + end + end +end diff --git a/spec/services/namespaces/statistics_refresher_service_spec.rb b/spec/services/namespaces/statistics_refresher_service_spec.rb new file mode 100644 index 00000000000..f4d9c96f7f4 --- /dev/null +++ b/spec/services/namespaces/statistics_refresher_service_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Namespaces::StatisticsRefresherService, '#execute' do + let(:group) { create(:group) } + let(:projects) { create_list(:project, 5, namespace: group) } + let(:service) { described_class.new } + + context 'without a root storage statistics relation' do + it 'creates one' do + expect do + service.execute(group) + end.to change(Namespace::RootStorageStatistics, :count).by(1) + + expect(group.reload.root_storage_statistics).to be_present + end + + it 'recalculate the namespace statistics' do + expect_any_instance_of(Namespace::RootStorageStatistics).to receive(:recalculate!).once + + service.execute(group) + end + end + + context 'with a root storage statistics relation' do + before do + Namespace::AggregationSchedule.safe_find_or_create_by!(namespace_id: group.id) + end + + it 'does not create one' do + expect do + service.execute(group) + end.not_to change(Namespace::RootStorageStatistics, :count) + end + + it 'recalculate the namespace statistics' do + expect(Namespace::RootStorageStatistics) + .to receive(:safe_find_or_create_by!).with({ namespace_id: group.id }) + .and_return(group.root_storage_statistics) + + service.execute(group) + end + end + + context 'when something goes wrong' do + before do + allow_any_instance_of(Namespace::RootStorageStatistics) + .to receive(:recalculate!).and_raise(ActiveRecord::ActiveRecordError) + end + + it 'raises RefreshError' do + expect do + service.execute(group) + end.to raise_error(Namespaces::StatisticsRefresherService::RefresherError) + end + end +end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 494ca95f66d..cd4ea9c401d 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -3,9 +3,9 @@ require 'spec_helper' describe Notes::CreateService do - let(:project) { create(:project) } - let(:issue) { create(:issue, project: project) } - let(:user) { create(:user) } + set(:project) { create(:project, :repository) } + set(:issue) { create(:issue, project: project) } + set(:user) { create(:user) } let(:opts) do { note: 'Awesome comment', noteable_type: 'Issue', noteable_id: issue.id } end @@ -197,64 +197,113 @@ describe Notes::CreateService do end context 'note with commands' do - context 'as a user who can update the target' do - context '/close, /label, /assign & /milestone' do - let(:note_text) { %(HELLO\n/close\n/assign @#{user.username}\nWORLD) } - - it 'saves the note and does not alter the note text' do - service = double(:service) - allow(Issues::UpdateService).to receive(:new).and_return(service) - expect(service).to receive(:execute) - - note = described_class.new(project, user, opts.merge(note: note_text)).execute - - expect(note.note).to eq "HELLO\nWORLD" + context 'all quick actions' do + set(:milestone) { create(:milestone, project: project, title: "sprint") } + set(:bug_label) { create(:label, project: project, title: 'bug') } + set(:to_be_copied_label) { create(:label, project: project, title: 'to be copied') } + set(:feature_label) { create(:label, project: project, title: 'feature') } + set(:issue) { create(:issue, project: project, labels: [bug_label], due_date: '2019-01-01') } + set(:issue_2) { create(:issue, project: project, labels: [bug_label, to_be_copied_label]) } + + context 'for issues' do + let(:issuable) { issue } + let(:note_params) { opts } + let(:issue_quick_actions) do + [ + QuickAction.new( + action_text: '/confidential', + expectation: ->(noteable, can_use_quick_action) { + if can_use_quick_action + expect(noteable).to be_confidential + else + expect(noteable).not_to be_confidential + end + } + ), + QuickAction.new( + action_text: '/due 2016-08-28', + expectation: ->(noteable, can_use_quick_action) { + expect(noteable.due_date == Date.new(2016, 8, 28)).to eq(can_use_quick_action) + } + ), + QuickAction.new( + action_text: '/remove_due_date', + expectation: ->(noteable, can_use_quick_action) { + if can_use_quick_action + expect(noteable.due_date).to be_nil + else + expect(noteable.due_date).not_to be_nil + end + } + ), + QuickAction.new( + action_text: "/duplicate #{issue_2.to_reference}", + before_action: -> { + issuable.reopen + }, + expectation: ->(noteable, can_use_quick_action) { + expect(noteable.closed?).to eq(can_use_quick_action) + } + ) + ] end - end - context '/merge with sha option' do - let(:note_text) { %(HELLO\n/merge\nWORLD) } - let(:params) { opts.merge(note: note_text, merge_request_diff_head_sha: 'sha') } - - it 'saves the note and exectues merge command' do - note = described_class.new(project, user, params).execute - - expect(note.note).to eq "HELLO\nWORLD" + it_behaves_like 'issuable quick actions' do + let(:quick_actions) { issuable_quick_actions + issue_quick_actions } end end - context 'when note only have commands' do - it 'adds commands applied message to note errors' do - note_text = %(/close) - service = double(:service) - allow(Issues::UpdateService).to receive(:new).and_return(service) - expect(service).to receive(:execute) - - note = described_class.new(project, user, opts.merge(note: note_text)).execute + context 'for merge requests' do + set(:merge_request) { create(:merge_request, source_project: project, labels: [bug_label]) } + let(:issuable) { merge_request } + let(:note_params) { opts.merge(noteable_type: 'MergeRequest', noteable_id: merge_request.id) } + let(:merge_request_quick_actions) do + [ + QuickAction.new( + action_text: "/target_branch fix", + expectation: ->(noteable, can_use_quick_action) { + expect(noteable.target_branch == "fix").to eq(can_use_quick_action) + } + ), + # Set WIP status + QuickAction.new( + action_text: "/wip", + before_action: -> { + issuable.reload.update(title: "title") + }, + expectation: ->(issuable, can_use_quick_action) { + expect(issuable.work_in_progress?).to eq(can_use_quick_action) + } + ), + # Remove WIP status + QuickAction.new( + action_text: "/wip", + before_action: -> { + issuable.reload.update(title: "WIP: title") + }, + expectation: ->(noteable, can_use_quick_action) { + expect(noteable.work_in_progress?).not_to eq(can_use_quick_action) + } + ) + ] + end - expect(note.errors[:commands_only]).to be_present + it_behaves_like 'issuable quick actions' do + let(:quick_actions) { issuable_quick_actions + merge_request_quick_actions } end end end - context 'as a user who cannot update the target' do - let(:note_text) { "HELLO\n/todo\n/assign #{user.to_reference}\nWORLD" } - let(:note) { described_class.new(project, user, opts.merge(note: note_text)).execute } - - before do - project.team.find_member(user.id).update!(access_level: Gitlab::Access::GUEST) - end + context 'when note only have commands' do + it 'adds commands applied message to note errors' do + note_text = %(/close) + service = double(:service) + allow(Issues::UpdateService).to receive(:new).and_return(service) + expect(service).to receive(:execute) - it 'applies commands the user can execute' do - expect { note }.to change { user.todos_pending_count }.from(0).to(1) - end + note = described_class.new(project, user, opts.merge(note: note_text)).execute - it 'does not apply commands the user cannot execute' do - expect { note }.not_to change { issue.assignees } - end - - it 'saves the note' do - expect(note.note).to eq "HELLO\nWORLD" + expect(note.errors[:commands_only]).to be_present end end end @@ -316,5 +365,43 @@ describe Notes::CreateService do .and change { existing_note.updated_at } end end + + describe "usage counter" do + let(:counter) { Gitlab::UsageDataCounters::NoteCounter } + + context 'snippet note' do + let(:snippet) { create(:project_snippet, project: project) } + let(:opts) { { note: 'reply', noteable_type: 'Snippet', noteable_id: snippet.id, project: project } } + + it 'increments usage counter' do + expect do + note = described_class.new(project, user, opts).execute + + expect(note).to be_valid + end.to change { counter.read(:create, opts[:noteable_type]) }.by 1 + end + + it 'does not increment usage counter when creation fails' do + expect do + note = described_class.new(project, user, { note: '' }).execute + + expect(note).to be_invalid + end.not_to change { counter.read(:create, opts[:noteable_type]) } + end + end + + context 'issue note' do + let(:issue) { create(:issue, project: project) } + let(:opts) { { note: 'reply', noteable_type: 'Issue', noteable_id: issue.id, project: project } } + + it 'does not increment usage counter' do + expect do + note = described_class.new(project, user, opts).execute + + expect(note).to be_valid + end.not_to change { counter.read(:create, opts[:noteable_type]) } + end + end + end end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 4b40c86574f..d925aa2b6c3 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -111,7 +111,7 @@ describe NotificationService, :mailer do should_email(participant) end - context 'for subgroups', :nested_groups do + context 'for subgroups' do before do build_group(project) end @@ -215,13 +215,14 @@ describe NotificationService, :mailer do let(:project) { create(:project, :private) } let(:issue) { create(:issue, project: project, assignees: [assignee]) } let(:mentioned_issue) { create(:issue, assignees: issue.assignees) } - let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@mention referenced, @unsubscribed_mentioned and @outsider also') } + let(:author) { create(:user) } + let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@mention referenced, @unsubscribed_mentioned and @outsider also') } before do - build_team(note.project) + build_team(project) project.add_maintainer(issue.author) project.add_maintainer(assignee) - project.add_maintainer(note.author) + project.add_maintainer(author) @u_custom_off = create_user_with_notification(:custom, 'custom_off') project.add_guest(@u_custom_off) @@ -239,43 +240,50 @@ describe NotificationService, :mailer do end describe '#new_note' do - it do - add_users_with_subscription(note.project, issue) - reset_delivered_emails! + context do + before do + add_users(project) + add_user_subscriptions(issue) + reset_delivered_emails! + end - expect(SentNotification).to receive(:record).with(issue, any_args).exactly(10).times + it do + expect(SentNotification).to receive(:record).with(issue, any_args).exactly(10).times - notification.new_note(note) + notification.new_note(note) - should_email(@u_watcher) - should_email(note.noteable.author) - should_email(note.noteable.assignees.first) - should_email(@u_custom_global) - should_email(@u_mentioned) - should_email(@subscriber) - should_email(@watcher_and_subscriber) - should_email(@subscribed_participant) - should_email(@u_custom_off) - should_email(@unsubscribed_mentioned) - should_not_email(@u_guest_custom) - should_not_email(@u_guest_watcher) - should_not_email(note.author) - should_not_email(@u_participating) - should_not_email(@u_disabled) - should_not_email(@unsubscriber) - should_not_email(@u_outsider_mentioned) - should_not_email(@u_lazy_participant) - end + should_email(@u_watcher) + should_email(note.noteable.author) + should_email(note.noteable.assignees.first) + should_email(@u_custom_global) + should_email(@u_mentioned) + should_email(@subscriber) + should_email(@watcher_and_subscriber) + should_email(@subscribed_participant) + should_email(@u_custom_off) + should_email(@unsubscribed_mentioned) + should_not_email(@u_guest_custom) + should_not_email(@u_guest_watcher) + should_not_email(note.author) + should_not_email(@u_participating) + should_not_email(@u_disabled) + should_not_email(@unsubscriber) + should_not_email(@u_outsider_mentioned) + should_not_email(@u_lazy_participant) + end - it "emails the note author if they've opted into notifications about their activity" do - add_users_with_subscription(note.project, issue) - reset_delivered_emails! + it "emails the note author if they've opted into notifications about their activity" do + note.author.notified_of_own_activity = true - note.author.notified_of_own_activity = true + notification.new_note(note) - notification.new_note(note) + should_email(note.author) + end - should_email(note.author) + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end end it 'filters out "mentioned in" notes' do @@ -334,7 +342,12 @@ describe NotificationService, :mailer do it_behaves_like 'new note notifications' - context 'which is a subgroup', :nested_groups do + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end + + context 'which is a subgroup' do let!(:parent) { create(:group) } let!(:group) { create(:group, parent: parent) } @@ -385,7 +398,7 @@ describe NotificationService, :mailer do should_email(admin) end - context 'on project that belongs to subgroup', :nested_groups do + context 'on project that belongs to subgroup' do let(:group_reporter) { create(:user) } let(:group_guest) { create(:user) } let(:parent_group) { create(:group) } @@ -415,13 +428,15 @@ describe NotificationService, :mailer do let(:project) { create(:project, :public) } let(:issue) { create(:issue, project: project, assignees: [assignee]) } let(:mentioned_issue) { create(:issue, assignees: issue.assignees) } - let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@all mentioned') } + let(:author) { create(:user) } + let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@all mentioned') } before do - build_team(note.project) - build_group(note.project) - note.project.add_maintainer(note.author) - add_users_with_subscription(note.project, issue) + build_team(project) + build_group(project) + add_users(project) + add_user_subscriptions(issue) + project.add_maintainer(author) reset_delivered_emails! end @@ -453,7 +468,7 @@ describe NotificationService, :mailer do should_not_email_nested_group_user(@pg_disabled) end - it 'notifies parent group members with mention level', :nested_groups do + it 'notifies parent group members with mention level' do note = create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: "@#{@pg_mention.username}") notification.new_note(note) @@ -467,23 +482,29 @@ describe NotificationService, :mailer do expect(Notify).not_to receive(:note_issue_email) notification.new_note(mentioned_note) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end end end context 'project snippet note' do let!(:project) { create(:project, :public) } let(:snippet) { create(:project_snippet, project: project, author: create(:user)) } - let(:note) { create(:note_on_project_snippet, noteable: snippet, project_id: project.id, note: '@all mentioned') } + let(:author) { create(:user) } + let(:note) { create(:note_on_project_snippet, author: author, noteable: snippet, project_id: project.id, note: '@all mentioned') } before do build_team(project) build_group(project) + project.add_maintainer(author) # make sure these users can read the project snippet! project.add_guest(@u_guest_watcher) project.add_guest(@u_guest_custom) add_member_for_parent_group(@pg_watcher, project) - note.project.add_maintainer(note.author) reset_delivered_emails! end @@ -613,6 +634,11 @@ describe NotificationService, :mailer do notification.new_note(note) should_not_email(@u_committer) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end end end @@ -639,6 +665,11 @@ describe NotificationService, :mailer do .to contain_exactly(*merge_request.assignees.pluck(:id), merge_request.author.id, @u_watcher.id) expect(SentNotification.last.in_reply_to_discussion_id).to eq(note.discussion_id) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end end end end @@ -708,10 +739,11 @@ describe NotificationService, :mailer do let(:issue) { create :issue, project: project, assignees: [assignee], description: 'cc @participant @unsubscribed_mentioned' } before do - build_team(issue.project) - build_group(issue.project) + build_team(project) + build_group(project) - add_users_with_subscription(issue.project, issue) + add_users(project) + add_user_subscriptions(issue) reset_delivered_emails! update_custom_notification(:new_issue, @u_guest_custom, resource: project) update_custom_notification(:new_issue, @u_custom_global) @@ -812,6 +844,11 @@ describe NotificationService, :mailer do should_email(user_4) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.new_issue(issue, @u_disabled) } + end + context 'confidential issues' do let(:author) { create(:user) } let(:assignee) { create(:user) } @@ -854,6 +891,11 @@ describe NotificationService, :mailer do let(:mentionable) { issue } include_examples 'notifications for new mentions' + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { send_notifications(@u_watcher, @u_participant_mentioned, @u_custom_global, @u_mentioned) } + end end describe '#reassigned_issue' do @@ -962,6 +1004,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled, [assignee]) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled, [assignee]) } + end end describe '#relabeled_issue' do @@ -1021,6 +1068,11 @@ describe NotificationService, :mailer do should_email(subscriber_to_both) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.relabeled_issue(issue, [group_label_2, label_2], @u_disabled) } + end + context 'confidential issues' do let(:author) { create(:user) } let(:assignee) { create(:user) } @@ -1058,12 +1110,19 @@ describe NotificationService, :mailer do end describe '#removed_milestone_issue' do - it_behaves_like 'altered milestone notification on issue' do + context do let(:milestone) { create(:milestone, project: project, issues: [issue]) } let!(:subscriber_to_new_milestone) { create(:user) { |u| issue.toggle_subscription(u, project) } } - before do - notification.removed_milestone_issue(issue, issue.author) + it_behaves_like 'altered milestone notification on issue' do + before do + notification.removed_milestone_issue(issue, issue.author) + end + end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.removed_milestone_issue(issue, issue.author) } end end @@ -1103,12 +1162,19 @@ describe NotificationService, :mailer do end describe '#changed_milestone_issue' do - it_behaves_like 'altered milestone notification on issue' do + context do let(:new_milestone) { create(:milestone, project: project, issues: [issue]) } let!(:subscriber_to_new_milestone) { create(:user) { |u| issue.toggle_subscription(u, project) } } - before do - notification.changed_milestone_issue(issue, new_milestone, issue.author) + it_behaves_like 'altered milestone notification on issue' do + before do + notification.changed_milestone_issue(issue, new_milestone, issue.author) + end + end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.changed_milestone_issue(issue, new_milestone, issue.author) } end end @@ -1176,6 +1242,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.close_issue(issue, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.close_issue(issue, @u_disabled) } + end end describe '#reopen_issue' do @@ -1207,6 +1278,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.reopen_issue(issue, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.reopen_issue(issue, @u_disabled) } + end end describe '#issue_moved' do @@ -1233,6 +1309,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.issue_moved(issue, new_issue, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.issue_moved(issue, new_issue, @u_disabled) } + end end describe '#issue_due' do @@ -1273,6 +1354,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.issue_due(issue) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.issue_due(issue) } + end end end @@ -1281,13 +1367,16 @@ describe NotificationService, :mailer do let(:project) { create(:project, :public, :repository, namespace: group) } let(:another_project) { create(:project, :public, namespace: group) } let(:assignee) { create(:user) } - let(:merge_request) { create :merge_request, source_project: project, assignees: [assignee], description: 'cc @participant' } + let(:assignees) { Array.wrap(assignee) } + let(:author) { create(:user) } + let(:merge_request) { create :merge_request, author: author, source_project: project, assignees: assignees, description: 'cc @participant' } before do - project.add_maintainer(merge_request.author) - merge_request.assignees.each { |assignee| project.add_maintainer(assignee) } - build_team(merge_request.target_project) - add_users_with_subscription(merge_request.target_project, merge_request) + project.add_maintainer(author) + assignees.each { |assignee| project.add_maintainer(assignee) } + build_team(project) + add_users(project) + add_user_subscriptions(merge_request) update_custom_notification(:new_merge_request, @u_guest_custom, resource: project) update_custom_notification(:new_merge_request, @u_custom_global) reset_delivered_emails! @@ -1364,6 +1453,11 @@ describe NotificationService, :mailer do should_email(user_4) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.new_merge_request(merge_request, @u_disabled) } + end + context 'participating' do it_should_behave_like 'participating by assignee notification' do let(:participant) { create(:user, username: 'user-participant')} @@ -1396,6 +1490,11 @@ describe NotificationService, :mailer do let(:mentionable) { merge_request } include_examples 'notifications for new mentions' + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { send_notifications(@u_watcher, @u_participant_mentioned, @u_custom_global, @u_mentioned) } + end end describe '#reassigned_merge_request' do @@ -1439,6 +1538,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.reassigned_merge_request(merge_request, current_user, [assignee]) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.reassigned_merge_request(merge_request, current_user, [assignee]) } + end end describe '#push_to_merge_request' do @@ -1469,6 +1573,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.push_to_merge_request(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.push_to_merge_request(merge_request, @u_disabled) } + end end describe '#relabel_merge_request' do @@ -1502,28 +1611,43 @@ describe NotificationService, :mailer do should_not_email(@u_participating) should_not_email(@u_lazy_participant) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.relabeled_merge_request(merge_request, [group_label_2, label_2], @u_disabled) } + end end describe '#removed_milestone_merge_request' do - it_behaves_like 'altered milestone notification on merge request' do - let(:milestone) { create(:milestone, project: project, merge_requests: [merge_request]) } - let!(:subscriber_to_new_milestone) { create(:user) { |u| merge_request.toggle_subscription(u, project) } } + let(:milestone) { create(:milestone, project: project, merge_requests: [merge_request]) } + let!(:subscriber_to_new_milestone) { create(:user) { |u| merge_request.toggle_subscription(u, project) } } + it_behaves_like 'altered milestone notification on merge request' do before do notification.removed_milestone_merge_request(merge_request, merge_request.author) end end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.removed_milestone_merge_request(merge_request, merge_request.author) } + end end describe '#changed_milestone_merge_request' do - it_behaves_like 'altered milestone notification on merge request' do - let(:new_milestone) { create(:milestone, project: project, merge_requests: [merge_request]) } - let!(:subscriber_to_new_milestone) { create(:user) { |u| merge_request.toggle_subscription(u, project) } } + let(:new_milestone) { create(:milestone, project: project, merge_requests: [merge_request]) } + let!(:subscriber_to_new_milestone) { create(:user) { |u| merge_request.toggle_subscription(u, project) } } + it_behaves_like 'altered milestone notification on merge request' do before do notification.changed_milestone_merge_request(merge_request, new_milestone, merge_request.author) end end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.changed_milestone_merge_request(merge_request, new_milestone, merge_request.author) } + end end describe '#merge_request_unmergeable' do @@ -1534,6 +1658,11 @@ describe NotificationService, :mailer do expect(email_recipients.size).to eq(1) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.merge_request_unmergeable(merge_request) } + end + describe 'when merge_when_pipeline_succeeds is true' do before do merge_request.update( @@ -1580,6 +1709,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.close_mr(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.close_mr(merge_request, @u_disabled) } + end end describe '#merged_merge_request' do @@ -1632,6 +1766,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.merge_mr(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.merge_mr(merge_request, @u_disabled) } + end end describe '#reopen_merge_request' do @@ -1662,6 +1801,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.reopen_mr(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.reopen_mr(merge_request, @u_disabled) } + end end describe "#resolve_all_discussions" do @@ -1685,6 +1829,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.resolve_all_discussions(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.resolve_all_discussions(merge_request, @u_disabled) } + end end end @@ -1709,6 +1858,11 @@ describe NotificationService, :mailer do should_not_email(@u_disabled) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.project_was_moved(project, "gitlab/gitlab") } + end + context 'users not having access to the new location' do it 'does not send email' do old_user = create(:user) @@ -1752,6 +1906,11 @@ describe NotificationService, :mailer do should_only_email(@u_participating) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.project_exported(project, @u_participating) } + end end describe '#project_not_exported' do @@ -1760,6 +1919,11 @@ describe NotificationService, :mailer do should_only_email(@u_participating) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.project_not_exported(project, @u_participating, ['error']) } + end end end end @@ -1790,6 +1954,11 @@ describe NotificationService, :mailer do should_email(maintainer) should_not_email(developer) end + + it_behaves_like 'group emails are disabled' do + let(:notification_target) { group } + let(:notification_trigger) { group.request_access(added_user) } + end end describe '#decline_group_invite' do @@ -1829,12 +1998,17 @@ describe NotificationService, :mailer do should_not_email_anyone end end + + it_behaves_like 'group emails are disabled' do + let(:notification_target) { group } + let(:notification_trigger) { group.add_guest(added_user) } + end end end describe 'ProjectMember' do let(:project) { create(:project) } - set(:added_user) { create(:user) } + let(:added_user) { create(:user) } describe '#new_access_request' do context 'for a project in a user namespace' do @@ -1849,6 +2023,11 @@ describe NotificationService, :mailer do should_only_email(project.owner) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { project.request_access(added_user) } + end end context 'for a project in a group' do @@ -1868,7 +2047,7 @@ describe NotificationService, :mailer do end end - describe '#decline_group_invite' do + describe '#decline_project_invite' do let(:member) { create(:user) } before do @@ -1890,6 +2069,11 @@ describe NotificationService, :mailer do should_only_email(added_user) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { create_member! } + end + context 'when notifications are disabled' do before do create_global_setting_for(added_user, :disabled) @@ -2053,27 +2237,69 @@ describe NotificationService, :mailer do end context 'when the creator has custom notifications enabled' do - before do - pipeline = create_pipeline(u_custom_notification_enabled, :success) - notification.pipeline_finished(pipeline) - end + let(:pipeline) { create_pipeline(u_custom_notification_enabled, :success) } it 'emails only the creator' do + notification.pipeline_finished(pipeline) + should_only_email(u_custom_notification_enabled, kind: :bcc) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { pipeline } + let(:notification_trigger) { notification.pipeline_finished(pipeline) } + end + + context 'when the creator has group notification email set' do + let(:group_notification_email) { 'user+group@example.com' } + + before do + group = create(:group) + + project.update(group: group) + create(:notification_setting, user: u_custom_notification_enabled, source: group, notification_email: group_notification_email) + end + + it 'sends to group notification email' do + notification.pipeline_finished(pipeline) + + expect(email_recipients(kind: :bcc).first).to eq(group_notification_email) + end + end end end context 'with a failed pipeline' do context 'when the creator has no custom notification set' do - before do - pipeline = create_pipeline(u_member, :failed) - notification.pipeline_finished(pipeline) - end + let(:pipeline) { create_pipeline(u_member, :failed) } it 'emails only the creator' do + notification.pipeline_finished(pipeline) + should_only_email(u_member, kind: :bcc) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { pipeline } + let(:notification_trigger) { notification.pipeline_finished(pipeline) } + end + + context 'when the creator has group notification email set' do + let(:group_notification_email) { 'user+group@example.com' } + + before do + group = create(:group) + + project.update(group: group) + create(:notification_setting, user: u_member, source: group, notification_email: group_notification_email) + end + + it 'sends to group notification email' do + notification.pipeline_finished(pipeline) + + expect(email_recipients(kind: :bcc).first).to eq(group_notification_email) + end + end end context 'when the creator has watch set' do @@ -2173,6 +2399,11 @@ describe NotificationService, :mailer do should_only_email(u_maintainer1, u_maintainer2, u_owner) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { domain } + let(:notification_trigger) { notify! } + end + it 'emails nobody if the project is missing' do domain.project = nil @@ -2182,30 +2413,6 @@ describe NotificationService, :mailer do end end end - - describe '#pages_domain_verification_failed' do - it 'emails current watching maintainers' do - notification.pages_domain_verification_failed(domain) - - should_only_email(u_maintainer1, u_maintainer2, u_owner) - end - end - - describe '#pages_domain_enabled' do - it 'emails current watching maintainers' do - notification.pages_domain_enabled(domain) - - should_only_email(u_maintainer1, u_maintainer2, u_owner) - end - end - - describe '#pages_domain_disabled' do - it 'emails current watching maintainers' do - notification.pages_domain_disabled(domain) - - should_only_email(u_maintainer1, u_maintainer2, u_owner) - end - end end context 'Auto DevOps notifications' do @@ -2217,10 +2424,17 @@ describe NotificationService, :mailer do let(:pipeline) { create(:ci_pipeline, :failed, project: project, user: pipeline_user) } it 'emails project owner and user that triggered the pipeline' do + project.add_developer(pipeline_user) + notification.autodevops_disabled(pipeline, [owner.email, pipeline_user.email]) - should_email(owner) - should_email(pipeline_user) + should_email(owner, times: 1) # Once for the disable pipeline. + should_email(pipeline_user, times: 2) # Once for the new permission, once for the disable. + end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.autodevops_disabled(pipeline, [owner.email, pipeline_user.email]) } end end end @@ -2235,6 +2449,11 @@ describe NotificationService, :mailer do should_email(user) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.repository_cleanup_success(project, user) } + end end describe '#repository_cleanup_failure' do @@ -2243,6 +2462,11 @@ describe NotificationService, :mailer do should_email(user) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.repository_cleanup_failure(project, user, 'Some error') } + end end end @@ -2276,6 +2500,11 @@ describe NotificationService, :mailer do should_only_email(u_maintainer1, u_maintainer2, u_owner) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.remote_mirror_update_failed(remote_mirror) } + end end end @@ -2366,56 +2595,44 @@ describe NotificationService, :mailer do group end - # Creates a nested group only if supported - # to avoid errors on MySQL def create_nested_group(visibility) - if Group.supports_nested_objects? - parent_group = create(:group, visibility) - child_group = create(:group, visibility, parent: parent_group) + parent_group = create(:group, visibility) + child_group = create(:group, visibility, parent: parent_group) - # Parent group member: global=disabled, parent_group=watch, child_group=global - @pg_watcher ||= create_user_with_notification(:watch, 'parent_group_watcher', parent_group) - @pg_watcher.notification_settings_for(nil).disabled! + # Parent group member: global=disabled, parent_group=watch, child_group=global + @pg_watcher ||= create_user_with_notification(:watch, 'parent_group_watcher', parent_group) + @pg_watcher.notification_settings_for(nil).disabled! - # Parent group member: global=global, parent_group=disabled, child_group=global - @pg_disabled ||= create_user_with_notification(:disabled, 'parent_group_disabled', parent_group) - @pg_disabled.notification_settings_for(nil).global! + # Parent group member: global=global, parent_group=disabled, child_group=global + @pg_disabled ||= create_user_with_notification(:disabled, 'parent_group_disabled', parent_group) + @pg_disabled.notification_settings_for(nil).global! - # Parent group member: global=global, parent_group=mention, child_group=global - @pg_mention ||= create_user_with_notification(:mention, 'parent_group_mention', parent_group) - @pg_mention.notification_settings_for(nil).global! + # Parent group member: global=global, parent_group=mention, child_group=global + @pg_mention ||= create_user_with_notification(:mention, 'parent_group_mention', parent_group) + @pg_mention.notification_settings_for(nil).global! - # Parent group member: global=global, parent_group=participating, child_group=global - @pg_participant ||= create_user_with_notification(:participating, 'parent_group_participant', parent_group) - @pg_mention.notification_settings_for(nil).global! + # Parent group member: global=global, parent_group=participating, child_group=global + @pg_participant ||= create_user_with_notification(:participating, 'parent_group_participant', parent_group) + @pg_mention.notification_settings_for(nil).global! - child_group - else - create(:group, visibility) - end + child_group end def add_member_for_parent_group(user, project) - return unless Group.supports_nested_objects? - project.reload project.group.parent.add_maintainer(user) end def should_email_nested_group_user(user, times: 1, recipients: email_recipients) - return unless Group.supports_nested_objects? - should_email(user, times: 1, recipients: email_recipients) end def should_not_email_nested_group_user(user, recipients: email_recipients) - return unless Group.supports_nested_objects? - should_not_email(user, recipients: email_recipients) end - def add_users_with_subscription(project, issuable) + def add_users(project) @subscriber = create :user @unsubscriber = create :user @unsubscribed_mentioned = create :user, username: 'unsubscribed_mentioned' @@ -2427,7 +2644,9 @@ describe NotificationService, :mailer do project.add_maintainer(@unsubscriber) project.add_maintainer(@watcher_and_subscriber) project.add_maintainer(@unsubscribed_mentioned) + end + def add_user_subscriptions(issuable) issuable.subscriptions.create(user: @unsubscribed_mentioned, project: project, subscribed: false) issuable.subscriptions.create(user: @subscriber, project: project, subscribed: true) issuable.subscriptions.create(user: @subscribed_participant, project: project, subscribed: true) diff --git a/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb b/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb index 6d7be27939c..af79a42b611 100644 --- a/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb +++ b/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb @@ -12,6 +12,12 @@ describe PagesDomains::ObtainLetsEncryptCertificateService do stub_lets_encrypt_settings end + around do |example| + Sidekiq::Testing.fake! do + example.run + end + end + def expect_to_create_acme_challenge expect(::PagesDomains::CreateAcmeOrderService).to receive(:new).with(pages_domain) .and_wrap_original do |m, *args| @@ -34,8 +40,12 @@ describe PagesDomains::ObtainLetsEncryptCertificateService do end context 'when there is no acme order' do - it 'creates acme order' do + it 'creates acme order and schedules next step' do expect_to_create_acme_challenge + expect(PagesDomainSslRenewalWorker).to( + receive(:perform_in).with(described_class::CHALLENGE_PROCESSING_DELAY, pages_domain.id) + .and_return(nil).once + ) service.execute end @@ -82,8 +92,12 @@ describe PagesDomains::ObtainLetsEncryptCertificateService do stub_lets_encrypt_order(existing_order.url, 'ready') end - it 'request certificate' do + it 'request certificate and schedules next step' do expect(api_order).to receive(:request_certificate).and_call_original + expect(PagesDomainSslRenewalWorker).to( + receive(:perform_in).with(described_class::CERTIFICATE_PROCESSING_DELAY, pages_domain.id) + .and_return(nil).once + ) service.execute end @@ -137,6 +151,12 @@ describe PagesDomains::ObtainLetsEncryptCertificateService do expect(pages_domain.certificate).to eq(certificate) end + it 'marks certificate as gitlab_provided' do + service.execute + + expect(pages_domain.certificate_source).to eq("gitlab_provided") + end + it 'removes order from database' do service.execute diff --git a/spec/services/projects/after_rename_service_spec.rb b/spec/services/projects/after_rename_service_spec.rb index b8055a285f2..8585d495ffb 100644 --- a/spec/services/projects/after_rename_service_spec.rb +++ b/spec/services/projects/after_rename_service_spec.rb @@ -23,6 +23,7 @@ describe Projects::AfterRenameService do allow(project).to receive(:gitlab_shell).and_return(gitlab_shell) stub_feature_flags(skip_hashed_storage_upgrade: false) + stub_application_setting(hashed_storage_enabled: false) end it 'renames a repository' do diff --git a/spec/services/projects/autocomplete_service_spec.rb b/spec/services/projects/autocomplete_service_spec.rb index 2f70c8ea94d..b625653bc77 100644 --- a/spec/services/projects/autocomplete_service_spec.rb +++ b/spec/services/projects/autocomplete_service_spec.rb @@ -118,7 +118,7 @@ describe Projects::AutocompleteService do expect(milestone_titles).to eq([group_milestone2.title, group_milestone1.title]) end - context 'with nested groups', :nested_groups do + context 'with nested groups' do let(:subgroup) { create(:group, :public, parent: group) } let!(:subgroup_milestone) { create(:milestone, group: subgroup) } diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index f54f9200661..b0b74407812 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -152,6 +152,33 @@ describe Projects::CreateService, '#execute' do end end + context 'default visibility level' do + let(:group) { create(:group, :private) } + + before do + stub_application_setting(default_project_visibility: Gitlab::VisibilityLevel::INTERNAL) + group.add_developer(user) + + opts.merge!( + visibility: 'private', + name: 'test', + namespace: group, + path: 'foo' + ) + end + + it 'creates a private project' do + project = create_project(user, opts) + + expect(project).to respond_to(:errors) + + expect(project.errors.any?).to be(false) + expect(project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE) + expect(project.saved?).to be(true) + expect(project.valid?).to be(true) + end + end + context 'restricted visibility level' do before do stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) @@ -201,6 +228,7 @@ describe Projects::CreateService, '#execute' do context 'with legacy storage' do before do + stub_application_setting(hashed_storage_enabled: false) gitlab_shell.create_repository(repository_storage, "#{user.namespace.full_path}/existing", 'group/project') end @@ -232,7 +260,6 @@ describe Projects::CreateService, '#execute' do let(:hashed_path) { '@hashed/6b/86/6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b' } before do - stub_application_setting(hashed_storage_enabled: true) allow(Digest::SHA2).to receive(:hexdigest) { hash } end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 3af7ee3ad50..9a6f64b825a 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -121,7 +121,22 @@ describe Projects::DestroyService do it { expect(Dir.exist?(remove_path)).to be_truthy } end - context 'when flushing caches fail' do + context 'when flushing caches fail due to Git errors' do + before do + allow(project.repository).to receive(:before_delete).and_raise(::Gitlab::Git::CommandError) + allow(Gitlab::GitLogger).to receive(:warn).with( + class: described_class.name, + project_id: project.id, + disk_path: project.disk_path, + message: 'Gitlab::Git::CommandError').and_call_original + + perform_enqueued_jobs { destroy_project(project, user, {}) } + end + + it_behaves_like 'deleting the project' + end + + context 'when flushing caches fail due to Redis' do before do new_user = create(:user) project.team.add_user(new_user, Gitlab::Access::DEVELOPER) @@ -226,6 +241,18 @@ describe Projects::DestroyService do expect(destroy_project(project, user)).to be false end end + + context 'when registry is disabled' do + before do + stub_container_registry_config(enabled: false) + end + + it 'does not attempting to remove any tags' do + expect(Projects::ContainerRepository::DestroyService).not_to receive(:new) + + destroy_project(project, user) + end + end end context 'when there are tags for legacy root repository' do diff --git a/spec/services/projects/download_service_spec.rb b/spec/services/projects/download_service_spec.rb index f25233ceeb1..06efc2ff825 100644 --- a/spec/services/projects/download_service_spec.rb +++ b/spec/services/projects/download_service_spec.rb @@ -20,13 +20,8 @@ describe Projects::DownloadService do context 'for URLs that are on the whitelist' do before do - sham_rack_app = ShamRack.at('mycompany.fogbugz.com').stub - sham_rack_app.register_resource('/rails_sample.jpg', File.read(Rails.root + 'spec/fixtures/rails_sample.jpg'), 'image/jpg') - sham_rack_app.register_resource('/doc_sample.txt', File.read(Rails.root + 'spec/fixtures/doc_sample.txt'), 'text/plain') - end - - after do - ShamRack.unmount_all + stub_request(:get, 'http://mycompany.fogbugz.com/rails_sample.jpg').to_return(body: File.read(Rails.root + 'spec/fixtures/rails_sample.jpg')) + stub_request(:get, 'http://mycompany.fogbugz.com/doc_sample.txt').to_return(body: File.read(Rails.root + 'spec/fixtures/doc_sample.txt')) end context 'an image file' do diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index 6afc91d5e95..0c109e26a6a 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -116,11 +116,12 @@ describe Projects::ForkService do end end - context 'repository already exists' do + context 'repository in legacy storage already exists' do let(:repository_storage) { 'default' } let(:repository_storage_path) { Gitlab.config.repositories.storages[repository_storage].legacy_disk_path } before do + stub_application_setting(hashed_storage_enabled: false) gitlab_shell.create_repository(repository_storage, "#{@to_user.namespace.full_path}/#{@from_project.path}", "#{@to_user.namespace.full_path}/#{@from_project.path}") end diff --git a/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb index 80debcd3a7a..dabfd61d3f5 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb @@ -33,7 +33,7 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do before do allow(project).to receive(:lfs_enabled?).and_return(true) - response = instance_double(HTTParty::Response) + response = instance_double(Gitlab::HTTP::Response) allow(response).to receive(:body).and_return(objects_response.to_json) allow(response).to receive(:success?).and_return(true) allow(Gitlab::HTTP).to receive(:post).and_return(response) @@ -95,7 +95,7 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do shared_examples 'JSON parse errors' do |body| it 'raises error' do - response = instance_double(HTTParty::Response) + response = instance_double(Gitlab::HTTP::Response) allow(response).to receive(:body).and_return(body) allow(response).to receive(:success?).and_return(true) allow(Gitlab::HTTP).to receive(:post).and_return(response) diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index 75d534c59bf..970e82e7107 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -17,7 +17,7 @@ describe Projects::LfsPointers::LfsDownloadService do before do ApplicationSetting.create_from_defaults - stub_application_setting(allow_local_requests_from_hooks_and_services: local_request_setting) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: local_request_setting) allow(project).to receive(:lfs_enabled?).and_return(true) end diff --git a/spec/services/projects/propagate_service_template_spec.rb b/spec/services/projects/propagate_service_template_spec.rb index f93e5aae82a..2c3effec617 100644 --- a/spec/services/projects/propagate_service_template_spec.rb +++ b/spec/services/projects/propagate_service_template_spec.rb @@ -72,7 +72,7 @@ describe Projects::PropagateServiceTemplate do expect(project.pushover_service.properties).to eq(service_template.properties) end - describe 'bulk update' do + describe 'bulk update', :use_sql_query_cache do let(:project_total) { 5 } before do diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb index be2811ab1e7..4396ccab584 100644 --- a/spec/services/projects/update_remote_mirror_service_spec.rb +++ b/spec/services/projects/update_remote_mirror_service_spec.rb @@ -10,49 +10,91 @@ describe Projects::UpdateRemoteMirrorService do subject(:service) { described_class.new(project, project.creator) } - describe "#execute" do + describe '#execute' do + subject(:execute!) { service.execute(remote_mirror, 0) } + before do project.repository.add_branch(project.owner, 'existing-branch', 'master') allow(remote_mirror).to receive(:update_repository).and_return(true) end - it "ensures the remote exists" do + it 'ensures the remote exists' do stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) expect(remote_mirror).to receive(:ensure_remote!) - service.execute(remote_mirror) + execute! end - it "fetches the remote repository" do + it 'fetches the remote repository' do expect(project.repository) .to receive(:fetch_remote) - .with(remote_mirror.remote_name, no_tags: true, ssh_auth: remote_mirror) + .with(remote_mirror.remote_name, no_tags: true, ssh_auth: remote_mirror) - service.execute(remote_mirror) + execute! end - it "returns success when updated succeeds" do + it 'marks the mirror as started when beginning' do + expect(remote_mirror).to receive(:update_start!).and_call_original + + execute! + end + + it 'marks the mirror as successfully finished' do stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) - result = service.execute(remote_mirror) + result = execute! expect(result[:status]).to eq(:success) + expect(remote_mirror).to be_finished + end + + it 'marks the mirror as failed and raises the error when an unexpected error occurs' do + allow(project.repository).to receive(:fetch_remote).and_raise('Badly broken') + + expect { execute! }.to raise_error /Badly broken/ + + expect(remote_mirror).to be_failed + expect(remote_mirror.last_error).to include('Badly broken') + end + + context 'when the update fails because of a `Gitlab::Git::CommandError`' do + before do + allow(project.repository).to receive(:fetch_remote).and_raise(Gitlab::Git::CommandError.new('fetch failed')) + end + + it 'wraps `Gitlab::Git::CommandError`s in a service error' do + expect(execute!).to eq(status: :error, message: 'fetch failed') + end + + it 'marks the mirror as to be retried' do + execute! + + expect(remote_mirror).to be_to_retry + expect(remote_mirror.last_error).to include('fetch failed') + end + + it "marks the mirror as failed after #{described_class::MAX_TRIES} tries" do + service.execute(remote_mirror, described_class::MAX_TRIES) + + expect(remote_mirror).to be_failed + expect(remote_mirror.last_error).to include('fetch failed') + end end context 'when syncing all branches' do - it "push all the branches the first time" do + it 'push all the branches the first time' do stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) expect(remote_mirror).to receive(:update_repository).with({}) - service.execute(remote_mirror) + execute! end end context 'when only syncing protected branches' do - it "sync updated protected branches" do + it 'sync updated protected branches' do stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) protected_branch = create_protected_branch(project) remote_mirror.only_protected_branches = true @@ -61,7 +103,7 @@ describe Projects::UpdateRemoteMirrorService do .to receive(:update_repository) .with(only_branches_matching: [protected_branch.name]) - service.execute(remote_mirror) + execute! end def create_protected_branch(project) diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 1dcfb739eb6..31bd0f0f836 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -186,7 +186,10 @@ describe Projects::UpdateService do expect_any_instance_of(ProjectWiki).to receive(:wiki).and_raise(ProjectWiki::CouldNotCreateWikiError) expect_any_instance_of(described_class).to receive(:log_error).with("Could not create wiki for #{project.full_name}") - expect(Gitlab::Metrics).to receive(:counter) + + counter = double(:counter) + expect(Gitlab::Metrics).to receive(:counter).with(:wiki_can_not_be_created_total, 'Counts the times we failed to create a wiki').and_return(counter) + expect(counter).to receive(:increment) update_project(project, user, project_feature_attributes: { wiki_access_level: ProjectFeature::ENABLED }) end @@ -347,13 +350,13 @@ describe Projects::UpdateService do context 'when updating #pages_access_level' do subject(:call_service) do - update_project(project, admin, project_feature_attributes: { pages_access_level: ProjectFeature::PRIVATE }) + update_project(project, admin, project_feature_attributes: { pages_access_level: ProjectFeature::ENABLED }) end it 'updates the attribute' do expect { call_service } .to change { project.project_feature.pages_access_level } - .to(ProjectFeature::PRIVATE) + .to(ProjectFeature::ENABLED) end it 'calls Projects::UpdatePagesConfigurationService' do @@ -366,9 +369,28 @@ describe Projects::UpdateService do end end + context 'when updating #emails_disabled' do + it 'updates the attribute for the project owner' do + expect { update_project(project, user, emails_disabled: true) } + .to change { project.emails_disabled } + .to(true) + end + + it 'does not update when not project owner' do + maintainer = create(:user) + project.add_user(maintainer, :maintainer) + + expect { update_project(project, maintainer, emails_disabled: true) } + .not_to change { project.emails_disabled } + end + end + context 'with external authorization enabled' do before do enable_external_authorization_service_check + + allow(::Gitlab::ExternalAuthorization) + .to receive(:access_allowed?).with(user, 'default_label', project.full_path).and_call_original end it 'does not save the project with an error if the service denies access' do @@ -399,8 +421,7 @@ describe Projects::UpdateService do end it 'does not check the label when it does not change' do - expect(::Gitlab::ExternalAuthorization) - .not_to receive(:access_allowed?) + expect(::Gitlab::ExternalAuthorization).to receive(:access_allowed?).once update_project(project, user, { name: 'New name' }) end diff --git a/spec/services/prometheus/proxy_service_spec.rb b/spec/services/prometheus/proxy_service_spec.rb index 4bdb20de4c9..03bda94e9c6 100644 --- a/spec/services/prometheus/proxy_service_spec.rb +++ b/spec/services/prometheus/proxy_service_spec.rb @@ -131,7 +131,7 @@ describe Prometheus::ProxyService do allow(environment).to receive(:prometheus_adapter) .and_return(prometheus_adapter) allow(prometheus_adapter).to receive(:can_query?).and_return(true) - allow(prometheus_adapter).to receive(:prometheus_client_wrapper) + allow(prometheus_adapter).to receive(:prometheus_client) .and_return(prometheus_client) end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 95a131e8c86..c9714964fc9 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -28,61 +28,108 @@ describe QuickActions::InterpretService do shared_examples 'reopen command' do it 'returns state_event: "reopen" if content contains /reopen' do issuable.close! - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(state_event: 'reopen') end + + it 'returns the reopen message' do + issuable.close! + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Reopened this #{issuable.to_ability_name.humanize(capitalize: false)}.") + end end shared_examples 'close command' do it 'returns state_event: "close" if content contains /close' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(state_event: 'close') end + + it 'returns the close message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Closed this #{issuable.to_ability_name.humanize(capitalize: false)}.") + end end shared_examples 'title command' do it 'populates title: "A brand new title" if content contains /title A brand new title' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(title: 'A brand new title') end + + it 'returns the title message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq(%{Changed the title to "A brand new title".}) + end end shared_examples 'milestone command' do it 'fetches milestone and populates milestone_id if content contains /milestone' do milestone # populate the milestone - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(milestone_id: milestone.id) end + + it 'returns the milestone message' do + milestone # populate the milestone + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Set the milestone to #{milestone.to_reference}.") + end + + it 'returns empty milestone message when milestone is wrong' do + _, _, message = service.execute('/milestone %wrong-milestone', issuable) + + expect(message).to be_empty + end end shared_examples 'remove_milestone command' do it 'populates milestone_id: nil if content contains /remove_milestone' do issuable.update!(milestone_id: milestone.id) - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(milestone_id: nil) end + + it 'returns removed milestone message' do + issuable.update!(milestone_id: milestone.id) + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Removed #{milestone.to_reference} milestone.") + end end shared_examples 'label command' do it 'fetches label ids and populates add_label_ids if content contains /label' do bug # populate the label inprogress # populate the label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(add_label_ids: [bug.id, inprogress.id]) end + + it 'returns the label message' do + bug # populate the label + inprogress # populate the label + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Added #{bug.to_reference(format: :name)} #{inprogress.to_reference(format: :name)} labels.") + end end shared_examples 'multiple label command' do it 'fetches label ids and populates add_label_ids if content contains multiple /label' do bug # populate the label inprogress # populate the label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(add_label_ids: [inprogress.id, bug.id]) end @@ -91,7 +138,7 @@ describe QuickActions::InterpretService do shared_examples 'multiple label with same argument' do it 'prevents duplicate label ids and populates add_label_ids if content contains multiple /label' do inprogress # populate the label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(add_label_ids: [inprogress.id]) end @@ -120,16 +167,23 @@ describe QuickActions::InterpretService do shared_examples 'unlabel command' do it 'fetches label ids and populates remove_label_ids if content contains /unlabel' do issuable.update!(label_ids: [inprogress.id]) # populate the label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(remove_label_ids: [inprogress.id]) end + + it 'returns the unlabel message' do + issuable.update!(label_ids: [inprogress.id]) # populate the label + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Removed #{inprogress.to_reference(format: :name)} label.") + end end shared_examples 'multiple unlabel command' do it 'fetches label ids and populates remove_label_ids if content contains mutiple /unlabel' do issuable.update!(label_ids: [inprogress.id, bug.id]) # populate the label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(remove_label_ids: [inprogress.id, bug.id]) end @@ -138,7 +192,7 @@ describe QuickActions::InterpretService do shared_examples 'unlabel command with no argument' do it 'populates label_ids: [] if content contains /unlabel with no arguments' do issuable.update!(label_ids: [inprogress.id]) # populate the label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(label_ids: []) end @@ -148,91 +202,161 @@ describe QuickActions::InterpretService do it 'populates label_ids: [] if content contains /relabel' do issuable.update!(label_ids: [bug.id]) # populate the label inprogress # populate the label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(label_ids: [inprogress.id]) end + + it 'returns the relabel message' do + issuable.update!(label_ids: [bug.id]) # populate the label + inprogress # populate the label + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Replaced all labels with #{inprogress.to_reference(format: :name)} label.") + end end shared_examples 'todo command' do it 'populates todo_event: "add" if content contains /todo' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(todo_event: 'add') end + + it 'returns the todo message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Added a To Do.') + end end shared_examples 'done command' do it 'populates todo_event: "done" if content contains /done' do TodoService.new.mark_todo(issuable, developer) - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(todo_event: 'done') end + + it 'returns the done message' do + TodoService.new.mark_todo(issuable, developer) + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Marked To Do as done.') + end end shared_examples 'subscribe command' do it 'populates subscription_event: "subscribe" if content contains /subscribe' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(subscription_event: 'subscribe') end + + it 'returns the subscribe message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Subscribed to this #{issuable.to_ability_name.humanize(capitalize: false)}.") + end end shared_examples 'unsubscribe command' do it 'populates subscription_event: "unsubscribe" if content contains /unsubscribe' do issuable.subscribe(developer, project) - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(subscription_event: 'unsubscribe') end + + it 'returns the unsubscribe message' do + issuable.subscribe(developer, project) + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Unsubscribed from this #{issuable.to_ability_name.humanize(capitalize: false)}.") + end end shared_examples 'due command' do + let(:expected_date) { Date.new(2016, 8, 28) } + it 'populates due_date: Date.new(2016, 8, 28) if content contains /due 2016-08-28' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) + + expect(updates).to eq(due_date: expected_date) + end - expect(updates).to eq(due_date: defined?(expected_date) ? expected_date : Date.new(2016, 8, 28)) + it 'returns due_date message: Date.new(2016, 8, 28) if content contains /due 2016-08-28' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Set the due date to #{expected_date.to_s(:medium)}.") end end shared_examples 'remove_due_date command' do - it 'populates due_date: nil if content contains /remove_due_date' do + before do issuable.update!(due_date: Date.today) - _, updates = service.execute(content, issuable) + end + + it 'populates due_date: nil if content contains /remove_due_date' do + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(due_date: nil) end + + it 'returns Removed the due date' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Removed the due date.') + end end shared_examples 'wip command' do it 'returns wip_event: "wip" if content contains /wip' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(wip_event: 'wip') end + + it 'returns the wip message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Marked this #{issuable.to_ability_name.humanize(capitalize: false)} as Work In Progress.") + end end shared_examples 'unwip command' do it 'returns wip_event: "unwip" if content contains /wip' do issuable.update!(title: issuable.wip_title) - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(wip_event: 'unwip') end + + it 'returns the unwip message' do + issuable.update!(title: issuable.wip_title) + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Unmarked this #{issuable.to_ability_name.humanize(capitalize: false)} as Work In Progress.") + end end shared_examples 'estimate command' do it 'populates time_estimate: 3600 if content contains /estimate 1h' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(time_estimate: 3600) end + + it 'returns the time_estimate formatted message' do + _, _, message = service.execute('/estimate 79d', issuable) + + expect(message).to eq('Set time estimate to 3mo 3w 4d.') + end end shared_examples 'spend command' do it 'populates spend_time: 3600 if content contains /spend 1h' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(spend_time: { duration: 3600, @@ -240,11 +364,17 @@ describe QuickActions::InterpretService do spent_at: DateTime.now.to_date }) end + + it 'returns the spend_time message including the formatted duration and verb' do + _, _, message = service.execute('/spend -120m', issuable) + + expect(message).to eq('Subtracted 2h spent time.') + end end shared_examples 'spend command with negative time' do it 'populates spend_time: -1800 if content contains /spend -30m' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(spend_time: { duration: -1800, @@ -256,7 +386,7 @@ describe QuickActions::InterpretService do shared_examples 'spend command with valid date' do it 'populates spend time: 1800 with date in date type format' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(spend_time: { duration: 1800, @@ -268,7 +398,7 @@ describe QuickActions::InterpretService do shared_examples 'spend command with invalid date' do it 'will not create any note and timelog' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq({}) end @@ -276,7 +406,7 @@ describe QuickActions::InterpretService do shared_examples 'spend command with future date' do it 'will not create any note and timelog' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq({}) end @@ -284,18 +414,30 @@ describe QuickActions::InterpretService do shared_examples 'remove_estimate command' do it 'populates time_estimate: 0 if content contains /remove_estimate' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(time_estimate: 0) end + + it 'returns the remove_estimate message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Removed time estimate.') + end end shared_examples 'remove_time_spent command' do it 'populates spend_time: :reset if content contains /remove_time_spent' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(spend_time: { duration: :reset, user_id: developer.id }) end + + it 'returns the remove_time_spent message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Removed spent time.') + end end shared_examples 'lock command' do @@ -303,10 +445,16 @@ describe QuickActions::InterpretService do let(:merge_request) { create(:merge_request, source_project: project, discussion_locked: false) } it 'returns discussion_locked: true if content contains /lock' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(discussion_locked: true) end + + it 'returns the lock discussion message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Locked the discussion.') + end end shared_examples 'unlock command' do @@ -314,45 +462,79 @@ describe QuickActions::InterpretService do let(:merge_request) { create(:merge_request, source_project: project, discussion_locked: true) } it 'returns discussion_locked: true if content contains /unlock' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(discussion_locked: false) end + + it 'returns the unlock discussion message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Unlocked the discussion.') + end end - shared_examples 'empty command' do + shared_examples 'empty command' do |error_msg| it 'populates {} if content contains an unsupported command' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to be_empty end + + it "returns #{error_msg || 'an empty'} message" do + _, _, message = service.execute(content, issuable) + + if error_msg + expect(message).to eq(error_msg) + else + expect(message).to be_empty + end + end end shared_examples 'merge command' do let(:project) { create(:project, :repository) } it 'runs merge command if content contains /merge' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(merge: merge_request.diff_head_sha) end + + it 'returns them merge message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Scheduled to merge this merge request when the pipeline succeeds.') + end end shared_examples 'award command' do it 'toggle award 100 emoji if content contains /award :100:' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(emoji_award: "100") end + + it 'returns the award message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Toggled :100: emoji award.') + end end shared_examples 'duplicate command' do it 'fetches issue and populates canonical_issue_id if content contains /duplicate issue_reference' do issue_duplicate # populate the issue - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(canonical_issue_id: issue_duplicate.id) end + + it 'returns the duplicate message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Marked this issue as a duplicate of #{issue_duplicate.to_reference(project)}.") + end end shared_examples 'copy_metadata command' do @@ -360,7 +542,7 @@ describe QuickActions::InterpretService do source_issuable # populate the issue todo_label # populate this label inreview_label # populate this label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates[:add_label_ids]).to match_array([inreview_label.id, todo_label.id]) @@ -370,19 +552,45 @@ describe QuickActions::InterpretService do expect(updates).not_to have_key(:milestone_id) end end + + it 'returns the copy metadata message' do + _, _, message = service.execute("/copy_metadata #{source_issuable.to_reference}", issuable) + + expect(message).to eq("Copied labels and milestone from #{source_issuable.to_reference}.") + end + end + + describe 'move issue command' do + it 'returns the move issue message' do + _, _, message = service.execute("/move #{project.full_path}", issue) + + expect(message).to eq("Moved this issue to #{project.full_path}.") + end + + it 'returns move issue failure message when the referenced issue is not found' do + _, _, message = service.execute('/move invalid', issue) + + expect(message).to eq("Failed to move this issue because target project doesn't exist.") + end end shared_examples 'confidential command' do it 'marks issue as confidential if content contains /confidential' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(confidential: true) end + + it 'returns the confidential message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Made this issue confidential.') + end end shared_examples 'shrug command' do it 'appends ¯\_(ツ)_/¯ to the comment' do - new_content, _ = service.execute(content, issuable) + new_content, _, _ = service.execute(content, issuable) expect(new_content).to end_with(described_class::SHRUG) end @@ -390,7 +598,7 @@ describe QuickActions::InterpretService do shared_examples 'tableflip command' do it 'appends (╯°□°)╯︵ ┻━┻ to the comment' do - new_content, _ = service.execute(content, issuable) + new_content, _, _ = service.execute(content, issuable) expect(new_content).to end_with(described_class::TABLEFLIP) end @@ -398,18 +606,34 @@ describe QuickActions::InterpretService do shared_examples 'tag command' do it 'tags a commit' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(tag_name: tag_name, tag_message: tag_message) end + + it 'returns the tag message' do + _, _, message = service.execute(content, issuable) + + if tag_message.present? + expect(message).to eq(%{Tagged this commit to #{tag_name} with "#{tag_message}".}) + else + expect(message).to eq("Tagged this commit to #{tag_name}.") + end + end end shared_examples 'assign command' do it 'assigns to a single user' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(assignee_ids: [developer.id]) end + + it 'returns the assign message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Assigned #{developer.to_reference}.") + end end it_behaves_like 'reopen command' do @@ -463,7 +687,7 @@ describe QuickActions::InterpretService do let(:service) { described_class.new(project, developer, {}) } it 'precheck passes and returns merge command' do - _, updates = service.execute('/merge', merge_request) + _, updates, _ = service.execute('/merge', merge_request) expect(updates).to eq(merge: nil) end @@ -559,7 +783,7 @@ describe QuickActions::InterpretService do end end - it_behaves_like 'empty command' do + it_behaves_like 'empty command', "Failed to assign a user because no user was found." do let(:content) { '/assign @abcd1234' } let(:issuable) { issue } end @@ -574,20 +798,34 @@ describe QuickActions::InterpretService do context 'Issue' do it 'populates assignee_ids: [] if content contains /unassign' do - issue.update(assignee_ids: [developer.id]) - _, updates = service.execute(content, issue) + issue.update!(assignee_ids: [developer.id]) + _, updates, _ = service.execute(content, issue) expect(updates).to eq(assignee_ids: []) end + + it 'returns the unassign message for all the assignee if content contains /unassign' do + issue.update(assignee_ids: [developer.id, developer2.id]) + _, _, message = service.execute(content, issue) + + expect(message).to eq("Removed assignees #{developer.to_reference} and #{developer2.to_reference}.") + end end context 'Merge Request' do it 'populates assignee_ids: [] if content contains /unassign' do - merge_request.update(assignee_ids: [developer.id]) - _, updates = service.execute(content, merge_request) + merge_request.update!(assignee_ids: [developer.id]) + _, updates, _ = service.execute(content, merge_request) expect(updates).to eq(assignee_ids: []) end + + it 'returns the unassign message for all the assignee if content contains /unassign' do + merge_request.update(assignee_ids: [developer.id, developer2.id]) + _, _, message = service.execute(content, merge_request) + + expect(message).to eq("Removed assignees #{developer.to_reference} and #{developer2.to_reference}.") + end end end @@ -979,12 +1217,12 @@ describe QuickActions::InterpretService do let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'empty command', 'Failed to mark this issue as a duplicate because referenced issue was not found.' do let(:content) { "/duplicate imaginary#1234" } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'empty command', 'Failed to mark this issue as a duplicate because referenced issue was not found.' do let(:other_project) { create(:project, :private) } let(:issue_duplicate) { create(:issue, project: other_project) } @@ -1049,7 +1287,7 @@ describe QuickActions::InterpretService do let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'empty command', 'Failed to mark this issue as a duplicate because referenced issue was not found.' do let(:content) { '/duplicate #{issue.to_reference}' } let(:issuable) { issue } end @@ -1132,13 +1370,13 @@ describe QuickActions::InterpretService do let(:service) { described_class.new(non_empty_project, developer)} it 'updates target_branch if /target_branch command is executed' do - _, updates = service.execute('/target_branch merge-test', merge_request) + _, updates, _ = service.execute('/target_branch merge-test', merge_request) expect(updates).to eq(target_branch: 'merge-test') end it 'handles blanks around param' do - _, updates = service.execute('/target_branch merge-test ', merge_request) + _, updates, _ = service.execute('/target_branch merge-test ', merge_request) expect(updates).to eq(target_branch: 'merge-test') end @@ -1156,6 +1394,12 @@ describe QuickActions::InterpretService do let(:issuable) { another_merge_request } end end + + it 'returns the target_branch message' do + _, _, message = service.execute('/target_branch merge-test', merge_request) + + expect(message).to eq('Set target branch to merge-test.') + end end context '/board_move command' do @@ -1171,13 +1415,13 @@ describe QuickActions::InterpretService do it 'populates remove_label_ids for all current board columns' do issue.update!(label_ids: [todo.id, inprogress.id]) - _, updates = service.execute(content, issue) + _, updates, _ = service.execute(content, issue) expect(updates[:remove_label_ids]).to match_array([todo.id, inprogress.id]) end it 'populates add_label_ids with the id of the given label' do - _, updates = service.execute(content, issue) + _, updates, _ = service.execute(content, issue) expect(updates[:add_label_ids]).to eq([inreview.id]) end @@ -1185,7 +1429,7 @@ describe QuickActions::InterpretService do it 'does not include the given label id in remove_label_ids' do issue.update!(label_ids: [todo.id, inreview.id]) - _, updates = service.execute(content, issue) + _, updates, _ = service.execute(content, issue) expect(updates[:remove_label_ids]).to match_array([todo.id]) end @@ -1193,11 +1437,19 @@ describe QuickActions::InterpretService do it 'does not remove label ids that are not lists on the board' do issue.update!(label_ids: [todo.id, bug.id]) - _, updates = service.execute(content, issue) + _, updates, _ = service.execute(content, issue) expect(updates[:remove_label_ids]).to match_array([todo.id]) end + it 'returns board_move message' do + issue.update!(label_ids: [todo.id, inprogress.id]) + + _, _, message = service.execute(content, issue) + + expect(message).to eq("Moved issue to ~#{inreview.id} column in the board.") + end + context 'if the project has multiple boards' do let(:issuable) { issue } @@ -1211,19 +1463,19 @@ describe QuickActions::InterpretService do context 'if the given label does not exist' do let(:issuable) { issue } let(:content) { '/board_move ~"Fake Label"' } - it_behaves_like 'empty command' + it_behaves_like 'empty command', 'Failed to move this issue because label was not found.' end context 'if multiple labels are given' do let(:issuable) { issue } let(:content) { %{/board_move ~"#{inreview.title}" ~"#{todo.title}"} } - it_behaves_like 'empty command' + it_behaves_like 'empty command', 'Failed to move this issue because only a single label can be provided.' end context 'if the given label is not a list on the board' do let(:issuable) { issue } let(:content) { %{/board_move ~"#{bug.title}"} } - it_behaves_like 'empty command' + it_behaves_like 'empty command', 'Failed to move this issue because label was not found.' end context 'if issuable is not an Issue' do @@ -1292,10 +1544,16 @@ describe QuickActions::InterpretService do end it 'populates create_merge_request with branch_name and issue iid' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(create_merge_request: { branch_name: branch_name, issue_iid: issuable.iid }) end + + it 'returns the create_merge_request message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Created branch '#{branch_name}' and a merge request to resolve this issue.") + end end end @@ -1556,7 +1814,13 @@ describe QuickActions::InterpretService do it 'uses the default branch name' do _, explanations = service.explain(content, issue) - expect(explanations).to eq(['Creates a branch and a merge request to resolve this issue']) + expect(explanations).to eq(['Creates a branch and a merge request to resolve this issue.']) + end + + it 'returns the execution message using the default branch name' do + _, _, message = service.execute(content, issue) + + expect(message).to eq('Created a branch and a merge request to resolve this issue.') end end @@ -1566,7 +1830,13 @@ describe QuickActions::InterpretService do it 'uses the given branch name' do _, explanations = service.explain(content, issue) - expect(explanations).to eq(["Creates branch 'foo' and a merge request to resolve this issue"]) + expect(explanations).to eq(["Creates branch 'foo' and a merge request to resolve this issue."]) + end + + it 'returns the execution message using the given branch name' do + _, _, message = service.execute(content, issue) + + expect(message).to eq("Created branch 'foo' and a merge request to resolve this issue.") end end end diff --git a/spec/services/self_monitoring/project/create_service_spec.rb b/spec/services/self_monitoring/project/create_service_spec.rb new file mode 100644 index 00000000000..def20448bd9 --- /dev/null +++ b/spec/services/self_monitoring/project/create_service_spec.rb @@ -0,0 +1,234 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe SelfMonitoring::Project::CreateService do + describe '#execute' do + let(:result) { subject.execute } + + let(:prometheus_settings) do + OpenStruct.new( + enable: true, + listen_address: 'localhost:9090' + ) + end + + before do + allow(Gitlab.config).to receive(:prometheus).and_return(prometheus_settings) + end + + context 'without admin users' do + it 'returns error' do + expect(subject).to receive(:log_error).and_call_original + expect(result).to eq( + status: :error, + message: 'No active admin user found', + failed_step: :validate_admins + ) + end + end + + context 'with admin users' do + let(:project) { result[:project] } + let(:group) { result[:group] } + let(:application_setting) { Gitlab::CurrentSettings.current_application_settings } + + let!(:user) { create(:user, :admin) } + + before do + application_setting.allow_local_requests_from_web_hooks_and_services = true + end + + shared_examples 'has prometheus service' do |listen_address| + it do + expect(result[:status]).to eq(:success) + + prometheus = project.prometheus_service + expect(prometheus).not_to eq(nil) + expect(prometheus.api_url).to eq(listen_address) + expect(prometheus.active).to eq(true) + expect(prometheus.manual_configuration).to eq(true) + end + end + + it_behaves_like 'has prometheus service', 'http://localhost:9090' + + it 'creates group' do + expect(result[:status]).to eq(:success) + expect(group).to be_persisted + expect(group.name).to eq(described_class::GROUP_NAME) + expect(group.path).to start_with(described_class::GROUP_PATH) + expect(group.path.split('-').last.length).to eq(8) + expect(group.visibility_level).to eq(described_class::VISIBILITY_LEVEL) + end + + it 'creates project with internal visibility' do + expect(result[:status]).to eq(:success) + expect(project.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL) + expect(project).to be_persisted + end + + it 'creates project with internal visibility even when internal visibility is restricted' do + application_setting.restricted_visibility_levels = [Gitlab::VisibilityLevel::INTERNAL] + + expect(result[:status]).to eq(:success) + expect(project.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL) + expect(project).to be_persisted + end + + it 'creates project with correct name and description' do + expect(result[:status]).to eq(:success) + expect(project.name).to eq(described_class::PROJECT_NAME) + expect(project.description).to eq(described_class::PROJECT_DESCRIPTION) + end + + it 'adds all admins as maintainers' do + admin1 = create(:user, :admin) + admin2 = create(:user, :admin) + create(:user) + + expect(result[:status]).to eq(:success) + expect(project.owner).to eq(group) + expect(group.members.collect(&:user)).to contain_exactly(user, admin1, admin2) + expect(group.members.collect(&:access_level)).to contain_exactly( + Gitlab::Access::OWNER, + Gitlab::Access::MAINTAINER, + Gitlab::Access::MAINTAINER + ) + end + + it 'saves the project id' do + expect(result[:status]).to eq(:success) + expect(application_setting.instance_administration_project_id).to eq(project.id) + end + + it 'returns error when saving project ID fails' do + allow(application_setting).to receive(:update) { false } + + expect(result[:status]).to eq(:error) + expect(result[:failed_step]).to eq(:save_project_id) + expect(result[:message]).to eq('Could not save project ID') + end + + it 'does not fail when a project already exists' do + expect(result[:status]).to eq(:success) + + second_result = subject.execute + + expect(second_result[:status]).to eq(:success) + expect(second_result[:project]).to eq(project) + expect(second_result[:group]).to eq(group) + end + + context 'when local requests from hooks and services are not allowed' do + before do + application_setting.allow_local_requests_from_web_hooks_and_services = false + end + + it_behaves_like 'has prometheus service', 'http://localhost:9090' + + it 'does not overwrite the existing whitelist' do + application_setting.outbound_local_requests_whitelist = ['example.com'] + + expect(result[:status]).to eq(:success) + expect(application_setting.outbound_local_requests_whitelist).to contain_exactly( + 'example.com', 'localhost' + ) + end + end + + context 'with non default prometheus address' do + before do + prometheus_settings.listen_address = 'https://localhost:9090' + end + + it_behaves_like 'has prometheus service', 'https://localhost:9090' + end + + context 'when prometheus setting is not present in gitlab.yml' do + before do + allow(Gitlab.config).to receive(:prometheus).and_raise(Settingslogic::MissingSetting) + end + + it 'does not fail' do + expect(result).to include(status: :success) + expect(project.prometheus_service).to be_nil + end + end + + context 'when prometheus setting is disabled in gitlab.yml' do + before do + prometheus_settings.enable = false + end + + it 'does not configure prometheus' do + expect(result).to include(status: :success) + expect(project.prometheus_service).to be_nil + end + end + + context 'when prometheus listen address is blank in gitlab.yml' do + before do + prometheus_settings.listen_address = '' + end + + it 'does not configure prometheus' do + expect(result).to include(status: :success) + expect(project.prometheus_service).to be_nil + end + end + + context 'when project cannot be created' do + let(:project) { build(:project) } + + before do + project.errors.add(:base, "Test error") + + expect_next_instance_of(::Projects::CreateService) do |project_create_service| + expect(project_create_service).to receive(:execute) + .and_return(project) + end + end + + it 'returns error' do + expect(subject).to receive(:log_error).and_call_original + expect(result).to eq({ + status: :error, + message: 'Could not create project', + failed_step: :create_project + }) + end + end + + context 'when user cannot be added to project' do + before do + subject.instance_variable_set(:@instance_admins, [user, build(:user, :admin)]) + end + + it 'returns error' do + expect(subject).to receive(:log_error).and_call_original + expect(result).to eq({ + status: :error, + message: 'Could not add admins as members', + failed_step: :add_group_members + }) + end + end + + context 'when prometheus manual configuration cannot be saved' do + before do + prometheus_settings.listen_address = 'httpinvalid://localhost:9090' + end + + it 'returns error' do + expect(subject).to receive(:log_error).and_call_original + expect(result).to eq( + status: :error, + message: 'Could not save prometheus manual configuration', + failed_step: :add_prometheus_manual_configuration + ) + end + end + end + end +end diff --git a/spec/services/service_response_spec.rb b/spec/services/service_response_spec.rb index 30bd4d6820b..e790d272e61 100644 --- a/spec/services/service_response_spec.rb +++ b/spec/services/service_response_spec.rb @@ -16,6 +16,13 @@ describe ServiceResponse do expect(response).to be_success expect(response.message).to eq('Good orange') end + + it 'creates a successful response with payload' do + response = described_class.success(payload: { good: 'orange' }) + + expect(response).to be_success + expect(response.payload).to eq(good: 'orange') + end end describe '.error' do @@ -33,6 +40,15 @@ describe ServiceResponse do expect(response.message).to eq('Bad apple') expect(response.http_status).to eq(400) end + + it 'creates a failed response with payload' do + response = described_class.error(message: 'Bad apple', + payload: { bad: 'apple' }) + + expect(response).to be_error + expect(response.message).to eq('Bad apple') + expect(response.payload).to eq(bad: 'apple') + end end describe '#success?' do diff --git a/spec/services/submodules/update_service_spec.rb b/spec/services/submodules/update_service_spec.rb index cf92350c1b2..47b31d4bcbf 100644 --- a/spec/services/submodules/update_service_spec.rb +++ b/spec/services/submodules/update_service_spec.rb @@ -142,7 +142,7 @@ describe Submodules::UpdateService do let(:branch_name) { nil } it_behaves_like 'returns error result' do - let(:error_message) { 'You can only create or edit files when you are on a branch' } + let(:error_message) { 'Invalid parameters' } end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 2420817e1f7..486d0ca0c56 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -332,7 +332,7 @@ describe SystemNoteService do create(:merge_request, source_project: project, target_project: project) end - subject { described_class.merge_when_pipeline_succeeds(noteable, project, author, noteable.diff_head_commit) } + subject { described_class.merge_when_pipeline_succeeds(noteable, project, author, pipeline.sha) } it_behaves_like 'a system note' do let(:action) { 'merge' } @@ -359,6 +359,22 @@ describe SystemNoteService do end end + describe '.abort_merge_when_pipeline_succeeds' do + let(:noteable) do + create(:merge_request, source_project: project, target_project: project) + end + + subject { described_class.abort_merge_when_pipeline_succeeds(noteable, project, author, 'merge request was closed') } + + it_behaves_like 'a system note' do + let(:action) { 'merge' } + end + + it "posts the 'merge when pipeline succeeds' system note" do + expect(subject.note).to eq "aborted the automatic merge because merge request was closed" + end + end + describe '.change_title' do let(:noteable) { create(:issue, project: project, title: 'Lorem ipsum') } @@ -454,17 +470,33 @@ describe SystemNoteService do end describe '.new_issue_branch' do - subject { described_class.new_issue_branch(noteable, project, author, "1-mepmep") } + let(:branch) { '1-mepmep' } - it_behaves_like 'a system note' do - let(:action) { 'branch' } - end + subject { described_class.new_issue_branch(noteable, project, author, branch, branch_project: branch_project) } - context 'when a branch is created from the new branch button' do - it 'sets the note text' do - expect(subject.note).to start_with("created branch [`1-mepmep`]") + shared_examples_for 'a system note for new issue branch' do + it_behaves_like 'a system note' do + let(:action) { 'branch' } + end + + context 'when a branch is created from the new branch button' do + it 'sets the note text' do + expect(subject.note).to start_with("created branch [`#{branch}`]") + end end end + + context 'branch_project is set' do + let(:branch_project) { create(:project, :repository) } + + it_behaves_like 'a system note for new issue branch' + end + + context 'branch_project is not set' do + let(:branch_project) { nil } + + it_behaves_like 'a system note for new issue branch' + end end describe '.new_merge_request' do @@ -477,7 +509,31 @@ describe SystemNoteService do end it 'sets the new merge request note text' do - expect(subject.note).to eq("created merge request #{merge_request.to_reference} to address this issue") + expect(subject.note).to eq("created merge request #{merge_request.to_reference(project)} to address this issue") + end + end + + describe '.zoom_link_added' do + subject { described_class.zoom_link_added(issue, project, author) } + + it_behaves_like 'a system note' do + let(:action) { 'pinned_embed' } + end + + it 'sets the zoom link added note text' do + expect(subject.note).to eq('a Zoom call was added to this issue') + end + end + + describe '.zoom_link_removed' do + subject { described_class.zoom_link_removed(issue, project, author) } + + it_behaves_like 'a system note' do + let(:action) { 'pinned_embed' } + end + + it 'sets the zoom link removed note text' do + expect(subject.note).to eq('a Zoom call was removed from this issue') end end @@ -750,7 +806,7 @@ describe SystemNoteService do end end - describe 'JIRA integration' do + describe 'Jira integration' do include JiraServiceHelper let(:project) { create(:jira_project, :repository) } @@ -946,6 +1002,18 @@ describe SystemNoteService do expect(subject.note).to eq "changed time estimate to 1w 4d 5h" end + + context 'when time_tracking_limit_to_hours setting is true' do + before do + stub_application_setting(time_tracking_limit_to_hours: true) + end + + it 'sets the note text' do + noteable.update_attribute(:time_estimate, 277200) + + expect(subject.note).to eq "changed time estimate to 77h" + end + end end context 'without a time estimate' do @@ -1022,6 +1090,18 @@ describe SystemNoteService do end end + context 'when time_tracking_limit_to_hours setting is true' do + before do + stub_application_setting(time_tracking_limit_to_hours: true) + end + + it 'sets the note text' do + spend_time!(277200) + + expect(subject.note).to eq "added 77h of time spent" + end + end + def spend_time!(seconds) noteable.spend_time(duration: seconds, user_id: author.id) noteable.save! @@ -1108,7 +1188,7 @@ describe SystemNoteService do end it 'sets the note text' do - expect(subject.note).to eq 'resolved all discussions' + expect(subject.note).to eq 'resolved all threads' end end @@ -1135,16 +1215,30 @@ describe SystemNoteService do end it 'links to the diff in the system note' do - expect(subject.note).to include('version 1') - diff_id = merge_request.merge_request_diff.id line_code = change_position.line_code(project.repository) - expect(subject.note).to include(diffs_project_merge_request_path(project, merge_request, diff_id: diff_id, anchor: line_code)) + link = diffs_project_merge_request_path(project, merge_request, diff_id: diff_id, anchor: line_code) + + expect(subject.note).to eq("changed this line in [version 1 of the diff](#{link})") + end + + context 'discussion is on an image' do + let(:discussion) { create(:image_diff_note_on_merge_request, project: project).to_discussion } + + it 'links to the diff in the system note' do + diff_id = merge_request.merge_request_diff.id + file_hash = change_position.file_hash + link = diffs_project_merge_request_path(project, merge_request, diff_id: diff_id, anchor: file_hash) + + expect(subject.note).to eq("changed this file in [version 1 of the diff](#{link})") + end end end - context 'when the change_position is invalid for the discussion' do - let(:change_position) { project.commit(sample_commit.id) } + context 'when the change_position does not point to a valid version' do + before do + allow(merge_request).to receive(:version_params_for).and_return(nil) + end it 'creates a new note in the discussion' do # we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded. diff --git a/spec/services/task_list_toggle_service_spec.rb b/spec/services/task_list_toggle_service_spec.rb index 9adaee6481b..a309951bbcb 100644 --- a/spec/services/task_list_toggle_service_spec.rb +++ b/spec/services/task_list_toggle_service_spec.rb @@ -114,6 +114,23 @@ describe TaskListToggleService do expect(toggler.execute).to be_falsey end + it 'properly handles tasks in a blockquote' do + markdown = + <<-EOT.strip_heredoc + > > * [ ] Task 1 + > * [x] Task 2 + EOT + + markdown_html = Banzai::Pipeline::FullPipeline.call(markdown, project: nil)[:output].to_html + toggler = described_class.new(markdown, markdown_html, + toggle_as_checked: true, + line_source: '> > * [ ] Task 1', line_number: 1) + + expect(toggler.execute).to be_truthy + expect(toggler.updated_markdown.lines[0]).to eq "> > * [x] Task 1\n" + expect(toggler.updated_markdown_html).to include('disabled checked> Task 1') + end + it 'properly handles a GitLab blockquote' do markdown = <<-EOT.strip_heredoc diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb index 2a553e18807..ce809bbf6c5 100644 --- a/spec/services/todos/destroy/entity_leave_service_spec.rb +++ b/spec/services/todos/destroy/entity_leave_service_spec.rb @@ -176,7 +176,7 @@ describe Todos::Destroy::EntityLeaveService do end end - context 'with nested groups', :nested_groups do + context 'with nested groups' do let(:subgroup) { create(:group, :private, parent: group) } let(:subgroup2) { create(:group, :private, parent: group) } let(:subproject) { create(:project, group: subgroup) } diff --git a/spec/services/todos/destroy/group_private_service_spec.rb b/spec/services/todos/destroy/group_private_service_spec.rb index a1798686d7c..7dd495847b3 100644 --- a/spec/services/todos/destroy/group_private_service_spec.rb +++ b/spec/services/todos/destroy/group_private_service_spec.rb @@ -35,7 +35,7 @@ describe Todos::Destroy::GroupPrivateService do expect(project_member.todos).to match_array([todo_project_member]) end - context 'with nested groups', :nested_groups do + context 'with nested groups' do let(:parent_group) { create(:group) } let(:subgroup) { create(:group, :private, parent: group) } let(:subproject) { create(:project, group: subgroup) } diff --git a/spec/services/update_snippet_service_spec.rb b/spec/services/update_snippet_service_spec.rb index 23ea4e003f8..0678f54c195 100644 --- a/spec/services/update_snippet_service_spec.rb +++ b/spec/services/update_snippet_service_spec.rb @@ -40,6 +40,23 @@ describe UpdateSnippetService do end end + describe 'usage counter' do + let(:counter) { Gitlab::UsageDataCounters::SnippetCounter } + let(:snippet) { create_snippet(nil, @user, @opts) } + + it 'increments count' do + expect do + update_snippet(nil, @admin, snippet, @opts) + end.to change { counter.read(:update) }.by 1 + end + + it 'does not increment count if create fails' do + expect do + update_snippet(nil, @admin, snippet, { title: '' }) + end.not_to change { counter.read(:update) } + end + end + def create_snippet(project, user, opts) CreateSnippetService.new(project, user, opts).execute end diff --git a/spec/services/users/refresh_authorized_projects_service_spec.rb b/spec/services/users/refresh_authorized_projects_service_spec.rb index 0287a24808d..f5a914bb482 100644 --- a/spec/services/users/refresh_authorized_projects_service_spec.rb +++ b/spec/services/users/refresh_authorized_projects_service_spec.rb @@ -135,7 +135,7 @@ describe Users::RefreshAuthorizedProjectsService do end end - context 'projects of subgroups of groups the user is a member of', :nested_groups do + context 'projects of subgroups of groups the user is a member of' do let(:group) { create(:group) } let(:nested_group) { create(:group, parent: group) } let!(:other_project) { create(:project, group: nested_group) } @@ -163,7 +163,7 @@ describe Users::RefreshAuthorizedProjectsService do end end - context 'projects shared with subgroups of groups the user is a member of', :nested_groups do + context 'projects shared with subgroups of groups the user is a member of' do let(:group) { create(:group) } let(:nested_group) { create(:group, parent: group) } let(:other_project) { create(:project) } diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 37bafc0c002..50167a2e059 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -19,17 +19,37 @@ describe WebHookService do let(:service_instance) { described_class.new(project_hook, data, :push_hooks) } describe '#initialize' do - it 'allow_local_requests is true if hook is a SystemHook' do - instance = described_class.new(build(:system_hook), data, :system_hook) - expect(instance.request_options[:allow_local_requests]).to be_truthy + before do + stub_application_setting(setting_name => setting) end - it 'allow_local_requests is false if hook is not a SystemHook' do - %i(project_hook service_hook web_hook_log).each do |hook| - instance = described_class.new(build(hook), data, hook) - expect(instance.request_options[:allow_local_requests]).to be_falsey + shared_examples_for 'respects outbound network setting' do + context 'when local requests are allowed' do + let(:setting) { true } + + it { expect(hook.request_options[:allow_local_requests]).to be_truthy } + end + + context 'when local requests are not allowed' do + let(:setting) { false } + + it { expect(hook.request_options[:allow_local_requests]).to be_falsey } end end + + context 'when SystemHook' do + let(:setting_name) { :allow_local_requests_from_system_hooks } + let(:hook) { described_class.new(build(:system_hook), data, :system_hook) } + + include_examples 'respects outbound network setting' + end + + context 'when ProjectHook' do + let(:setting_name) { :allow_local_requests_from_web_hooks_and_services } + let(:hook) { described_class.new(build(:project_hook), data, :project_hook) } + + include_examples 'respects outbound network setting' + end end describe '#execute' do diff --git a/spec/services/wiki_pages/base_service_spec.rb b/spec/services/wiki_pages/base_service_spec.rb new file mode 100644 index 00000000000..2e70246c6f2 --- /dev/null +++ b/spec/services/wiki_pages/base_service_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe WikiPages::BaseService do + let(:project) { double('project') } + let(:user) { double('user') } + + subject(:service) { described_class.new(project, user, {}) } + + describe '#increment_usage' do + counter = Gitlab::UsageDataCounters::WikiPageCounter + error = counter::UnknownEvent + + it 'raises an error on unknown events' do + expect { subject.send(:increment_usage, :bad_event) }.to raise_error error + end + + context 'the event is valid' do + counter::KNOWN_EVENTS.each do |e| + it "updates the #{e} counter" do + expect { subject.send(:increment_usage, e) }.to change { counter.read(e) } + end + end + end + end +end diff --git a/spec/services/wiki_pages/create_service_spec.rb b/spec/services/wiki_pages/create_service_spec.rb index 84510dcf700..ef03a2e9788 100644 --- a/spec/services/wiki_pages/create_service_spec.rb +++ b/spec/services/wiki_pages/create_service_spec.rb @@ -14,6 +14,10 @@ describe WikiPages::CreateService do } end + let(:bad_opts) do + { title: '' } + end + subject(:service) { described_class.new(project, user, opts) } before do @@ -36,5 +40,26 @@ describe WikiPages::CreateService do service.execute end + + it 'counts wiki page creation' do + counter = Gitlab::UsageDataCounters::WikiPageCounter + + expect { service.execute }.to change { counter.read(:create) }.by 1 + end + + context 'when the options are bad' do + subject(:service) { described_class.new(project, user, bad_opts) } + + it 'does not count a creation event' do + counter = Gitlab::UsageDataCounters::WikiPageCounter + + expect { service.execute }.not_to change { counter.read(:create) } + end + + it 'reports the error' do + expect(service.execute).to be_invalid + .and have_attributes(errors: be_present) + end + end end end diff --git a/spec/services/wiki_pages/destroy_service_spec.rb b/spec/services/wiki_pages/destroy_service_spec.rb index c74eac4dad6..350a7eb123b 100644 --- a/spec/services/wiki_pages/destroy_service_spec.rb +++ b/spec/services/wiki_pages/destroy_service_spec.rb @@ -20,5 +20,17 @@ describe WikiPages::DestroyService do service.execute(page) end + + it 'increments the delete count' do + counter = Gitlab::UsageDataCounters::WikiPageCounter + + expect { service.execute(page) }.to change { counter.read(:delete) }.by 1 + end + + it 'does not increment the delete count if the deletion failed' do + counter = Gitlab::UsageDataCounters::WikiPageCounter + + expect { service.execute(nil) }.not_to change { counter.read(:delete) } + end end end diff --git a/spec/services/wiki_pages/update_service_spec.rb b/spec/services/wiki_pages/update_service_spec.rb index 19866bd3bfc..d5f46e7b2db 100644 --- a/spec/services/wiki_pages/update_service_spec.rb +++ b/spec/services/wiki_pages/update_service_spec.rb @@ -16,6 +16,10 @@ describe WikiPages::UpdateService do } end + let(:bad_opts) do + { title: '' } + end + subject(:service) { described_class.new(project, user, opts) } before do @@ -39,5 +43,26 @@ describe WikiPages::UpdateService do service.execute(page) end + + it 'counts edit events' do + counter = Gitlab::UsageDataCounters::WikiPageCounter + + expect { service.execute page }.to change { counter.read(:update) }.by 1 + end + + context 'when the options are bad' do + subject(:service) { described_class.new(project, user, bad_opts) } + + it 'does not count an edit event' do + counter = Gitlab::UsageDataCounters::WikiPageCounter + + expect { service.execute page }.not_to change { counter.read(:update) } + end + + it 'reports the error' do + expect(service.execute page).to be_invalid + .and have_attributes(errors: be_present) + end + end end end diff --git a/spec/services/zoom_notes_service_spec.rb b/spec/services/zoom_notes_service_spec.rb new file mode 100644 index 00000000000..419ecf3f374 --- /dev/null +++ b/spec/services/zoom_notes_service_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ZoomNotesService do + describe '#execute' do + let(:issue) { OpenStruct.new(description: description) } + let(:project) { Object.new } + let(:user) { Object.new } + let(:description) { 'an issue description' } + let(:old_description) { nil } + + subject { described_class.new(issue, project, user, old_description: old_description) } + + shared_examples 'no notifications' do + it "doesn't create notifications" do + expect(SystemNoteService).not_to receive(:zoom_link_added) + expect(SystemNoteService).not_to receive(:zoom_link_removed) + + subject.execute + end + end + + it_behaves_like 'no notifications' + + context 'when the zoom link exists in both description and old_description' do + let(:description) { 'a changed issue description https://zoom.us/j/123' } + let(:old_description) { 'an issue description https://zoom.us/j/123' } + + it_behaves_like 'no notifications' + end + + context "when the zoom link doesn't exist in both description and old_description" do + let(:description) { 'a changed issue description' } + let(:old_description) { 'an issue description' } + + it_behaves_like 'no notifications' + end + + context 'when description == old_description' do + let(:old_description) { 'an issue description' } + + it_behaves_like 'no notifications' + end + + context 'when the description contains a zoom link and old_description is nil' do + let(:description) { 'a changed issue description https://zoom.us/j/123' } + + it 'creates a zoom_link_added notification' do + expect(SystemNoteService).to receive(:zoom_link_added).with(issue, project, user) + expect(SystemNoteService).not_to receive(:zoom_link_removed) + + subject.execute + end + end + + context 'when the zoom link has been added to the description' do + let(:description) { 'a changed issue description https://zoom.us/j/123' } + let(:old_description) { 'an issue description' } + + it 'creates a zoom_link_added notification' do + expect(SystemNoteService).to receive(:zoom_link_added).with(issue, project, user) + expect(SystemNoteService).not_to receive(:zoom_link_removed) + + subject.execute + end + end + + context 'when the zoom link has been removed from the description' do + let(:description) { 'a changed issue description' } + let(:old_description) { 'an issue description https://zoom.us/j/123' } + + it 'creates a zoom_link_removed notification' do + expect(SystemNoteService).not_to receive(:zoom_link_added).with(issue, project, user) + expect(SystemNoteService).to receive(:zoom_link_removed) + + subject.execute + end + end + end +end |