diff options
Diffstat (limited to 'spec/services')
87 files changed, 2665 insertions, 1310 deletions
diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb index 00841de9ff4..ba7acd3d3df 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -6,143 +6,4 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do include AdminModeHelper it_behaves_like 'a container registry auth service' - - context 'when in migration mode' do - include_context 'container registry auth service context' - - let_it_be(:current_user) { create(:user) } - let_it_be(:project) { create(:project) } - - before do - project.add_developer(current_user) - end - - shared_examples 'a modified token with migration eligibility' do |eligible| - it_behaves_like 'a valid token' - it { expect(payload['access']).to include(include('migration_eligible' => eligible)) } - end - - shared_examples 'a modified token' do - context 'with a non eligible root ancestor and project' do - before do - stub_feature_flags(container_registry_migration_phase1_deny: project.root_ancestor) - stub_feature_flags(container_registry_migration_phase1_allow: false) - end - - it_behaves_like 'a modified token with migration eligibility', false - end - - context 'with a non eligible root ancestor and eligible project' do - before do - stub_feature_flags(container_registry_migration_phase1_deny: false) - stub_feature_flags(container_registry_migration_phase1_deny: project.root_ancestor) - stub_feature_flags(container_registry_migration_phase1_allow: project) - end - - it_behaves_like 'a modified token with migration eligibility', false - end - - context 'with an eligible root ancestor and non eligible project' do - before do - stub_feature_flags(container_registry_migration_phase1_deny: false) - stub_feature_flags(container_registry_migration_phase1_allow: false) - end - - it_behaves_like 'a modified token with migration eligibility', false - end - - context 'with an eligible root ancestor and project' do - before do - stub_feature_flags(container_registry_migration_phase1_deny: false) - stub_feature_flags(container_registry_migration_phase1_allow: project) - end - - it_behaves_like 'a modified token with migration eligibility', true - end - end - - context 'with pull action' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull"] } - end - - it_behaves_like 'a modified token' - end - - context 'with push action' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:push"] } - end - - it_behaves_like 'a modified token' - end - - context 'with multiple actions' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull,push,delete"] } - end - - it_behaves_like 'a modified token' - end - - describe '#access_token' do - let(:token) { described_class.access_token(%w[push], [project.full_path]) } - - subject { { token: token } } - - it_behaves_like 'a modified token' - end - - context 'with a project with a path with trailing underscore' do - let(:bad_project) { create(:project) } - - before do - bad_project.update!(path: bad_project.path + '_') - bad_project.add_developer(current_user) - end - - describe '#full_access_token' do - let(:token) { described_class.full_access_token(bad_project.full_path) } - let(:access) do - [{ 'type' => 'repository', - 'name' => bad_project.full_path, - 'actions' => ['*'], - 'migration_eligible' => false }] - end - - subject { { token: token } } - - it 'logs an exception and returns a valid access token' do - expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception) - - expect(token).to be_present - expect(payload).to be_a(Hash) - expect(payload).to include('access' => access) - end - end - end - end - - context 'when not in migration mode' do - include_context 'container registry auth service context' - - let_it_be(:project) { create(:project) } - - before do - stub_feature_flags(container_registry_migration_phase1: false) - end - - shared_examples 'an unmodified token' do - it_behaves_like 'a valid token' - it { expect(payload['access']).not_to include(have_key('migration_eligible')) } - end - - describe '#access_token' do - let(:token) { described_class.access_token(%w[push], [project.full_path]) } - - subject { { token: token } } - - it_behaves_like 'an unmodified token' - end - end end diff --git a/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb b/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb index c6b184bd003..691fb3f60f4 100644 --- a/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb +++ b/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb @@ -40,7 +40,7 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do it 'is called' do ProjectAuthorization.delete_all - expect(callback).to receive(:call).with(project.id, Gitlab::Access::MAINTAINER).once + expect(callback).to receive(:call).with(project.id, Gitlab::Access::OWNER).once service.execute end @@ -60,20 +60,7 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do to_be_removed = [project2.id] to_be_added = [ - { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER } - ] - - expect(service.execute).to eq([to_be_removed, to_be_added]) - end - - it 'finds duplicate entries that has to be removed' do - [Gitlab::Access::MAINTAINER, Gitlab::Access::REPORTER].each do |access_level| - user.project_authorizations.create!(project: project, access_level: access_level) - end - - to_be_removed = [project.id] - to_be_added = [ - { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER } + { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::OWNER } ] expect(service.execute).to eq([to_be_removed, to_be_added]) @@ -85,7 +72,7 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do to_be_removed = [project.id] to_be_added = [ - { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER } + { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::OWNER } ] expect(service.execute).to eq([to_be_removed, to_be_added]) @@ -143,16 +130,16 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do end it 'sets the keys to the project IDs' do - expect(hash.keys).to eq([project.id]) + expect(hash.keys).to match_array([project.id]) end it 'sets the values to the access levels' do - expect(hash.values).to eq([Gitlab::Access::MAINTAINER]) + expect(hash.values).to match_array([Gitlab::Access::OWNER]) end context 'personal projects' do it 'includes the project with the right access level' do - expect(hash[project.id]).to eq(Gitlab::Access::MAINTAINER) + expect(hash[project.id]).to eq(Gitlab::Access::OWNER) end end @@ -242,7 +229,7 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do value = hash.values[0] expect(value.project_id).to eq(project.id) - expect(value.access_level).to eq(Gitlab::Access::MAINTAINER) + expect(value.access_level).to eq(Gitlab::Access::OWNER) end end @@ -267,7 +254,7 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do end it 'includes the access level for every row' do - expect(row.access_level).to eq(Gitlab::Access::MAINTAINER) + expect(row.access_level).to eq(Gitlab::Access::OWNER) end end end @@ -283,7 +270,7 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do rows = service.fresh_authorizations.to_a expect(rows.length).to eq(1) - expect(rows.first.access_level).to eq(Gitlab::Access::MAINTAINER) + expect(rows.first.access_level).to eq(Gitlab::Access::OWNER) end context 'every returned row' do @@ -294,7 +281,7 @@ RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService do end it 'includes the access level' do - expect(row.access_level).to eq(Gitlab::Access::MAINTAINER) + expect(row.access_level).to eq(Gitlab::Access::OWNER) end end end diff --git a/spec/services/bulk_create_integration_service_spec.rb b/spec/services/bulk_create_integration_service_spec.rb index 63bdc39857c..68c5af33fd8 100644 --- a/spec/services/bulk_create_integration_service_spec.rb +++ b/spec/services/bulk_create_integration_service_spec.rb @@ -13,15 +13,23 @@ RSpec.describe BulkCreateIntegrationService do let_it_be(:excluded_project) { create(:project, group: excluded_group) } let(:instance_integration) { create(:jira_integration, :instance) } - let(:template_integration) { create(:jira_integration, :template) } - let(:excluded_attributes) { %w[id project_id group_id inherit_from_id instance template created_at updated_at] } + let(:excluded_attributes) do + %w[ + id project_id group_id inherit_from_id instance template + created_at updated_at + encrypted_properties encrypted_properties_iv + ] + end shared_examples 'creates integration from batch ids' do + def attributes(record) + record.reload.attributes.except(*excluded_attributes) + end + it 'updates the inherited integrations' do described_class.new(integration, batch, association).execute - expect(created_integration.attributes.except(*excluded_attributes)) - .to eq(integration.reload.attributes.except(*excluded_attributes)) + expect(attributes(created_integration)).to eq attributes(integration) end context 'integration with data fields' do @@ -30,8 +38,8 @@ RSpec.describe BulkCreateIntegrationService do it 'updates the data fields from inherited integrations' do described_class.new(integration, batch, association).execute - expect(created_integration.reload.data_fields.attributes.except(*excluded_attributes)) - .to eq(integration.reload.data_fields.attributes.except(*excluded_attributes)) + expect(attributes(created_integration.data_fields)) + .to eq attributes(integration.data_fields) end end end diff --git a/spec/services/ci/abort_pipelines_service_spec.rb b/spec/services/ci/abort_pipelines_service_spec.rb index e31a45cb123..db25faff70f 100644 --- a/spec/services/ci/abort_pipelines_service_spec.rb +++ b/spec/services/ci/abort_pipelines_service_spec.rb @@ -7,24 +7,51 @@ RSpec.describe Ci::AbortPipelinesService do let_it_be(:project) { create(:project, namespace: user.namespace) } let_it_be(:cancelable_pipeline, reload: true) { create(:ci_pipeline, :running, project: project, user: user) } - let_it_be(:manual_pipeline, reload: true) { create(:ci_pipeline, status: :manual, project: project, user: user) } # not cancelable + let_it_be(:manual_pipeline, reload: true) { create(:ci_pipeline, status: :manual, project: project, user: user) } let_it_be(:other_users_pipeline, reload: true) { create(:ci_pipeline, :running, project: project, user: create(:user)) } # not this user's pipeline + let_it_be(:cancelable_build, reload: true) { create(:ci_build, :running, pipeline: cancelable_pipeline) } let_it_be(:non_cancelable_build, reload: true) { create(:ci_build, :success, pipeline: cancelable_pipeline) } let_it_be(:cancelable_stage, reload: true) { create(:ci_stage_entity, name: 'stageA', status: :running, pipeline: cancelable_pipeline, project: project) } let_it_be(:non_cancelable_stage, reload: true) { create(:ci_stage_entity, name: 'stageB', status: :success, pipeline: cancelable_pipeline, project: project) } + let_it_be(:manual_pipeline_cancelable_build, reload: true) { create(:ci_build, :created, pipeline: manual_pipeline) } + let_it_be(:manual_pipeline_non_cancelable_build, reload: true) { create(:ci_build, :manual, pipeline: manual_pipeline) } + let_it_be(:manual_pipeline_cancelable_stage, reload: true) { create(:ci_stage_entity, name: 'stageA', status: :created, pipeline: manual_pipeline, project: project) } + let_it_be(:manual_pipeline_non_cancelable_stage, reload: true) { create(:ci_stage_entity, name: 'stageB', status: :success, pipeline: manual_pipeline, project: project) } + describe '#execute' do - def expect_correct_cancellations + def expect_correct_pipeline_cancellations expect(cancelable_pipeline.finished_at).not_to be_nil - expect(cancelable_pipeline.status).to eq('failed') - expect((cancelable_pipeline.stages - [non_cancelable_stage]).map(&:status)).to all(eq('failed')) - expect(cancelable_build.status).to eq('failed') + expect(cancelable_pipeline).to be_failed + + expect(manual_pipeline.finished_at).not_to be_nil + expect(manual_pipeline).to be_failed + end + + def expect_correct_stage_cancellations + expect(cancelable_pipeline.stages - [non_cancelable_stage]).to all(be_failed) + expect(manual_pipeline.stages - [manual_pipeline_non_cancelable_stage]).to all(be_failed) + + expect(non_cancelable_stage).not_to be_failed + expect(manual_pipeline_non_cancelable_stage).not_to be_failed + end + + def expect_correct_build_cancellations + expect(cancelable_build).to be_failed expect(cancelable_build.finished_at).not_to be_nil - expect(manual_pipeline.status).not_to eq('failed') - expect(non_cancelable_stage.status).not_to eq('failed') - expect(non_cancelable_build.status).not_to eq('failed') + expect(manual_pipeline_cancelable_build).to be_failed + expect(manual_pipeline_cancelable_build.finished_at).not_to be_nil + + expect(non_cancelable_build).not_to be_failed + expect(manual_pipeline_non_cancelable_build).not_to be_failed + end + + def expect_correct_cancellations + expect_correct_pipeline_cancellations + expect_correct_stage_cancellations + expect_correct_build_cancellations end context 'with project pipelines' do diff --git a/spec/services/ci/after_requeue_job_service_spec.rb b/spec/services/ci/after_requeue_job_service_spec.rb index d2acf3ad2f1..2f2baa15945 100644 --- a/spec/services/ci/after_requeue_job_service_spec.rb +++ b/spec/services/ci/after_requeue_job_service_spec.rb @@ -2,69 +2,236 @@ require 'spec_helper' -RSpec.describe Ci::AfterRequeueJobService do - let_it_be(:project) { create(:project) } +RSpec.describe Ci::AfterRequeueJobService, :sidekiq_inline do + let_it_be(:project) { create(:project, :empty_repo) } let_it_be(:user) { project.first_owner } - let(:pipeline) { create(:ci_pipeline, project: project) } + before_all do + project.repository.create_file(user, 'init', 'init', message: 'init', branch_name: 'master') + end - let!(:build1) { create(:ci_build, name: 'build1', pipeline: pipeline, stage_idx: 0) } - let!(:test1) { create(:ci_build, :success, name: 'test1', pipeline: pipeline, stage_idx: 1) } - let!(:test2) { create(:ci_build, :skipped, name: 'test2', pipeline: pipeline, stage_idx: 1) } - let!(:test3) { create(:ci_build, :skipped, :dependent, name: 'test3', pipeline: pipeline, stage_idx: 1, needed: build1) } - let!(:deploy) { create(:ci_build, :skipped, :dependent, name: 'deploy', pipeline: pipeline, stage_idx: 2, needed: test3) } + subject(:service) { described_class.new(project, user) } - subject(:execute_service) { described_class.new(project, user).execute(build1) } + context 'stage-dag mixed pipeline' do + let(:config) do + <<-EOY + stages: [a, b, c] - shared_examples 'processing subsequent skipped jobs' do - it 'marks subsequent skipped jobs as processable' do - expect(test1.reload).to be_success - expect(test2.reload).to be_skipped - expect(test3.reload).to be_skipped - expect(deploy.reload).to be_skipped + a1: + stage: a + script: exit $(($RANDOM % 2)) + + a2: + stage: a + script: exit 0 + needs: [a1] - execute_service + b1: + stage: b + script: exit 0 + needs: [] - expect(test1.reload).to be_success - expect(test2.reload).to be_created - expect(test3.reload).to be_created - expect(deploy.reload).to be_created + b2: + stage: b + script: exit 0 + needs: [a2] + + c1: + stage: c + script: exit 0 + needs: [b2] + + c2: + stage: c + script: exit 0 + EOY end - end - it_behaves_like 'processing subsequent skipped jobs' - - context 'when there is a job need from the same stage' do - let!(:build2) do - create(:ci_build, - :skipped, - :dependent, - name: 'build2', - pipeline: pipeline, - stage_idx: 0, - scheduling_type: :dag, - needed: build1) + let(:pipeline) do + Ci::CreatePipelineService.new(project, user, { ref: 'master' }).execute(:push).payload end - shared_examples 'processing the same stage job' do - it 'marks subsequent skipped jobs as processable' do - expect { execute_service }.to change { build2.reload.status }.from('skipped').to('created') - end + let(:a1) { find_job('a1') } + let(:b1) { find_job('b1') } + + before do + stub_ci_pipeline_yaml_file(config) + check_jobs_statuses( + a1: 'pending', + a2: 'created', + b1: 'pending', + b2: 'created', + c1: 'created', + c2: 'created' + ) + + b1.success! + check_jobs_statuses( + a1: 'pending', + a2: 'created', + b1: 'success', + b2: 'created', + c1: 'created', + c2: 'created' + ) + + a1.drop! + check_jobs_statuses( + a1: 'failed', + a2: 'skipped', + b1: 'success', + b2: 'skipped', + c1: 'skipped', + c2: 'skipped' + ) + + new_a1 = Ci::RetryBuildService.new(project, user).clone!(a1) + new_a1.enqueue! + check_jobs_statuses( + a1: 'pending', + a2: 'skipped', + b1: 'success', + b2: 'skipped', + c1: 'skipped', + c2: 'skipped' + ) end - it_behaves_like 'processing subsequent skipped jobs' - it_behaves_like 'processing the same stage job' + it 'marks subsequent skipped jobs as processable' do + execute_after_requeue_service(a1) + + check_jobs_statuses( + a1: 'pending', + a2: 'created', + b1: 'success', + b2: 'created', + c1: 'created', + c2: 'created' + ) + end end - context 'when the pipeline is a downstream pipeline and the bridge is depended' do - let!(:trigger_job) { create(:ci_bridge, :strategy_depend, name: 'trigger_job', status: 'success') } + context 'stage-dag mixed pipeline with some same-stage needs' do + let(:config) do + <<-EOY + stages: [a, b, c] + + a1: + stage: a + script: exit $(($RANDOM % 2)) + + a2: + stage: a + script: exit 0 + needs: [a1] + + b1: + stage: b + script: exit 0 + needs: [b2] + + b2: + stage: b + script: exit 0 + + c1: + stage: c + script: exit 0 + needs: [b2] + + c2: + stage: c + script: exit 0 + EOY + end + + let(:pipeline) do + Ci::CreatePipelineService.new(project, user, { ref: 'master' }).execute(:push).payload + end + + let(:a1) { find_job('a1') } before do - create(:ci_sources_pipeline, pipeline: pipeline, source_job: trigger_job) + stub_ci_pipeline_yaml_file(config) + check_jobs_statuses( + a1: 'pending', + a2: 'created', + b1: 'created', + b2: 'created', + c1: 'created', + c2: 'created' + ) + + a1.drop! + check_jobs_statuses( + a1: 'failed', + a2: 'skipped', + b1: 'skipped', + b2: 'skipped', + c1: 'skipped', + c2: 'skipped' + ) + + new_a1 = Ci::RetryBuildService.new(project, user).clone!(a1) + new_a1.enqueue! + check_jobs_statuses( + a1: 'pending', + a2: 'skipped', + b1: 'skipped', + b2: 'skipped', + c1: 'skipped', + c2: 'skipped' + ) end - it 'marks source bridge as pending' do - expect { execute_service }.to change { trigger_job.reload.status }.from('success').to('pending') + it 'marks subsequent skipped jobs as processable' do + execute_after_requeue_service(a1) + + check_jobs_statuses( + a1: 'pending', + a2: 'created', + b1: 'created', + b2: 'created', + c1: 'created', + c2: 'created' + ) + end + + context 'when the FF ci_fix_order_of_subsequent_jobs is disabled' do + before do + stub_feature_flags(ci_fix_order_of_subsequent_jobs: false) + end + + it 'does not mark b1 as processable' do + execute_after_requeue_service(a1) + + check_jobs_statuses( + a1: 'pending', + a2: 'created', + b1: 'skipped', + b2: 'created', + c1: 'created', + c2: 'created' + ) + end end end + + private + + def find_job(name) + processables.find_by!(name: name) + end + + def check_jobs_statuses(statuses) + expect(processables.order(:name).pluck(:name, :status)).to contain_exactly(*statuses.stringify_keys.to_a) + end + + def processables + pipeline.processables.latest + end + + def execute_after_requeue_service(processable) + service.execute(processable) + end end diff --git a/spec/services/ci/create_downstream_pipeline_service_spec.rb b/spec/services/ci/create_downstream_pipeline_service_spec.rb index 43eb57df66c..6142704b00e 100644 --- a/spec/services/ci/create_downstream_pipeline_service_spec.rb +++ b/spec/services/ci/create_downstream_pipeline_service_spec.rb @@ -485,14 +485,6 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end it_behaves_like 'detects cyclical pipelines' - - context 'when ci_drop_cyclical_triggered_pipelines is not enabled' do - before do - stub_feature_flags(ci_drop_cyclical_triggered_pipelines: false) - end - - it_behaves_like 'passes cyclical pipeline precondition' - end end context 'when source in the ancestry differ' do diff --git a/spec/services/ci/create_pipeline_service/artifacts_spec.rb b/spec/services/ci/create_pipeline_service/artifacts_spec.rb new file mode 100644 index 00000000000..1ec30d68666 --- /dev/null +++ b/spec/services/ci/create_pipeline_service/artifacts_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Ci::CreatePipelineService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { project.first_owner } + + let(:ref) { 'refs/heads/master' } + let(:source) { :push } + + let(:service) { described_class.new(project, user, { ref: ref }) } + let(:pipeline) { service.execute(source).payload } + + describe 'artifacts:' do + before do + stub_ci_pipeline_yaml_file(config) + allow_next_instance_of(Ci::BuildScheduleWorker) do |instance| + allow(instance).to receive(:perform).and_return(true) + end + end + + describe 'reports:' do + context 'with valid config' do + let(:config) do + <<~YAML + test-job: + script: "echo 'hello world' > cobertura.xml" + artifacts: + reports: + coverage_report: + coverage_format: 'cobertura' + path: 'cobertura.xml' + + dependency-scanning-job: + script: "echo 'hello world' > gl-dependency-scanning-report.json" + artifacts: + reports: + dependency_scanning: 'gl-dependency-scanning-report.json' + YAML + end + + it 'creates pipeline with builds' do + expect(pipeline).to be_persisted + expect(pipeline).not_to have_yaml_errors + expect(pipeline.builds.pluck(:name)).to contain_exactly('test-job', 'dependency-scanning-job') + end + end + + context 'with invalid config' do + let(:config) do + <<~YAML + test-job: + script: "echo 'hello world' > cobertura.xml" + artifacts: + reports: + foo: 'bar' + YAML + end + + it 'creates pipeline with yaml errors' do + expect(pipeline).to be_persisted + expect(pipeline).to have_yaml_errors + end + end + end + end +end diff --git a/spec/services/ci/create_pipeline_service/parameter_content_spec.rb b/spec/services/ci/create_pipeline_service/parameter_content_spec.rb index c28bc9d8c13..f593707f460 100644 --- a/spec/services/ci/create_pipeline_service/parameter_content_spec.rb +++ b/spec/services/ci/create_pipeline_service/parameter_content_spec.rb @@ -15,7 +15,7 @@ RSpec.describe Ci::CreatePipelineService do variables: DAST_VERSION: 1 - SECURE_ANALYZERS_PREFIX: "registry.gitlab.com/gitlab-org/security-products/analyzers" + SECURE_ANALYZERS_PREFIX: "registry.gitlab.com/security-products" dast: stage: dast diff --git a/spec/services/ci/create_pipeline_service/tags_spec.rb b/spec/services/ci/create_pipeline_service/tags_spec.rb index 61c2415fa33..0774f9fff2a 100644 --- a/spec/services/ci/create_pipeline_service/tags_spec.rb +++ b/spec/services/ci/create_pipeline_service/tags_spec.rb @@ -81,31 +81,6 @@ RSpec.describe Ci::CreatePipelineService do end end - context 'when the feature flag is disabled' do - before do - stub_feature_flags(ci_bulk_insert_tags: false) - end - - it 'executes N+1s queries' do - stub_yaml_config(config_without_tags) - - # warm up the cached objects so we get a more accurate count - create_pipeline - - control = ActiveRecord::QueryRecorder.new(skip_cached: false) do - create_pipeline - end - - stub_yaml_config(config) - - expect { pipeline } - .to exceed_all_query_limit(control) - .with_threshold(4) - - expect(pipeline).to be_created_successfully - end - end - context 'when tags are already persisted' do it 'does not execute N+1 queries' do # warm up the cached objects so we get a more accurate count diff --git a/spec/services/ci/destroy_secure_file_service_spec.rb b/spec/services/ci/destroy_secure_file_service_spec.rb new file mode 100644 index 00000000000..6a30d33f4ca --- /dev/null +++ b/spec/services/ci/destroy_secure_file_service_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::DestroySecureFileService do + let_it_be(:maintainer_user) { create(:user) } + let_it_be(:developer_user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:secure_file) { create(:ci_secure_file, project: project) } + let_it_be(:project_member) { create(:project_member, :maintainer, user: maintainer_user, project: project) } + let_it_be(:project_member2) { create(:project_member, :developer, user: developer_user, project: project) } + + subject { described_class.new(project, user).execute(secure_file) } + + context 'user is a maintainer' do + let(:user) { maintainer_user } + + it 'destroys the secure file' do + subject + + expect { secure_file.reload }.to raise_error(ActiveRecord::RecordNotFound) + end + end + + context 'user is a developer' do + let(:user) { developer_user } + + it 'raises an exception' do + expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError) + end + end +end diff --git a/spec/services/ci/job_artifacts/create_service_spec.rb b/spec/services/ci/job_artifacts/create_service_spec.rb index 2d309bfe425..b8487e438a9 100644 --- a/spec/services/ci/job_artifacts/create_service_spec.rb +++ b/spec/services/ci/job_artifacts/create_service_spec.rb @@ -175,7 +175,7 @@ RSpec.describe Ci::JobArtifacts::CreateService do end expect(subject[:status]).to eq(:success) - expect(job.job_variables.as_json).to contain_exactly( + expect(job.job_variables.as_json(only: [:key, :value, :source])).to contain_exactly( hash_including('key' => 'KEY1', 'value' => 'VAR1', 'source' => 'dotenv'), hash_including('key' => 'KEY2', 'value' => 'VAR2', 'source' => 'dotenv')) end diff --git a/spec/services/ci/parse_dotenv_artifact_service_spec.rb b/spec/services/ci/parse_dotenv_artifact_service_spec.rb index 6bf22b7c8b2..aaab849cd93 100644 --- a/spec/services/ci/parse_dotenv_artifact_service_spec.rb +++ b/spec/services/ci/parse_dotenv_artifact_service_spec.rb @@ -18,7 +18,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do it 'parses the artifact' do expect(subject[:status]).to eq(:success) - expect(build.job_variables.as_json).to contain_exactly( + expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly( hash_including('key' => 'KEY1', 'value' => 'VAR1'), hash_including('key' => 'KEY2', 'value' => 'VAR2')) end @@ -57,7 +57,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do expect(subject[:status]).to eq(:success) - expect(build.job_variables.as_json).to contain_exactly( + expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly( hash_including('key' => 'KEY1', 'value' => 'VAR4'), hash_including('key' => 'KEY2', 'value' => 'VAR3')) end @@ -101,7 +101,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do it 'trims the trailing space' do subject - expect(build.job_variables.as_json).to contain_exactly( + expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly( hash_including('key' => 'KEY1', 'value' => 'VAR1')) end end @@ -112,7 +112,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do it 'parses the dotenv data' do subject - expect(build.job_variables.as_json).to contain_exactly( + expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly( hash_including('key' => 'KEY', 'value' => 'VARCONTAINING=EQLS')) end end @@ -133,7 +133,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do it 'parses the dotenv data' do subject - expect(build.job_variables.as_json).to contain_exactly( + expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly( hash_including('key' => 'skateboard', 'value' => '🛹')) end end @@ -154,7 +154,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do it 'parses the dotenv data' do subject - expect(build.job_variables.as_json).to contain_exactly( + expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly( hash_including('key' => 'KEY1', 'value' => 'V A R 1')) end end @@ -165,7 +165,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do it 'parses the value as-is' do subject - expect(build.job_variables.as_json).to contain_exactly( + expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly( hash_including('key' => 'KEY1', 'value' => '"VAR1"')) end end @@ -176,7 +176,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do it 'parses the value as-is' do subject - expect(build.job_variables.as_json).to contain_exactly( + expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly( hash_including('key' => 'KEY1', 'value' => "'VAR1'")) end end @@ -187,7 +187,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do it 'parses the value as-is' do subject - expect(build.job_variables.as_json).to contain_exactly( + expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly( hash_including('key' => 'KEY1', 'value' => '" VAR1 "')) end end @@ -208,7 +208,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do it 'parses the dotenv data' do subject - expect(build.job_variables.as_json).to contain_exactly( + expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly( hash_including('key' => 'KEY1', 'value' => '')) end end @@ -250,7 +250,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do it 'does not support variable expansion in dotenv parser' do subject - expect(build.job_variables.as_json).to contain_exactly( + expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly( hash_including('key' => 'KEY1', 'value' => 'VAR1'), hash_including('key' => 'KEY2', 'value' => '${KEY1}_Test')) end @@ -284,7 +284,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do it 'does not support comment in dotenv parser' do subject - expect(build.job_variables.as_json).to contain_exactly( + expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly( hash_including('key' => 'KEY1', 'value' => 'VAR1 # This is variable')) end end diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb index 12106b70969..df1e159b5c0 100644 --- a/spec/services/ci/retry_pipeline_service_spec.rb +++ b/spec/services/ci/retry_pipeline_service_spec.rb @@ -137,7 +137,7 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do end end - context 'when the last stage was skipepd' do + context 'when the last stage was skipped' do before do create_build('build 1', :success, 0) create_build('test 2', :failed, 1) @@ -336,12 +336,32 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do expect(pipeline.reload).to be_running end end + + context 'when user is not allowed to retry build' do + before do + build = create(:ci_build, pipeline: pipeline, status: :failed) + allow_next_instance_of(Ci::RetryBuildService) do |service| + allow(service).to receive(:can?).with(user, :update_build, build).and_return(false) + end + end + + it 'returns an error' do + response = service.execute(pipeline) + + expect(response.http_status).to eq(:forbidden) + expect(response.errors).to include('403 Forbidden') + expect(pipeline.reload).not_to be_running + end + end end context 'when user is not allowed to retry pipeline' do - it 'raises an error' do - expect { service.execute(pipeline) } - .to raise_error Gitlab::Access::AccessDeniedError + it 'returns an error' do + response = service.execute(pipeline) + + expect(response.http_status).to eq(:forbidden) + expect(response.errors).to include('403 Forbidden') + expect(pipeline.reload).not_to be_running end end @@ -359,9 +379,12 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do create_build('verify', :canceled, 1) end - it 'raises an error' do - expect { service.execute(pipeline) } - .to raise_error Gitlab::Access::AccessDeniedError + it 'returns an error' do + response = service.execute(pipeline) + + expect(response.http_status).to eq(:forbidden) + expect(response.errors).to include('403 Forbidden') + expect(pipeline.reload).not_to be_running end end @@ -372,9 +395,12 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do create_build('verify', :canceled, 2) end - it 'raises an error' do - expect { service.execute(pipeline) } - .to raise_error Gitlab::Access::AccessDeniedError + it 'returns an error' do + response = service.execute(pipeline) + + expect(response.http_status).to eq(:forbidden) + expect(response.errors).to include('403 Forbidden') + expect(pipeline.reload).not_to be_running end end end diff --git a/spec/services/ci/runners/assign_runner_service_spec.rb b/spec/services/ci/runners/assign_runner_service_spec.rb new file mode 100644 index 00000000000..00b176bb759 --- /dev/null +++ b/spec/services/ci/runners/assign_runner_service_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::Runners::AssignRunnerService, '#execute' do + subject { described_class.new(runner, project, user).execute } + + let_it_be(:runner) { create(:ci_runner, :project, projects: [project]) } + let_it_be(:project) { create(:project) } + + context 'without user' do + let(:user) { nil } + + it 'does not call assign_to on runner and returns false' do + expect(runner).not_to receive(:assign_to) + + is_expected.to eq(false) + end + end + + context 'with unauthorized user' do + let(:user) { build(:user) } + + it 'does not call assign_to on runner and returns false' do + expect(runner).not_to receive(:assign_to) + + is_expected.to eq(false) + end + end + + context 'with admin user', :enable_admin_mode do + let(:user) { create_default(:user, :admin) } + + it 'calls assign_to on runner and returns value unchanged' do + expect(runner).to receive(:assign_to).with(project, user).once.and_return('assign_to return value') + + is_expected.to eq('assign_to return value') + end + end +end diff --git a/spec/services/ci/register_runner_service_spec.rb b/spec/services/ci/runners/register_runner_service_spec.rb index 491582bbd13..f43fd823078 100644 --- a/spec/services/ci/register_runner_service_spec.rb +++ b/spec/services/ci/runners/register_runner_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ::Ci::RegisterRunnerService, '#execute' do +RSpec.describe ::Ci::Runners::RegisterRunnerService, '#execute' do let(:registration_token) { 'abcdefg123456' } let(:token) { } let(:args) { {} } diff --git a/spec/services/ci/runners/reset_registration_token_service_spec.rb b/spec/services/ci/runners/reset_registration_token_service_spec.rb new file mode 100644 index 00000000000..c4bfff51cc8 --- /dev/null +++ b/spec/services/ci/runners/reset_registration_token_service_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::Runners::ResetRegistrationTokenService, '#execute' do + subject { described_class.new(scope, current_user).execute } + + let_it_be(:user) { build(:user) } + let_it_be(:admin_user) { create(:user, :admin) } + + shared_examples 'a registration token reset operation' do + context 'without user' do + let(:current_user) { nil } + + it 'does not reset registration token and returns nil' do + expect(scope).not_to receive(token_reset_method_name) + + is_expected.to be_nil + end + end + + context 'with unauthorized user' do + let(:current_user) { user } + + it 'does not reset registration token and returns nil' do + expect(scope).not_to receive(token_reset_method_name) + + is_expected.to be_nil + end + end + + context 'with admin user', :enable_admin_mode do + let(:current_user) { admin_user } + + it 'resets registration token and returns value unchanged' do + expect(scope).to receive(token_reset_method_name).once do + expect(scope).to receive(token_method_name).once.and_return("#{token_method_name} return value") + end + + is_expected.to eq("#{token_method_name} return value") + end + end + end + + context 'with instance scope' do + let_it_be(:scope) { create(:application_setting) } + + before do + allow(ApplicationSetting).to receive(:current).and_return(scope) + allow(ApplicationSetting).to receive(:current_without_cache).and_return(scope) + end + + it_behaves_like 'a registration token reset operation' do + let(:token_method_name) { :runners_registration_token } + let(:token_reset_method_name) { :reset_runners_registration_token! } + end + end + + context 'with group scope' do + let_it_be(:scope) { create(:group) } + + it_behaves_like 'a registration token reset operation' do + let(:token_method_name) { :runners_token } + let(:token_reset_method_name) { :reset_runners_token! } + end + end + + context 'with project scope' do + let_it_be(:scope) { create(:project) } + + it_behaves_like 'a registration token reset operation' do + let(:token_method_name) { :runners_token } + let(:token_reset_method_name) { :reset_runners_token! } + end + end +end diff --git a/spec/services/ci/runners/unassign_runner_service_spec.rb b/spec/services/ci/runners/unassign_runner_service_spec.rb new file mode 100644 index 00000000000..3fb6925f4bd --- /dev/null +++ b/spec/services/ci/runners/unassign_runner_service_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::Runners::UnassignRunnerService, '#execute' do + subject(:service) { described_class.new(runner_project, user).execute } + + let_it_be(:runner) { create(:ci_runner, :project, projects: [project]) } + let_it_be(:project) { create(:project) } + + let(:runner_project) { runner.runner_projects.last } + + context 'without user' do + let(:user) { nil } + + it 'does not destroy runner_project', :aggregate_failures do + expect(runner_project).not_to receive(:destroy) + expect { service }.not_to change { runner.runner_projects.count }.from(1) + + is_expected.to eq(false) + end + end + + context 'with unauthorized user' do + let(:user) { build(:user) } + + it 'does not call destroy on runner_project' do + expect(runner).not_to receive(:destroy) + + service + end + end + + context 'with admin user', :enable_admin_mode do + let(:user) { create_default(:user, :admin) } + + it 'destroys runner_project' do + expect(runner_project).to receive(:destroy).once + + service + end + end +end diff --git a/spec/services/ci/unregister_runner_service_spec.rb b/spec/services/ci/runners/unregister_runner_service_spec.rb index f427e04f228..df1a0a90067 100644 --- a/spec/services/ci/unregister_runner_service_spec.rb +++ b/spec/services/ci/runners/unregister_runner_service_spec.rb @@ -2,8 +2,8 @@ require 'spec_helper' -RSpec.describe ::Ci::UnregisterRunnerService, '#execute' do - subject { described_class.new(runner).execute } +RSpec.describe ::Ci::Runners::UnregisterRunnerService, '#execute' do + subject { described_class.new(runner, 'some_token').execute } let(:runner) { create(:ci_runner) } diff --git a/spec/services/ci/update_runner_service_spec.rb b/spec/services/ci/runners/update_runner_service_spec.rb index eee80bfef47..b02ea8f58b0 100644 --- a/spec/services/ci/update_runner_service_spec.rb +++ b/spec/services/ci/runners/update_runner_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::UpdateRunnerService do +RSpec.describe Ci::Runners::UpdateRunnerService do let(:runner) { create(:ci_runner) } describe '#update' do diff --git a/spec/services/concerns/rate_limited_service_spec.rb b/spec/services/concerns/rate_limited_service_spec.rb index 97f5ca53c0d..04007e8e75a 100644 --- a/spec/services/concerns/rate_limited_service_spec.rb +++ b/spec/services/concerns/rate_limited_service_spec.rb @@ -36,79 +36,28 @@ RSpec.describe RateLimitedService do subject { described_class::RateLimiterScopedAndKeyed.new(key: key, opts: opts, rate_limiter: rate_limiter) } describe '#rate_limit!' do - let(:project_with_feature_enabled) { create(:project) } - let(:project_without_feature_enabled) { create(:project) } + let_it_be(:project) { create(:project) } + let_it_be(:current_user) { create(:user) } - let(:project) { nil } - - let(:current_user) { create(:user) } let(:service) { instance_double(Issues::CreateService, project: project, current_user: current_user) } let(:evaluated_scope) { [project, current_user] } let(:evaluated_opts) { { scope: evaluated_scope, users_allowlist: %w[support-bot] } } - let(:rate_limited_service_issues_create_feature_enabled) { nil } - - before do - stub_feature_flags(rate_limited_service_issues_create: rate_limited_service_issues_create_feature_enabled) - end - shared_examples 'a service that does not attempt to throttle' do - it 'does not attempt to throttle' do - expect(rate_limiter).not_to receive(:throttled?) + context 'when rate limiting is not in effect' do + let(:throttled) { false } + it 'does not raise an exception' do expect(subject.rate_limit!(service)).to be_nil end end - shared_examples 'a service that does attempt to throttle' do + context 'when rate limiting is in effect' do before do - allow(rate_limiter).to receive(:throttled?).and_return(throttled) - end - - context 'when rate limiting is not in effect' do - let(:throttled) { false } - - it 'does not raise an exception' do - expect(subject.rate_limit!(service)).to be_nil - end - end - - context 'when rate limiting is in effect' do - let(:throttled) { true } - - it 'raises a RateLimitedError exception' do - expect { subject.rate_limit!(service) }.to raise_error(described_class::RateLimitedError, 'This endpoint has been requested too many times. Try again later.') - end + allow(rate_limiter).to receive(:throttled?).and_return(true) end - end - - context 'when :rate_limited_service_issues_create feature is globally disabled' do - let(:rate_limited_service_issues_create_feature_enabled) { false } - - it_behaves_like 'a service that does not attempt to throttle' - end - - context 'when :rate_limited_service_issues_create feature is globally enabled' do - let(:throttled) { nil } - let(:rate_limited_service_issues_create_feature_enabled) { true } - let(:project) { project_without_feature_enabled } - - it_behaves_like 'a service that does attempt to throttle' - end - - context 'when :rate_limited_service_issues_create feature is enabled for project_with_feature_enabled' do - let(:throttled) { nil } - let(:rate_limited_service_issues_create_feature_enabled) { project_with_feature_enabled } - - context 'for project_without_feature_enabled' do - let(:project) { project_without_feature_enabled } - - it_behaves_like 'a service that does not attempt to throttle' - end - - context 'for project_with_feature_enabled' do - let(:project) { project_with_feature_enabled } - it_behaves_like 'a service that does attempt to throttle' + it 'raises a RateLimitedError exception' do + expect { subject.rate_limit!(service) }.to raise_error(described_class::RateLimitedError, 'This endpoint has been requested too many times. Try again later.') end end end diff --git a/spec/services/error_tracking/base_service_spec.rb b/spec/services/error_tracking/base_service_spec.rb index ffbda37d417..2f2052f0189 100644 --- a/spec/services/error_tracking/base_service_spec.rb +++ b/spec/services/error_tracking/base_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe ErrorTracking::BaseService do describe '#compose_response' do let(:project) { double('project') } - let(:user) { double('user') } + let(:user) { double('user', id: non_existing_record_id) } let(:service) { described_class.new(project, user) } it 'returns bad_request error when response has an error key' do @@ -68,6 +68,16 @@ RSpec.describe ErrorTracking::BaseService do expect(result[:animal]).to eq(:fish) expect(result[:status]).to eq(:success) end + + context 'when tracking_event is provided' do + let(:service) { described_class.new(project, user, tracking_event: :error_tracking_view_list) } + + it_behaves_like 'tracking unique hll events' do + let(:target_event) { 'error_tracking_view_list' } + let(:expected_value) { non_existing_record_id } + let(:request) { service.send(:compose_response, data) } + end + end end end end diff --git a/spec/services/error_tracking/collect_error_service_spec.rb b/spec/services/error_tracking/collect_error_service_spec.rb index 2b16612dac3..faca3c12a48 100644 --- a/spec/services/error_tracking/collect_error_service_spec.rb +++ b/spec/services/error_tracking/collect_error_service_spec.rb @@ -51,25 +51,30 @@ RSpec.describe ErrorTracking::CollectErrorService do end end - context 'unusual payload' do + context 'with unusual payload' do let(:modified_event) { parsed_event } + let(:event) { described_class.new(project, nil, event: modified_event).execute } - context 'missing transaction' do + context 'when transaction is missing' do it 'builds actor from stacktrace' do modified_event.delete('transaction') - event = described_class.new(project, nil, event: modified_event).execute + expect(event.error.actor).to eq 'find()' + end + end + + context 'when transaction is an empty string' do \ + it 'builds actor from stacktrace' do + modified_event['transaction'] = '' expect(event.error.actor).to eq 'find()' end end - context 'timestamp is numeric' do + context 'when timestamp is numeric' do it 'parses timestamp' do modified_event['timestamp'] = '1631015580.50' - event = described_class.new(project, nil, event: modified_event).execute - expect(event.occurred_at).to eq '2021-09-07T11:53:00.5' end end diff --git a/spec/services/google_cloud/create_service_accounts_service_spec.rb b/spec/services/google_cloud/create_service_accounts_service_spec.rb index 53d21df713a..3f500e7c235 100644 --- a/spec/services/google_cloud/create_service_accounts_service_spec.rb +++ b/spec/services/google_cloud/create_service_accounts_service_spec.rb @@ -26,6 +26,8 @@ RSpec.describe GoogleCloud::CreateServiceAccountsService do end it 'creates unprotected vars', :aggregate_failures do + allow(ProtectedBranch).to receive(:protected?).and_return(false) + project = create(:project) service = described_class.new( @@ -45,5 +47,28 @@ RSpec.describe GoogleCloud::CreateServiceAccountsService do expect(project.variables.second.protected).to eq(false) expect(project.variables.third.protected).to eq(false) end + + it 'creates protected vars', :aggregate_failures do + allow(ProtectedBranch).to receive(:protected?).and_return(true) + + project = create(:project) + + service = described_class.new( + project, + nil, + google_oauth2_token: 'mock-token', + gcp_project_id: 'mock-gcp-project-id', + environment_name: '*' + ) + + response = service.execute + + expect(response.status).to eq(:success) + expect(response.message).to eq('Service account generated successfully') + expect(project.variables.count).to eq(3) + expect(project.variables.first.protected).to eq(true) + expect(project.variables.second.protected).to eq(true) + expect(project.variables.third.protected).to eq(true) + end end end diff --git a/spec/services/google_cloud/gcp_region_add_or_replace_service_spec.rb b/spec/services/google_cloud/gcp_region_add_or_replace_service_spec.rb new file mode 100644 index 00000000000..e2f5a2e719e --- /dev/null +++ b/spec/services/google_cloud/gcp_region_add_or_replace_service_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GoogleCloud::GcpRegionAddOrReplaceService do + it 'adds and replaces GCP region vars' do + project = create(:project, :public) + service = described_class.new(project) + + service.execute('env_1', 'loc_1') + service.execute('env_2', 'loc_2') + service.execute('env_1', 'loc_3') + + list = project.variables.reload.filter { |variable| variable.key == Projects::GoogleCloudController::GCP_REGION_CI_VAR_KEY } + list = list.sort_by(&:environment_scope) + + aggregate_failures 'testing list of gcp regions' do + expect(list.length).to eq(2) + + # asserting that the first region is replaced + expect(list.first.environment_scope).to eq('env_1') + expect(list.first.value).to eq('loc_3') + + expect(list.second.environment_scope).to eq('env_2') + expect(list.second.value).to eq('loc_2') + end + end +end diff --git a/spec/services/google_cloud/service_accounts_service_spec.rb b/spec/services/google_cloud/service_accounts_service_spec.rb index 17c1f61a96e..10e387126a3 100644 --- a/spec/services/google_cloud/service_accounts_service_spec.rb +++ b/spec/services/google_cloud/service_accounts_service_spec.rb @@ -37,17 +37,17 @@ RSpec.describe GoogleCloud::ServiceAccountsService do aggregate_failures 'testing list of service accounts' do expect(list.length).to eq(3) - expect(list.first[:environment]).to eq('*') + expect(list.first[:ref]).to eq('*') expect(list.first[:gcp_project]).to eq('prj1') expect(list.first[:service_account_exists]).to eq(false) expect(list.first[:service_account_key_exists]).to eq(true) - expect(list.second[:environment]).to eq('staging') + expect(list.second[:ref]).to eq('staging') expect(list.second[:gcp_project]).to eq('prj2') expect(list.second[:service_account_exists]).to eq(true) expect(list.second[:service_account_key_exists]).to eq(false) - expect(list.third[:environment]).to eq('production') + expect(list.third[:ref]).to eq('production') expect(list.third[:gcp_project]).to eq('prj3') expect(list.third[:service_account_exists]).to eq(true) expect(list.third[:service_account_key_exists]).to eq(true) @@ -68,12 +68,12 @@ RSpec.describe GoogleCloud::ServiceAccountsService do aggregate_failures 'testing list of service accounts' do expect(list.length).to eq(2) - expect(list.first[:environment]).to eq('env_1') + expect(list.first[:ref]).to eq('env_1') expect(list.first[:gcp_project]).to eq('gcp_prj_id_1') expect(list.first[:service_account_exists]).to eq(true) expect(list.first[:service_account_key_exists]).to eq(true) - expect(list.second[:environment]).to eq('env_2') + expect(list.second[:ref]).to eq('env_2') expect(list.second[:gcp_project]).to eq('gcp_prj_id_2') expect(list.second[:service_account_exists]).to eq(true) expect(list.second[:service_account_key_exists]).to eq(true) @@ -89,12 +89,12 @@ RSpec.describe GoogleCloud::ServiceAccountsService do expect(list.length).to eq(2) # asserting that the first service account is replaced - expect(list.first[:environment]).to eq('env_1') + expect(list.first[:ref]).to eq('env_1') expect(list.first[:gcp_project]).to eq('new_project') expect(list.first[:service_account_exists]).to eq(true) expect(list.first[:service_account_key_exists]).to eq(true) - expect(list.second[:environment]).to eq('env_2') + expect(list.second[:ref]).to eq('env_2') expect(list.second[:gcp_project]).to eq('gcp_prj_id_2') expect(list.second[:service_account_exists]).to eq(true) expect(list.second[:service_account_key_exists]).to eq(true) diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index 7ec523a1f2b..819569d6e67 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -85,14 +85,6 @@ RSpec.describe Groups::CreateService, '#execute' do context 'with before_commit callback' do it_behaves_like 'has sync-ed traversal_ids' end - - context 'with after_create callback' do - before do - stub_feature_flags(sync_traversal_ids_before_commit: false) - end - - it_behaves_like 'has sync-ed traversal_ids' - end end context 'when user can not create a group' do @@ -119,17 +111,7 @@ RSpec.describe Groups::CreateService, '#execute' do expect { subject }.not_to change(OnboardingProgress, :count).from(0) end - context 'with before_commit callback' do - it_behaves_like 'has sync-ed traversal_ids' - end - - context 'with after_create callback' do - before do - stub_feature_flags(sync_traversal_ids_before_commit: false) - end - - it_behaves_like 'has sync-ed traversal_ids' - end + it_behaves_like 'has sync-ed traversal_ids' end context 'as guest' do diff --git a/spec/services/groups/deploy_tokens/revoke_service_spec.rb b/spec/services/groups/deploy_tokens/revoke_service_spec.rb new file mode 100644 index 00000000000..fcf11bbb8e6 --- /dev/null +++ b/spec/services/groups/deploy_tokens/revoke_service_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Groups::DeployTokens::RevokeService do + let_it_be(:entity) { create(:group) } + let_it_be(:deploy_token) { create(:deploy_token, :group, groups: [entity]) } + let_it_be(:user) { create(:user) } + let_it_be(:deploy_token_params) { { id: deploy_token.id } } + + describe '#execute' do + subject { described_class.new(entity, user, deploy_token_params).execute } + + it "revokes a group deploy token" do + expect(deploy_token.revoked).to eq(false) + + expect { subject }.to change { deploy_token.reload.revoked }.to eq(true) + end + + context 'invalid token id' do + let(:deploy_token_params) { { token_id: non_existing_record_id } } + + it 'raises an error' do + expect { subject }.to raise_error(ActiveRecord::RecordNotFound) + end + end + end +end diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index 5135be8fff5..628943e40ff 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' RSpec.describe Groups::DestroyService do - include DatabaseConnectionHelpers - let!(:user) { create(:user) } let!(:group) { create(:group) } let!(:nested_group) { create(:group, parent: group) } @@ -112,6 +110,17 @@ RSpec.describe Groups::DestroyService do end end + context 'when group owner is blocked' do + before do + user.block! + end + + it 'returns a more descriptive error message' do + expect { destroy_group(group, user, false) } + .to raise_error(Groups::DestroyService::DestroyError, "You can't delete this group because you're blocked.") + end + end + describe 'repository removal' do before do destroy_group(group, user, false) diff --git a/spec/services/import/gitlab_projects/create_project_from_remote_file_service_spec.rb b/spec/services/import/gitlab_projects/create_project_from_remote_file_service_spec.rb deleted file mode 100644 index 92c46cf7052..00000000000 --- a/spec/services/import/gitlab_projects/create_project_from_remote_file_service_spec.rb +++ /dev/null @@ -1,201 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ::Import::GitlabProjects::CreateProjectFromRemoteFileService do - let(:remote_url) { 'https://external.file.path/file' } - - let(:params) do - { - path: 'path', - namespace: user.namespace, - name: 'name', - remote_import_url: remote_url - } - end - - let_it_be(:user) { create(:user) } - - subject { described_class.new(user, params) } - - shared_examples 'successfully import' do |content_type| - it 'creates a project and returns a successful response' do - stub_headers_for(remote_url, { - 'content-type' => content_type, - 'content-length' => '10' - }) - - response = nil - expect { response = subject.execute } - .to change(Project, :count).by(1) - - expect(response).to be_success - expect(response.http_status).to eq(:ok) - expect(response.payload).to be_instance_of(Project) - expect(response.payload.name).to eq('name') - expect(response.payload.path).to eq('path') - expect(response.payload.namespace).to eq(user.namespace) - end - end - - it_behaves_like 'successfully import', 'application/gzip' - it_behaves_like 'successfully import', 'application/x-tar' - - context 'when the file url is invalid' do - it 'returns an erred response with the reason of the failure' do - stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) - - params[:remote_import_url] = 'https://localhost/file' - - response = nil - expect { response = subject.execute } - .not_to change(Project, :count) - - expect(response).not_to be_success - expect(response.http_status).to eq(:bad_request) - expect(response.message).to eq('Requests to localhost are not allowed') - end - end - - context 'validate file type' do - it 'returns erred response when the file type is not informed' do - stub_headers_for(remote_url, { 'content-length' => '10' }) - - response = nil - expect { response = subject.execute } - .not_to change(Project, :count) - - expect(response).not_to be_success - expect(response.http_status).to eq(:bad_request) - expect(response.message) - .to eq("Missing 'ContentType' header") - end - - it 'returns erred response when the file type is not allowed' do - stub_headers_for(remote_url, { - 'content-type' => 'application/js', - 'content-length' => '10' - }) - - response = nil - expect { response = subject.execute } - .not_to change(Project, :count) - - expect(response).not_to be_success - expect(response.http_status).to eq(:bad_request) - expect(response.message) - .to eq("Remote file content type 'application/js' not allowed. (Allowed content types: application/gzip, application/x-tar)") - end - end - - context 'validate content type' do - it 'returns erred response when the file size is not informed' do - stub_headers_for(remote_url, { 'content-type' => 'application/gzip' }) - - response = nil - expect { response = subject.execute } - .not_to change(Project, :count) - - expect(response).not_to be_success - expect(response.http_status).to eq(:bad_request) - expect(response.message) - .to eq("Missing 'ContentLength' header") - end - - it 'returns error response when the file size is a text' do - stub_headers_for(remote_url, { - 'content-type' => 'application/gzip', - 'content-length' => 'some text' - }) - - response = nil - expect { response = subject.execute } - .not_to change(Project, :count) - - expect(response).not_to be_success - expect(response.http_status).to eq(:bad_request) - expect(response.message) - .to eq("Missing 'ContentLength' header") - end - - it 'returns erred response when the file is larger then allowed' do - stub_headers_for(remote_url, { - 'content-type' => 'application/gzip', - 'content-length' => 11.gigabytes.to_s - }) - - response = nil - expect { response = subject.execute } - .not_to change(Project, :count) - - expect(response).not_to be_success - expect(response.http_status).to eq(:bad_request) - expect(response.message) - .to eq('Remote file larger than limit. (limit 10 GB)') - end - end - - it 'does not validate content-type or content-length when the file is stored in AWS-S3' do - stub_headers_for(remote_url, { - 'Server' => 'AmazonS3', - 'x-amz-request-id' => 'Something' - }) - - response = nil - expect { response = subject.execute } - .to change(Project, :count) - - expect(response).to be_success - expect(response.http_status).to eq(:ok) - end - - context 'when required parameters are not provided' do - let(:params) { {} } - - it 'returns an erred response with the reason of the failure' do - stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) - - response = nil - expect { response = subject.execute } - .not_to change(Project, :count) - - expect(response).not_to be_success - expect(response.http_status).to eq(:bad_request) - expect(response.message).to eq("Parameter 'path' is required") - - expect(subject.errors.full_messages).to match_array([ - "Missing 'ContentLength' header", - "Missing 'ContentType' header", - "Parameter 'namespace' is required", - "Parameter 'path' is required", - "Parameter 'remote_import_url' is required" - ]) - end - end - - context 'when the project is invalid' do - it 'returns an erred response with the reason of the failure' do - create(:project, namespace: user.namespace, path: 'path') - - stub_headers_for(remote_url, { - 'content-type' => 'application/gzip', - 'content-length' => '10' - }) - - response = nil - expect { response = subject.execute } - .not_to change(Project, :count) - - expect(response).not_to be_success - expect(response.http_status).to eq(:bad_request) - expect(response.message).to eq('Path has already been taken') - end - end - - def stub_headers_for(url, headers = {}) - allow(Gitlab::HTTP) - .to receive(:head) - .with(url) - .and_return(double(headers: headers)) - end -end diff --git a/spec/services/import/gitlab_projects/create_project_from_uploaded_file_service_spec.rb b/spec/services/import/gitlab_projects/create_project_from_uploaded_file_service_spec.rb deleted file mode 100644 index a0e04a9a696..00000000000 --- a/spec/services/import/gitlab_projects/create_project_from_uploaded_file_service_spec.rb +++ /dev/null @@ -1,71 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ::Import::GitlabProjects::CreateProjectFromUploadedFileService do - let(:file_upload) do - fixture_file_upload('spec/features/projects/import_export/test_project_export.tar.gz') - end - - let(:params) do - { - path: 'path', - namespace: user.namespace, - name: 'name', - file: file_upload - } - end - - let_it_be(:user) { create(:user) } - - subject { described_class.new(user, params) } - - it 'creates a project and returns a successful response' do - response = nil - expect { response = subject.execute } - .to change(Project, :count).by(1) - - expect(response).to be_success - expect(response.http_status).to eq(:ok) - expect(response.payload).to be_instance_of(Project) - expect(response.payload.name).to eq('name') - expect(response.payload.path).to eq('path') - expect(response.payload.namespace).to eq(user.namespace) - end - - context 'when required parameters are not provided' do - let(:params) { {} } - - it 'returns an erred response with the reason of the failure' do - stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) - - response = nil - expect { response = subject.execute } - .not_to change(Project, :count) - - expect(response).not_to be_success - expect(response.http_status).to eq(:bad_request) - expect(response.message).to eq("Parameter 'path' is required") - - expect(subject.errors.full_messages).to match_array([ - "Parameter 'namespace' is required", - "Parameter 'path' is required", - "Parameter 'file' is required" - ]) - end - end - - context 'when the project is invalid' do - it 'returns an erred response with the reason of the failure' do - create(:project, namespace: user.namespace, path: 'path') - - response = nil - expect { response = subject.execute } - .not_to change(Project, :count) - - expect(response).not_to be_success - expect(response.http_status).to eq(:bad_request) - expect(response.message).to eq('Path has already been taken') - end - end -end diff --git a/spec/services/import/gitlab_projects/create_project_service_spec.rb b/spec/services/import/gitlab_projects/create_project_service_spec.rb new file mode 100644 index 00000000000..0da897448b8 --- /dev/null +++ b/spec/services/import/gitlab_projects/create_project_service_spec.rb @@ -0,0 +1,179 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Import::GitlabProjects::CreateProjectService, :aggregate_failures do + let(:fake_file_acquisition_strategy) do + Class.new do + attr_reader :errors + + def initialize(...) + @errors = ActiveModel::Errors.new(self) + end + + def valid? + true + end + + def project_params + {} + end + end + end + + let(:params) do + { + path: 'path', + namespace: user.namespace, + name: 'name' + } + end + + let_it_be(:user) { create(:user) } + + subject { described_class.new(user, params: params, file_acquisition_strategy: FakeStrategy) } + + before do + stub_const('FakeStrategy', fake_file_acquisition_strategy) + end + + describe 'validation' do + it { expect(subject).to be_valid } + + it 'validates presence of path' do + params[:path] = nil + + invalid = described_class.new(user, params: params, file_acquisition_strategy: FakeStrategy) + + expect(invalid).not_to be_valid + expect(invalid.errors.full_messages).to include("Path can't be blank") + end + + it 'validates presence of name' do + params[:namespace] = nil + + invalid = described_class.new(user, params: params, file_acquisition_strategy: FakeStrategy) + + expect(invalid).not_to be_valid + expect(invalid.errors.full_messages).to include("Namespace can't be blank") + end + + it 'is invalid if the strategy is invalid' do + expect_next_instance_of(FakeStrategy) do |strategy| + allow(strategy).to receive(:valid?).and_return(false) + allow(strategy).to receive(:errors).and_wrap_original do |original| + original.call.tap do |errors| + errors.add(:base, "some error") + end + end + end + + invalid = described_class.new(user, params: params, file_acquisition_strategy: FakeStrategy) + + expect(invalid).not_to be_valid + expect(invalid.errors.full_messages).to include("some error") + expect(invalid.errors.full_messages).to include("some error") + end + end + + describe '#execute' do + it 'creates a project successfully' do + response = nil + expect { response = subject.execute } + .to change(Project, :count).by(1) + + expect(response).to be_success + expect(response.http_status).to eq(:ok) + expect(response.payload).to be_instance_of(Project) + expect(response.payload.name).to eq('name') + expect(response.payload.path).to eq('path') + expect(response.payload.namespace).to eq(user.namespace) + + project = Project.last + expect(project.name).to eq('name') + expect(project.path).to eq('path') + expect(project.namespace_id).to eq(user.namespace.id) + expect(project.import_type).to eq('gitlab_project') + end + + context 'when the project creation raises an error' do + it 'fails to create a project' do + expect_next_instance_of(Projects::GitlabProjectsImportService) do |service| + expect(service).to receive(:execute).and_raise(StandardError, "failed to create project") + end + + response = nil + expect { response = subject.execute } + .to change(Project, :count).by(0) + + expect(response).to be_error + expect(response.http_status).to eq(:bad_request) + expect(response.message).to eq("failed to create project") + expect(response.payload).to eq(other_errors: []) + end + end + + context 'when the validation fail' do + it 'fails to create a project' do + params.delete(:path) + + response = nil + expect { response = subject.execute } + .to change(Project, :count).by(0) + + expect(response).to be_error + expect(response.http_status).to eq(:bad_request) + expect(response.message).to eq("Path can't be blank") + expect(response.payload).to eq(other_errors: []) + end + + context 'when the project contains multilple errors' do + it 'fails to create a project' do + params.merge!(name: '_ an invalid name _', path: '_ an invalid path _') + + response = nil + expect { response = subject.execute } + .to change(Project, :count).by(0) + + expect(response).to be_error + expect(response.http_status).to eq(:bad_request) + expect(response.message) + .to eq(%{Project namespace path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-', end in '.git' or end in '.atom'}) + expect(response.payload).to eq(other_errors: [ + %{Path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-', end in '.git' or end in '.atom'}, + %{Path must not start or end with a special character and must not contain consecutive special characters.} + ]) + end + end + end + + context 'when the strategy adds project parameters' do + before do + expect_next_instance_of(FakeStrategy) do |strategy| + expect(strategy).to receive(:project_params).and_return(name: 'the strategy name') + end + + subject.valid? + end + + it 'merges the strategy project parameters' do + response = nil + expect { response = subject.execute } + .to change(Project, :count).by(1) + + expect(response).to be_success + expect(response.http_status).to eq(:ok) + expect(response.payload).to be_instance_of(Project) + expect(response.payload.name).to eq('the strategy name') + expect(response.payload.path).to eq('path') + expect(response.payload.namespace).to eq(user.namespace) + + project = Project.last + expect(project.name).to eq('the strategy name') + expect(project.path).to eq('path') + expect(project.namespace_id).to eq(user.namespace.id) + expect(project.import_type).to eq('gitlab_project') + end + end + end +end diff --git a/spec/services/import/gitlab_projects/file_acquisition_strategies/file_upload_spec.rb b/spec/services/import/gitlab_projects/file_acquisition_strategies/file_upload_spec.rb new file mode 100644 index 00000000000..28af6219812 --- /dev/null +++ b/spec/services/import/gitlab_projects/file_acquisition_strategies/file_upload_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Import::GitlabProjects::FileAcquisitionStrategies::FileUpload, :aggregate_failures do + let(:file) { UploadedFile.new( File.join('spec', 'features', 'projects', 'import_export', 'test_project_export.tar.gz') ) } + + describe 'validation' do + it 'validates presence of file' do + valid = described_class.new(params: { file: file }) + expect(valid).to be_valid + + invalid = described_class.new(params: {}) + expect(invalid).not_to be_valid + expect(invalid.errors.full_messages).to include("File must be uploaded") + end + end + + describe '#project_params' do + it 'returns the file to upload in the params' do + subject = described_class.new(params: { file: file }) + + expect(subject.project_params).to eq(file: file) + end + end +end diff --git a/spec/services/import/gitlab_projects/file_acquisition_strategies/remote_file_s3_spec.rb b/spec/services/import/gitlab_projects/file_acquisition_strategies/remote_file_s3_spec.rb new file mode 100644 index 00000000000..d9042e95149 --- /dev/null +++ b/spec/services/import/gitlab_projects/file_acquisition_strategies/remote_file_s3_spec.rb @@ -0,0 +1,136 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Import::GitlabProjects::FileAcquisitionStrategies::RemoteFileS3, :aggregate_failures do + let(:region_name) { 'region_name' } + let(:bucket_name) { 'bucket_name' } + let(:file_key) { 'file_key' } + let(:access_key_id) { 'access_key_id' } + let(:secret_access_key) { 'secret_access_key' } + let(:file_exists) { true } + let(:content_type) { 'application/x-tar' } + let(:content_length) { 2.gigabytes } + let(:presigned_url) { 'https://external.file.path/file.tar.gz?PRESIGNED=true&TOKEN=some-token' } + + let(:s3_double) do + instance_double( + Aws::S3::Object, + exists?: file_exists, + content_type: content_type, + content_length: content_length, + presigned_url: presigned_url + ) + end + + let(:params) do + { + region: region_name, + bucket_name: bucket_name, + file_key: file_key, + access_key_id: access_key_id, + secret_access_key: secret_access_key + } + end + + subject { described_class.new(params: params) } + + before do + # Avoid network requests + expect(Aws::S3::Client).to receive(:new).and_return(double) + expect(Aws::S3::Object).to receive(:new).and_return(s3_double) + end + + describe 'validation' do + it { expect(subject).to be_valid } + + %i[region bucket_name file_key access_key_id secret_access_key].each do |key| + context "#{key} validation" do + before do + params[key] = nil + end + + it "validates presence of #{key}" do + expect(subject).not_to be_valid + expect(subject.errors.full_messages) + .to include("#{key.to_s.humanize} can't be blank") + end + end + end + + context 'content-length validation' do + let(:content_length) { 11.gigabytes } + + it 'validates the remote content-length' do + expect(subject).not_to be_valid + expect(subject.errors.full_messages) + .to include('Content length is too big (should be at most 10 GB)') + end + end + + context 'content-type validation' do + let(:content_type) { 'unknown' } + + it 'validates the remote content-type' do + expect(subject).not_to be_valid + expect(subject.errors.full_messages) + .to include("Content type 'unknown' not allowed. (Allowed: application/gzip, application/x-tar, application/x-gzip)") + end + end + + context 'file_url validation' do + let(:presigned_url) { 'ftp://invalid.url/file.tar.gz' } + + it 'validates the file_url scheme' do + expect(subject).not_to be_valid + expect(subject.errors.full_messages) + .to include("File url is blocked: Only allowed schemes are https") + end + + context 'when localhost urls are not allowed' do + let(:presigned_url) { 'https://localhost:3000/file.tar.gz' } + + it 'validates the file_url' do + stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) + + expect(subject).not_to be_valid + expect(subject.errors.full_messages) + .to include("File url is blocked: Requests to localhost are not allowed") + end + end + end + + context 'when the remote file does not exist' do + it 'foo' do + expect(s3_double).to receive(:exists?).and_return(false) + + expect(subject).not_to be_valid + expect(subject.errors.full_messages) + .to include("File not found 'file_key' in 'bucket_name'") + end + end + + context 'when it fails to build the s3 object' do + it 'foo' do + expect(s3_double).to receive(:exists?).and_raise(StandardError, "some error") + + expect(subject).not_to be_valid + expect(subject.errors.full_messages) + .to include("Failed to open 'file_key' in 'bucket_name': some error") + end + end + end + + describe '#project_params' do + it 'returns import_export_upload in the params' do + subject = described_class.new(params: { remote_import_url: presigned_url }) + + expect(subject.project_params).to match( + import_export_upload: an_instance_of(::ImportExportUpload) + ) + expect(subject.project_params[:import_export_upload]).to have_attributes( + remote_import_url: presigned_url + ) + end + end +end diff --git a/spec/services/import/gitlab_projects/file_acquisition_strategies/remote_file_spec.rb b/spec/services/import/gitlab_projects/file_acquisition_strategies/remote_file_spec.rb new file mode 100644 index 00000000000..8565299b9b7 --- /dev/null +++ b/spec/services/import/gitlab_projects/file_acquisition_strategies/remote_file_spec.rb @@ -0,0 +1,149 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Import::GitlabProjects::FileAcquisitionStrategies::RemoteFile, :aggregate_failures do + let(:remote_url) { 'https://external.file.path/file.tar.gz' } + let(:params) { { remote_import_url: remote_url } } + + subject { described_class.new(params: params) } + + before do + stub_headers_for(remote_url, { + 'content-length' => 10.gigabytes, + 'content-type' => 'application/gzip' + }) + end + + describe 'validation' do + it { expect(subject).to be_valid } + + context 'file_url validation' do + let(:remote_url) { 'ftp://invalid.url/file.tar.gz' } + + it 'validates the file_url scheme' do + expect(subject).not_to be_valid + expect(subject.errors.full_messages) + .to include("File url is blocked: Only allowed schemes are https") + end + + context 'when localhost urls are not allowed' do + let(:remote_url) { 'https://localhost:3000/file.tar.gz' } + + it 'validates the file_url' do + stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) + + expect(subject).not_to be_valid + expect(subject.errors.full_messages) + .to include("File url is blocked: Requests to localhost are not allowed") + end + end + end + + context 'when import_project_from_remote_file_s3 is enabled' do + before do + stub_feature_flags(import_project_from_remote_file_s3: true) + end + + context 'when the HTTP request fail to recover the headers' do + it 'adds the error message' do + expect(Gitlab::HTTP) + .to receive(:head) + .and_raise(StandardError, 'request invalid') + + expect(subject).not_to be_valid + expect(subject.errors.full_messages) + .to include('Failed to retrive headers: request invalid') + end + end + + it 'validates the remote content-length' do + stub_headers_for(remote_url, { 'content-length' => 11.gigabytes }) + + expect(subject).not_to be_valid + expect(subject.errors.full_messages) + .to include('Content length is too big (should be at most 10 GB)') + end + + it 'validates the remote content-type' do + stub_headers_for(remote_url, { 'content-type' => 'unknown' }) + + expect(subject).not_to be_valid + expect(subject.errors.full_messages) + .to include("Content type 'unknown' not allowed. (Allowed: application/gzip, application/x-tar, application/x-gzip)") + end + + context 'when trying to import from AWS S3' do + it 'adds an error suggesting to use `projects/remote-import-s3`' do + stub_headers_for( + remote_url, + 'Server' => 'AmazonS3', + 'x-amz-request-id' => 'some-id' + ) + + expect(subject).not_to be_valid + expect(subject.errors.full_messages) + .to include('To import from AWS S3 use `projects/remote-import-s3`') + end + end + end + + context 'when import_project_from_remote_file_s3 is disabled' do + before do + stub_feature_flags(import_project_from_remote_file_s3: false) + end + + context 'when trying to import from AWS S3' do + it 'does not validate the remote content-length or content-type' do + stub_headers_for( + remote_url, + 'Server' => 'AmazonS3', + 'x-amz-request-id' => 'some-id', + 'content-length' => 11.gigabytes, + 'content-type' => 'unknown' + ) + + expect(subject).to be_valid + end + end + + context 'when NOT trying to import from AWS S3' do + it 'validates content-length and content-type' do + stub_headers_for( + remote_url, + 'Server' => 'NOT AWS S3', + 'content-length' => 11.gigabytes, + 'content-type' => 'unknown' + ) + + expect(subject).not_to be_valid + + expect(subject.errors.full_messages) + .to include("Content type 'unknown' not allowed. (Allowed: application/gzip, application/x-tar, application/x-gzip)") + expect(subject.errors.full_messages) + .to include('Content length is too big (should be at most 10 GB)') + end + end + end + end + + describe '#project_params' do + it 'returns import_export_upload in the params' do + subject = described_class.new(params: { remote_import_url: remote_url }) + + expect(subject.project_params).to match( + import_export_upload: an_instance_of(::ImportExportUpload) + ) + expect(subject.project_params[:import_export_upload]).to have_attributes( + remote_import_url: remote_url + ) + end + end + + def stub_headers_for(url, headers = {}) + allow(Gitlab::HTTP) + .to receive(:head) + .with(remote_url, timeout: 1.second) + .and_return(double(headers: headers)) # rubocop: disable RSpec/VerifiedDoubles + end +end diff --git a/spec/services/issue_links/create_service_spec.rb b/spec/services/issue_links/create_service_spec.rb index 1bca717acb7..9cb5980716a 100644 --- a/spec/services/issue_links/create_service_spec.rb +++ b/spec/services/issue_links/create_service_spec.rb @@ -4,180 +4,42 @@ require 'spec_helper' RSpec.describe IssueLinks::CreateService do describe '#execute' do - let(:namespace) { create :namespace } - let(:project) { create :project, namespace: namespace } - let(:issue) { create :issue, project: project } - let(:user) { create :user } - let(:params) do - {} - end + let_it_be(:user) { create :user } + let_it_be(:namespace) { create :namespace } + let_it_be(:project) { create :project, namespace: namespace } + let_it_be(:issuable) { create :issue, project: project } + let_it_be(:issuable2) { create :issue, project: project } + let_it_be(:guest_issuable) { create :issue } + let_it_be(:another_project) { create :project, namespace: project.namespace } + let_it_be(:issuable3) { create :issue, project: another_project } + let_it_be(:issuable_a) { create :issue, project: project } + let_it_be(:issuable_b) { create :issue, project: project } + let_it_be(:issuable_link) { create :issue_link, source: issuable, target: issuable_b, link_type: IssueLink::TYPE_RELATES_TO } + + let(:issuable_parent) { issuable.project } + let(:issuable_type) { :issue } + let(:issuable_link_class) { IssueLink } + let(:params) { {} } before do project.add_developer(user) + guest_issuable.project.add_guest(user) + another_project.add_developer(user) end - subject { described_class.new(issue, user, params).execute } + it_behaves_like 'issuable link creation' - context 'when the reference list is empty' do - let(:params) do - { issuable_references: [] } - end + context 'when target is an incident' do + let_it_be(:issue) { create(:incident, project: project) } - it 'returns error' do - is_expected.to eq(message: 'No matching issue found. Make sure that you are adding a valid issue URL.', status: :error, http_status: 404) - end - end - - context 'when Issue not found' do let(:params) do - { issuable_references: ["##{non_existing_record_iid}"] } - end - - it 'returns error' do - is_expected.to eq(message: 'No matching issue found. Make sure that you are adding a valid issue URL.', status: :error, http_status: 404) + { issuable_references: [issuable2.to_reference, issuable3.to_reference(another_project)] } end - it 'no relationship is created' do - expect { subject }.not_to change(IssueLink, :count) - end - end - - context 'when user has no permission to target project Issue' do - let(:target_issuable) { create :issue } - - let(:params) do - { issuable_references: [target_issuable.to_reference(project)] } - end - - it 'returns error' do - target_issuable.project.add_guest(user) - - is_expected.to eq(message: 'No matching issue found. Make sure that you are adding a valid issue URL.', status: :error, http_status: 404) - end - - it 'no relationship is created' do - expect { subject }.not_to change(IssueLink, :count) - end - end - - context 'source and target are the same issue' do - let(:params) do - { issuable_references: [issue.to_reference] } - end - - it 'does not create notes' do - expect(SystemNoteService).not_to receive(:relate_issue) - - subject - end - - it 'no relationship is created' do - expect { subject }.not_to change(IssueLink, :count) - end - end - - context 'when there is an issue to relate' do - let(:issue_a) { create :issue, project: project } - let(:another_project) { create :project, namespace: project.namespace } - let(:another_project_issue) { create :issue, project: another_project } - - let(:issue_a_ref) { issue_a.to_reference } - let(:another_project_issue_ref) { another_project_issue.to_reference(project) } - - let(:params) do - { issuable_references: [issue_a_ref, another_project_issue_ref] } - end - - before do - another_project.add_developer(user) - end - - it 'creates relationships' do - expect { subject }.to change(IssueLink, :count).from(0).to(2) - - expect(IssueLink.find_by!(target: issue_a)).to have_attributes(source: issue, link_type: 'relates_to') - expect(IssueLink.find_by!(target: another_project_issue)).to have_attributes(source: issue, link_type: 'relates_to') - end - - it 'returns success status' do - is_expected.to eq(status: :success) - end - - it 'creates notes' do - # First two-way relation notes - expect(SystemNoteService).to receive(:relate_issue) - .with(issue, issue_a, user) - expect(SystemNoteService).to receive(:relate_issue) - .with(issue_a, issue, user) - - # Second two-way relation notes - expect(SystemNoteService).to receive(:relate_issue) - .with(issue, another_project_issue, user) - expect(SystemNoteService).to receive(:relate_issue) - .with(another_project_issue, issue, user) - - subject - end - - context 'issue is an incident' do - let(:issue) { create(:incident, project: project) } - - it_behaves_like 'an incident management tracked event', :incident_management_incident_relate do - let(:current_user) { user } - end - end - end - - context 'when reference of any already related issue is present' do - let(:issue_a) { create :issue, project: project } - let(:issue_b) { create :issue, project: project } - let(:issue_c) { create :issue, project: project } - - before do - create :issue_link, source: issue, target: issue_b, link_type: IssueLink::TYPE_RELATES_TO - create :issue_link, source: issue, target: issue_c, link_type: IssueLink::TYPE_RELATES_TO - end - - let(:params) do - { - issuable_references: [ - issue_a.to_reference, - issue_b.to_reference, - issue_c.to_reference - ], - link_type: IssueLink::TYPE_RELATES_TO - } - end - - it 'creates notes only for new relations' do - expect(SystemNoteService).to receive(:relate_issue).with(issue, issue_a, anything) - expect(SystemNoteService).to receive(:relate_issue).with(issue_a, issue, anything) - expect(SystemNoteService).not_to receive(:relate_issue).with(issue, issue_b, anything) - expect(SystemNoteService).not_to receive(:relate_issue).with(issue_b, issue, anything) - expect(SystemNoteService).not_to receive(:relate_issue).with(issue, issue_c, anything) - expect(SystemNoteService).not_to receive(:relate_issue).with(issue_c, issue, anything) - - subject - end - end - - context 'when there are invalid references' do - let(:issue_a) { create :issue, project: project } - - let(:params) do - { issuable_references: [issue.to_reference, issue_a.to_reference] } - end - - it 'creates links only for valid references' do - expect { subject }.to change { IssueLink.count }.by(1) - end + subject { described_class.new(issue, user, params).execute } - it 'returns error status' do - expect(subject).to eq( - status: :error, - http_status: 422, - message: "#{issue.to_reference} cannot be added: cannot be related to itself" - ) + it_behaves_like 'an incident management tracked event', :incident_management_incident_relate do + let(:current_user) { user } end end end diff --git a/spec/services/issue_links/destroy_service_spec.rb b/spec/services/issue_links/destroy_service_spec.rb index f441629f892..a478a2c1448 100644 --- a/spec/services/issue_links/destroy_service_spec.rb +++ b/spec/services/issue_links/destroy_service_spec.rb @@ -4,65 +4,26 @@ require 'spec_helper' RSpec.describe IssueLinks::DestroyService do describe '#execute' do - let(:project) { create(:project_empty_repo) } - let(:user) { create(:user) } + let_it_be(:project) { create(:project_empty_repo, :private) } + let_it_be(:user) { create(:user) } + let_it_be(:issue_a) { create(:issue, project: project) } + let_it_be(:issue_b) { create(:issue, project: project) } - subject { described_class.new(issue_link, user).execute } + let!(:issuable_link) { create(:issue_link, source: issue_a, target: issue_b) } - context 'when successfully removes an issue link' do - let(:issue_a) { create(:issue, project: project) } - let(:issue_b) { create(:issue, project: project) } + subject { described_class.new(issuable_link, user).execute } - let!(:issue_link) { create(:issue_link, source: issue_a, target: issue_b) } + it_behaves_like 'a destroyable issuable link' + context 'when target is an incident' do before do project.add_reporter(user) end - it 'removes related issue' do - expect { subject }.to change(IssueLink, :count).from(1).to(0) - end - - it 'creates notes' do - # Two-way notes creation - expect(SystemNoteService).to receive(:unrelate_issue) - .with(issue_link.source, issue_link.target, user) - expect(SystemNoteService).to receive(:unrelate_issue) - .with(issue_link.target, issue_link.source, user) - - subject - end - - it 'returns success message' do - is_expected.to eq(message: 'Relation was removed', status: :success) - end - - context 'target is an incident' do - let(:issue_b) { create(:incident, project: project) } - - it_behaves_like 'an incident management tracked event', :incident_management_incident_unrelate do - let(:current_user) { user } - end - end - end - - context 'when failing to remove an issue link' do - let(:unauthorized_project) { create(:project) } - let(:issue_a) { create(:issue, project: project) } - let(:issue_b) { create(:issue, project: unauthorized_project) } - - let!(:issue_link) { create(:issue_link, source: issue_a, target: issue_b) } - - it 'does not remove relation' do - expect { subject }.not_to change(IssueLink, :count).from(1) - end - - it 'does not create notes' do - expect(SystemNoteService).not_to receive(:unrelate_issue) - end + let(:issue_b) { create(:incident, project: project) } - it 'returns error message' do - is_expected.to eq(message: 'No Issue Link found', status: :error, http_status: 404) + it_behaves_like 'an incident management tracked event', :incident_management_incident_unrelate do + let(:current_user) { user } end end end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index f4bb1f0877b..6b7b72d83fc 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -11,22 +11,12 @@ RSpec.describe Issues::CreateService do let(:spam_params) { double } - describe '.rate_limiter_scoped_and_keyed' do - it 'is set via the rate_limit call' do - expect(described_class.rate_limiter_scoped_and_keyed).to be_a(RateLimitedService::RateLimiterScopedAndKeyed) - - expect(described_class.rate_limiter_scoped_and_keyed.key).to eq(:issues_create) - expect(described_class.rate_limiter_scoped_and_keyed.opts[:scope]).to eq(%i[project current_user external_author]) - expect(described_class.rate_limiter_scoped_and_keyed.rate_limiter).to eq(Gitlab::ApplicationRateLimiter) - end - end - - describe '#rate_limiter_bypassed' do - let(:subject) { described_class.new(project: project, spam_params: {}) } - - it 'is nil by default' do - expect(subject.rate_limiter_bypassed).to be_nil - end + it_behaves_like 'rate limited service' do + let(:key) { :issues_create } + let(:key_scope) { %i[project current_user external_author] } + let(:application_limit_key) { :issues_create_limit } + let(:created_model) { Issue } + let(:service) { described_class.new(project: project, current_user: user, params: { title: 'title' }, spam_params: double) } end describe '#execute' do @@ -331,44 +321,6 @@ RSpec.describe Issues::CreateService do described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute end - context 'when rate limiting is in effect', :freeze_time, :clean_gitlab_redis_rate_limiting do - let(:user) { create(:user) } - - before do - stub_feature_flags(rate_limited_service_issues_create: true) - stub_application_setting(issues_create_limit: 1) - end - - subject do - 2.times { described_class.new(project: project, current_user: user, params: opts, spam_params: double).execute } - end - - context 'when too many requests are sent by one user' do - it 'raises an error' do - expect do - subject - end.to raise_error(RateLimitedService::RateLimitedError) - end - - it 'creates 1 issue' do - expect do - subject - rescue RateLimitedService::RateLimitedError - end.to change { Issue.count }.by(1) - end - end - - context 'when limit is higher than count of issues being created' do - before do - stub_application_setting(issues_create_limit: 2) - end - - it 'creates 2 issues' do - expect { subject }.to change { Issue.count }.by(2) - end - end - end - context 'after_save callback to store_mentions' do context 'when mentionable attributes change' do let(:opts) { { title: 'Title', description: "Description with #{user.to_reference}" } } @@ -574,6 +526,31 @@ RSpec.describe Issues::CreateService do end end + context 'add related issue' do + let_it_be(:related_issue) { create(:issue, project: project) } + + let(:opts) do + { title: 'A new issue', add_related_issue: related_issue } + end + + it 'ignores related issue if not accessible' do + expect { issue }.not_to change { IssueLink.count } + expect(issue).to be_persisted + end + + context 'when user has access to the related issue' do + before do + project.add_developer(user) + end + + it 'adds a link to the issue' do + expect { issue }.to change { IssueLink.count }.by(1) + expect(issue).to be_persisted + expect(issue.related_issues(user)).to eq([related_issue]) + end + end + end + context 'checking spam' do let(:params) do { diff --git a/spec/services/issues/set_crm_contacts_service_spec.rb b/spec/services/issues/set_crm_contacts_service_spec.rb index 64011a7a003..b0befb9f77c 100644 --- a/spec/services/issues/set_crm_contacts_service_spec.rb +++ b/spec/services/issues/set_crm_contacts_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Issues::SetCrmContactsService do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group, :crm_enabled) } - let_it_be(:project) { create(:project, group: group) } + let_it_be(:project) { create(:project, group: create(:group, parent: group)) } let_it_be(:contacts) { create_list(:contact, 4, group: group) } let_it_be(:issue, reload: true) { create(:issue, project: project) } let_it_be(:issue_contact_1) do diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 95394ba6597..6d3c3dd4e39 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -1157,6 +1157,13 @@ RSpec.describe Issues::UpdateService, :mailer do expect(issue.escalation_status.status_name).to eq(expected_status) end + + it 'triggers webhooks' do + expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) + expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :issue_hooks) + + update_issue(opts) + end end shared_examples 'does not change the status record' do @@ -1169,7 +1176,8 @@ RSpec.describe Issues::UpdateService, :mailer do end it 'does not trigger side-effects' do - expect(escalation_update_class).not_to receive(:new) + expect(project).not_to receive(:execute_hooks) + expect(project).not_to receive(:execute_integrations) update_issue(opts) end @@ -1324,32 +1332,14 @@ RSpec.describe Issues::UpdateService, :mailer do context 'broadcasting issue assignee updates' do let(:update_params) { { assignee_ids: [user2.id] } } - context 'when feature flag is enabled' do - before do - stub_feature_flags(broadcast_issue_updates: true) - end + it 'triggers the GraphQL subscription' do + expect(GraphqlTriggers).to receive(:issuable_assignees_updated).with(issue) - it 'triggers the GraphQL subscription' do - expect(GraphqlTriggers).to receive(:issuable_assignees_updated).with(issue) - - update_issue(update_params) - end - - context 'when assignee is not updated' do - let(:update_params) { { title: 'Some other title' } } - - it 'does not trigger the GraphQL subscription' do - expect(GraphqlTriggers).not_to receive(:issuable_assignees_updated).with(issue) - - update_issue(update_params) - end - end + update_issue(update_params) end - context 'when feature flag is disabled' do - before do - stub_feature_flags(broadcast_issue_updates: false) - end + context 'when assignee is not updated' do + let(:update_params) { { title: 'Some other title' } } it 'does not trigger the GraphQL subscription' do expect(GraphqlTriggers).not_to receive(:issuable_assignees_updated).with(issue) diff --git a/spec/services/labels/create_service_spec.rb b/spec/services/labels/create_service_spec.rb index 7a31a5a7cae..02dec8ae690 100644 --- a/spec/services/labels/create_service_spec.rb +++ b/spec/services/labels/create_service_spec.rb @@ -14,7 +14,7 @@ RSpec.describe Labels::CreateService do let(:unknown_color) { 'unknown' } let(:no_color) { '' } - let(:expected_saved_color) { hex_color } + let(:expected_saved_color) { ::Gitlab::Color.of(hex_color) } context 'in a project' do context 'with color in hex-code' do @@ -47,7 +47,6 @@ RSpec.describe Labels::CreateService do context 'with color surrounded by spaces' do it 'creates a label' do label = described_class.new(params_with(spaced_color)).execute(project: project) - expect(label).to be_persisted expect(label.color).to eq expected_saved_color end diff --git a/spec/services/labels/promote_service_spec.rb b/spec/services/labels/promote_service_spec.rb index 81c24b26c9f..a10aaa14030 100644 --- a/spec/services/labels/promote_service_spec.rb +++ b/spec/services/labels/promote_service_spec.rb @@ -202,7 +202,7 @@ RSpec.describe Labels::PromoteService do expect(new_label.title).to eq(promoted_label_name) expect(new_label.description).to eq(promoted_description) - expect(new_label.color).to eq(promoted_color) + expect(new_label.color).to be_color(promoted_color) end it_behaves_like 'promoting a project label to a group label' diff --git a/spec/services/labels/update_service_spec.rb b/spec/services/labels/update_service_spec.rb index af2403656af..abc456f75f9 100644 --- a/spec/services/labels/update_service_spec.rb +++ b/spec/services/labels/update_service_spec.rb @@ -13,7 +13,7 @@ RSpec.describe Labels::UpdateService do let(:unknown_color) { 'unknown' } let(:no_color) { '' } - let(:expected_saved_color) { hex_color } + let(:expected_saved_color) { ::Gitlab::Color.of(hex_color) } before do @label = Labels::CreateService.new(title: 'Initial', color: '#000000').execute(project: project) diff --git a/spec/services/members/projects/creator_service_spec.rb b/spec/services/members/projects/creator_service_spec.rb index c6917a21bcd..7ba183759bc 100644 --- a/spec/services/members/projects/creator_service_spec.rb +++ b/spec/services/members/projects/creator_service_spec.rb @@ -9,8 +9,8 @@ RSpec.describe Members::Projects::CreatorService do end describe '.access_levels' do - it 'returns Gitlab::Access.sym_options' do - expect(described_class.access_levels).to eq(Gitlab::Access.sym_options) + it 'returns Gitlab::Access.sym_options_with_owner' do + expect(described_class.access_levels).to eq(Gitlab::Access.sym_options_with_owner) end end end diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb index 4d20d62b864..9b064da44b8 100644 --- a/spec/services/merge_requests/approval_service_spec.rb +++ b/spec/services/merge_requests/approval_service_spec.rb @@ -61,7 +61,7 @@ RSpec.describe MergeRequests::ApprovalService do it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: user, merge_request: merge_request, user: user) + .with(project: project, current_user: user, merge_request: merge_request) .and_call_original service.execute(merge_request) diff --git a/spec/services/merge_requests/bulk_remove_attention_requested_service_spec.rb b/spec/services/merge_requests/bulk_remove_attention_requested_service_spec.rb index ae8846974ce..b2326a28e63 100644 --- a/spec/services/merge_requests/bulk_remove_attention_requested_service_spec.rb +++ b/spec/services/merge_requests/bulk_remove_attention_requested_service_spec.rb @@ -40,6 +40,10 @@ RSpec.describe MergeRequests::BulkRemoveAttentionRequestedService do expect(reviewer.state).to eq 'reviewed' expect(assignee.state).to eq 'reviewed' end + + it_behaves_like 'invalidates attention request cache' do + let(:users) { [assignee_user, user] } + end end end end diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index a196c944eda..49f691e97e2 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -454,7 +454,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end end - context 'when source and target projects are different' do + shared_examples 'when source and target projects are different' do let(:target_project) { fork_project(project, nil, repository: true) } let(:opts) do @@ -497,9 +497,14 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end it 'creates the merge request', :sidekiq_might_not_need_inline do + expect_next_instance_of(MergeRequest) do |instance| + expect(instance).to receive(:eager_fetch_ref!).and_call_original + end + merge_request = described_class.new(project: project, current_user: user, params: opts).execute expect(merge_request).to be_persisted + expect(merge_request.iid).to be > 0 end it 'does not create the merge request when the target project is archived' do @@ -511,6 +516,8 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end end + it_behaves_like 'when source and target projects are different' + context 'when user sets source project id' do let(:another_project) { create(:project) } diff --git a/spec/services/merge_requests/handle_assignees_change_service_spec.rb b/spec/services/merge_requests/handle_assignees_change_service_spec.rb index fa3b1614e21..26f53f55b0f 100644 --- a/spec/services/merge_requests/handle_assignees_change_service_spec.rb +++ b/spec/services/merge_requests/handle_assignees_change_service_spec.rb @@ -89,12 +89,18 @@ RSpec.describe MergeRequests::HandleAssigneesChangeService do it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: user, merge_request: merge_request, user: user) + .with(project: project, current_user: user, merge_request: merge_request) .and_call_original execute end + it 'updates attention requested by of assignee' do + execute + + expect(merge_request.find_assignee(assignee).updated_state_by).to eq(user) + end + it 'tracks users assigned event' do expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) .to receive(:track_users_assigned_to_mr).once.with(users: [assignee]) diff --git a/spec/services/merge_requests/merge_orchestration_service_spec.rb b/spec/services/merge_requests/merge_orchestration_service_spec.rb index da37cc97857..ebcd2f0e277 100644 --- a/spec/services/merge_requests/merge_orchestration_service_spec.rb +++ b/spec/services/merge_requests/merge_orchestration_service_spec.rb @@ -64,7 +64,7 @@ RSpec.describe MergeRequests::MergeOrchestrationService do context 'when merge request is not mergeable' do before do - allow(merge_request).to receive(:mergeable_state?) { false } + allow(merge_request).to receive(:mergeable?) { false } end it 'does nothing' do @@ -87,7 +87,7 @@ RSpec.describe MergeRequests::MergeOrchestrationService do context 'when merge request is not mergeable' do before do - allow(merge_request).to receive(:mergeable_state?) { false } + allow(merge_request).to receive(:mergeable?) { false } end it { is_expected.to eq(false) } diff --git a/spec/services/merge_requests/mergeability/check_broken_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_broken_status_service_spec.rb new file mode 100644 index 00000000000..9e178c121ef --- /dev/null +++ b/spec/services/merge_requests/mergeability/check_broken_status_service_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::Mergeability::CheckBrokenStatusService do + subject(:check_broken_status) { described_class.new(merge_request: merge_request, params: {}) } + + let(:merge_request) { build(:merge_request) } + + describe '#execute' do + before do + expect(merge_request).to receive(:broken?).and_return(broken) + end + + context 'when the merge request is broken' do + let(:broken) { true } + + it 'returns a check result with status failed' do + expect(check_broken_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS + end + end + + context 'when the merge request is not broken' do + let(:broken) { false } + + it 'returns a check result with status success' do + expect(check_broken_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + end + + describe '#skip?' do + it 'returns false' do + expect(check_broken_status.skip?).to eq false + end + end + + describe '#cacheable?' do + it 'returns false' do + expect(check_broken_status.cacheable?).to eq false + end + end +end diff --git a/spec/services/merge_requests/mergeability/check_discussions_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_discussions_status_service_spec.rb new file mode 100644 index 00000000000..c24d40967c4 --- /dev/null +++ b/spec/services/merge_requests/mergeability/check_discussions_status_service_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::Mergeability::CheckDiscussionsStatusService do + subject(:check_discussions_status) { described_class.new(merge_request: merge_request, params: params) } + + let(:merge_request) { build(:merge_request) } + let(:params) { { skip_discussions_check: skip_check } } + let(:skip_check) { false } + + describe '#execute' do + before do + expect(merge_request).to receive(:mergeable_discussions_state?).and_return(mergeable) + end + + context 'when the merge request is in a mergable state' do + let(:mergeable) { true } + + it 'returns a check result with status success' do + expect(check_discussions_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + + context 'when the merge request is not in a mergeable state' do + let(:mergeable) { false } + + it 'returns a check result with status failed' do + expect(check_discussions_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS + end + end + end + + describe '#skip?' do + context 'when skip check is true' do + let(:skip_check) { true } + + it 'returns true' do + expect(check_discussions_status.skip?).to eq true + end + end + + context 'when skip check is false' do + let(:skip_check) { false } + + it 'returns false' do + expect(check_discussions_status.skip?).to eq false + end + end + end + + describe '#cacheable?' do + it 'returns false' do + expect(check_discussions_status.cacheable?).to eq false + end + end +end diff --git a/spec/services/merge_requests/mergeability/check_draft_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_draft_status_service_spec.rb new file mode 100644 index 00000000000..923cff220ef --- /dev/null +++ b/spec/services/merge_requests/mergeability/check_draft_status_service_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::Mergeability::CheckDraftStatusService do + subject(:check_draft_status) { described_class.new(merge_request: merge_request, params: {}) } + + let(:merge_request) { build(:merge_request) } + + describe '#execute' do + before do + expect(merge_request).to receive(:draft?).and_return(draft) + end + + context 'when the merge request is a draft' do + let(:draft) { true } + + it 'returns a check result with status failed' do + expect(check_draft_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS + end + end + + context 'when the merge request is not a draft' do + let(:draft) { false } + + it 'returns a check result with status success' do + expect(check_draft_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + end + + describe '#skip?' do + it 'returns false' do + expect(check_draft_status.skip?).to eq false + end + end + + describe '#cacheable?' do + it 'returns false' do + expect(check_draft_status.cacheable?).to eq false + end + end +end diff --git a/spec/services/merge_requests/mergeability/check_open_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_open_status_service_spec.rb new file mode 100644 index 00000000000..b1c9a930317 --- /dev/null +++ b/spec/services/merge_requests/mergeability/check_open_status_service_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::Mergeability::CheckOpenStatusService do + subject(:check_open_status) { described_class.new(merge_request: merge_request, params: {}) } + + let(:merge_request) { build(:merge_request) } + + describe '#execute' do + before do + expect(merge_request).to receive(:open?).and_return(open) + end + + context 'when the merge request is open' do + let(:open) { true } + + it 'returns a check result with status success' do + expect(check_open_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + + context 'when the merge request is not open' do + let(:open) { false } + + it 'returns a check result with status failed' do + expect(check_open_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS + end + end + end + + describe '#skip?' do + it 'returns false' do + expect(check_open_status.skip?).to eq false + end + end + + describe '#cacheable?' do + it 'returns false' do + expect(check_open_status.cacheable?).to eq false + end + end +end diff --git a/spec/services/merge_requests/mergeability/run_checks_service_spec.rb b/spec/services/merge_requests/mergeability/run_checks_service_spec.rb index 71ad23bc68c..d4ee4afd71d 100644 --- a/spec/services/merge_requests/mergeability/run_checks_service_spec.rb +++ b/spec/services/merge_requests/mergeability/run_checks_service_spec.rb @@ -35,12 +35,19 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService do context 'when a check is skipped' do it 'does not execute the check' do + described_class::CHECKS.each do |check| + allow_next_instance_of(check) do |service| + allow(service).to receive(:skip?).and_return(false) + allow(service).to receive(:execute).and_return(success_result) + end + end + expect_next_instance_of(MergeRequests::Mergeability::CheckCiStatusService) do |service| expect(service).to receive(:skip?).and_return(true) expect(service).not_to receive(:execute) end - expect(execute).to match_array([]) + expect(execute).to match_array([success_result, success_result, success_result, success_result]) end end @@ -49,6 +56,12 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService do let(:merge_check) { instance_double(MergeRequests::Mergeability::CheckCiStatusService) } before do + described_class::CHECKS.each do |check| + allow_next_instance_of(check) do |service| + allow(service).to receive(:skip?).and_return(true) + end + end + expect(MergeRequests::Mergeability::CheckCiStatusService).to receive(:new).and_return(merge_check) expect(merge_check).to receive(:skip?).and_return(false) allow(merge_check).to receive(:cacheable?).and_return(cacheable) diff --git a/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb b/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb index b333d4af6cf..20b5cf5e3a1 100644 --- a/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb +++ b/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb @@ -47,15 +47,5 @@ RSpec.describe MergeRequests::ReloadMergeHeadDiffService do expect(merge_request.reload.merge_head_diff).not_to eq(existing_merge_head_diff) end end - - context 'when default_merge_ref_for_diffs feature flag is disabled' do - before do - stub_feature_flags(default_merge_ref_for_diffs: false) - end - - it 'returns error' do - expect(subject[:status]).to eq(:error) - end - end end end diff --git a/spec/services/merge_requests/remove_attention_requested_service_spec.rb b/spec/services/merge_requests/remove_attention_requested_service_spec.rb index 875afc2dc7e..450204ebfdd 100644 --- a/spec/services/merge_requests/remove_attention_requested_service_spec.rb +++ b/spec/services/merge_requests/remove_attention_requested_service_spec.rb @@ -4,23 +4,20 @@ require 'spec_helper' RSpec.describe MergeRequests::RemoveAttentionRequestedService do let(:current_user) { create(:user) } - let(:user) { create(:user) } - let(:assignee_user) { create(:user) } - let(:merge_request) { create(:merge_request, reviewers: [user], assignees: [assignee_user]) } - let(:reviewer) { merge_request.find_reviewer(user) } - let(:assignee) { merge_request.find_assignee(assignee_user) } + let(:merge_request) { create(:merge_request, reviewers: [current_user], assignees: [current_user]) } + let(:reviewer) { merge_request.find_reviewer(current_user) } + let(:assignee) { merge_request.find_assignee(current_user) } let(:project) { merge_request.project } - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request, user: user) } + let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request) } let(:result) { service.execute } before do project.add_developer(current_user) - project.add_developer(user) end describe '#execute' do context 'invalid permissions' do - let(:service) { described_class.new(project: project, current_user: create(:user), merge_request: merge_request, user: user) } + let(:service) { described_class.new(project: project, current_user: create(:user), merge_request: merge_request) } it 'returns an error' do expect(result[:status]).to eq :error @@ -28,7 +25,7 @@ RSpec.describe MergeRequests::RemoveAttentionRequestedService do end context 'reviewer does not exist' do - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request, user: create(:user)) } + let(:service) { described_class.new(project: project, current_user: create(:user), merge_request: merge_request) } it 'returns an error' do expect(result[:status]).to eq :error @@ -46,10 +43,14 @@ RSpec.describe MergeRequests::RemoveAttentionRequestedService do expect(reviewer.state).to eq 'reviewed' end + + it_behaves_like 'invalidates attention request cache' do + let(:users) { [current_user] } + end end context 'assignee exists' do - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request, user: assignee_user) } + let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request) } before do assignee.update!(state: :reviewed) @@ -65,12 +66,16 @@ RSpec.describe MergeRequests::RemoveAttentionRequestedService do expect(assignee.state).to eq 'reviewed' end + + it_behaves_like 'invalidates attention request cache' do + let(:users) { [current_user] } + end end context 'assignee is the same as reviewer' do - let(:merge_request) { create(:merge_request, reviewers: [user], assignees: [user]) } - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request, user: user) } - let(:assignee) { merge_request.find_assignee(user) } + let(:merge_request) { create(:merge_request, reviewers: [current_user], assignees: [current_user]) } + let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request) } + let(:assignee) { merge_request.find_assignee(current_user) } it 'updates reviewers and assignees state' do service.execute diff --git a/spec/services/merge_requests/toggle_attention_requested_service_spec.rb b/spec/services/merge_requests/toggle_attention_requested_service_spec.rb index 63fa61b8097..dcaac5d2699 100644 --- a/spec/services/merge_requests/toggle_attention_requested_service_spec.rb +++ b/spec/services/merge_requests/toggle_attention_requested_service_spec.rb @@ -59,6 +59,13 @@ RSpec.describe MergeRequests::ToggleAttentionRequestedService do expect(reviewer.state).to eq 'attention_requested' end + it 'adds who toggled attention' do + service.execute + reviewer.reload + + expect(reviewer.updated_state_by).to eq current_user + end + it 'creates a new todo for the reviewer' do expect(todo_service).to receive(:create_attention_requested_todo).with(merge_request, current_user, user) @@ -73,11 +80,21 @@ RSpec.describe MergeRequests::ToggleAttentionRequestedService do it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: current_user, merge_request: merge_request, user: current_user) + .with(project: project, current_user: current_user, merge_request: merge_request) .and_call_original service.execute end + + it 'invalidates cache' do + cache_mock = double + + expect(cache_mock).to receive(:delete).with(['users', user.id, 'attention_requested_open_merge_requests_count']) + + allow(Rails).to receive(:cache).and_return(cache_mock) + + service.execute + end end context 'assignee exists' do @@ -112,11 +129,15 @@ RSpec.describe MergeRequests::ToggleAttentionRequestedService do it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: current_user, merge_request: merge_request, user: current_user) + .with(project: project, current_user: current_user, merge_request: merge_request) .and_call_original service.execute end + + it_behaves_like 'invalidates attention request cache' do + let(:users) { [assignee_user] } + end end context 'assignee is the same as reviewer' do diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 48d9f019274..eb587797201 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -215,6 +215,14 @@ RSpec.describe MergeRequests::UpdateService, :mailer do MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) end + + it 'updates attention requested by of reviewer' do + opts[:reviewers] = [user2] + + MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) + + expect(merge_request.find_reviewer(user2).updated_state_by).to eq(user) + end end context 'when reviewers did not change' do diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 9cbc16f0c95..399b2b4be2d 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe NotificationService, :mailer do include ExternalAuthorizationServiceHelpers include NotificationHelpers - let_it_be_with_refind(:project) { create(:project, :public) } + let_it_be_with_refind(:project, reload: true) { create(:project, :public) } let_it_be_with_refind(:assignee) { create(:user) } let(:notification) { described_class.new } @@ -258,6 +258,27 @@ RSpec.describe NotificationService, :mailer do end describe 'AccessToken' do + describe '#access_token_created' do + let_it_be(:user) { create(:user) } + let_it_be(:pat) { create(:personal_access_token, user: user) } + + subject(:notification_service) { notification.access_token_created(user, pat.name) } + + it 'sends email to the token owner' do + expect { notification_service }.to have_enqueued_email(user, pat.name, mail: "access_token_created_email") + end + + context 'when user is not allowed to receive notifications' do + before do + user.block! + end + + it 'does not send email to the token owner' do + expect { notification_service }.not_to have_enqueued_email(user, pat.name, mail: "access_token_created_email") + end + end + end + describe '#access_token_about_to_expire' do let_it_be(:user) { create(:user) } let_it_be(:pat) { create(:personal_access_token, user: user, expires_at: 5.days.from_now) } @@ -1051,6 +1072,7 @@ RSpec.describe NotificationService, :mailer do end before do + project.reload add_user_subscriptions(issue) reset_delivered_emails! update_custom_notification(:new_issue, @u_guest_custom, resource: project) @@ -3312,7 +3334,7 @@ RSpec.describe NotificationService, :mailer do describe "##{sym}" do subject(:notify!) { notification.send(sym, domain) } - it 'emails current watching maintainers' do + it 'emails current watching maintainers and owners' do expect(Notify).to receive(:"#{sym}_email").at_least(:once).and_call_original notify! @@ -3410,7 +3432,7 @@ RSpec.describe NotificationService, :mailer do reset_delivered_emails! end - it 'emails current watching maintainers' do + it 'emails current watching maintainers and owners' do notification.remote_mirror_update_failed(remote_mirror) should_only_email(u_maintainer1, u_maintainer2, u_owner) diff --git a/spec/services/packages/pypi/create_package_service_spec.rb b/spec/services/packages/pypi/create_package_service_spec.rb index 3d0c10724d4..f84a77f80f7 100644 --- a/spec/services/packages/pypi/create_package_service_spec.rb +++ b/spec/services/packages/pypi/create_package_service_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'spec_helper' -RSpec.describe Packages::Pypi::CreatePackageService do +RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do include PackagesManagerApiSpecHelpers let_it_be(:project) { create(:project) } @@ -39,6 +39,18 @@ RSpec.describe Packages::Pypi::CreatePackageService do end end + context 'without required_python' do + before do + params.delete(:requires_python) + end + + it 'creates the package' do + expect { subject }.to change { Packages::Package.pypi.count }.by(1) + + expect(created_package.pypi_metadatum.required_python).to eq '' + end + end + context 'with an invalid metadata' do let(:requires_python) { 'x' * 256 } @@ -73,7 +85,7 @@ RSpec.describe Packages::Pypi::CreatePackageService do .and raise_error(/File name has already been taken/) end - context 'with a pending_destruction package', :aggregate_failures do + context 'with a pending_destruction package' do before do Packages::Package.pypi.last.pending_destruction! end diff --git a/spec/services/personal_access_tokens/create_service_spec.rb b/spec/services/personal_access_tokens/create_service_spec.rb index 842bebd13a1..b8a4c8f30d2 100644 --- a/spec/services/personal_access_tokens/create_service_spec.rb +++ b/spec/services/personal_access_tokens/create_service_spec.rb @@ -18,6 +18,14 @@ RSpec.describe PersonalAccessTokens::CreateService do subject end + + it 'notifies the user' do + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive(:access_token_created).with(user, params[:name]) + end + + subject + end end shared_examples_for 'an unsuccessfully created token' do diff --git a/spec/services/projects/branches_by_mode_service_spec.rb b/spec/services/projects/branches_by_mode_service_spec.rb index e8bcda8a9c4..9a63563b37b 100644 --- a/spec/services/projects/branches_by_mode_service_spec.rb +++ b/spec/services/projects/branches_by_mode_service_spec.rb @@ -13,20 +13,22 @@ RSpec.describe Projects::BranchesByModeService do describe '#execute' do context 'page is passed' do - let(:params) { { page: 4, mode: 'all', offset: 3 } } + let(:page) { (TestEnv::BRANCH_SHA.length.to_f / Kaminari.config.default_per_page).ceil } + let(:params) { { page: page, mode: 'all', offset: page - 1 } } it 'uses offset pagination' do expect(finder).to receive(:fetch_branches_via_offset_pagination).and_call_original branches, prev_page, next_page = subject + remaining = TestEnv::BRANCH_SHA.length % Kaminari.config.default_per_page - expect(branches.size).to eq(11) + expect(branches.size).to eq(remaining > 0 ? remaining : 20) expect(next_page).to be_nil - expect(prev_page).to eq("/#{project.full_path}/-/branches/all?offset=2&page=3") + expect(prev_page).to eq("/#{project.full_path}/-/branches/all?offset=#{page - 2}&page=#{page - 1}") end context 'but the page does not contain any branches' do - let(:params) { { page: 10, mode: 'all' } } + let(:params) { { page: 100, mode: 'all' } } it 'uses offset pagination' do expect(finder).to receive(:fetch_branches_via_offset_pagination).and_call_original @@ -61,9 +63,10 @@ RSpec.describe Projects::BranchesByModeService do expect(finder).to receive(:fetch_branches_via_offset_pagination).and_call_original branches, prev_page, next_page = subject + expected_page_token = ERB::Util.url_encode(TestEnv::BRANCH_SHA.sort[19][0]) expect(branches.size).to eq(20) - expect(next_page).to eq("/#{project.full_path}/-/branches/all?offset=1&page_token=conflict-resolvable") + expect(next_page).to eq("/#{project.full_path}/-/branches/all?offset=1&page_token=#{expected_page_token}") expect(prev_page).to be_nil end end @@ -75,26 +78,31 @@ RSpec.describe Projects::BranchesByModeService do it 'returns branches for the first page' do branches, prev_page, next_page = subject + expected_page_token = ERB::Util.url_encode(TestEnv::BRANCH_SHA.sort[19][0]) expect(branches.size).to eq(20) - expect(next_page).to eq("/#{project.full_path}/-/branches/all?offset=1&page_token=conflict-resolvable") + expect(next_page).to eq("/#{project.full_path}/-/branches/all?offset=1&page_token=#{expected_page_token}") expect(prev_page).to be_nil end context 'when second page is requested' do - let(:params) { { page_token: 'conflict-resolvable', mode: 'all', sort: 'name_asc', offset: 1 } } + let(:page_token) { 'conflict-resolvable' } + let(:params) { { page_token: page_token, mode: 'all', sort: 'name_asc', offset: 1 } } it 'returns branches for the first page' do branches, prev_page, next_page = subject + branch_index = TestEnv::BRANCH_SHA.sort.find_index { |a| a[0] == page_token } + expected_page_token = ERB::Util.url_encode(TestEnv::BRANCH_SHA.sort[20 + branch_index][0]) expect(branches.size).to eq(20) - expect(next_page).to eq("/#{project.full_path}/-/branches/all?offset=2&page_token=improve%2Fawesome&sort=name_asc") + expect(next_page).to eq("/#{project.full_path}/-/branches/all?offset=2&page_token=#{expected_page_token}&sort=name_asc") expect(prev_page).to eq("/#{project.full_path}/-/branches/all?offset=0&page=1&sort=name_asc") end end context 'when last page is requested' do - let(:params) { { page_token: 'signed-commits', mode: 'all', sort: 'name_asc', offset: 4 } } + let(:page_token) { TestEnv::BRANCH_SHA.sort[-16][0] } + let(:params) { { page_token: page_token, mode: 'all', sort: 'name_asc', offset: 4 } } it 'returns branches after the specified branch' do branches, prev_page, next_page = subject diff --git a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb index a41ba8216cc..38a3e00c8e7 100644 --- a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb @@ -267,12 +267,30 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService, :clean_gitlab_ 'container_expiration_policy' => true } end - it 'succeeds without a user' do + before do expect_delete(%w(Bb Ba C), container_expiration_policy: true) + end + + it { is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3)) } + + context 'caching' do + it 'expects caching to be used' do + expect_caching - expect_caching + subject + end + + context 'when setting set to false' do + before do + stub_application_setting(container_registry_expiration_policies_caching: false) + end - is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3)) + it 'does not use caching' do + expect_no_caching + + subject + end + end end end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 10f694827e1..96a50b26871 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -23,11 +23,11 @@ RSpec.describe Projects::CreateService, '#execute' do end it 'creates labels on project creation' do - created_label = project.labels.last - - expect(created_label.type).to eq('ProjectLabel') - expect(created_label.project_id).to eq(project.id) - expect(created_label.title).to eq('bug') + expect(project.labels).to include have_attributes( + type: eq('ProjectLabel'), + project_id: eq(project.id), + title: eq('bug') + ) end context 'using gitlab project import' do @@ -121,7 +121,8 @@ RSpec.describe Projects::CreateService, '#execute' do expect(project).to be_valid expect(project.first_owner).to eq(user) - expect(project.team.maintainers).to include(user) + expect(project.team.maintainers).not_to include(user) + expect(project.team.owners).to contain_exactly(user) expect(project.namespace).to eq(user.namespace) expect(project.project_namespace).to be_in_sync_with_project(project) end @@ -162,7 +163,7 @@ RSpec.describe Projects::CreateService, '#execute' do expect(project).to be_persisted expect(project.owner).to eq(user) expect(project.first_owner).to eq(user) - expect(project.team.maintainers).to contain_exactly(user) + expect(project.team.owners).to contain_exactly(user) expect(project.namespace).to eq(user.namespace) expect(project.project_namespace).to be_in_sync_with_project(project) end @@ -205,17 +206,7 @@ RSpec.describe Projects::CreateService, '#execute' do expect(project.project_namespace).to be_in_sync_with_project(project) end - context 'with before_commit callback' do - it_behaves_like 'has sync-ed traversal_ids' - end - - context 'with after_create callback' do - before do - stub_feature_flags(sync_traversal_ids_before_commit: false) - end - - it_behaves_like 'has sync-ed traversal_ids' - end + it_behaves_like 'has sync-ed traversal_ids' end context 'group sharing', :sidekiq_inline do diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index d60ec8c2958..cd923720631 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::DestroyService, :aggregate_failures do +RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publisher do include ProjectForksHelper let_it_be(:user) { create(:user) } @@ -15,7 +15,6 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do before do stub_container_registry_config(enabled: true) stub_container_registry_tags(repository: :any, tags: []) - allow(Gitlab::EventStore).to receive(:publish) end shared_examples 'deleting the project' do @@ -30,23 +29,8 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do it 'publishes a ProjectDeleted event with project id and namespace id' do expected_data = { project_id: project.id, namespace_id: project.namespace_id } - expect(Gitlab::EventStore) - .to receive(:publish) - .with(event_type(Projects::ProjectDeletedEvent).containing(expected_data)) - destroy_project(project, user, {}) - end - - context 'when feature flag publish_project_deleted_event is disabled' do - before do - stub_feature_flags(publish_project_deleted_event: false) - end - - it 'does not publish an event' do - expect(Gitlab::EventStore).not_to receive(:publish).with(event_type(Projects::ProjectDeletedEvent)) - - destroy_project(project, user, {}) - end + expect { destroy_project(project, user, {}) }.to publish_event(Projects::ProjectDeletedEvent).with(expected_data) end end @@ -59,6 +43,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do let!(:report_result) { create(:ci_build_report_result, build: build) } let!(:pending_state) { create(:ci_build_pending_state, build: build) } let!(:pipeline_artifact) { create(:ci_pipeline_artifact, pipeline: pipeline) } + let!(:secure_file) { create(:ci_secure_file, project: project) } it 'deletes build and pipeline related records' do expect { destroy_project(project, user, {}) } @@ -72,6 +57,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do .and change { Ci::BuildReportResult.count }.by(-1) .and change { Ci::BuildRunnerSession.count }.by(-1) .and change { Ci::Pipeline.count }.by(-1) + .and change { Ci::SecureFile.count }.by(-1) end it 'avoids N+1 queries' do @@ -449,11 +435,12 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do destroy_project(project, user) end - it 'calls the bulk snippet destroy service' do + it 'calls the bulk snippet destroy service with the hard_delete param set to true' do expect(project.snippets.count).to eq 2 - expect(Snippets::BulkDestroyService).to receive(:new) - .with(user, project.snippets).and_call_original + expect_next_instance_of(Snippets::BulkDestroyService, user, project.snippets) do |instance| + expect(instance).to receive(:execute).with(hard_delete: true).and_call_original + end expect do destroy_project(project, user) @@ -461,11 +448,15 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do end context 'when an error is raised deleting snippets' do + let(:error_message) { 'foo' } + it 'does not delete project' do allow_next_instance_of(Snippets::BulkDestroyService) do |instance| - allow(instance).to receive(:execute).and_return(ServiceResponse.error(message: 'foo')) + allow(instance).to receive(:execute).and_return(ServiceResponse.error(message: error_message)) end + expect(Gitlab::AppLogger).to receive(:error).with("Snippet deletion failed on #{project.full_path} with the following message: #{error_message}") + expect(Gitlab::AppLogger).to receive(:error).with(/Failed to remove project snippets/) expect(destroy_project(project, user)).to be_falsey expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_truthy end diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index ccfd119b55b..ab9f99f893d 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -199,12 +199,13 @@ RSpec.describe Projects::ImportService do context 'with valid importer' do before do - stub_github_omniauth_provider + provider = double(:provider).as_null_object + stub_omniauth_setting(providers: [provider]) project.import_url = 'https://github.com/vim/vim.git' project.import_type = 'github' - allow(project).to receive(:import_data).and_return(double.as_null_object) + allow(project).to receive(:import_data).and_return(double(:import_data).as_null_object) end it 'succeeds if importer succeeds' do @@ -296,22 +297,5 @@ RSpec.describe Projects::ImportService do subject.execute end end - - def stub_github_omniauth_provider - provider = ActiveSupport::InheritableOptions.new( - 'name' => 'github', - 'app_id' => 'asd123', - 'app_secret' => 'asd123', - 'args' => { - 'client_options' => { - 'site' => 'https://github.com/api/v3', - 'authorize_url' => 'https://github.com/login/oauth/authorize', - 'token_url' => 'https://github.com/login/oauth/access_token' - } - } - ) - - stub_omniauth_setting(providers: [provider]) - end end end diff --git a/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb b/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb new file mode 100644 index 00000000000..41de8c6bdbb --- /dev/null +++ b/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::RefreshBuildArtifactsSizeStatisticsService, :clean_gitlab_redis_shared_state do + let(:service) { described_class.new } + + describe '#execute' do + let_it_be(:project) { create(:project) } + + let_it_be(:artifact_1) { create(:ci_job_artifact, project: project, size: 1, created_at: 14.days.ago) } + let_it_be(:artifact_2) { create(:ci_job_artifact, project: project, size: 2, created_at: 13.days.ago) } + let_it_be(:artifact_3) { create(:ci_job_artifact, project: project, size: 5, created_at: 12.days.ago) } + + # This should not be included in the recalculation as it is created later than the refresh start time + let_it_be(:future_artifact) { create(:ci_job_artifact, project: project, size: 8, created_at: 2.days.from_now) } + + let!(:refresh) do + create( + :project_build_artifacts_size_refresh, + :created, + project: project, + updated_at: 2.days.ago, + refresh_started_at: nil, + last_job_artifact_id: nil + ) + end + + let(:now) { Time.zone.now } + + around do |example| + freeze_time { example.run } + end + + before do + stub_const("#{described_class}::BATCH_SIZE", 2) + + stats = create(:project_statistics, project: project, build_artifacts_size: 120) + stats.increment_counter(:build_artifacts_size, 30) + end + + it 'resets the build artifacts size stats' do + expect { service.execute }.to change { project.statistics.reload.build_artifacts_size }.to(0) + end + + it 'increments the counter attribute by the total size of the current batch of artifacts' do + expect { service.execute }.to change { project.statistics.get_counter_value(:build_artifacts_size) }.to(3) + end + + it 'updates the last_job_artifact_id to the ID of the last artifact from the batch' do + expect { service.execute }.to change { refresh.reload.last_job_artifact_id.to_i }.to(artifact_2.id) + end + + it 'requeues the refresh job' do + service.execute + expect(refresh.reload).to be_pending + end + + context 'when an error happens after the recalculation has started' do + let!(:refresh) do + create( + :project_build_artifacts_size_refresh, + :pending, + project: project, + last_job_artifact_id: artifact_2.id + ) + end + + before do + allow(Gitlab::Redis::SharedState).to receive(:with).and_raise(StandardError, 'error') + + expect { service.execute }.to raise_error(StandardError) + end + + it 'keeps the last_job_artifact_id unchanged' do + expect(refresh.reload.last_job_artifact_id).to eq(artifact_2.id) + end + + it 'keeps the state of the refresh record at running' do + expect(refresh.reload).to be_running + end + end + + context 'when there are no more artifacts to recalculate for the next refresh job' do + let!(:refresh) do + create( + :project_build_artifacts_size_refresh, + :pending, + project: project, + updated_at: 2.days.ago, + refresh_started_at: now, + last_job_artifact_id: artifact_3.id + ) + end + + it 'deletes the refresh record' do + service.execute + expect(Projects::BuildArtifactsSizeRefresh.where(id: refresh.id)).not_to exist + end + end + end +end diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 5810024a1ef..6407b8d3940 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -39,7 +39,6 @@ RSpec.describe Projects::UpdatePagesService do expect(project.pages_deployed?).to be_falsey expect(execute).to eq(:success) expect(project.pages_metadatum).to be_deployed - expect(project.pages_metadatum.artifacts_archive).to eq(artifacts_archive) expect(project.pages_deployed?).to be_truthy end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index afeb95a3ca3..94e0e8a9ea1 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -759,6 +759,7 @@ RSpec.describe QuickActions::InterpretService do context 'merge command' do let(:service) { described_class.new(project, developer, { merge_request_diff_head_sha: merge_request.diff_head_sha }) } + let(:merge_request) { create(:merge_request, source_project: repository_project) } it_behaves_like 'merge immediately command' do let(:content) { '/merge' } @@ -789,7 +790,7 @@ RSpec.describe QuickActions::InterpretService do context 'can not be merged when sha does not match' do let(:service) { described_class.new(project, developer, { merge_request_diff_head_sha: 'othersha' }) } - it_behaves_like 'failed command', 'Could not apply merge command.' do + it_behaves_like 'failed command', 'Branch has been updated since the merge was requested.' do let(:content) { "/merge" } let(:issuable) { merge_request } end @@ -799,10 +800,9 @@ RSpec.describe QuickActions::InterpretService do let(:project) { repository_project } let(:service) { described_class.new(project, developer, {}) } - it 'precheck passes and returns merge command' do - _, updates, _ = service.execute('/merge', merge_request) - - expect(updates).to eq(merge: nil) + it_behaves_like 'failed command', 'Merge request diff sha parameter is required for the merge quick action.' do + let(:content) { "/merge" } + let(:issuable) { merge_request } end end diff --git a/spec/services/repositories/destroy_rollback_service_spec.rb b/spec/services/repositories/destroy_rollback_service_spec.rb index 717e52f0e40..a52dff62760 100644 --- a/spec/services/repositories/destroy_rollback_service_spec.rb +++ b/spec/services/repositories/destroy_rollback_service_spec.rb @@ -43,16 +43,19 @@ RSpec.describe Repositories::DestroyRollbackService do expect(repository).to receive(:disk_path).and_return('foo') expect(repository).not_to receive(:before_delete) - result = subject + expect(subject[:status]).to eq :success + end - expect(result[:status]).to eq :success + it 'gracefully handles exception if the repository does not exist on disk' do + expect(repository).to receive(:before_delete).and_raise(Gitlab::Git::Repository::NoRepository) + expect(subject[:status]).to eq :success end context 'when move operation cannot be performed' do let(:service) { described_class.new(repository) } before do - allow(service).to receive(:mv_repository).and_return(false) + expect(service).to receive(:mv_repository).and_return(false) end it 'returns error' do @@ -66,6 +69,14 @@ RSpec.describe Repositories::DestroyRollbackService do service.execute end + + context 'when repository does not exist' do + it 'returns success' do + allow(service).to receive(:repo_exists?).and_return(true, false) + + expect(service.execute[:status]).to eq :success + end + end end def destroy_project(project, user) diff --git a/spec/services/repositories/destroy_service_spec.rb b/spec/services/repositories/destroy_service_spec.rb index 240f837e973..3766467d708 100644 --- a/spec/services/repositories/destroy_service_spec.rb +++ b/spec/services/repositories/destroy_service_spec.rb @@ -69,22 +69,23 @@ RSpec.describe Repositories::DestroyService do expect(repository).to receive(:disk_path).and_return('foo') expect(repository).not_to receive(:before_delete) - result = subject + expect(subject[:status]).to eq :success + end - expect(result[:status]).to eq :success + it 'gracefully handles exception if the repository does not exist on disk' do + expect(repository).to receive(:before_delete).and_raise(Gitlab::Git::Repository::NoRepository) + expect(subject[:status]).to eq :success end context 'when move operation cannot be performed' do let(:service) { described_class.new(repository) } before do - allow(service).to receive(:mv_repository).and_return(false) + expect(service).to receive(:mv_repository).and_return(false) end it 'returns error' do - result = service.execute - - expect(result[:status]).to eq :error + expect(service.execute[:status]).to eq :error end it 'logs the error' do @@ -92,6 +93,15 @@ RSpec.describe Repositories::DestroyService do service.execute end + + context 'when repository does not exist' do + it 'returns success' do + allow(service).to receive(:repo_exists?).and_return(true, false) + + expect(Repositories::ShellDestroyService).not_to receive(:new) + expect(service.execute[:status]).to eq :success + end + end end context 'with a project wiki repository' do diff --git a/spec/services/security/merge_reports_service_spec.rb b/spec/services/security/merge_reports_service_spec.rb index 120ce12aa58..e61977297c5 100644 --- a/spec/services/security/merge_reports_service_spec.rb +++ b/spec/services/security/merge_reports_service_spec.rb @@ -153,7 +153,18 @@ RSpec.describe Security::MergeReportsService, '#execute' do report_2.add_error('zoo', 'baz') end - it { is_expected.to eq([{ type: 'foo', message: 'bar' }, { type: 'zoo', message: 'baz' }]) } + it { is_expected.to match_array([{ type: 'foo', message: 'bar' }, { type: 'zoo', message: 'baz' }]) } + end + + describe 'warnings on target report' do + subject { merged_report.warnings } + + before do + report_1.add_warning('foo', 'bar') + report_2.add_warning('zoo', 'baz') + end + + it { is_expected.to match_array([{ type: 'foo', message: 'bar' }, { type: 'zoo', message: 'baz' }]) } end it 'copies scanners into target report and eliminates duplicates' do diff --git a/spec/services/service_ping/build_payload_service_spec.rb b/spec/services/service_ping/build_payload_service_spec.rb index cd2685069c9..b90e5e66518 100644 --- a/spec/services/service_ping/build_payload_service_spec.rb +++ b/spec/services/service_ping/build_payload_service_spec.rb @@ -4,6 +4,10 @@ require 'spec_helper' RSpec.describe ServicePing::BuildPayloadService do describe '#execute', :without_license do + before do + stub_feature_flags(merge_service_ping_instrumented_metrics: false) + end + subject(:service_ping_payload) { described_class.new.execute } include_context 'stubbed service ping metrics definitions' do diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb index 8ddfa7ed3a0..bd8418d7092 100644 --- a/spec/services/spam/spam_action_service_spec.rb +++ b/spec/services/spam/spam_action_service_spec.rb @@ -170,26 +170,13 @@ RSpec.describe Spam::SpamActionService do allow(fake_verdict_service).to receive(:execute).and_return(DISALLOW) end - context 'when allow_possible_spam feature flag is false' do - before do - stub_feature_flags(allow_possible_spam: false) - end + it_behaves_like 'creates a spam log' - it 'marks as spam' do - response = subject - - expect(response.message).to match(expected_service_check_response_message) - expect(issue).to be_spam - end - end - - context 'when allow_possible_spam feature flag is true' do - it 'does not mark as spam' do - response = subject + it 'marks as spam' do + response = subject - expect(response.message).to match(expected_service_check_response_message) - expect(issue).not_to be_spam - end + expect(response.message).to match(expected_service_check_response_message) + expect(issue).to be_spam end end @@ -198,26 +185,13 @@ RSpec.describe Spam::SpamActionService do allow(fake_verdict_service).to receive(:execute).and_return(BLOCK_USER) end - context 'when allow_possible_spam feature flag is false' do - before do - stub_feature_flags(allow_possible_spam: false) - end - - it 'marks as spam' do - response = subject + it_behaves_like 'creates a spam log' - expect(response.message).to match(expected_service_check_response_message) - expect(issue).to be_spam - end - end - - context 'when allow_possible_spam feature flag is true' do - it 'does not mark as spam' do - response = subject + it 'marks as spam' do + response = subject - expect(response.message).to match(expected_service_check_response_message) - expect(issue).not_to be_spam - end + expect(response.message).to match(expected_service_check_response_message) + expect(issue).to be_spam end end @@ -226,37 +200,42 @@ RSpec.describe Spam::SpamActionService do allow(fake_verdict_service).to receive(:execute).and_return(CONDITIONAL_ALLOW) end - context 'when allow_possible_spam feature flag is false' do - before do - stub_feature_flags(allow_possible_spam: false) - end + it_behaves_like 'creates a spam log' - it_behaves_like 'creates a spam log' + it 'does not mark as spam' do + response = subject - it 'does not mark as spam' do - response = subject + expect(response.message).to match(expected_service_check_response_message) + expect(issue).not_to be_spam + end - expect(response.message).to match(expected_service_check_response_message) - expect(issue).not_to be_spam - end + it 'marks as needing reCAPTCHA' do + response = subject - it 'marks as needing reCAPTCHA' do - response = subject + expect(response.message).to match(expected_service_check_response_message) + expect(issue).to be_needs_recaptcha + end + end - expect(response.message).to match(expected_service_check_response_message) - expect(issue).to be_needs_recaptcha - end + context 'when spam verdict service returns OVERRIDE_VIA_ALLOW_POSSIBLE_SPAM' do + before do + allow(fake_verdict_service).to receive(:execute).and_return(OVERRIDE_VIA_ALLOW_POSSIBLE_SPAM) end - context 'when allow_possible_spam feature flag is true' do - it_behaves_like 'creates a spam log' + it_behaves_like 'creates a spam log' - it 'does not mark as needing reCAPTCHA' do - response = subject + it 'does not mark as spam' do + response = subject + + expect(response.message).to match(expected_service_check_response_message) + expect(issue).not_to be_spam + end + + it 'does not mark as needing CAPTCHA' do + response = subject - expect(response.message).to match(expected_service_check_response_message) - expect(issue.needs_recaptcha).to be_falsey - end + expect(response.message).to match(expected_service_check_response_message) + expect(issue).not_to be_needs_recaptcha end end diff --git a/spec/services/spam/spam_params_spec.rb b/spec/services/spam/spam_params_spec.rb index e7e8b468adb..7e74641c0fa 100644 --- a/spec/services/spam/spam_params_spec.rb +++ b/spec/services/spam/spam_params_spec.rb @@ -3,18 +3,25 @@ require 'spec_helper' RSpec.describe Spam::SpamParams do + shared_examples 'constructs from a request' do + it 'constructs from a request' do + expected = ::Spam::SpamParams.new( + captcha_response: captcha_response, + spam_log_id: spam_log_id, + ip_address: ip_address, + user_agent: user_agent, + referer: referer + ) + expect(described_class.new_from_request(request: request)).to eq(expected) + end + end + describe '.new_from_request' do let(:captcha_response) { 'abc123' } let(:spam_log_id) { 42 } let(:ip_address) { '0.0.0.0' } let(:user_agent) { 'Lynx' } let(:referer) { 'http://localhost' } - let(:headers) do - { - 'X-GitLab-Captcha-Response' => captcha_response, - 'X-GitLab-Spam-Log-Id' => spam_log_id - } - end let(:env) do { @@ -24,17 +31,28 @@ RSpec.describe Spam::SpamParams do } end - let(:request) {double(:request, headers: headers, env: env)} + let(:request) { double(:request, headers: headers, env: env) } - it 'constructs from a request' do - expected = ::Spam::SpamParams.new( - captcha_response: captcha_response, - spam_log_id: spam_log_id, - ip_address: ip_address, - user_agent: user_agent, - referer: referer - ) - expect(described_class.new_from_request(request: request)).to eq(expected) + context 'with a normal Rails request' do + let(:headers) do + { + 'X-GitLab-Captcha-Response' => captcha_response, + 'X-GitLab-Spam-Log-Id' => spam_log_id + } + end + + it_behaves_like 'constructs from a request' + end + + context 'with a grape request' do + let(:headers) do + { + 'X-Gitlab-Captcha-Response' => captcha_response, + 'X-Gitlab-Spam-Log-Id' => spam_log_id + } + end + + it_behaves_like 'constructs from a request' end end end diff --git a/spec/services/spam/spam_verdict_service_spec.rb b/spec/services/spam/spam_verdict_service_spec.rb index 99047f3233b..082b8f909f9 100644 --- a/spec/services/spam/spam_verdict_service_spec.rb +++ b/spec/services/spam/spam_verdict_service_spec.rb @@ -27,6 +27,10 @@ RSpec.describe Spam::SpamVerdictService do extra_attributes end + before do + stub_feature_flags(allow_possible_spam: false) + end + describe '#execute' do subject { service.execute } @@ -114,6 +118,32 @@ RSpec.describe Spam::SpamVerdictService do end end + context 'if allow_possible_spam flag is true' do + before do + stub_feature_flags(allow_possible_spam: true) + end + + context 'and a service returns a verdict that should be overridden' do + before do + allow(service).to receive(:spamcheck_verdict).and_return([BLOCK_USER, attribs]) + end + + it 'overrides and renders the override verdict' do + expect(subject).to eq OVERRIDE_VIA_ALLOW_POSSIBLE_SPAM + end + end + + context 'and a service returns a verdict that does not need to be overridden' do + before do + allow(service).to receive(:spamcheck_verdict).and_return([ALLOW, attribs]) + end + + it 'does not override and renders the original verdict' do + expect(subject).to eq ALLOW + end + end + end + context 'records metrics' do let(:histogram) { instance_double(Prometheus::Client::Histogram) } diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index a719487a219..c322ec35e86 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -100,7 +100,7 @@ RSpec.describe SystemNoteService do end end - describe '.relate_issue' do + describe '.relate_issuable' do let(:noteable_ref) { double } let(:noteable) { double } @@ -110,14 +110,14 @@ RSpec.describe SystemNoteService do it 'calls IssuableService' do expect_next_instance_of(::SystemNotes::IssuablesService) do |service| - expect(service).to receive(:relate_issue).with(noteable_ref) + expect(service).to receive(:relate_issuable).with(noteable_ref) end - described_class.relate_issue(noteable, noteable_ref, double) + described_class.relate_issuable(noteable, noteable_ref, double) end end - describe '.unrelate_issue' do + describe '.unrelate_issuable' do let(:noteable_ref) { double } let(:noteable) { double } @@ -127,10 +127,10 @@ RSpec.describe SystemNoteService do it 'calls IssuableService' do expect_next_instance_of(::SystemNotes::IssuablesService) do |service| - expect(service).to receive(:unrelate_issue).with(noteable_ref) + expect(service).to receive(:unrelate_issuable).with(noteable_ref) end - described_class.unrelate_issue(noteable, noteable_ref, double) + described_class.unrelate_issuable(noteable, noteable_ref, double) end end diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index e1c97026418..5bc7ea82976 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -14,10 +14,10 @@ RSpec.describe ::SystemNotes::IssuablesService do let(:service) { described_class.new(noteable: noteable, project: project, author: author) } - describe '#relate_issue' do + describe '#relate_issuable' do let(:noteable_ref) { create(:issue) } - subject { service.relate_issue(noteable_ref) } + subject { service.relate_issuable(noteable_ref) } it_behaves_like 'a system note' do let(:action) { 'relate' } @@ -30,10 +30,10 @@ RSpec.describe ::SystemNotes::IssuablesService do end end - describe '#unrelate_issue' do + describe '#unrelate_issuable' do let(:noteable_ref) { create(:issue) } - subject { service.unrelate_issue(noteable_ref) } + subject { service.unrelate_issuable(noteable_ref) } it_behaves_like 'a system note' do let(:action) { 'unrelate' } diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 7103cb0b66a..6e10d0281b7 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -628,12 +628,32 @@ RSpec.describe TodoService do stub_feature_flags(multiple_todos: true) end - it 'creates a todo even if user already has a pending todo' do + it 'creates a MENTIONED todo even if user already has a pending MENTIONED todo' do create(:todo, :mentioned, user: member, project: project, target: issue, author: author) expect { service.update_issue(issue, author) }.to change(member.todos, :count) end + it 'creates a DIRECTLY_ADDRESSED todo even if user already has a pending DIRECTLY_ADDRESSED todo' do + create(:todo, :directly_addressed, user: member, project: project, target: issue, author: author) + + issue.update!(description: "#{member.to_reference}, what do you think?") + + expect { service.update_issue(issue, author) }.to change(member.todos, :count) + end + + it 'creates an ASSIGNED todo even if user already has a pending MARKED todo' do + create(:todo, :marked, user: john_doe, project: project, target: assigned_issue, author: author) + + expect { service.reassigned_assignable(assigned_issue, author) }.to change(john_doe.todos, :count) + end + + it 'does not create an ASSIGNED todo if user already has an ASSIGNED todo' do + create(:todo, :assigned, user: john_doe, project: project, target: assigned_issue, author: author) + + expect { service.reassigned_assignable(assigned_issue, author) }.not_to change(john_doe.todos, :count) + end + it 'creates multiple todos if a user is assigned and mentioned in a new issue' do assigned_issue.description = mentions service.new_issue(assigned_issue, author) diff --git a/spec/services/users/refresh_authorized_projects_service_spec.rb b/spec/services/users/refresh_authorized_projects_service_spec.rb index a31902c7f16..e6ccb2b16e7 100644 --- a/spec/services/users/refresh_authorized_projects_service_spec.rb +++ b/spec/services/users/refresh_authorized_projects_service_spec.rb @@ -52,7 +52,7 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do it 'is called' do ProjectAuthorization.delete_all - expect(callback).to receive(:call).with(project.id, Gitlab::Access::MAINTAINER).once + expect(callback).to receive(:call).with(project.id, Gitlab::Access::OWNER).once service.execute end @@ -73,7 +73,7 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do to_be_removed = [project_authorization.project_id] to_be_added = [ - { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER } + { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::OWNER } ] expect(service).to receive(:update_authorizations) @@ -82,31 +82,6 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do service.execute_without_lease end - it 'removes duplicate entries' do - [Gitlab::Access::MAINTAINER, Gitlab::Access::REPORTER].each do |access_level| - user.project_authorizations.create!(project: project, access_level: access_level) - end - - to_be_removed = [project.id] - - to_be_added = [ - { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER } - ] - expect(service).to( - receive(:update_authorizations) - .with(to_be_removed, to_be_added) - .and_call_original) - - service.execute_without_lease - - expect(user.project_authorizations.count).to eq(1) - project_authorization = ProjectAuthorization.where( - project_id: project.id, - user_id: user.id, - access_level: Gitlab::Access::MAINTAINER) - expect(project_authorization).to exist - end - it 'sets the access level of a project to the highest available level' do user.project_authorizations.delete_all @@ -116,7 +91,7 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do to_be_removed = [project_authorization.project_id] to_be_added = [ - { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::MAINTAINER } + { user_id: user.id, project_id: project.id, access_level: Gitlab::Access::OWNER } ] expect(service).to receive(:update_authorizations) diff --git a/spec/services/users/saved_replies/create_service_spec.rb b/spec/services/users/saved_replies/create_service_spec.rb new file mode 100644 index 00000000000..e01b6248308 --- /dev/null +++ b/spec/services/users/saved_replies/create_service_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::SavedReplies::CreateService do + describe '#execute' do + let_it_be(:current_user) { create(:user) } + let_it_be(:saved_reply) { create(:saved_reply, user: current_user) } + + subject { described_class.new(current_user: current_user, name: name, content: content).execute } + + context 'when create fails' do + let(:name) { saved_reply.name } + let(:content) { '' } + + it { is_expected.not_to be_success } + + it 'does not create new Saved Reply in database' do + expect { subject }.not_to change(::Users::SavedReply, :count) + end + + it 'returns error messages' do + expect(subject.errors).to match_array(["Content can't be blank", "Name has already been taken"]) + end + end + + context 'when create succeeds' do + let(:name) { 'new_saved_reply_name' } + let(:content) { 'New content for Saved Reply' } + + it { is_expected.to be_success } + + it 'creates new Saved Reply in database' do + expect { subject }.to change(::Users::SavedReply, :count).by(1) + end + + it 'returns new saved reply', :aggregate_failures do + expect(subject[:saved_reply]).to eq(::Users::SavedReply.last) + expect(subject[:saved_reply].name).to eq(name) + expect(subject[:saved_reply].content).to eq(content) + end + end + end +end diff --git a/spec/services/users/saved_replies/update_service_spec.rb b/spec/services/users/saved_replies/update_service_spec.rb new file mode 100644 index 00000000000..b67d09977c6 --- /dev/null +++ b/spec/services/users/saved_replies/update_service_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::SavedReplies::UpdateService do + describe '#execute' do + let_it_be(:current_user) { create(:user) } + let_it_be(:saved_reply) { create(:saved_reply, user: current_user) } + let_it_be(:other_saved_reply) { create(:saved_reply, user: current_user) } + let_it_be(:saved_reply_from_other_user) { create(:saved_reply) } + + subject { described_class.new(current_user: current_user, saved_reply: saved_reply, name: name, content: content).execute } + + context 'when update fails' do + let(:name) { other_saved_reply.name } + let(:content) { '' } + + it { is_expected.not_to be_success } + + it 'returns error messages' do + expect(subject.errors).to match_array(["Content can't be blank", "Name has already been taken"]) + end + end + + context 'when update succeeds' do + let(:name) { saved_reply_from_other_user.name } + let(:content) { 'New content for Saved Reply' } + + it { is_expected.to be_success } + + it 'updates new Saved Reply in database' do + expect { subject }.not_to change(::Users::SavedReply, :count) + end + + it 'returns saved reply' do + expect(subject[:saved_reply]).to eq(saved_reply) + end + end + end +end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 64371f97908..c938ad9ee39 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -14,10 +14,6 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state let(:service_instance) { described_class.new(project_hook, data, :push_hooks) } - around do |example| - travel_to(Time.current) { example.run } - end - describe '#initialize' do before do stub_application_setting(setting_name => setting) @@ -257,14 +253,6 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state end context 'execution logging' do - let(:hook_log) { project_hook.web_hook_logs.last } - - def run_service - service_instance.execute - ::WebHooks::LogExecutionWorker.drain - project_hook.reload - end - context 'with success' do before do stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success') @@ -280,42 +268,38 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state .with(hook: project_hook, log_data: Hash, response_category: :ok) .and_return(double(execute: nil)) - run_service + service_instance.execute end end - it 'log successful execution' do - run_service - - expect(hook_log.trigger).to eq('push_hooks') - expect(hook_log.url).to eq(project_hook.url) - expect(hook_log.request_headers).to eq(headers) - expect(hook_log.response_body).to eq('Success') - expect(hook_log.response_status).to eq('200') - expect(hook_log.execution_duration).to be > 0 - expect(hook_log.internal_error_message).to be_nil - end - - it 'does not log in the service itself' do - expect { service_instance.execute }.not_to change(::WebHookLog, :count) - end + it 'queues LogExecutionWorker correctly' do + expect(WebHooks::LogExecutionWorker).to receive(:perform_async) + .with( + project_hook.id, + hash_including( + trigger: 'push_hooks', + url: project_hook.url, + request_headers: headers, + request_data: data, + response_body: 'Success', + response_headers: {}, + response_status: 200, + execution_duration: be > 0, + internal_error_message: nil + ), + :ok, + nil + ) - it 'does not increment the failure count' do - expect { run_service }.not_to change(project_hook, :recent_failures) + service_instance.execute end - it 'does not change the disabled_until attribute' do - expect { run_service }.not_to change(project_hook, :disabled_until) + it 'queues LogExecutionWorker correctly, resulting in a log record (integration-style test)', :sidekiq_inline do + expect { service_instance.execute }.to change(::WebHookLog, :count).by(1) end - context 'when the hook had previously failed' do - before do - project_hook.update!(recent_failures: 2) - end - - it 'resets the failure count' do - expect { run_service }.to change(project_hook, :recent_failures).to(0) - end + it 'does not log in the service itself' do + expect { service_instance.execute }.not_to change(::WebHookLog, :count) end end @@ -324,45 +308,26 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state stub_full_request(project_hook.url, method: :post).to_return(status: 400, body: 'Bad request') end - it 'logs failed execution' do - run_service - - expect(hook_log).to have_attributes( - trigger: eq('push_hooks'), - url: eq(project_hook.url), - request_headers: eq(headers), - response_body: eq('Bad request'), - response_status: eq('400'), - execution_duration: be > 0, - internal_error_message: be_nil - ) - end - - it 'increments the failure count' do - expect { run_service }.to change(project_hook, :recent_failures).by(1) - end - - it 'does not change the disabled_until attribute' do - expect { run_service }.not_to change(project_hook, :disabled_until) - end - - it 'does not allow the failure count to overflow' do - project_hook.update!(recent_failures: 32767) - - expect { run_service }.not_to change(project_hook, :recent_failures) - end - - context 'when the web_hooks_disable_failed FF is disabled' do - before do - # Hook will only be executed if the flag is disabled. - stub_feature_flags(web_hooks_disable_failed: false) - end - - it 'does not allow the failure count to overflow' do - project_hook.update!(recent_failures: 32767) + it 'queues LogExecutionWorker correctly' do + expect(WebHooks::LogExecutionWorker).to receive(:perform_async) + .with( + project_hook.id, + hash_including( + trigger: 'push_hooks', + url: project_hook.url, + request_headers: headers, + request_data: data, + response_body: 'Bad request', + response_headers: {}, + response_status: 400, + execution_duration: be > 0, + internal_error_message: nil + ), + :failed, + nil + ) - expect { run_service }.not_to change(project_hook, :recent_failures) - end + service_instance.execute end end @@ -371,65 +336,54 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state stub_full_request(project_hook.url, method: :post).to_raise(SocketError.new('Some HTTP Post error')) end - it 'log failed execution' do - run_service - - expect(hook_log.trigger).to eq('push_hooks') - expect(hook_log.url).to eq(project_hook.url) - expect(hook_log.request_headers).to eq(headers) - expect(hook_log.response_body).to eq('') - expect(hook_log.response_status).to eq('internal error') - expect(hook_log.execution_duration).to be > 0 - expect(hook_log.internal_error_message).to eq('Some HTTP Post error') - end - - it 'does not increment the failure count' do - expect { run_service }.not_to change(project_hook, :recent_failures) - end - - it 'backs off' do - expect { run_service }.to change(project_hook, :disabled_until) - end - - it 'increases the backoff count' do - expect { run_service }.to change(project_hook, :backoff_count).by(1) - end - - context 'when the previous cool-off was near the maximum' do - before do - project_hook.update!(disabled_until: 5.minutes.ago, backoff_count: 8) - end - - it 'sets the disabled_until attribute' do - expect { run_service }.to change(project_hook, :disabled_until).to(1.day.from_now) - end - end - - context 'when we have backed-off many many times' do - before do - project_hook.update!(disabled_until: 5.minutes.ago, backoff_count: 365) - end + it 'queues LogExecutionWorker correctly' do + expect(WebHooks::LogExecutionWorker).to receive(:perform_async) + .with( + project_hook.id, + hash_including( + trigger: 'push_hooks', + url: project_hook.url, + request_headers: headers, + request_data: data, + response_body: '', + response_headers: {}, + response_status: 'internal error', + execution_duration: be > 0, + internal_error_message: 'Some HTTP Post error' + ), + :error, + nil + ) - it 'sets the disabled_until attribute' do - expect { run_service }.to change(project_hook, :disabled_until).to(1.day.from_now) - end + service_instance.execute end end context 'with unsafe response body' do before do stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: "\xBB") - run_service end - it 'log successful execution' do - expect(hook_log.trigger).to eq('push_hooks') - expect(hook_log.url).to eq(project_hook.url) - expect(hook_log.request_headers).to eq(headers) - expect(hook_log.response_body).to eq('') - expect(hook_log.response_status).to eq('200') - expect(hook_log.execution_duration).to be > 0 - expect(hook_log.internal_error_message).to be_nil + it 'queues LogExecutionWorker with sanitized response_body' do + expect(WebHooks::LogExecutionWorker).to receive(:perform_async) + .with( + project_hook.id, + hash_including( + trigger: 'push_hooks', + url: project_hook.url, + request_headers: headers, + request_data: data, + response_body: '', + response_headers: {}, + response_status: 200, + execution_duration: be > 0, + internal_error_message: nil + ), + :ok, + nil + ) + + service_instance.execute end end end diff --git a/spec/services/web_hooks/log_execution_service_spec.rb b/spec/services/web_hooks/log_execution_service_spec.rb new file mode 100644 index 00000000000..0ba0372b99d --- /dev/null +++ b/spec/services/web_hooks/log_execution_service_spec.rb @@ -0,0 +1,237 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WebHooks::LogExecutionService do + include ExclusiveLeaseHelpers + using RSpec::Parameterized::TableSyntax + + describe '#execute' do + around do |example| + travel_to(Time.current) { example.run } + end + + let_it_be_with_reload(:project_hook) { create(:project_hook) } + + let(:response_category) { :ok } + let(:data) do + { + trigger: 'trigger_name', + url: 'https://example.com', + request_headers: { 'Header' => 'header value' }, + request_data: { 'Request Data' => 'request data value' }, + response_body: 'Response body', + response_status: '200', + execution_duration: 1.2, + internal_error_message: 'error message' + } + end + + subject(:service) { described_class.new(hook: project_hook, log_data: data, response_category: response_category) } + + it 'logs the data' do + expect { service.execute }.to change(::WebHookLog, :count).by(1) + + expect(WebHookLog.recent.first).to have_attributes(data) + end + + context 'obtaining an exclusive lease' do + let(:lease_key) { "web_hooks:update_hook_failure_state:#{project_hook.id}" } + + it 'updates failure state using a lease that ensures fresh state is written' do + service = described_class.new(hook: project_hook, log_data: data, response_category: :error) + WebHook.find(project_hook.id).update!(backoff_count: 1) + + lease = stub_exclusive_lease(lease_key, timeout: described_class::LOCK_TTL) + + expect(lease).to receive(:try_obtain) + expect(lease).to receive(:cancel) + expect { service.execute }.to change { WebHook.find(project_hook.id).backoff_count }.to(2) + end + + context 'when a lease cannot be obtained' do + where(:response_category, :executable, :needs_updating) do + :ok | true | false + :ok | false | true + :failed | true | true + :failed | false | false + :error | true | true + :error | false | false + end + + with_them do + subject(:service) { described_class.new(hook: project_hook, log_data: data, response_category: response_category) } + + before do + stub_exclusive_lease_taken(lease_key, timeout: described_class::LOCK_TTL) + allow(project_hook).to receive(:executable?).and_return(executable) + end + + it 'raises an error if the hook needs to be updated' do + if needs_updating + expect { service.execute }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) + else + expect { service.execute }.not_to raise_error + end + end + end + end + end + + context 'when response_category is :ok' do + it 'does not increment the failure count' do + expect { service.execute }.not_to change(project_hook, :recent_failures) + end + + it 'does not change the disabled_until attribute' do + expect { service.execute }.not_to change(project_hook, :disabled_until) + end + + context 'when the hook had previously failed' do + before do + project_hook.update!(recent_failures: 2) + end + + it 'resets the failure count' do + expect { service.execute }.to change(project_hook, :recent_failures).to(0) + end + + it 'sends a message to AuthLogger if the hook as not previously enabled' do + project_hook.update!(recent_failures: ::WebHook::FAILURE_THRESHOLD + 1) + + expect(Gitlab::AuthLogger).to receive(:info).with include( + message: 'WebHook change active_state', + # identification + hook_id: project_hook.id, + hook_type: project_hook.type, + project_id: project_hook.project_id, + group_id: nil, + # relevant data + prev_state: :permanently_disabled, + new_state: :enabled, + duration: 1.2, + response_status: '200', + recent_hook_failures: 0 + ) + + service.execute + end + end + end + + context 'when response_category is :failed' do + let(:response_category) { :failed } + + before do + data[:response_status] = '400' + end + + it 'increments the failure count' do + expect { service.execute }.to change(project_hook, :recent_failures).by(1) + end + + it 'does not change the disabled_until attribute' do + expect { service.execute }.not_to change(project_hook, :disabled_until) + end + + it 'does not allow the failure count to overflow' do + project_hook.update!(recent_failures: 32767) + + expect { service.execute }.not_to change(project_hook, :recent_failures) + end + + context 'when the web_hooks_disable_failed FF is disabled' do + before do + # Hook will only be executed if the flag is disabled. + stub_feature_flags(web_hooks_disable_failed: false) + end + + it 'does not allow the failure count to overflow' do + project_hook.update!(recent_failures: 32767) + + expect { service.execute }.not_to change(project_hook, :recent_failures) + end + end + + it 'sends a message to AuthLogger if the state would change' do + project_hook.update!(recent_failures: ::WebHook::FAILURE_THRESHOLD) + + expect(Gitlab::AuthLogger).to receive(:info).with include( + message: 'WebHook change active_state', + # identification + hook_id: project_hook.id, + hook_type: project_hook.type, + project_id: project_hook.project_id, + group_id: nil, + # relevant data + prev_state: :enabled, + new_state: :permanently_disabled, + duration: (be > 0), + response_status: data[:response_status], + recent_hook_failures: ::WebHook::FAILURE_THRESHOLD + 1 + ) + + service.execute + end + end + + context 'when response_category is :error' do + let(:response_category) { :error } + + before do + data[:response_status] = '500' + end + + it 'does not increment the failure count' do + expect { service.execute }.not_to change(project_hook, :recent_failures) + end + + it 'backs off' do + expect { service.execute }.to change(project_hook, :disabled_until) + end + + it 'increases the backoff count' do + expect { service.execute }.to change(project_hook, :backoff_count).by(1) + end + + it 'sends a message to AuthLogger if the state would change' do + expect(Gitlab::AuthLogger).to receive(:info).with include( + message: 'WebHook change active_state', + # identification + hook_id: project_hook.id, + hook_type: project_hook.type, + project_id: project_hook.project_id, + group_id: nil, + # relevant data + prev_state: :enabled, + new_state: :temporarily_disabled, + duration: (be > 0), + response_status: data[:response_status], + recent_hook_failures: 0 + ) + + service.execute + end + + context 'when the previous cool-off was near the maximum' do + before do + project_hook.update!(disabled_until: 5.minutes.ago, backoff_count: 8) + end + + it 'sets the disabled_until attribute' do + expect { service.execute }.to change(project_hook, :disabled_until).to(1.day.from_now) + end + end + + context 'when we have backed-off many many times' do + before do + project_hook.update!(disabled_until: 5.minutes.ago, backoff_count: 365) + end + + it 'sets the disabled_until attribute' do + expect { service.execute }.to change(project_hook, :disabled_until).to(1.day.from_now) + end + end + end + end +end diff --git a/spec/services/work_items/create_and_link_service_spec.rb b/spec/services/work_items/create_and_link_service_spec.rb new file mode 100644 index 00000000000..93c029bdab1 --- /dev/null +++ b/spec/services/work_items/create_and_link_service_spec.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::CreateAndLinkService do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:user) { create(:user) } + let_it_be(:related_work_item) { create(:work_item, project: project) } + + let(:spam_params) { double } + let(:link_params) { {} } + let(:params) do + { + title: 'Awesome work item', + description: 'please fix' + } + end + + before_all do + project.add_developer(user) + end + + describe '#execute' do + subject(:service_result) { described_class.new(project: project, current_user: user, params: params, spam_params: spam_params, link_params: link_params).execute } + + before do + stub_spam_services + end + + context 'when work item params are valid' do + it { is_expected.to be_success } + + it 'creates a work item successfully with no links' do + expect do + service_result + end.to change(WorkItem, :count).by(1).and( + not_change(IssueLink, :count) + ) + end + + context 'when link params are valid' do + let(:link_params) { { issuable_references: [related_work_item.to_reference] } } + + it 'creates a work item successfully with links' do + expect do + service_result + end.to change(WorkItem, :count).by(1).and( + change(IssueLink, :count).by(1) + ) + end + end + + context 'when link params are invalid' do + let(:link_params) { { issuable_references: ['invalid reference'] } } + + it { is_expected.to be_error } + + it 'does not create a link and does not rollback transaction' do + expect do + service_result + end.to not_change(IssueLink, :count).and( + change(WorkItem, :count).by(1) + ) + end + + it 'returns a link creation error message' do + expect(service_result.errors).to contain_exactly('No matching issue found. Make sure that you are adding a valid issue URL.') + end + end + end + + context 'when work item params are invalid' do + let(:params) do + { + title: '', + description: 'invalid work item' + } + end + + it { is_expected.to be_error } + + it 'does not create a work item or links' do + expect do + service_result + end.to not_change(WorkItem, :count).and( + not_change(IssueLink, :count) + ) + end + + it 'returns work item errors' do + expect(service_result.errors).to contain_exactly("Title can't be blank") + end + end + end +end diff --git a/spec/services/work_items/create_from_task_service_spec.rb b/spec/services/work_items/create_from_task_service_spec.rb new file mode 100644 index 00000000000..b4db925f053 --- /dev/null +++ b/spec/services/work_items/create_from_task_service_spec.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::CreateFromTaskService do + let_it_be(:project) { create(:project) } + let_it_be(:developer) { create(:user) } + let_it_be(:list_work_item, refind: true) { create(:work_item, project: project, description: "- [ ] Item to be converted\n second line\n third line") } + + let(:work_item_to_update) { list_work_item } + let(:spam_params) { double } + let(:link_params) { {} } + let(:current_user) { developer } + let(:params) do + { + title: 'Awesome work item', + work_item_type_id: WorkItems::Type.default_by_type(:task).id, + line_number_start: 1, + line_number_end: 3, + lock_version: work_item_to_update.lock_version + } + end + + before_all do + project.add_developer(developer) + end + + shared_examples 'CreateFromTask service with invalid params' do + it { is_expected.to be_error } + + it 'does not create a work item or links' do + expect do + service_result + end.to not_change(WorkItem, :count).and( + not_change(IssueLink, :count) + ) + end + end + + describe '#execute' do + subject(:service_result) { described_class.new(work_item: work_item_to_update, current_user: current_user, work_item_params: params, spam_params: spam_params).execute } + + before do + stub_spam_services + end + + context 'when work item params are valid' do + it { is_expected.to be_success } + + it 'creates a work item and links it to the original work item successfully' do + expect do + service_result + end.to change(WorkItem, :count).by(1).and( + change(IssueLink, :count) + ) + end + + it 'replaces the original issue markdown description with new work item reference' do + service_result + + created_work_item = WorkItem.last + + expect(list_work_item.description).to eq("- [ ] #{created_work_item.to_reference}+") + end + end + + context 'when last operation fails' do + before do + params.merge!(line_number_start: 0) + end + + it 'rollbacks all operations' do + expect do + service_result + end.to not_change(WorkItem, :count).and( + not_change(IssueLink, :count) + ) + end + + it { is_expected.to be_error } + + it 'returns an error message' do + expect(service_result.errors).to contain_exactly('line_number_start must be greater than 0') + end + end + + context 'when work item params are invalid' do + let(:params) { { title: '' } } + + it_behaves_like 'CreateFromTask service with invalid params' + + it 'returns work item errors' do + expect(service_result.errors).to contain_exactly("Title can't be blank") + end + end + end +end diff --git a/spec/services/work_items/task_list_reference_replacement_service_spec.rb b/spec/services/work_items/task_list_reference_replacement_service_spec.rb new file mode 100644 index 00000000000..e7914eb4a92 --- /dev/null +++ b/spec/services/work_items/task_list_reference_replacement_service_spec.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::TaskListReferenceReplacementService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:single_line_work_item, refind: true) { create(:work_item, project: project, description: '- [ ] single line', lock_version: 3) } + let_it_be(:multiple_line_work_item, refind: true) { create(:work_item, project: project, description: "Any text\n\n* [ ] Item to be converted\n second line\n third line", lock_version: 3) } + + let(:line_number_start) { 3 } + let(:line_number_end) { 5 } + let(:title) { 'work item title' } + let(:reference) { 'any reference' } + let(:work_item) { multiple_line_work_item } + let(:lock_version) { 3 } + let(:expected_additional_text) { '' } + + shared_examples 'successful work item task reference replacement service' do + it { is_expected.to be_success } + + it 'replaces the original issue markdown description with new work item reference' do + result + + expect(work_item.description).to eq("#{expected_additional_text}#{task_prefix} #{reference}+") + end + end + + shared_examples 'failing work item task reference replacement service' do |error_message| + it { is_expected.to be_error } + + it 'returns an error message' do + expect(result.errors).to contain_exactly(error_message) + end + end + + describe '#execute' do + subject(:result) do + described_class.new( + work_item: work_item, + work_item_reference: reference, + line_number_start: line_number_start, + line_number_end: line_number_end, + title: title, + lock_version: lock_version + ).execute + end + + context 'when task mardown spans a single line' do + let(:line_number_start) { 1 } + let(:line_number_end) { 1 } + let(:work_item) { single_line_work_item } + let(:task_prefix) { '- [ ]' } + + it_behaves_like 'successful work item task reference replacement service' + end + + context 'when task mardown spans multiple lines' do + let(:task_prefix) { '* [ ]' } + let(:expected_additional_text) { "Any text\n\n" } + + it_behaves_like 'successful work item task reference replacement service' + end + + context 'when description does not contain a task' do + let_it_be(:no_matching_work_item) { create(:work_item, project: project, description: 'no matching task') } + + let(:work_item) { no_matching_work_item } + + it_behaves_like 'failing work item task reference replacement service', 'Unable to detect a task on line 3' + end + + context 'when description is empty' do + let_it_be(:empty_work_item) { create(:work_item, project: project, description: '') } + + let(:work_item) { empty_work_item } + + it_behaves_like 'failing work item task reference replacement service', "Work item description can't be blank" + end + + context 'when line_number_start is lower than 1' do + let(:line_number_start) { 0 } + + it_behaves_like 'failing work item task reference replacement service', 'line_number_start must be greater than 0' + end + + context 'when line_number_end is lower than line_number_start' do + let(:line_number_end) { line_number_start - 1 } + + it_behaves_like 'failing work item task reference replacement service', 'line_number_end must be greater or equal to line_number_start' + end + + context 'when lock_version is older than current' do + let(:lock_version) { 2 } + + it_behaves_like 'failing work item task reference replacement service', 'Stale work item. Check lock version' + end + + context 'when work item is stale before updating' do + it_behaves_like 'failing work item task reference replacement service', 'Stale work item. Check lock version' do + before do + ::WorkItem.where(id: work_item.id).update_all(lock_version: lock_version + 1) + end + end + end + end +end diff --git a/spec/services/work_items/update_service_spec.rb b/spec/services/work_items/update_service_spec.rb index f71f1060e40..b2d3f428899 100644 --- a/spec/services/work_items/update_service_spec.rb +++ b/spec/services/work_items/update_service_spec.rb @@ -23,6 +23,9 @@ RSpec.describe WorkItems::UpdateService do it 'triggers issuable_title_updated graphql subscription' do expect(GraphqlTriggers).to receive(:issuable_title_updated).with(work_item).and_call_original + expect(Gitlab::UsageDataCounters::WorkItemActivityUniqueCounter).to receive(:track_work_item_title_changed_action).with(author: current_user) + # During the work item transition we also want to track work items as issues + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_title_changed_action) update_work_item end @@ -33,6 +36,7 @@ RSpec.describe WorkItems::UpdateService do it 'does not trigger issuable_title_updated graphql subscription' do expect(GraphqlTriggers).not_to receive(:issuable_title_updated) + expect(Gitlab::UsageDataCounters::WorkItemActivityUniqueCounter).not_to receive(:track_work_item_title_changed_action) update_work_item end |