From a7b3560714b4d9cc4ab32dffcd1f74a284b93580 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Fri, 18 Feb 2022 09:45:46 +0000 Subject: Add latest changes from gitlab-org/gitlab@14-8-stable-ee --- .../alert_management/alerts/update_service_spec.rb | 13 +- .../create_alert_issue_service_spec.rb | 4 +- .../application_settings/update_service_spec.rb | 18 ++ ...ntainer_registry_authentication_service_spec.rb | 24 -- spec/services/branches/create_service_spec.rb | 2 +- ...opy_cross_database_associations_service_spec.rb | 18 ++ .../ci/create_downstream_pipeline_service_spec.rb | 101 +++++-- .../atomic_processing_service_spec.rb | 16 - spec/services/ci/pipeline_schedule_service_spec.rb | 17 ++ .../ci/process_sync_events_service_spec.rb | 76 +++-- spec/services/ci/register_job_service_spec.rb | 4 + spec/services/ci/register_runner_service_spec.rb | 330 +++++++++++---------- spec/services/ci/retry_build_service_spec.rb | 20 +- spec/services/ci/unregister_runner_service_spec.rb | 15 + .../services/ci/update_build_queue_service_spec.rb | 32 +- spec/services/ci/update_runner_service_spec.rb | 14 + .../services/concerns/rate_limited_service_spec.rb | 20 +- spec/services/draft_notes/create_service_spec.rb | 4 +- spec/services/environments/stop_service_spec.rb | 8 +- spec/services/feature_flags/update_service_spec.rb | 2 +- .../create_service_accounts_service_spec.rb | 16 +- .../google_cloud/enable_cloud_run_service_spec.rb | 41 +++ .../google_cloud/generate_pipeline_service_spec.rb | 230 ++++++++++++++ spec/services/groups/create_service_spec.rb | 57 ++++ .../groups/update_statistics_service_spec.rb | 55 ++++ .../create_incident_label_service_spec.rb | 7 - .../incidents/create_service_spec.rb | 36 +-- .../after_update_service_spec.rb | 11 + .../prepare_update_service_spec.rb | 8 +- spec/services/issues/close_service_spec.rb | 10 +- spec/services/issues/create_service_spec.rb | 24 +- spec/services/issues/move_service_spec.rb | 42 +++ spec/services/issues/reorder_service_spec.rb | 12 +- .../issues/set_crm_contacts_service_spec.rb | 112 ++++--- spec/services/issues/update_service_spec.rb | 20 +- .../batch_cleaner_service_spec.rb | 78 +++++ spec/services/members/create_service_spec.rb | 27 ++ .../merge_requests/after_create_service_spec.rb | 32 +- ...bulk_remove_attention_requested_service_spec.rb | 4 +- .../services/merge_requests/create_service_spec.rb | 10 +- spec/services/merge_requests/merge_service_spec.rb | 2 +- .../mergeability_check_service_spec.rb | 18 +- .../services/merge_requests/rebase_service_spec.rb | 2 +- .../services/merge_requests/squash_service_spec.rb | 6 +- .../services/merge_requests/update_service_spec.rb | 6 +- spec/services/notes/create_service_spec.rb | 4 +- .../packages/maven/metadata/sync_service_spec.rb | 19 +- .../nuget/metadata_extraction_service_spec.rb | 4 +- spec/services/pages/zip_directory_service_spec.rb | 6 +- .../services/projects/autocomplete_service_spec.rb | 26 ++ .../delete_tags_service_spec.rb | 18 +- spec/services/projects/create_service_spec.rb | 98 +++--- spec/services/projects/destroy_service_spec.rb | 45 ++- .../projects/import_export/export_service_spec.rb | 15 +- spec/services/projects/import_service_spec.rb | 2 +- .../projects/overwrite_project_service_spec.rb | 69 ++++- .../projects/readme_renderer_service_spec.rb | 75 +++++ spec/services/projects/transfer_service_spec.rb | 31 +- .../quick_actions/interpret_service_spec.rb | 232 +++++++++++++-- spec/services/releases/create_service_spec.rb | 2 +- .../container_scanning_create_service_spec.rb | 19 ++ .../submit_service_ping_service_spec.rb | 39 ++- spec/services/system_note_service_spec.rb | 61 +++- .../system_notes/alert_management_service_spec.rb | 38 ++- .../services/system_notes/incident_service_spec.rb | 24 +- .../system_notes/issuables_service_spec.rb | 48 +++ spec/services/test_hooks/project_service_spec.rb | 18 +- spec/services/test_hooks/system_service_spec.rb | 8 +- .../update_container_registry_info_service_spec.rb | 15 +- spec/services/web_hook_service_spec.rb | 65 ++-- spec/services/work_items/create_service_spec.rb | 56 ++-- spec/services/work_items/delete_service_spec.rb | 50 ++++ spec/services/work_items/update_service_spec.rb | 69 +++++ 73 files changed, 2073 insertions(+), 687 deletions(-) create mode 100644 spec/services/ci/copy_cross_database_associations_service_spec.rb create mode 100644 spec/services/ci/unregister_runner_service_spec.rb create mode 100644 spec/services/google_cloud/enable_cloud_run_service_spec.rb create mode 100644 spec/services/google_cloud/generate_pipeline_service_spec.rb create mode 100644 spec/services/groups/update_statistics_service_spec.rb delete mode 100644 spec/services/incident_management/create_incident_label_service_spec.rb create mode 100644 spec/services/projects/readme_renderer_service_spec.rb create mode 100644 spec/services/security/ci_configuration/container_scanning_create_service_spec.rb create mode 100644 spec/services/work_items/delete_service_spec.rb create mode 100644 spec/services/work_items/update_service_spec.rb (limited to 'spec/services') diff --git a/spec/services/alert_management/alerts/update_service_spec.rb b/spec/services/alert_management/alerts/update_service_spec.rb index 35697ac79a0..882543fd701 100644 --- a/spec/services/alert_management/alerts/update_service_spec.rb +++ b/spec/services/alert_management/alerts/update_service_spec.rb @@ -28,8 +28,11 @@ RSpec.describe AlertManagement::Alerts::UpdateService do specify { expect { response }.not_to change(Note, :count) } end - shared_examples 'adds a system note' do - specify { expect { response }.to change { alert.reload.notes.count }.by(1) } + shared_examples 'adds a system note' do |note_matcher = nil| + specify do + expect { response }.to change { alert.reload.notes.count }.by(1) + expect(alert.notes.last.note).to match(note_matcher) if note_matcher + end end shared_examples 'error response' do |message| @@ -288,6 +291,12 @@ RSpec.describe AlertManagement::Alerts::UpdateService do end end end + + context 'when a status change reason is included' do + let(:params) { { status: new_status, status_change_reason: ' by changing the incident status' } } + + it_behaves_like 'adds a system note', /changed the status to \*\*Acknowledged\*\* by changing the incident status/ + end end end end diff --git a/spec/services/alert_management/create_alert_issue_service_spec.rb b/spec/services/alert_management/create_alert_issue_service_spec.rb index 55f8e47717c..083e5b8c6f1 100644 --- a/spec/services/alert_management/create_alert_issue_service_spec.rb +++ b/spec/services/alert_management/create_alert_issue_service_spec.rb @@ -43,10 +43,10 @@ RSpec.describe AlertManagement::CreateAlertIssueService do expect(execute).to be_success end - it 'updates alert.issue_id' do + it 'sets alert.issue_id in the same ActiveRecord query execution' do execute - expect(alert.reload.issue_id).to eq(created_issue.id) + expect(alert.issue_id).to eq(created_issue.id) end it 'creates a system note' do diff --git a/spec/services/application_settings/update_service_spec.rb b/spec/services/application_settings/update_service_spec.rb index 5c9d2c5e680..e20d59fb0ef 100644 --- a/spec/services/application_settings/update_service_spec.rb +++ b/spec/services/application_settings/update_service_spec.rb @@ -475,6 +475,24 @@ RSpec.describe ApplicationSettings::UpdateService do end end + context 'when users_get_by_id_limit and users_get_by_id_limit_allowlist_raw are passed' do + let(:params) do + { + users_get_by_id_limit: 456, + users_get_by_id_limit_allowlist_raw: 'someone, someone_else' + } + end + + it 'updates users_get_by_id_limit and users_get_by_id_limit_allowlist value' do + subject.execute + + application_settings.reload + + expect(application_settings.users_get_by_id_limit).to eq(456) + expect(application_settings.users_get_by_id_limit_allowlist).to eq(%w[someone someone_else]) + end + end + context 'when require_admin_approval_after_user_signup changes' do context 'when it goes from enabled to disabled' do let(:params) { { require_admin_approval_after_user_signup: false } } diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb index 83f77780b80..00841de9ff4 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -145,28 +145,4 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do it_behaves_like 'an unmodified token' end end - - context 'CDN redirection' do - include_context 'container registry auth service context' - - let_it_be(:current_user) { create(:user) } - let_it_be(:project) { create(:project) } - let_it_be(:current_params) { { scopes: ["repository:#{project.full_path}:pull"] } } - - before do - project.add_developer(current_user) - end - - it_behaves_like 'a valid token' - it { expect(payload['access']).to include(include('cdn_redirect' => true)) } - - context 'when the feature flag is disabled' do - before do - stub_feature_flags(container_registry_cdn_redirect: false) - end - - it_behaves_like 'a valid token' - it { expect(payload['access']).not_to include(have_key('cdn_redirect')) } - end - end end diff --git a/spec/services/branches/create_service_spec.rb b/spec/services/branches/create_service_spec.rb index 1962aca35e1..0d2f5838574 100644 --- a/spec/services/branches/create_service_spec.rb +++ b/spec/services/branches/create_service_spec.rb @@ -65,7 +65,7 @@ RSpec.describe Branches::CreateService do allow(project.repository).to receive(:add_branch).and_raise(pre_receive_error) - expect(Gitlab::ErrorTracking).to receive(:track_exception).with( + expect(Gitlab::ErrorTracking).to receive(:log_exception).with( pre_receive_error, pre_receive_message: raw_message, branch_name: 'new-feature', diff --git a/spec/services/ci/copy_cross_database_associations_service_spec.rb b/spec/services/ci/copy_cross_database_associations_service_spec.rb new file mode 100644 index 00000000000..5938ac258d0 --- /dev/null +++ b/spec/services/ci/copy_cross_database_associations_service_spec.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::CopyCrossDatabaseAssociationsService do + let_it_be(:project) { create(:project) } + let_it_be(:pipeline) { create(:ci_pipeline, project: project) } + let_it_be(:old_build) { create(:ci_build, pipeline: pipeline) } + let_it_be(:new_build) { create(:ci_build, pipeline: pipeline) } + + subject(:execute) { described_class.new.execute(old_build, new_build) } + + describe '#execute' do + it 'returns a success response' do + expect(execute).to be_success + end + 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 d61abf6a6ee..43eb57df66c 100644 --- a/spec/services/ci/create_downstream_pipeline_service_spec.rb +++ b/spec/services/ci/create_downstream_pipeline_service_spec.rb @@ -441,44 +441,99 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end end - context 'when relationship between pipelines is cyclical' do - before do - pipeline_a = create(:ci_pipeline, project: upstream_project) - pipeline_b = create(:ci_pipeline, project: downstream_project) - pipeline_c = create(:ci_pipeline, project: upstream_project) + describe 'cyclical dependency detection' do + shared_examples 'detects cyclical pipelines' do + it 'does not create a new pipeline' do + expect { service.execute(bridge) } + .not_to change { Ci::Pipeline.count } + end + + it 'changes status of the bridge build' do + service.execute(bridge) - create_source_pipeline(pipeline_a, pipeline_b) - create_source_pipeline(pipeline_b, pipeline_c) - create_source_pipeline(pipeline_c, upstream_pipeline) + expect(bridge.reload).to be_failed + expect(bridge.failure_reason).to eq 'pipeline_loop_detected' + end end - it 'does not create a new pipeline' do - expect { service.execute(bridge) } - .not_to change { Ci::Pipeline.count } + shared_examples 'passes cyclical pipeline precondition' do + it 'creates a new pipeline' do + expect { service.execute(bridge) } + .to change { Ci::Pipeline.count } + end + + it 'expect bridge build not to be failed' do + service.execute(bridge) + + expect(bridge.reload).not_to be_failed + end end - it 'changes status of the bridge build' do - service.execute(bridge) + context 'when pipeline ancestry contains 2 cycles of dependencies' do + before do + # A(push on master) -> B(pipeline on master) -> A(push on master) -> + # B(pipeline on master) -> A(push on master) + pipeline_1 = create(:ci_pipeline, project: upstream_project, source: :push) + pipeline_2 = create(:ci_pipeline, project: downstream_project, source: :pipeline) + pipeline_3 = create(:ci_pipeline, project: upstream_project, source: :push) + pipeline_4 = create(:ci_pipeline, project: downstream_project, source: :pipeline) + + create_source_pipeline(pipeline_1, pipeline_2) + create_source_pipeline(pipeline_2, pipeline_3) + create_source_pipeline(pipeline_3, pipeline_4) + create_source_pipeline(pipeline_4, upstream_pipeline) + end - expect(bridge.reload).to be_failed - expect(bridge.failure_reason).to eq 'pipeline_loop_detected' + 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 ci_drop_cyclical_triggered_pipelines is not enabled' do + context 'when source in the ancestry differ' do before do - stub_feature_flags(ci_drop_cyclical_triggered_pipelines: false) + # A(push on master) -> B(pipeline on master) -> A(pipeline on master) + pipeline_1 = create(:ci_pipeline, project: upstream_project, source: :push) + pipeline_2 = create(:ci_pipeline, project: downstream_project, source: :pipeline) + upstream_pipeline.update!(source: :pipeline) + + create_source_pipeline(pipeline_1, pipeline_2) + create_source_pipeline(pipeline_2, upstream_pipeline) end - it 'creates a new pipeline' do - expect { service.execute(bridge) } - .to change { Ci::Pipeline.count } + it_behaves_like 'passes cyclical pipeline precondition' + end + + context 'when ref in the ancestry differ' do + before do + # A(push on master) -> B(pipeline on master) -> A(push on feature-1) + pipeline_1 = create(:ci_pipeline, ref: 'master', project: upstream_project, source: :push) + pipeline_2 = create(:ci_pipeline, ref: 'master', project: downstream_project, source: :pipeline) + upstream_pipeline.update!(ref: 'feature-1') + + create_source_pipeline(pipeline_1, pipeline_2) + create_source_pipeline(pipeline_2, upstream_pipeline) end - it 'expect bridge build not to be failed' do - service.execute(bridge) + it_behaves_like 'passes cyclical pipeline precondition' + end - expect(bridge.reload).not_to be_failed + context 'when only 1 cycle is detected' do + before do + # A(push on master) -> B(pipeline on master) -> A(push on master) + pipeline_1 = create(:ci_pipeline, ref: 'master', project: upstream_project, source: :push) + pipeline_2 = create(:ci_pipeline, ref: 'master', project: downstream_project, source: :pipeline) + + create_source_pipeline(pipeline_1, pipeline_2) + create_source_pipeline(pipeline_2, upstream_pipeline) end + + it_behaves_like 'passes cyclical pipeline precondition' end end diff --git a/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb b/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb index 26bc6f747e1..7365ad162d2 100644 --- a/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb +++ b/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb @@ -1043,22 +1043,6 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService do expect(all_builds_names).to eq(%w[A1 A2 B]) expect(all_builds_statuses).to eq(%w[pending created created]) end - - context 'when the FF ci_order_subsequent_jobs_by_stage is disabled' do - before do - stub_feature_flags(ci_order_subsequent_jobs_by_stage: false) - end - - it 'processes subsequent jobs in an incorrect order when playing first job' do - expect(all_builds_names).to eq(%w[A1 A2 B]) - expect(all_builds_statuses).to eq(%w[manual skipped skipped]) - - play_manual_action('A1') - - expect(all_builds_names).to eq(%w[A1 A2 B]) - expect(all_builds_statuses).to eq(%w[pending created skipped]) - end - end end private diff --git a/spec/services/ci/pipeline_schedule_service_spec.rb b/spec/services/ci/pipeline_schedule_service_spec.rb index 65bbd13c5e7..b8e4fb19f5d 100644 --- a/spec/services/ci/pipeline_schedule_service_spec.rb +++ b/spec/services/ci/pipeline_schedule_service_spec.rb @@ -32,5 +32,22 @@ RSpec.describe Ci::PipelineScheduleService do expect { subject }.not_to raise_error end end + + context 'when the project is missing' do + before do + project.delete + end + + it 'does not raise an exception' do + expect { subject }.not_to raise_error + end + + it 'does not run RunPipelineScheduleWorker' do + expect(RunPipelineScheduleWorker) + .not_to receive(:perform_async).with(schedule.id, schedule.owner.id) + + subject + end + end end end diff --git a/spec/services/ci/process_sync_events_service_spec.rb b/spec/services/ci/process_sync_events_service_spec.rb index 8b7717fe4bf..6b9717fe57d 100644 --- a/spec/services/ci/process_sync_events_service_spec.rb +++ b/spec/services/ci/process_sync_events_service_spec.rb @@ -25,6 +25,8 @@ RSpec.describe Ci::ProcessSyncEventsService do project2.update!(group: parent_group_2) end + it { is_expected.to eq(service_results(2, 2, 2)) } + it 'consumes events' do expect { execute }.to change(Projects::SyncEvent, :count).from(2).to(0) @@ -36,20 +38,32 @@ RSpec.describe Ci::ProcessSyncEventsService do ) end - it 'enqueues Projects::ProcessSyncEventsWorker if any left' do - stub_const("#{described_class}::BATCH_SIZE", 1) + context 'when any event left after processing' do + before do + stub_const("#{described_class}::BATCH_SIZE", 1) + end - expect(Projects::ProcessSyncEventsWorker).to receive(:perform_async) + it { is_expected.to eq(service_results(2, 1, 1)) } - execute + it 'enqueues Projects::ProcessSyncEventsWorker' do + expect(Projects::ProcessSyncEventsWorker).to receive(:perform_async) + + execute + end end - it 'does not enqueue Projects::ProcessSyncEventsWorker if no left' do - stub_const("#{described_class}::BATCH_SIZE", 2) + context 'when no event left after processing' do + before do + stub_const("#{described_class}::BATCH_SIZE", 2) + end - expect(Projects::ProcessSyncEventsWorker).not_to receive(:perform_async) + it { is_expected.to eq(service_results(2, 2, 2)) } - execute + it 'does not enqueue Projects::ProcessSyncEventsWorker' do + expect(Projects::ProcessSyncEventsWorker).not_to receive(:perform_async) + + execute + end end context 'when there is no event' do @@ -57,37 +71,45 @@ RSpec.describe Ci::ProcessSyncEventsService do Projects::SyncEvent.delete_all end + it { is_expected.to eq(service_results(0, 0, nil)) } + it 'does nothing' do expect { execute }.not_to change(Projects::SyncEvent, :count) end end - context 'when the FF ci_namespace_project_mirrors is disabled' do + context 'when there is non-executed events' do before do - stub_feature_flags(ci_namespace_project_mirrors: false) - end + new_project = create(:project) + sync_event_class.delete_all - it 'does nothing' do - expect { execute }.not_to change(Projects::SyncEvent, :count) - end - end + project1.update!(group: parent_group_2) + new_project.update!(group: parent_group_1) + project2.update!(group: parent_group_1) - it 'does not delete non-executed events' do - new_project = create(:project) - sync_event_class.delete_all + @new_project_sync_event = new_project.sync_events.last - project1.update!(group: parent_group_2) - new_project.update!(group: parent_group_1) - project2.update!(group: parent_group_1) + allow(sync_event_class).to receive(:preload_synced_relation).and_return( + sync_event_class.where.not(id: @new_project_sync_event) + ) + end - new_project_sync_event = new_project.sync_events.last + it { is_expected.to eq(service_results(3, 2, 2)) } - allow(sync_event_class).to receive(:preload_synced_relation).and_return( - sync_event_class.where.not(id: new_project_sync_event) - ) + it 'does not delete non-executed events' do + expect { execute }.to change(Projects::SyncEvent, :count).from(3).to(1) + expect(@new_project_sync_event.reload).to be_persisted + end + end + + private - expect { execute }.to change(Projects::SyncEvent, :count).from(3).to(1) - expect(new_project_sync_event.reload).to be_persisted + def service_results(total, consumable, processed) + { + estimated_total_events: total, + consumable_events: consumable, + processed_events: processed + }.compact end end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 251159864f5..2127a4fa0fc 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -750,6 +750,8 @@ module Ci context 'with ci_queuing_use_denormalized_data_strategy disabled' do before do + skip_if_multiple_databases_are_setup + stub_feature_flags(ci_queuing_use_denormalized_data_strategy: false) end @@ -773,6 +775,8 @@ module Ci context 'when not using pending builds table' do before do + skip_if_multiple_databases_are_setup + stub_feature_flags(ci_pending_builds_queue_source: false) end diff --git a/spec/services/ci/register_runner_service_spec.rb b/spec/services/ci/register_runner_service_spec.rb index e813a1d8b31..491582bbd13 100644 --- a/spec/services/ci/register_runner_service_spec.rb +++ b/spec/services/ci/register_runner_service_spec.rb @@ -2,8 +2,10 @@ require 'spec_helper' -RSpec.describe ::Ci::RegisterRunnerService do +RSpec.describe ::Ci::RegisterRunnerService, '#execute' do let(:registration_token) { 'abcdefg123456' } + let(:token) { } + let(:args) { {} } before do stub_feature_flags(runner_registration_control: false) @@ -11,213 +13,219 @@ RSpec.describe ::Ci::RegisterRunnerService do stub_application_setting(valid_runner_registrars: ApplicationSetting::VALID_RUNNER_REGISTRAR_TYPES) end - describe '#execute' do - let(:token) { } - let(:args) { {} } + subject { described_class.new.execute(token, args) } - subject { described_class.new.execute(token, args) } + context 'when no token is provided' do + let(:token) { '' } - context 'when no token is provided' do - let(:token) { '' } - - it 'returns nil' do - is_expected.to be_nil - end + it 'returns nil' do + is_expected.to be_nil end + end - context 'when invalid token is provided' do - let(:token) { 'invalid' } + context 'when invalid token is provided' do + let(:token) { 'invalid' } - it 'returns nil' do - is_expected.to be_nil - end + it 'returns nil' do + is_expected.to be_nil end + end - context 'when valid token is provided' do - context 'with a registration token' do - let(:token) { registration_token } + context 'when valid token is provided' do + context 'with a registration token' do + let(:token) { registration_token } + + it 'creates runner with default values' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.persisted?).to be_truthy + expect(subject.run_untagged).to be true + expect(subject.active).to be true + expect(subject.token).not_to eq(registration_token) + expect(subject).to be_instance_type + end - it 'creates runner with default values' do - is_expected.to be_an_instance_of(::Ci::Runner) - expect(subject.persisted?).to be_truthy - expect(subject.run_untagged).to be true - expect(subject.active).to be true - expect(subject.token).not_to eq(registration_token) - expect(subject).to be_instance_type - end - - context 'with non-default arguments' do - let(:args) do - { - description: 'some description', - active: false, - locked: true, - run_untagged: false, - tag_list: %w(tag1 tag2), - access_level: 'ref_protected', - maximum_timeout: 600, - name: 'some name', - version: 'some version', - revision: 'some revision', - platform: 'some platform', - architecture: 'some architecture', - ip_address: '10.0.0.1', - config: { - gpus: 'some gpu config' - } + context 'with non-default arguments' do + let(:args) do + { + description: 'some description', + active: false, + locked: true, + run_untagged: false, + tag_list: %w(tag1 tag2), + access_level: 'ref_protected', + maximum_timeout: 600, + name: 'some name', + version: 'some version', + revision: 'some revision', + platform: 'some platform', + architecture: 'some architecture', + ip_address: '10.0.0.1', + config: { + gpus: 'some gpu config' } - end + } + end - it 'creates runner with specified values', :aggregate_failures do - is_expected.to be_an_instance_of(::Ci::Runner) - expect(subject.active).to eq args[:active] - expect(subject.locked).to eq args[:locked] - expect(subject.run_untagged).to eq args[:run_untagged] - expect(subject.tags).to contain_exactly( - an_object_having_attributes(name: 'tag1'), - an_object_having_attributes(name: 'tag2') - ) - expect(subject.access_level).to eq args[:access_level] - expect(subject.maximum_timeout).to eq args[:maximum_timeout] - expect(subject.name).to eq args[:name] - expect(subject.version).to eq args[:version] - expect(subject.revision).to eq args[:revision] - expect(subject.platform).to eq args[:platform] - expect(subject.architecture).to eq args[:architecture] - expect(subject.ip_address).to eq args[:ip_address] - end + it 'creates runner with specified values', :aggregate_failures do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.active).to eq args[:active] + expect(subject.locked).to eq args[:locked] + expect(subject.run_untagged).to eq args[:run_untagged] + expect(subject.tags).to contain_exactly( + an_object_having_attributes(name: 'tag1'), + an_object_having_attributes(name: 'tag2') + ) + expect(subject.access_level).to eq args[:access_level] + expect(subject.maximum_timeout).to eq args[:maximum_timeout] + expect(subject.name).to eq args[:name] + expect(subject.version).to eq args[:version] + expect(subject.revision).to eq args[:revision] + expect(subject.platform).to eq args[:platform] + expect(subject.architecture).to eq args[:architecture] + expect(subject.ip_address).to eq args[:ip_address] end end - context 'when project token is used' do - let(:project) { create(:project) } - let(:token) { project.runners_token } + context 'with runner token expiration interval', :freeze_time do + before do + stub_application_setting(runner_token_expiration_interval: 5.days) + end - it 'creates project runner' do + it 'creates runner with token expiration' do is_expected.to be_an_instance_of(::Ci::Runner) - expect(project.runners.size).to eq(1) - is_expected.to eq(project.runners.first) - expect(subject.token).not_to eq(registration_token) - expect(subject.token).not_to eq(project.runners_token) - expect(subject).to be_project_type + expect(subject.token_expires_at).to eq(5.days.from_now) end + end + end - context 'when it exceeds the application limits' do - before do - create(:ci_runner, runner_type: :project_type, projects: [project], contacted_at: 1.second.ago) - create(:plan_limits, :default_plan, ci_registered_project_runners: 1) - end + context 'when project token is used' do + let(:project) { create(:project) } + let(:token) { project.runners_token } + + it 'creates project runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(project.runners.size).to eq(1) + is_expected.to eq(project.runners.first) + expect(subject.token).not_to eq(registration_token) + expect(subject.token).not_to eq(project.runners_token) + expect(subject).to be_project_type + end - it 'does not create runner' do - is_expected.to be_an_instance_of(::Ci::Runner) - expect(subject.persisted?).to be_falsey - expect(subject.errors.messages).to eq('runner_projects.base': ['Maximum number of ci registered project runners (1) exceeded']) - expect(project.runners.reload.size).to eq(1) - end + context 'when it exceeds the application limits' do + before do + create(:ci_runner, runner_type: :project_type, projects: [project], contacted_at: 1.second.ago) + create(:plan_limits, :default_plan, ci_registered_project_runners: 1) end - context 'when abandoned runners cause application limits to not be exceeded' do - before do - create(:ci_runner, runner_type: :project_type, projects: [project], created_at: 14.months.ago, contacted_at: 13.months.ago) - create(:plan_limits, :default_plan, ci_registered_project_runners: 1) - end + it 'does not create runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.persisted?).to be_falsey + expect(subject.errors.messages).to eq('runner_projects.base': ['Maximum number of ci registered project runners (1) exceeded']) + expect(project.runners.reload.size).to eq(1) + end + end - it 'creates runner' do - is_expected.to be_an_instance_of(::Ci::Runner) - expect(subject.errors).to be_empty - expect(project.runners.reload.size).to eq(2) - expect(project.runners.recent.size).to eq(1) - end + context 'when abandoned runners cause application limits to not be exceeded' do + before do + create(:ci_runner, runner_type: :project_type, projects: [project], created_at: 14.months.ago, contacted_at: 13.months.ago) + create(:plan_limits, :default_plan, ci_registered_project_runners: 1) end - context 'when valid runner registrars do not include project' do + it 'creates runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.errors).to be_empty + expect(project.runners.reload.size).to eq(2) + expect(project.runners.recent.size).to eq(1) + end + end + + context 'when valid runner registrars do not include project' do + before do + stub_application_setting(valid_runner_registrars: ['group']) + end + + context 'when feature flag is enabled' do before do - stub_application_setting(valid_runner_registrars: ['group']) + stub_feature_flags(runner_registration_control: true) end - context 'when feature flag is enabled' do - before do - stub_feature_flags(runner_registration_control: true) - end - - it 'returns 403 error' do - is_expected.to be_nil - end + it 'returns 403 error' do + is_expected.to be_nil end + end - context 'when feature flag is disabled' do - it 'registers the runner' do - is_expected.to be_an_instance_of(::Ci::Runner) - expect(subject.errors).to be_empty - expect(subject.active).to be true - end + context 'when feature flag is disabled' do + it 'registers the runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.errors).to be_empty + expect(subject.active).to be true end end end + end + + context 'when group token is used' do + let(:group) { create(:group) } + let(:token) { group.runners_token } + + it 'creates a group runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.errors).to be_empty + expect(group.runners.reload.size).to eq(1) + expect(subject.token).not_to eq(registration_token) + expect(subject.token).not_to eq(group.runners_token) + expect(subject).to be_group_type + end - context 'when group token is used' do - let(:group) { create(:group) } - let(:token) { group.runners_token } + context 'when it exceeds the application limits' do + before do + create(:ci_runner, runner_type: :group_type, groups: [group], contacted_at: nil, created_at: 1.month.ago) + create(:plan_limits, :default_plan, ci_registered_group_runners: 1) + end - it 'creates a group runner' do + it 'does not create runner' do is_expected.to be_an_instance_of(::Ci::Runner) - expect(subject.errors).to be_empty + expect(subject.persisted?).to be_falsey + expect(subject.errors.messages).to eq('runner_namespaces.base': ['Maximum number of ci registered group runners (1) exceeded']) expect(group.runners.reload.size).to eq(1) - expect(subject.token).not_to eq(registration_token) - expect(subject.token).not_to eq(group.runners_token) - expect(subject).to be_group_type end + end - context 'when it exceeds the application limits' do - before do - create(:ci_runner, runner_type: :group_type, groups: [group], contacted_at: nil, created_at: 1.month.ago) - create(:plan_limits, :default_plan, ci_registered_group_runners: 1) - end - - it 'does not create runner' do - is_expected.to be_an_instance_of(::Ci::Runner) - expect(subject.persisted?).to be_falsey - expect(subject.errors.messages).to eq('runner_namespaces.base': ['Maximum number of ci registered group runners (1) exceeded']) - expect(group.runners.reload.size).to eq(1) - end + context 'when abandoned runners cause application limits to not be exceeded' do + before do + create(:ci_runner, runner_type: :group_type, groups: [group], created_at: 4.months.ago, contacted_at: 3.months.ago) + create(:ci_runner, runner_type: :group_type, groups: [group], contacted_at: nil, created_at: 4.months.ago) + create(:plan_limits, :default_plan, ci_registered_group_runners: 1) end - context 'when abandoned runners cause application limits to not be exceeded' do - before do - create(:ci_runner, runner_type: :group_type, groups: [group], created_at: 4.months.ago, contacted_at: 3.months.ago) - create(:ci_runner, runner_type: :group_type, groups: [group], contacted_at: nil, created_at: 4.months.ago) - create(:plan_limits, :default_plan, ci_registered_group_runners: 1) - end + it 'creates runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.errors).to be_empty + expect(group.runners.reload.size).to eq(3) + expect(group.runners.recent.size).to eq(1) + end + end - it 'creates runner' do - is_expected.to be_an_instance_of(::Ci::Runner) - expect(subject.errors).to be_empty - expect(group.runners.reload.size).to eq(3) - expect(group.runners.recent.size).to eq(1) - end + context 'when valid runner registrars do not include group' do + before do + stub_application_setting(valid_runner_registrars: ['project']) end - context 'when valid runner registrars do not include group' do + context 'when feature flag is enabled' do before do - stub_application_setting(valid_runner_registrars: ['project']) + stub_feature_flags(runner_registration_control: true) end - context 'when feature flag is enabled' do - before do - stub_feature_flags(runner_registration_control: true) - end - - it 'returns nil' do - is_expected.to be_nil - end + it 'returns nil' do + is_expected.to be_nil end + end - context 'when feature flag is disabled' do - it 'registers the runner' do - is_expected.to be_an_instance_of(::Ci::Runner) - expect(subject.errors).to be_empty - expect(subject.active).to be true - end + context 'when feature flag is disabled' do + it 'registers the runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.errors).to be_empty + expect(subject.active).to be true end end end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 4e8e41ca6e6..2421fd56c47 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -60,7 +60,8 @@ RSpec.describe Ci::RetryBuildService do artifacts_file artifacts_metadata artifacts_size commands resource resource_group_id processed security_scans author pipeline_id report_results pending_state pages_deployments - queuing_entry runtime_metadata trace_metadata].freeze + queuing_entry runtime_metadata trace_metadata + dast_site_profile dast_scanner_profile].freeze shared_examples 'build duplication' do let_it_be(:another_pipeline) { create(:ci_empty_pipeline, project: project) } @@ -370,23 +371,6 @@ RSpec.describe Ci::RetryBuildService do it_behaves_like 'when build with deployment is retried' it_behaves_like 'when build with dynamic environment is retried' - context 'when create_deployment_in_separate_transaction feature flag is disabled' do - let(:new_build) do - travel_to(1.second.from_now) do - ::Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification.allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/345668') do - service.clone!(build) - end - end - end - - before do - stub_feature_flags(create_deployment_in_separate_transaction: false) - end - - it_behaves_like 'when build with deployment is retried' - it_behaves_like 'when build with dynamic environment is retried' - end - context 'when build has needs' do before do create(:ci_build_need, build: build, name: 'build1') diff --git a/spec/services/ci/unregister_runner_service_spec.rb b/spec/services/ci/unregister_runner_service_spec.rb new file mode 100644 index 00000000000..f427e04f228 --- /dev/null +++ b/spec/services/ci/unregister_runner_service_spec.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::UnregisterRunnerService, '#execute' do + subject { described_class.new(runner).execute } + + let(:runner) { create(:ci_runner) } + + it 'destroys runner' do + expect(runner).to receive(:destroy).once.and_call_original + expect { subject }.to change { Ci::Runner.count }.by(-1) + expect(runner[:errors]).to be_nil + end +end diff --git a/spec/services/ci/update_build_queue_service_spec.rb b/spec/services/ci/update_build_queue_service_spec.rb index 2e2ef120f1b..ef43866d8d4 100644 --- a/spec/services/ci/update_build_queue_service_spec.rb +++ b/spec/services/ci/update_build_queue_service_spec.rb @@ -308,36 +308,12 @@ RSpec.describe Ci::UpdateBuildQueueService do let!(:build) { create(:ci_build, pipeline: pipeline, tag_list: %w[a b]) } let!(:project_runner) { create(:ci_runner, :project, :online, projects: [project], tag_list: %w[a b c]) } - context 'when ci_preload_runner_tags is enabled' do - before do - stub_feature_flags( - ci_preload_runner_tags: true - ) - end - - it 'does execute the same amount of queries regardless of number of runners' do - control_count = ActiveRecord::QueryRecorder.new { subject.tick(build) }.count - - create_list(:ci_runner, 10, :project, :online, projects: [project], tag_list: %w[b c d]) - - expect { subject.tick(build) }.not_to exceed_all_query_limit(control_count) - end - end - - context 'when ci_preload_runner_tags are disabled' do - before do - stub_feature_flags( - ci_preload_runner_tags: false - ) - end - - it 'does execute more queries for more runners' do - control_count = ActiveRecord::QueryRecorder.new { subject.tick(build) }.count + it 'does execute the same amount of queries regardless of number of runners' do + control_count = ActiveRecord::QueryRecorder.new { subject.tick(build) }.count - create_list(:ci_runner, 10, :project, :online, projects: [project], tag_list: %w[b c d]) + create_list(:ci_runner, 10, :project, :online, projects: [project], tag_list: %w[b c d]) - expect { subject.tick(build) }.to exceed_all_query_limit(control_count) - end + expect { subject.tick(build) }.not_to exceed_all_query_limit(control_count) end end end diff --git a/spec/services/ci/update_runner_service_spec.rb b/spec/services/ci/update_runner_service_spec.rb index 1c875b2f54a..eee80bfef47 100644 --- a/spec/services/ci/update_runner_service_spec.rb +++ b/spec/services/ci/update_runner_service_spec.rb @@ -23,6 +23,20 @@ RSpec.describe Ci::UpdateRunnerService do end end + context 'with paused param' do + let(:params) { { paused: true } } + + it 'updates the runner and ticking the queue' do + expect(runner.active).to be_truthy + expect(update).to be_truthy + + runner.reload + + expect(runner).to have_received(:tick_runner_queue) + expect(runner.active).to be_falsey + end + end + context 'with cost factor params' do let(:params) { { public_projects_minutes_cost_factor: 1.1, private_projects_minutes_cost_factor: 2.2 }} diff --git a/spec/services/concerns/rate_limited_service_spec.rb b/spec/services/concerns/rate_limited_service_spec.rb index f73871b7e44..97f5ca53c0d 100644 --- a/spec/services/concerns/rate_limited_service_spec.rb +++ b/spec/services/concerns/rate_limited_service_spec.rb @@ -6,11 +6,10 @@ RSpec.describe RateLimitedService do let(:key) { :issues_create } let(:scope) { [:project, :current_user] } let(:opts) { { scope: scope, users_allowlist: -> { [User.support_bot.username] } } } - let(:rate_limiter_klass) { ::Gitlab::ApplicationRateLimiter } - let(:rate_limiter_instance) { rate_limiter_klass.new(key, **opts) } + let(:rate_limiter) { ::Gitlab::ApplicationRateLimiter } describe 'RateLimitedError' do - subject { described_class::RateLimitedError.new(key: key, rate_limiter: rate_limiter_instance) } + subject { described_class::RateLimitedError.new(key: key, rate_limiter: rate_limiter) } describe '#headers' do it 'returns a Hash of HTTP headers' do @@ -26,7 +25,7 @@ RSpec.describe RateLimitedService do request = instance_double(Grape::Request) user = instance_double(User) - expect(rate_limiter_klass).to receive(:log_request).with(request, "#{key}_request_limit".to_sym, user) + expect(rate_limiter).to receive(:log_request).with(request, "#{key}_request_limit".to_sym, user) subject.log_request(request, user) end @@ -34,7 +33,7 @@ RSpec.describe RateLimitedService do end describe 'RateLimiterScopedAndKeyed' do - subject { described_class::RateLimiterScopedAndKeyed.new(key: key, opts: opts, rate_limiter_klass: rate_limiter_klass) } + subject { described_class::RateLimiterScopedAndKeyed.new(key: key, opts: opts, rate_limiter: rate_limiter) } describe '#rate_limit!' do let(:project_with_feature_enabled) { create(:project) } @@ -49,13 +48,12 @@ RSpec.describe RateLimitedService do let(:rate_limited_service_issues_create_feature_enabled) { nil } before do - allow(rate_limiter_klass).to receive(:new).with(key, **evaluated_opts).and_return(rate_limiter_instance) 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_instance).not_to receive(:throttled?) + expect(rate_limiter).not_to receive(:throttled?) expect(subject.rate_limit!(service)).to be_nil end @@ -63,7 +61,7 @@ RSpec.describe RateLimitedService do shared_examples 'a service that does attempt to throttle' do before do - allow(rate_limiter_instance).to receive(:throttled?).and_return(throttled) + allow(rate_limiter).to receive(:throttled?).and_return(throttled) end context 'when rate limiting is not in effect' do @@ -134,7 +132,7 @@ RSpec.describe RateLimitedService do end before do - allow(RateLimitedService::RateLimiterScopedAndKeyed).to receive(:new).with(key: key, opts: opts, rate_limiter_klass: rate_limiter_klass).and_return(rate_limiter_scoped_and_keyed) + allow(RateLimitedService::RateLimiterScopedAndKeyed).to receive(:new).with(key: key, opts: opts, rate_limiter: rate_limiter).and_return(rate_limiter_scoped_and_keyed) end context 'bypasses rate limiting' do @@ -173,12 +171,12 @@ RSpec.describe RateLimitedService do end before do - allow(RateLimitedService::RateLimiterScopedAndKeyed).to receive(:new).with(key: key, opts: opts, rate_limiter_klass: rate_limiter_klass).and_return(rate_limiter_scoped_and_keyed) + allow(RateLimitedService::RateLimiterScopedAndKeyed).to receive(:new).with(key: key, opts: opts, rate_limiter: rate_limiter).and_return(rate_limiter_scoped_and_keyed) end context 'and applies rate limiting' do it 'raises an RateLimitedService::RateLimitedError exception' do - expect(rate_limiter_scoped_and_keyed).to receive(:rate_limit!).with(subject).and_raise(RateLimitedService::RateLimitedError.new(key: key, rate_limiter: rate_limiter_instance)) + expect(rate_limiter_scoped_and_keyed).to receive(:rate_limit!).with(subject).and_raise(RateLimitedService::RateLimitedError.new(key: key, rate_limiter: rate_limiter)) expect { subject.execute }.to raise_error(RateLimitedService::RateLimitedError) end diff --git a/spec/services/draft_notes/create_service_spec.rb b/spec/services/draft_notes/create_service_spec.rb index 9e084dbed1c..528c8717525 100644 --- a/spec/services/draft_notes/create_service_spec.rb +++ b/spec/services/draft_notes/create_service_spec.rb @@ -92,7 +92,7 @@ RSpec.describe DraftNotes::CreateService do expect(merge_request).to receive_message_chain(:diffs, :clear_cache) - create_draft(note: 'This is a test') + create_draft(note: 'This is a test', line_code: '123') end end @@ -104,7 +104,7 @@ RSpec.describe DraftNotes::CreateService do expect(merge_request).not_to receive(:diffs) - create_draft(note: 'This is a test') + create_draft(note: 'This is a test', line_code: '123') end end end diff --git a/spec/services/environments/stop_service_spec.rb b/spec/services/environments/stop_service_spec.rb index acc9869002f..362071c1c26 100644 --- a/spec/services/environments/stop_service_spec.rb +++ b/spec/services/environments/stop_service_spec.rb @@ -185,7 +185,7 @@ RSpec.describe Environments::StopService do end it 'has active environment at first' do - expect(pipeline.environments.first).to be_available + expect(pipeline.environments_in_self_and_descendants.first).to be_available end context 'when user is a developer' do @@ -196,7 +196,7 @@ RSpec.describe Environments::StopService do it 'stops the active environment' do subject - expect(pipeline.environments.first).to be_stopped + expect(pipeline.environments_in_self_and_descendants.first).to be_stopped end end @@ -208,7 +208,7 @@ RSpec.describe Environments::StopService do it 'does not stop the active environment' do subject - expect(pipeline.environments.first).to be_available + expect(pipeline.environments_in_self_and_descendants.first).to be_available end end @@ -232,7 +232,7 @@ RSpec.describe Environments::StopService do it 'does not stop the active environment' do subject - expect(pipeline.environments.first).to be_available + expect(pipeline.environments_in_self_and_descendants.first).to be_available end end end diff --git a/spec/services/feature_flags/update_service_spec.rb b/spec/services/feature_flags/update_service_spec.rb index abe0112b27e..f5e94c4af0f 100644 --- a/spec/services/feature_flags/update_service_spec.rb +++ b/spec/services/feature_flags/update_service_spec.rb @@ -130,7 +130,7 @@ RSpec.describe FeatureFlags::UpdateService do it 'executes hooks' do hook = create(:project_hook, :all_events_enabled, project: project) - expect(WebHookWorker).to receive(:perform_async).with(hook.id, an_instance_of(Hash), 'feature_flag_hooks') + expect(WebHookWorker).to receive(:perform_async).with(hook.id, an_instance_of(Hash), 'feature_flag_hooks', an_instance_of(Hash)) subject 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 190e1a8098c..53d21df713a 100644 --- a/spec/services/google_cloud/create_service_accounts_service_spec.rb +++ b/spec/services/google_cloud/create_service_accounts_service_spec.rb @@ -2,22 +2,26 @@ require 'spec_helper' -# Mock Types -MockGoogleOAuth2Credentials = Struct.new(:app_id, :app_secret) -MockServiceAccount = Struct.new(:project_id, :unique_id) - RSpec.describe GoogleCloud::CreateServiceAccountsService do describe '#execute' do before do + mock_google_oauth2_creds = Struct.new(:app_id, :app_secret) + .new('mock-app-id', 'mock-app-secret') allow(Gitlab::Auth::OAuth::Provider).to receive(:config_for) .with('google_oauth2') - .and_return(MockGoogleOAuth2Credentials.new('mock-app-id', 'mock-app-secret')) + .and_return(mock_google_oauth2_creds) allow_next_instance_of(GoogleApi::CloudPlatform::Client) do |client| + mock_service_account = Struct.new(:project_id, :unique_id, :email) + .new('mock-project-id', 'mock-unique-id', 'mock-email') allow(client).to receive(:create_service_account) - .and_return(MockServiceAccount.new('mock-project-id', 'mock-unique-id')) + .and_return(mock_service_account) + allow(client).to receive(:create_service_account_key) .and_return('mock-key') + + allow(client) + .to receive(:grant_service_account_roles) end end diff --git a/spec/services/google_cloud/enable_cloud_run_service_spec.rb b/spec/services/google_cloud/enable_cloud_run_service_spec.rb new file mode 100644 index 00000000000..6d2b1f5cfd5 --- /dev/null +++ b/spec/services/google_cloud/enable_cloud_run_service_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GoogleCloud::EnableCloudRunService do + describe 'when a project does not have any gcp projects' do + let_it_be(:project) { create(:project) } + + it 'returns error' do + result = described_class.new(project).execute + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('No GCP projects found. Configure a service account or GCP_PROJECT_ID ci variable.') + end + end + + describe 'when a project has 3 gcp projects' do + let_it_be(:project) { create(:project) } + + before do + project.variables.build(environment_scope: 'production', key: 'GCP_PROJECT_ID', value: 'prj-prod') + project.variables.build(environment_scope: 'staging', key: 'GCP_PROJECT_ID', value: 'prj-staging') + project.save! + end + + it 'enables cloud run, artifacts registry and cloud build', :aggregate_failures do + expect_next_instance_of(GoogleApi::CloudPlatform::Client) do |instance| + expect(instance).to receive(:enable_cloud_run).with('prj-prod') + expect(instance).to receive(:enable_artifacts_registry).with('prj-prod') + expect(instance).to receive(:enable_cloud_build).with('prj-prod') + expect(instance).to receive(:enable_cloud_run).with('prj-staging') + expect(instance).to receive(:enable_artifacts_registry).with('prj-staging') + expect(instance).to receive(:enable_cloud_build).with('prj-staging') + end + + result = described_class.new(project).execute + + expect(result[:status]).to eq(:success) + end + end +end diff --git a/spec/services/google_cloud/generate_pipeline_service_spec.rb b/spec/services/google_cloud/generate_pipeline_service_spec.rb new file mode 100644 index 00000000000..75494f229b5 --- /dev/null +++ b/spec/services/google_cloud/generate_pipeline_service_spec.rb @@ -0,0 +1,230 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GoogleCloud::GeneratePipelineService do + describe 'for cloud-run' do + describe 'when there is no existing pipeline' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:maintainer) { create(:user) } + let_it_be(:service_params) { { action: described_class::ACTION_DEPLOY_TO_CLOUD_RUN } } + let_it_be(:service) { described_class.new(project, maintainer, service_params) } + + before do + project.add_maintainer(maintainer) + end + + it 'creates a new branch with commit for cloud-run deployment' do + response = service.execute + + branch_name = response[:branch_name] + commit = response[:commit] + local_branches = project.repository.local_branches + created_branch = local_branches.find { |branch| branch.name == branch_name } + + expect(response[:status]).to eq(:success) + expect(branch_name).to start_with('deploy-to-cloud-run-') + expect(created_branch).to be_present + expect(created_branch.target).to eq(commit[:result]) + end + + it 'generated pipeline includes cloud-run deployment' do + response = service.execute + + ref = response[:commit][:result] + gitlab_ci_yml = project.repository.gitlab_ci_yml_for(ref) + + expect(response[:status]).to eq(:success) + expect(gitlab_ci_yml).to include('https://gitlab.com/gitlab-org/incubation-engineering/five-minute-production/library/-/raw/main/gcp/cloud-run.gitlab-ci.yml') + end + + context 'simulate errors' do + it 'fails to create branch' do + allow_next_instance_of(Branches::CreateService) do |create_service| + allow(create_service).to receive(:execute) + .and_return({ status: :error }) + end + + response = service.execute + expect(response[:status]).to eq(:error) + end + + it 'fails to commit changes' do + allow_next_instance_of(Files::CreateService) do |create_service| + allow(create_service).to receive(:execute) + .and_return({ status: :error }) + end + + response = service.execute + expect(response[:status]).to eq(:error) + end + end + end + + describe 'when there is an existing pipeline without `deploy` stage' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:maintainer) { create(:user) } + let_it_be(:service_params) { { action: GoogleCloud::GeneratePipelineService::ACTION_DEPLOY_TO_CLOUD_RUN } } + let_it_be(:service) { described_class.new(project, maintainer, service_params) } + + before do + project.add_maintainer(maintainer) + + file_name = '.gitlab-ci.yml' + file_content = <alert("Attack!")')) } + + it 'does not track signed_up event' do + expect(experiment(:logged_out_marketing_header)).not_to track(:namespace_created) + + subject + end + end + end end diff --git a/spec/services/groups/update_statistics_service_spec.rb b/spec/services/groups/update_statistics_service_spec.rb new file mode 100644 index 00000000000..5bef51c2727 --- /dev/null +++ b/spec/services/groups/update_statistics_service_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Groups::UpdateStatisticsService do + let_it_be(:group, reload: true) { create(:group) } + + let(:statistics) { %w(wiki_size) } + + subject(:service) { described_class.new(group, statistics: statistics)} + + describe '#execute', :aggregate_failures do + context 'when group is nil' do + let(:group) { nil } + + it 'does nothing' do + expect(NamespaceStatistics).not_to receive(:new) + + result = service.execute + + expect(result).to be_error + end + end + + context 'with an existing group' do + context 'when namespace statistics exists for the group' do + it 'uses the existing statistics and refreshes them' do + namespace_statistics = create(:namespace_statistics, namespace: group) + + expect(namespace_statistics).to receive(:refresh!).with(only: statistics.map(&:to_sym)).and_call_original + + result = service.execute + + expect(result).to be_success + end + end + + context 'when namespace statistics does not exist for the group' do + it 'creates the statistics and refreshes them' do + expect_next_instance_of(NamespaceStatistics) do |instance| + expect(instance).to receive(:refresh!).with(only: statistics.map(&:to_sym)).and_call_original + end + + result = nil + + expect do + result = service.execute + end.to change { NamespaceStatistics.count }.by(1) + + expect(result).to be_success + end + end + end + end +end diff --git a/spec/services/incident_management/create_incident_label_service_spec.rb b/spec/services/incident_management/create_incident_label_service_spec.rb deleted file mode 100644 index 441cddf1d2e..00000000000 --- a/spec/services/incident_management/create_incident_label_service_spec.rb +++ /dev/null @@ -1,7 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe IncidentManagement::CreateIncidentLabelService do - it_behaves_like 'incident management label service' -end diff --git a/spec/services/incident_management/incidents/create_service_spec.rb b/spec/services/incident_management/incidents/create_service_spec.rb index 47ce9d01f66..ac44bc4608c 100644 --- a/spec/services/incident_management/incidents/create_service_spec.rb +++ b/spec/services/incident_management/incidents/create_service_spec.rb @@ -14,7 +14,6 @@ RSpec.describe IncidentManagement::Incidents::CreateService do context 'when incident has title and description' do let(:title) { 'Incident title' } let(:new_issue) { Issue.last! } - let(:label_title) { attributes_for(:label, :incident)[:title] } it 'responds with success' do expect(create_incident).to be_success @@ -38,8 +37,6 @@ RSpec.describe IncidentManagement::Incidents::CreateService do end let(:issue) { new_issue } - - include_examples 'does not have incident label' end context 'with default severity' do @@ -69,20 +66,6 @@ RSpec.describe IncidentManagement::Incidents::CreateService do end end end - - context 'when incident label does not exists' do - it 'does not create incident label' do - expect { create_incident }.to not_change { project.labels.where(title: label_title).count } - end - end - - context 'when incident label already exists' do - let!(:label) { create(:label, project: project, title: label_title) } - - it 'does not create new labels' do - expect { create_incident }.not_to change(Label, :count) - end - end end context 'when incident has no title' do @@ -100,6 +83,25 @@ RSpec.describe IncidentManagement::Incidents::CreateService do it 'result payload contains an Issue object' do expect(create_incident.payload[:issue]).to be_kind_of(Issue) end + + context 'with alert' do + let(:alert) { create(:alert_management_alert, project: project) } + + subject(:create_incident) { described_class.new(project, user, title: title, description: description, alert: alert).execute } + + it 'associates the alert with the incident' do + expect(create_incident[:issue].alert_management_alert).to eq(alert) + end + + context 'the alert prevents the issue from saving' do + let(:alert) { create(:alert_management_alert, :with_validation_errors, project: project) } + + it 'responds with errors' do + expect(create_incident).to be_error + expect(create_incident.message).to eq('Hosts hosts array is over 255 chars') + end + end + end end end end diff --git a/spec/services/incident_management/issuable_escalation_statuses/after_update_service_spec.rb b/spec/services/incident_management/issuable_escalation_statuses/after_update_service_spec.rb index e9db6ba8d28..731406613dd 100644 --- a/spec/services/incident_management/issuable_escalation_statuses/after_update_service_spec.rb +++ b/spec/services/incident_management/issuable_escalation_statuses/after_update_service_spec.rb @@ -30,7 +30,15 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::AfterUpdateServic end end + shared_examples 'adds a status change system note' do + specify do + expect { result }.to change { issue.reload.notes.count }.by(1) + end + end + context 'with status attributes' do + it_behaves_like 'adds a status change system note' + it 'updates the alert with the new alert status' do expect(::AlertManagement::Alerts::UpdateService).to receive(:new).once.and_call_original expect(described_class).to receive(:new).once.and_call_original @@ -45,12 +53,15 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::AfterUpdateServic end it_behaves_like 'does not attempt to update the alert' + it_behaves_like 'adds a status change system note' end context 'when new status matches the current status' do let(:status_event) { :trigger } it_behaves_like 'does not attempt to update the alert' + + specify { expect { result }.not_to change { issue.reload.notes.count } } end end end diff --git a/spec/services/incident_management/issuable_escalation_statuses/prepare_update_service_spec.rb b/spec/services/incident_management/issuable_escalation_statuses/prepare_update_service_spec.rb index bfed5319028..b30b3a69ae6 100644 --- a/spec/services/incident_management/issuable_escalation_statuses/prepare_update_service_spec.rb +++ b/spec/services/incident_management/issuable_escalation_statuses/prepare_update_service_spec.rb @@ -37,7 +37,7 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::PrepareUpdateServ end shared_examples 'invalid params error response' do - include_examples 'error response', 'Invalid value was provided for a parameter.' + include_examples 'error response', 'Invalid value was provided for parameters: status' end it_behaves_like 'successful response', { status_event: :acknowledge } @@ -105,4 +105,10 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::PrepareUpdateServ it_behaves_like 'successful response', { status_event: :acknowledge } end end + + context 'with status_change_reason param' do + let(:params) { { status_change_reason: ' by changing the incident status' } } + + it_behaves_like 'successful response', { status_change_reason: ' by changing the incident status' } + end end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 158f9dec83e..1f6118e9fcc 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -118,7 +118,7 @@ RSpec.describe Issues::CloseService do expect { service.execute(issue) }.to change { issue.notes.count }.by(1) new_note = issue.notes.last - expect(new_note.note).to eq('changed the status to **Resolved** by closing the incident') + expect(new_note.note).to eq('changed the incident status to **Resolved** by closing the incident') expect(new_note.author).to eq(user) end @@ -334,8 +334,12 @@ RSpec.describe Issues::CloseService do let!(:alert) { create(:alert_management_alert, issue: issue, project: project) } it 'resolves an alert and sends a system note' do - expect_next_instance_of(SystemNotes::AlertManagementService) do |notes_service| - expect(notes_service).to receive(:closed_alert_issue).with(issue) + expect_any_instance_of(SystemNoteService) do |notes_service| + expect(notes_service).to receive(:change_alert_status).with( + alert, + current_user, + " by closing issue #{issue.to_reference(project)}" + ) end close_issue diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index b2dcfb5c6d3..f4bb1f0877b 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -17,7 +17,7 @@ RSpec.describe Issues::CreateService do 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_klass).to eq(Gitlab::ApplicationRateLimiter) + expect(described_class.rate_limiter_scoped_and_keyed.rate_limiter).to eq(Gitlab::ApplicationRateLimiter) end end @@ -92,6 +92,21 @@ RSpec.describe Issues::CreateService do end end + context 'when setting a position' do + let(:issue_before) { create(:issue, project: project, relative_position: 10) } + let(:issue_after) { create(:issue, project: project, relative_position: 50) } + + before do + opts.merge!(move_between_ids: [issue_before.id, issue_after.id]) + end + + it 'sets the correct relative position' do + expect(issue).to be_persisted + expect(issue.relative_position).to be_present + expect(issue.relative_position).to be_between(issue_before.relative_position, issue_after.relative_position) + end + end + it_behaves_like 'not an incident issue' context 'when issue is incident type' do @@ -100,7 +115,6 @@ RSpec.describe Issues::CreateService do end let(:current_user) { user } - let(:incident_label_attributes) { attributes_for(:label, :incident) } subject { issue } @@ -114,12 +128,6 @@ RSpec.describe Issues::CreateService do end it_behaves_like 'incident issue' - it_behaves_like 'does not have incident label' - - it 'does not create an incident label' do - expect { subject } - .to not_change { Label.where(incident_label_attributes).count } - end it 'calls IncidentManagement::Incidents::CreateEscalationStatusService' do expect_next(::IncidentManagement::IssuableEscalationStatuses::CreateService, a_kind_of(Issue)) diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index ef501f47f0d..35a380e01d0 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -168,6 +168,48 @@ RSpec.describe Issues::MoveService do end end + context 'issue with contacts' do + let_it_be(:contacts) { create_list(:contact, 2, group: group) } + + before do + old_issue.customer_relations_contacts = contacts + end + + it 'preserves contacts' do + new_issue = move_service.execute(old_issue, new_project) + + expect(new_issue.customer_relations_contacts).to eq(contacts) + end + + context 'when moving to another root group' do + let(:another_project) { create(:project, namespace: create(:group)) } + + before do + another_project.add_reporter(user) + end + + it 'does not preserve contacts' do + new_issue = move_service.execute(old_issue, another_project) + + expect(new_issue.customer_relations_contacts).to be_empty + end + end + + context 'when customer_relations feature is disabled' do + let(:another_project) { create(:project, namespace: create(:group)) } + + before do + stub_feature_flags(customer_relations: false) + end + + it 'does not preserve contacts' do + new_issue = move_service.execute(old_issue, new_project) + + expect(new_issue.customer_relations_contacts).to be_empty + end + end + end + context 'moving to same project' do let(:new_project) { old_project } diff --git a/spec/services/issues/reorder_service_spec.rb b/spec/services/issues/reorder_service_spec.rb index 15668a3aa23..392930c1b9f 100644 --- a/spec/services/issues/reorder_service_spec.rb +++ b/spec/services/issues/reorder_service_spec.rb @@ -67,20 +67,10 @@ RSpec.describe Issues::ReorderService do it_behaves_like 'issues reorder service' context 'when ordering in a group issue list' do - let(:params) { { move_after_id: issue2.id, move_before_id: issue3.id, group_full_path: group.full_path } } + let(:params) { { move_after_id: issue2.id, move_before_id: issue3.id } } subject { service(params) } - it 'sends the board_group_id parameter' do - match_params = { move_between_ids: [issue2.id, issue3.id], board_group_id: group.id } - - expect(Issues::UpdateService) - .to receive(:new).with(project: project, current_user: user, params: match_params) - .and_return(double(execute: build(:issue))) - - subject.execute(issue1) - end - it 'sorts issues' do project2 = create(:project, namespace: group) issue4 = create(:issue, project: project2) diff --git a/spec/services/issues/set_crm_contacts_service_spec.rb b/spec/services/issues/set_crm_contacts_service_spec.rb index 2418f317551..64011a7a003 100644 --- a/spec/services/issues/set_crm_contacts_service_spec.rb +++ b/spec/services/issues/set_crm_contacts_service_spec.rb @@ -7,20 +7,41 @@ RSpec.describe Issues::SetCrmContactsService do let_it_be(:group) { create(:group, :crm_enabled) } let_it_be(:project) { create(:project, group: 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 + create(:issue_customer_relations_contact, issue: issue, contact: contacts[0]).contact + end - let(:issue) { create(:issue, project: project) } - let(:does_not_exist_or_no_permission) { "The resource that you are attempting to access does not exist or you don't have permission to perform this action" } - - before do - create(:issue_customer_relations_contact, issue: issue, contact: contacts[0]) - create(:issue_customer_relations_contact, issue: issue, contact: contacts[1]) + let_it_be(:issue_contact_2) do + create(:issue_customer_relations_contact, issue: issue, contact: contacts[1]).contact end + let(:does_not_exist_or_no_permission) { "The resource that you are attempting to access does not exist or you don't have permission to perform this action" } + subject(:set_crm_contacts) do described_class.new(project: project, current_user: user, params: params).execute(issue) end describe '#execute' do + shared_examples 'setting contacts' do + it 'updates the issue with correct contacts' do + response = set_crm_contacts + + expect(response).to be_success + expect(issue.customer_relations_contacts).to match_array(expected_contacts) + end + end + + shared_examples 'adds system note' do |added_count, removed_count| + it 'calls SystemNoteService.change_issuable_contacts with correct counts' do + expect(SystemNoteService) + .to receive(:change_issuable_contacts) + .with(issue, project, user, added_count, removed_count) + + set_crm_contacts + end + end + context 'when the user has no permission' do let(:params) { { replace_ids: [contacts[1].id, contacts[2].id] } } @@ -67,56 +88,63 @@ RSpec.describe Issues::SetCrmContactsService do context 'replace' do let(:params) { { replace_ids: [contacts[1].id, contacts[2].id] } } + let(:expected_contacts) { [contacts[1], contacts[2]] } - it 'updates the issue with correct contacts' do - response = set_crm_contacts - - expect(response).to be_success - expect(issue.customer_relations_contacts).to match_array([contacts[1], contacts[2]]) - end + it_behaves_like 'setting contacts' + it_behaves_like 'adds system note', 1, 1 end context 'add' do - let(:params) { { add_ids: [contacts[3].id] } } - - it 'updates the issue with correct contacts' do - response = set_crm_contacts + let(:added_contact) { contacts[3] } + let(:params) { { add_ids: [added_contact.id] } } + let(:expected_contacts) { [issue_contact_1, issue_contact_2, added_contact] } - expect(response).to be_success - expect(issue.customer_relations_contacts).to match_array([contacts[0], contacts[1], contacts[3]]) - end + it_behaves_like 'setting contacts' + it_behaves_like 'adds system note', 1, 0 end context 'add by email' do - let(:params) { { add_emails: [contacts[3].email] } } + let(:added_contact) { contacts[3] } + let(:expected_contacts) { [issue_contact_1, issue_contact_2, added_contact] } - it 'updates the issue with correct contacts' do - response = set_crm_contacts + context 'with pure emails in params' do + let(:params) { { add_emails: [contacts[3].email] } } - expect(response).to be_success - expect(issue.customer_relations_contacts).to match_array([contacts[0], contacts[1], contacts[3]]) + it_behaves_like 'setting contacts' + it_behaves_like 'adds system note', 1, 0 + end + + context 'with autocomplete prefix emails in params' do + let(:params) { { add_emails: ["[\"contact:\"#{contacts[3].email}\"]"] } } + + it_behaves_like 'setting contacts' + it_behaves_like 'adds system note', 1, 0 end end context 'remove' do let(:params) { { remove_ids: [contacts[0].id] } } + let(:expected_contacts) { [contacts[1]] } - it 'updates the issue with correct contacts' do - response = set_crm_contacts - - expect(response).to be_success - expect(issue.customer_relations_contacts).to match_array([contacts[1]]) - end + it_behaves_like 'setting contacts' + it_behaves_like 'adds system note', 0, 1 end context 'remove by email' do - let(:params) { { remove_emails: [contacts[0].email] } } + let(:expected_contacts) { [contacts[1]] } - it 'updates the issue with correct contacts' do - response = set_crm_contacts + context 'with pure email in params' do + let(:params) { { remove_emails: [contacts[0].email] } } - expect(response).to be_success - expect(issue.customer_relations_contacts).to match_array([contacts[1]]) + it_behaves_like 'setting contacts' + it_behaves_like 'adds system note', 0, 1 + end + + context 'with autocomplete prefix and suffix email in params' do + let(:params) { { remove_emails: ["[contact:#{contacts[0].email}]"] } } + + it_behaves_like 'setting contacts' + it_behaves_like 'adds system note', 0, 1 end end @@ -145,15 +173,19 @@ RSpec.describe Issues::SetCrmContactsService do context 'when combining params' do let(:error_invalid_params) { 'You cannot combine replace_ids with add_ids or remove_ids' } + let(:expected_contacts) { [contacts[0], contacts[3]] } context 'add and remove' do - let(:params) { { remove_ids: [contacts[1].id], add_ids: [contacts[3].id] } } + context 'with contact ids' do + let(:params) { { remove_ids: [contacts[1].id], add_ids: [contacts[3].id] } } - it 'updates the issue with correct contacts' do - response = set_crm_contacts + it_behaves_like 'setting contacts' + end + + context 'with contact emails' do + let(:params) { { remove_emails: [contacts[1].email], add_emails: ["[\"contact:#{contacts[3].email}]"] } } - expect(response).to be_success - expect(issue.customer_relations_contacts).to match_array([contacts[0], contacts[3]]) + it_behaves_like 'setting contacts' end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 11ed47b84d9..95394ba6597 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -385,7 +385,6 @@ RSpec.describe Issues::UpdateService, :mailer do [issue_1, issue_2, issue_3].map(&:save) opts[:move_between_ids] = [issue_1.id, issue_2.id] - opts[:board_group_id] = group.id described_class.new(project: issue_3.project, current_user: user, params: opts).execute(issue_3) expect(issue_2.relative_position).to be_between(issue_1.relative_position, issue_2.relative_position) @@ -1147,11 +1146,11 @@ RSpec.describe Issues::UpdateService, :mailer do let(:opts) { { escalation_status: { status: 'acknowledged' } } } let(:escalation_update_class) { ::IncidentManagement::IssuableEscalationStatuses::AfterUpdateService } - shared_examples 'updates the escalation status record' do |expected_status| + shared_examples 'updates the escalation status record' do |expected_status, expected_reason = nil| let(:service_double) { instance_double(escalation_update_class) } it 'has correct value' do - expect(escalation_update_class).to receive(:new).with(issue, user).and_return(service_double) + expect(escalation_update_class).to receive(:new).with(issue, user, status_change_reason: expected_reason).and_return(service_double) expect(service_double).to receive(:execute) update_issue(opts) @@ -1197,6 +1196,12 @@ RSpec.describe Issues::UpdateService, :mailer do end end + context 'with a status change reason provided' do + let(:opts) { { escalation_status: { status: 'acknowledged', status_change_reason: ' by changing the alert status' } } } + + it_behaves_like 'updates the escalation status record', :acknowledged, ' by changing the alert status' + end + context 'with unsupported status value' do let(:opts) { { escalation_status: { status: 'unsupported-status' } } } @@ -1303,19 +1308,12 @@ RSpec.describe Issues::UpdateService, :mailer do end context 'when moving an issue ' do - it 'raises an error for invalid move ids within a project' do + it 'raises an error for invalid move ids' do opts = { move_between_ids: [9000, non_existing_record_id] } expect { described_class.new(project: issue.project, current_user: user, params: opts).execute(issue) } .to raise_error(ActiveRecord::RecordNotFound) end - - it 'raises an error for invalid move ids within a group' do - opts = { move_between_ids: [9000, non_existing_record_id], board_group_id: create(:group).id } - - expect { described_class.new(project: issue.project, current_user: user, params: opts).execute(issue) } - .to raise_error(ActiveRecord::RecordNotFound) - end end include_examples 'issuable update service' do diff --git a/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb b/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb index d3d57ea2444..538d9638879 100644 --- a/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb +++ b/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb @@ -115,4 +115,82 @@ RSpec.describe LooseForeignKeys::BatchCleanerService do expect(loose_fk_child_table_2.where(parent_id_with_different_column: other_parent_record.id).count).to eq(2) end end + + describe 'fair queueing' do + context 'when the execution is over the limit' do + let(:modification_tracker) { instance_double(LooseForeignKeys::ModificationTracker) } + let(:over_limit_return_values) { [true] } + let(:deleted_record) { LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 1).first } + let(:deleted_records_rescheduled_counter) { Gitlab::Metrics.registry.get(:loose_foreign_key_rescheduled_deleted_records) } + let(:deleted_records_incremented_counter) { Gitlab::Metrics.registry.get(:loose_foreign_key_incremented_deleted_records) } + + let(:cleaner) do + described_class.new(parent_table: '_test_loose_fk_parent_table', + loose_foreign_key_definitions: loose_foreign_key_definitions, + deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), + modification_tracker: modification_tracker + ) + end + + before do + parent_record_1.delete + allow(modification_tracker).to receive(:over_limit?).and_return(*over_limit_return_values) + allow(modification_tracker).to receive(:add_deletions) + end + + context 'when the deleted record is under the maximum allowed cleanup attempts' do + it 'updates the cleanup_attempts column', :aggregate_failures do + deleted_record.update!(cleanup_attempts: 1) + + cleaner.execute + + expect(deleted_record.reload.cleanup_attempts).to eq(2) + expect(deleted_records_incremented_counter.get(table: loose_fk_parent_table.table_name, db_config_name: 'main')).to eq(1) + end + + context 'when the deleted record is above the maximum allowed cleanup attempts' do + it 'reschedules the record', :aggregate_failures do + deleted_record.update!(cleanup_attempts: LooseForeignKeys::BatchCleanerService::CLEANUP_ATTEMPTS_BEFORE_RESCHEDULE + 1) + + freeze_time do + cleaner.execute + + expect(deleted_record.reload).to have_attributes( + cleanup_attempts: 0, + consume_after: 5.minutes.from_now + ) + expect(deleted_records_rescheduled_counter.get(table: loose_fk_parent_table.table_name, db_config_name: 'main')).to eq(1) + end + end + end + + describe 'when over limit happens on the second cleanup call without skip locked' do + # over_limit? is called twice, we test here the 2nd call + # - When invoking cleanup with SKIP LOCKED + # - When invoking cleanup (no SKIP LOCKED) + let(:over_limit_return_values) { [false, true] } + + it 'updates the cleanup_attempts column' do + expect(cleaner).to receive(:run_cleaner_service).twice + + deleted_record.update!(cleanup_attempts: 1) + + cleaner.execute + + expect(deleted_record.reload.cleanup_attempts).to eq(2) + end + end + end + + context 'when the lfk_fair_queueing FF is off' do + before do + stub_feature_flags(lfk_fair_queueing: false) + end + + it 'does nothing' do + expect { cleaner.execute }.not_to change { deleted_record.reload.cleanup_attempts } + end + end + end + end end diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb index 13f56fe7458..4d9e69719b4 100644 --- a/spec/services/members/create_service_spec.rb +++ b/spec/services/members/create_service_spec.rb @@ -39,6 +39,15 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ expect(source.users).to include member expect(OnboardingProgress.completed?(source, :user_added)).to be(true) end + + it 'triggers a members added event' do + expect(Gitlab::EventStore) + .to receive(:publish) + .with(an_instance_of(Members::MembersAddedEvent)) + .and_call_original + + expect(execute_service[:status]).to eq(:success) + end end end @@ -108,6 +117,24 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ user: user ) end + + context 'with an already existing member' do + before do + source.add_developer(member) + end + + it 'tracks the invite source from params' do + execute_service + + expect_snowplow_event( + category: described_class.name, + action: 'create_member', + label: '_invite_source_', + property: 'existing_user', + user: user + ) + end + end end context 'when it is a net_new_user' do diff --git a/spec/services/merge_requests/after_create_service_spec.rb b/spec/services/merge_requests/after_create_service_spec.rb index 781be57d709..2155b4ffad1 100644 --- a/spec/services/merge_requests/after_create_service_spec.rb +++ b/spec/services/merge_requests/after_create_service_spec.rb @@ -89,10 +89,10 @@ RSpec.describe MergeRequests::AfterCreateService do merge_request.mark_as_preparing! end - it 'marks the merge request as unchecked' do - execute_service + it 'checks for mergeability' do + expect(merge_request).to receive(:check_mergeability).with(async: true) - expect(merge_request.reload).to be_unchecked + execute_service end context 'when preparing for mergeability fails' do @@ -108,17 +108,6 @@ RSpec.describe MergeRequests::AfterCreateService do expect { execute_service }.to raise_error(StandardError) expect(merge_request.reload).to be_preparing end - - context 'when early_prepare_for_mergeability feature flag is disabled' do - before do - stub_feature_flags(early_prepare_for_mergeability: false) - end - - it 'does not mark the merge request as unchecked' do - expect { execute_service }.to raise_error(StandardError) - expect(merge_request.reload).to be_preparing - end - end end context 'when preparing merge request fails' do @@ -130,20 +119,9 @@ RSpec.describe MergeRequests::AfterCreateService do .and_raise(StandardError) end - it 'still marks the merge request as unchecked' do + it 'still checks for mergeability' do + expect(merge_request).to receive(:check_mergeability).with(async: true) expect { execute_service }.to raise_error(StandardError) - expect(merge_request.reload).to be_unchecked - end - - context 'when early_prepare_for_mergeability feature flag is disabled' do - before do - stub_feature_flags(early_prepare_for_mergeability: false) - end - - it 'does not mark the merge request as unchecked' do - expect { execute_service }.to raise_error(StandardError) - expect(merge_request.reload).to be_preparing - end end end end 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 fe4ce0dab5e..ae8846974ce 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 @@ -10,7 +10,7 @@ RSpec.describe MergeRequests::BulkRemoveAttentionRequestedService do let(:reviewer) { merge_request.find_reviewer(user) } let(:assignee) { merge_request.find_assignee(assignee_user) } let(:project) { merge_request.project } - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request) } + let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request, users: [user, assignee_user]) } let(:result) { service.execute } before do @@ -20,7 +20,7 @@ RSpec.describe MergeRequests::BulkRemoveAttentionRequestedService do describe '#execute' do context 'invalid permissions' do - let(:service) { described_class.new(project: project, current_user: create(:user), merge_request: merge_request) } + let(:service) { described_class.new(project: project, current_user: create(:user), merge_request: merge_request, users: [user]) } it 'returns an error' do expect(result[:status]).to eq :error diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index da547716e1e..a196c944eda 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -61,19 +61,19 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do expect(merge_request.reload).to be_preparing end - describe 'when marked with /wip' do + describe 'when marked with /draft' do context 'in title and in description' do let(:opts) do { - title: 'WIP: Awesome merge_request', - description: "well this is not done yet\n/wip", + title: 'Draft: Awesome merge_request', + description: "well this is not done yet\n/draft", source_branch: 'feature', target_branch: 'master', assignees: [user2] } end - it 'sets MR to WIP' do + it 'sets MR to draft' do expect(merge_request.work_in_progress?).to be(true) end end @@ -89,7 +89,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do } end - it 'sets MR to WIP' do + it 'sets MR to draft' do expect(merge_request.work_in_progress?).to be(true) end end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 127c94763d9..ecb856bd1a4 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -388,7 +388,7 @@ RSpec.describe MergeRequests::MergeService do end it 'logs and saves error if there is an error when squashing' do - error_message = 'Failed to squash. Should be done manually' + error_message = 'Squashing failed: Squash the commits locally, resolve any conflicts, then push the branch.' allow_any_instance_of(MergeRequests::SquashService).to receive(:squash!).and_return(nil) merge_request.update!(squash: true) diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb index 65599b7e046..c24b83e21a6 100644 --- a/spec/services/merge_requests/mergeability_check_service_spec.rb +++ b/spec/services/merge_requests/mergeability_check_service_spec.rb @@ -73,12 +73,10 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar let(:merge_request) { create(:merge_request, merge_status: :unchecked, source_project: project, target_project: project) } describe '#async_execute' do - shared_examples_for 'no job is enqueued' do - it 'does not enqueue MergeRequestMergeabilityCheckWorker' do - expect(MergeRequestMergeabilityCheckWorker).not_to receive(:perform_async) + it 'updates merge status to checking' do + described_class.new(merge_request).async_execute - described_class.new(merge_request).async_execute - end + expect(merge_request).to be_checking end it 'enqueues MergeRequestMergeabilityCheckWorker' do @@ -92,15 +90,11 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar allow(Gitlab::Database).to receive(:read_only?) { true } end - it_behaves_like 'no job is enqueued' - end + it 'does not enqueue MergeRequestMergeabilityCheckWorker' do + expect(MergeRequestMergeabilityCheckWorker).not_to receive(:perform_async) - context 'when merge_status is already checking' do - before do - merge_request.mark_as_checking + described_class.new(merge_request).async_execute end - - it_behaves_like 'no job is enqueued' end end diff --git a/spec/services/merge_requests/rebase_service_spec.rb b/spec/services/merge_requests/rebase_service_spec.rb index e671bbf2cd6..a47e626666b 100644 --- a/spec/services/merge_requests/rebase_service_spec.rb +++ b/spec/services/merge_requests/rebase_service_spec.rb @@ -82,7 +82,7 @@ RSpec.describe MergeRequests::RebaseService do context 'with a pre-receive failure' do let(:pre_receive_error) { "Commit message does not follow the pattern 'ACME'" } - let(:merge_error) { "Something went wrong during the rebase pre-receive hook: #{pre_receive_error}." } + let(:merge_error) { "The rebase pre-receive hook failed: #{pre_receive_error}." } before do allow(repository).to receive(:gitaly_operation_client).and_raise(Gitlab::Git::PreReceiveError, "GitLab: #{pre_receive_error}") diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb index e5bea0e7b14..387be8471b5 100644 --- a/spec/services/merge_requests/squash_service_spec.rb +++ b/spec/services/merge_requests/squash_service_spec.rb @@ -168,7 +168,7 @@ RSpec.describe MergeRequests::SquashService do it 'raises a squash error' do expect(service.execute).to match( status: :error, - message: a_string_including('does not allow squashing commits when merge requests are accepted')) + message: a_string_including('allow you to squash commits when merging')) end end @@ -205,7 +205,7 @@ RSpec.describe MergeRequests::SquashService do end it 'returns an error' do - expect(service.execute).to match(status: :error, message: a_string_including('squash')) + expect(service.execute).to match(status: :error, message: a_string_including('Squash')) end end end @@ -232,7 +232,7 @@ RSpec.describe MergeRequests::SquashService do end it 'returns an error' do - expect(service.execute).to match(status: :error, message: a_string_including('squash')) + expect(service.execute).to match(status: :error, message: a_string_including('Squash')) end it 'cleans up the temporary directory' do diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 2925dad7f6b..48d9f019274 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -102,16 +102,16 @@ RSpec.describe MergeRequests::UpdateService, :mailer do MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request2) end - it 'tracks Draft/WIP marking' do + it 'tracks Draft marking' do expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) .to receive(:track_marked_as_draft_action).once.with(user: user) - opts[:title] = "WIP: #{opts[:title]}" + opts[:title] = "Draft: #{opts[:title]}" MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request2) end - it 'tracks Draft/WIP un-marking' do + it 'tracks Draft un-marking' do expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) .to receive(:track_unmarked_as_draft_action).once.with(user: user) diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 1fb50b07b3b..babbd44a9f1 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -325,11 +325,11 @@ RSpec.describe Notes::CreateService do expect(issuable.work_in_progress?).to eq(can_use_quick_action) } ), - # Remove WIP status + # Remove draft status QuickAction.new( action_text: "/draft", before_action: -> { - issuable.reload.update!(title: "WIP: title") + issuable.reload.update!(title: "Draft: title") }, expectation: ->(noteable, can_use_quick_action) { expect(noteable.work_in_progress?).not_to eq(can_use_quick_action) diff --git a/spec/services/packages/maven/metadata/sync_service_spec.rb b/spec/services/packages/maven/metadata/sync_service_spec.rb index a736ed281f0..9a704d749b3 100644 --- a/spec/services/packages/maven/metadata/sync_service_spec.rb +++ b/spec/services/packages/maven/metadata/sync_service_spec.rb @@ -9,6 +9,7 @@ RSpec.describe ::Packages::Maven::Metadata::SyncService do let_it_be(:user) { create(:user) } let_it_be_with_reload(:versionless_package_for_versions) { create(:maven_package, name: 'test', version: nil, project: project) } let_it_be_with_reload(:metadata_file_for_versions) { create(:package_file, :xml, package: versionless_package_for_versions) } + let_it_be(:package_file_pending_destruction) { create(:package_file, :pending_destruction, package: versionless_package_for_versions, file_name: Packages::Maven::Metadata.filename) } let(:service) { described_class.new(container: project, current_user: user, params: { package_name: versionless_package_for_versions.name }) } @@ -265,22 +266,4 @@ RSpec.describe ::Packages::Maven::Metadata::SyncService do end end end - - # TODO When cleaning up packages_installable_package_files, consider adding a - # dummy package file pending for destruction on L10/11 and remove this context - context 'with package files pending destruction' do - let_it_be(:package_file_pending_destruction) { create(:package_file, :pending_destruction, package: versionless_package_for_versions, file_name: Packages::Maven::Metadata.filename) } - - subject { service.send(:metadata_package_file_for, versionless_package_for_versions) } - - it { is_expected.not_to eq(package_file_pending_destruction) } - - context 'with packages_installable_package_files disabled' do - before do - stub_feature_flags(packages_installable_package_files: false) - end - - it { is_expected.to eq(package_file_pending_destruction) } - end - end end diff --git a/spec/services/packages/nuget/metadata_extraction_service_spec.rb b/spec/services/packages/nuget/metadata_extraction_service_spec.rb index 8eddd27f8a2..fc21cfd502e 100644 --- a/spec/services/packages/nuget/metadata_extraction_service_spec.rb +++ b/spec/services/packages/nuget/metadata_extraction_service_spec.rb @@ -78,7 +78,7 @@ RSpec.describe Packages::Nuget::MetadataExtractionService do end context 'with invalid package file id' do - let(:package_file) { OpenStruct.new(id: 555) } + let(:package_file) { double('file', id: 555) } it { expect { subject }.to raise_error(::Packages::Nuget::MetadataExtractionService::ExtractionError, 'invalid package file') } end @@ -109,7 +109,7 @@ RSpec.describe Packages::Nuget::MetadataExtractionService do context 'with a too big nuspec file' do before do - allow_any_instance_of(Zip::File).to receive(:glob).and_return([OpenStruct.new(size: 6.megabytes)]) + allow_any_instance_of(Zip::File).to receive(:glob).and_return([double('file', size: 6.megabytes)]) end it { expect { subject }.to raise_error(::Packages::Nuget::MetadataExtractionService::ExtractionError, 'nuspec file too big') } diff --git a/spec/services/pages/zip_directory_service_spec.rb b/spec/services/pages/zip_directory_service_spec.rb index 9cce90c6c0d..00fe75dbbfd 100644 --- a/spec/services/pages/zip_directory_service_spec.rb +++ b/spec/services/pages/zip_directory_service_spec.rb @@ -27,6 +27,10 @@ RSpec.describe Pages::ZipDirectoryService do let(:archive) { result[:archive_path] } let(:entries_count) { result[:entries_count] } + it 'returns true if ZIP64 is enabled' do + expect(::Zip.write_zip64_support).to be true + end + shared_examples 'handles invalid public directory' do it 'returns success' do expect(status).to eq(:success) @@ -35,7 +39,7 @@ RSpec.describe Pages::ZipDirectoryService do end end - context "when work direcotry doesn't exist" do + context "when work directory doesn't exist" do let(:service_directory) { "/tmp/not/existing/dir" } include_examples 'handles invalid public directory' diff --git a/spec/services/projects/autocomplete_service_spec.rb b/spec/services/projects/autocomplete_service_spec.rb index ef7741c2d0f..ed043bacf31 100644 --- a/spec/services/projects/autocomplete_service_spec.rb +++ b/spec/services/projects/autocomplete_service_spec.rb @@ -148,6 +148,32 @@ RSpec.describe Projects::AutocompleteService do end end + describe '#contacts' 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(:contact_1) { create(:contact, group: group) } + let_it_be(:contact_2) { create(:contact, group: group) } + + subject { described_class.new(project, user).contacts.as_json } + + before do + stub_feature_flags(customer_relations: true) + group.add_developer(user) + end + + it 'returns contact data correctly' do + expected_contacts = [ + { 'id' => contact_1.id, 'email' => contact_1.email, + 'first_name' => contact_1.first_name, 'last_name' => contact_1.last_name }, + { 'id' => contact_2.id, 'email' => contact_2.email, + 'first_name' => contact_2.first_name, 'last_name' => contact_2.last_name } + ] + + expect(subject).to match_array(expected_contacts) + end + end + describe '#labels_as_hash' do def expect_labels_to_equal(labels, expected_labels) expect(labels.size).to eq(expected_labels.size) diff --git a/spec/services/projects/container_repository/delete_tags_service_spec.rb b/spec/services/projects/container_repository/delete_tags_service_spec.rb index 94037d6de1e..246ca301cfa 100644 --- a/spec/services/projects/container_repository/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/delete_tags_service_spec.rb @@ -14,7 +14,7 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do ] end - RSpec.shared_examples 'logging a success response' do + shared_examples 'logging a success response' do it 'logs an info message' do expect(service).to receive(:log_info).with( service_class: 'Projects::ContainerRepository::DeleteTagsService', @@ -28,7 +28,7 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do end end - RSpec.shared_examples 'logging an error response' do |message: 'could not delete tags', extra_log: {}| + shared_examples 'logging an error response' do |message: 'could not delete tags', extra_log: {}| it 'logs an error message' do log_data = { service_class: 'Projects::ContainerRepository::DeleteTagsService', @@ -45,7 +45,7 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do end end - RSpec.shared_examples 'calling the correct delete tags service' do |expected_service_class| + shared_examples 'calling the correct delete tags service' do |expected_service_class| let(:service_response) { { status: :success, deleted: tags } } let(:excluded_service_class) { available_service_classes.excluding(expected_service_class).first } @@ -69,7 +69,7 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do end end - RSpec.shared_examples 'handling invalid params' do + shared_examples 'handling invalid params' do context 'with invalid params' do before do expect(::Projects::ContainerRepository::Gitlab::DeleteTagsService).not_to receive(:new) @@ -91,7 +91,7 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do end end - RSpec.shared_examples 'supporting fast delete' do + shared_examples 'supporting fast delete' do context 'when the registry supports fast delete' do before do allow(repository.client).to receive(:supports_tag_delete?).and_return(true) @@ -155,6 +155,14 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do it_behaves_like 'handling invalid params' end + + context 'when the repository is importing' do + before do + repository.update_columns(migration_state: 'importing', migration_import_started_at: Time.zone.now) + end + + it { is_expected.to include(status: :error, message: 'repository importing') } + end end context 'without user' do diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index d5fbf96ce74..10f694827e1 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -7,9 +7,10 @@ RSpec.describe Projects::CreateService, '#execute' do include GitHelpers let(:user) { create :user } + let(:project_name) { 'GitLab' } let(:opts) do { - name: 'GitLab', + name: project_name, namespace_id: user.namespace.id } end @@ -144,6 +145,12 @@ RSpec.describe Projects::CreateService, '#execute' do subject { create_project(user, opts) } end + + it 'logs creation' do + expect(Gitlab::AppLogger).to receive(:info).with(/#{user.name} created a new project/) + + create_project(user, opts) + end end context "admin creates project with other user's namespace_id" do @@ -183,9 +190,13 @@ RSpec.describe Projects::CreateService, '#execute' do user.refresh_authorized_projects # Ensure cache is warm end - it do - project = create_project(user, opts.merge!(namespace_id: group.id)) + subject(:project) { create_project(user, opts.merge!(namespace_id: group.id)) } + shared_examples 'has sync-ed traversal_ids' do + specify { expect(project.reload.project_namespace.traversal_ids).to eq([project.namespace.traversal_ids, project.project_namespace.id].flatten.compact) } + end + + it 'creates the project' do expect(project).to be_valid expect(project.owner).to eq(group) expect(project.namespace).to eq(group) @@ -193,6 +204,18 @@ RSpec.describe Projects::CreateService, '#execute' do expect(user.authorized_projects).to include(project) 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 end context 'group sharing', :sidekiq_inline do @@ -202,7 +225,7 @@ RSpec.describe Projects::CreateService, '#execute' do let(:opts) do { - name: 'GitLab', + name: project_name, namespace_id: shared_group.id } end @@ -237,7 +260,7 @@ RSpec.describe Projects::CreateService, '#execute' do let(:share_max_access_level) { Gitlab::Access::MAINTAINER } let(:opts) do { - name: 'GitLab', + name: project_name, namespace_id: subgroup_for_projects.id } end @@ -583,58 +606,55 @@ RSpec.describe Projects::CreateService, '#execute' do opts[:initialize_with_readme] = '1' end - shared_examples 'creates README.md' do + shared_examples 'a repo with a README.md' do it { expect(project.repository.commit_count).to be(1) } it { expect(project.repository.readme.name).to eql('README.md') } - it { expect(project.repository.readme.data).to include('# GitLab') } + it { expect(project.repository.readme.data).to include(expected_content) } end - it_behaves_like 'creates README.md' + it_behaves_like 'a repo with a README.md' do + let(:expected_content) do + <<~MARKDOWN + cd existing_repo + git remote add origin #{project.http_url_to_repo} + git branch -M master + git push -uf origin master + MARKDOWN + end + end - context 'and a default_branch_name is specified' do + context 'and a readme_template is specified' do before do - allow(Gitlab::CurrentSettings) - .to receive(:default_branch_name) - .and_return('example_branch') + opts[:readme_template] = "# GitLab\nThis is customized readme." end - it_behaves_like 'creates README.md' + it_behaves_like 'a repo with a README.md' do + let(:expected_content) { "# GitLab\nThis is customized readme." } + end + end + + context 'and a default_branch_name is specified' do + before do + allow(Gitlab::CurrentSettings).to receive(:default_branch_name).and_return('example_branch') + end - it 'creates README.md within the specified branch rather than master' do + it 'creates the correct branch' do branches = project.repository.branches expect(branches.size).to eq(1) expect(branches.collect(&:name)).to contain_exactly('example_branch') end - describe 'advanced readme content', experiment: :new_project_readme_content do - before do - stub_experiments(new_project_readme_content: :advanced) - end - - it_behaves_like 'creates README.md' - - it 'includes advanced content in the README.md' do - content = project.repository.readme.data - expect(content).to include <<~MARKDOWN + it_behaves_like 'a repo with a README.md' do + let(:expected_content) do + <<~MARKDOWN + cd existing_repo git remote add origin #{project.http_url_to_repo} git branch -M example_branch git push -uf origin example_branch MARKDOWN end end - - context 'and readme_template is specified' do - before do - opts[:readme_template] = "# GitLab\nThis is customized template." - end - - it_behaves_like 'creates README.md' - - it 'creates README.md with specified template' do - expect(project.repository.readme.data).to include('This is customized template.') - end - end end end @@ -676,7 +696,7 @@ RSpec.describe Projects::CreateService, '#execute' do let(:opts) do { - name: 'GitLab', + name: project_name, namespace_id: group.id } end @@ -697,7 +717,7 @@ RSpec.describe Projects::CreateService, '#execute' do let(:opts) do { - name: 'GitLab', + name: project_name, namespace_id: subgroup.id } end @@ -808,7 +828,7 @@ RSpec.describe Projects::CreateService, '#execute' do let(:opts) do { - name: 'GitLab', + name: project_name, namespace_id: group.id } end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 9475f562d71..d60ec8c2958 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -15,20 +15,39 @@ 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 - before do - # Run sidekiq immediately to check that renamed repository will be removed + it 'deletes the project', :sidekiq_inline do destroy_project(project, user, {}) - end - it 'deletes the project', :sidekiq_inline do expect(Project.unscoped.all).not_to include(project) expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey end + + 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 + end end shared_examples 'deleting the project with pipeline and build' do @@ -97,6 +116,24 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do end end + context "deleting a project with merge requests" do + let!(:merge_request) { create(:merge_request, source_project: project) } + + before do + allow(project).to receive(:destroy!).and_return(true) + end + + it "deletes merge request and related records" do + merge_request_diffs = merge_request.merge_request_diffs + expect(merge_request_diffs.size).to eq(1) + + mrdc_count = MergeRequestDiffCommit.where(merge_request_diff_id: merge_request_diffs.first.id).count + + expect { destroy_project(project, user, {}) } + .to change { MergeRequestDiffCommit.count }.by(-mrdc_count) + end + end + it_behaves_like 'deleting the project' it 'invalidates personal_project_count cache' do diff --git a/spec/services/projects/import_export/export_service_spec.rb b/spec/services/projects/import_export/export_service_spec.rb index 6002aaf427a..54abbc04084 100644 --- a/spec/services/projects/import_export/export_service_spec.rb +++ b/spec/services/projects/import_export/export_service_spec.rb @@ -93,11 +93,23 @@ RSpec.describe Projects::ImportExport::ExportService do end it 'saves the project in the file system' do - expect(Gitlab::ImportExport::Saver).to receive(:save).with(exportable: project, shared: shared) + expect(Gitlab::ImportExport::Saver).to receive(:save).with(exportable: project, shared: shared).and_return(true) service.execute end + context 'when the upload fails' do + before do + expect(Gitlab::ImportExport::Saver).to receive(:save).with(exportable: project, shared: shared).and_return(false) + end + + it 'notifies the user of an error' do + expect(service).to receive(:notify_error).and_call_original + + expect { service.execute }.to raise_error(Gitlab::ImportExport::Error) + end + end + it 'calls the after export strategy' do expect(after_export_strategy).to receive(:execute) @@ -107,6 +119,7 @@ RSpec.describe Projects::ImportExport::ExportService do context 'when after export strategy fails' do before do allow(after_export_strategy).to receive(:execute).and_return(false) + expect(Gitlab::ImportExport::Saver).to receive(:save).with(exportable: project, shared: shared).and_return(true) end after do diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index 1d63f72ec38..ccfd119b55b 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -298,7 +298,7 @@ RSpec.describe Projects::ImportService do end def stub_github_omniauth_provider - provider = OpenStruct.new( + provider = ActiveSupport::InheritableOptions.new( 'name' => 'github', 'app_id' => 'asd123', 'app_secret' => 'asd123', diff --git a/spec/services/projects/overwrite_project_service_spec.rb b/spec/services/projects/overwrite_project_service_spec.rb index cc6a863a11d..7038910508f 100644 --- a/spec/services/projects/overwrite_project_service_spec.rb +++ b/spec/services/projects/overwrite_project_service_spec.rb @@ -81,16 +81,58 @@ RSpec.describe Projects::OverwriteProjectService do end end - it 'removes the original project' do - subject.execute(project_from) + it 'schedules original project for deletion' do + expect_next_instance_of(Projects::DestroyService) do |service| + expect(service).to receive(:async_execute) + end - expect { Project.find(project_from.id) }.to raise_error(ActiveRecord::RecordNotFound) + subject.execute(project_from) end it 'renames the project' do + original_path = project_from.full_path + subject.execute(project_from) - expect(project_to.full_path).to eq project_from.full_path + expect(project_to.full_path).to eq(original_path) + end + + it 'renames source project to temp name' do + allow(SecureRandom).to receive(:hex).and_return('test') + + subject.execute(project_from) + + expect(project_from.full_path).to include('-old-test') + end + + context 'when project rename fails' do + before do + expect(subject).to receive(:move_relationships_between).with(project_from, project_to) + expect(subject).to receive(:move_relationships_between).with(project_to, project_from) + end + + context 'source rename' do + it 'moves relations back to source project and raises an exception' do + allow(subject).to receive(:rename_project).and_return(status: :error) + + expect { subject.execute(project_from) }.to raise_error(StandardError, 'Source project rename failed during project overwrite') + end + end + + context 'new project rename' do + it 'moves relations back, renames source project back to original name and raises' do + name = project_from.name + path = project_from.path + + allow(subject).to receive(:rename_project).and_call_original + allow(subject).to receive(:rename_project).with(project_to, name, path).and_return(status: :error) + + expect { subject.execute(project_from) }.to raise_error(StandardError, 'New project rename failed during project overwrite') + + expect(project_from.name).to eq(name) + expect(project_from.path).to eq(path) + end + end end end @@ -121,7 +163,7 @@ RSpec.describe Projects::OverwriteProjectService do end end - context 'forks' do + context 'forks', :sidekiq_inline do context 'when moving a root forked project' do it 'moves the descendant forks' do expect(project_from.forks.count).to eq 2 @@ -147,6 +189,7 @@ RSpec.describe Projects::OverwriteProjectService do expect(project_to.fork_network.fork_network_members.map(&:project)).not_to include project_from end end + context 'when moving a intermediate forked project' do let(:project_to) { create(:project, namespace: lvl1_forked_project_1.namespace) } @@ -180,22 +223,26 @@ RSpec.describe Projects::OverwriteProjectService do end context 'if an exception is raised' do + before do + allow(subject).to receive(:rename_project).and_raise(StandardError) + end + it 'rollbacks changes' do updated_at = project_from.updated_at - allow(subject).to receive(:rename_project).and_raise(StandardError) - expect { subject.execute(project_from) }.to raise_error(StandardError) expect(Project.find(project_from.id)).not_to be_nil expect(project_from.reload.updated_at.change(usec: 0)).to eq updated_at.change(usec: 0) end - it 'tries to restore the original project repositories' do - allow(subject).to receive(:rename_project).and_raise(StandardError) - - expect(subject).to receive(:attempt_restore_repositories).with(project_from) + it 'removes fork network member' do + expect(ForkNetworkMember).to receive(:create!) + expect(ForkNetworkMember).to receive(:find_by) + expect(subject).to receive(:remove_source_project_from_fork_network).and_call_original expect { subject.execute(project_from) }.to raise_error(StandardError) + + expect(project_from.fork_network_member).to be_nil end end end diff --git a/spec/services/projects/readme_renderer_service_spec.rb b/spec/services/projects/readme_renderer_service_spec.rb new file mode 100644 index 00000000000..14cdcf67640 --- /dev/null +++ b/spec/services/projects/readme_renderer_service_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::ReadmeRendererService, '#execute' do + using RSpec::Parameterized::TableSyntax + + subject(:service) { described_class.new(project, nil, opts) } + + let_it_be(:project) { create(:project, title: 'My Project', description: '_custom_description_') } + + let(:opts) { {} } + + it 'renders the an ERB readme template' do + expect(service.execute).to start_with(<<~MARKDOWN) + # My Project + + _custom_description_ + + ## Getting started + + To make it easy for you to get started with GitLab, here's a list of recommended next steps. + + Already a pro? Just edit this README.md and make it your own. Want to make it easy? [Use the template at the bottom](#editing-this-readme)! + + ## Add your files + + - [ ] [Create](https://docs.gitlab.com/ee/user/project/repository/web_editor.html#create-a-file) or [upload](https://docs.gitlab.com/ee/user/project/repository/web_editor.html#upload-a-file) files + - [ ] [Add files using the command line](https://docs.gitlab.com/ee/gitlab-basics/add-file.html#add-a-file-using-the-command-line) or push an existing Git repository with the following command: + + ``` + cd existing_repo + git remote add origin #{project.http_url_to_repo} + git branch -M master + git push -uf origin master + ``` + MARKDOWN + end + + context 'with a custom template' do + before do + allow(File).to receive(:read).and_call_original + end + + it 'renders that template file' do + opts[:template_name] = :custom_readme + + expect(service).to receive(:sanitized_filename).with(:custom_readme).and_return('custom_readme.md.tt') + expect(File).to receive(:read).with('custom_readme.md.tt').and_return('_custom_readme_file_content_') + expect(service.execute).to eq('_custom_readme_file_content_') + end + + context 'with path traversal in mind' do + where(:template_name, :exception, :expected_path) do + '../path/traversal/bad' | [Gitlab::Utils::PathTraversalAttackError, 'Invalid path'] | nil + '/bad/template' | [StandardError, 'path /bad/template.md.tt is not allowed'] | nil + 'good/template' | nil | 'good/template.md.tt' + end + + with_them do + it 'raises the expected exception on bad paths' do + opts[:template_name] = template_name + + if exception + expect { subject.execute }.to raise_error(*exception) + else + expect(File).to receive(:read).with(described_class::TEMPLATE_PATH.join(expected_path).to_s).and_return('') + + expect { subject.execute }.not_to raise_error + end + end + end + end + end +end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index ddd16100b40..fb94e94fd18 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -5,13 +5,14 @@ require 'spec_helper' RSpec.describe Projects::TransferService do include GitHelpers - let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } + let_it_be(:user) { create(:user) } let_it_be(:group_integration) { create(:integrations_slack, :group, group: group, webhook: 'http://group.slack.com') } let(:project) { create(:project, :repository, :legacy_storage, namespace: user.namespace) } + let(:target) { group } - subject(:execute_transfer) { described_class.new(project, user).execute(group).tap { project.reload } } + subject(:execute_transfer) { described_class.new(project, user).execute(target).tap { project.reload } } context 'with npm packages' do before do @@ -690,6 +691,32 @@ RSpec.describe Projects::TransferService do end end + context 'handling issue contacts' do + let_it_be(:root_group) { create(:group) } + + let(:project) { create(:project, group: root_group) } + + before do + root_group.add_owner(user) + target.add_owner(user) + create_list(:issue_customer_relations_contact, 2, :for_issue, issue: create(:issue, project: project)) + end + + context 'with the same root_ancestor' do + let(:target) { create(:group, parent: root_group) } + + it 'retains issue contacts' do + expect { execute_transfer }.not_to change { CustomerRelations::IssueContact.count } + end + end + + context 'with a different root_ancestor' do + it 'deletes issue contacts' do + expect { execute_transfer }.to change { CustomerRelations::IssueContact.count }.by(-2) + end + end + end + def rugged_config rugged_repo(project.repository).config end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index e56e54db6f4..ca3ae437540 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -11,13 +11,14 @@ RSpec.describe QuickActions::InterpretService do let_it_be(:developer2) { create(:user) } let_it_be(:developer3) { create(:user) } let_it_be_with_reload(:issue) { create(:issue, project: project) } - let(:milestone) { create(:milestone, project: project, title: '9.10') } - let(:commit) { create(:commit, project: project) } let_it_be(:inprogress) { create(:label, project: project, title: 'In Progress') } let_it_be(:helmchart) { create(:label, project: project, title: 'Helm Chart Registry') } let_it_be(:bug) { create(:label, project: project, title: 'Bug') } - let(:service) { described_class.new(project, developer) } + let(:milestone) { create(:milestone, project: project, title: '9.10') } + let(:commit) { create(:commit, project: project) } + + subject(:service) { described_class.new(project, developer) } before_all do public_project.add_developer(developer) @@ -485,6 +486,8 @@ RSpec.describe QuickActions::InterpretService do end shared_examples 'failed command' do |error_msg| + let(:match_msg) { error_msg ? eq(error_msg) : be_empty } + it 'populates {} if content contains an unsupported command' do _, updates, _ = service.execute(content, issuable) @@ -494,11 +497,7 @@ RSpec.describe QuickActions::InterpretService do it "returns #{error_msg || 'an empty'} message" do _, _, message = service.execute(content, issuable) - if error_msg - expect(message).to eq(error_msg) - else - expect(message).to be_empty - end + expect(message).to match_msg end end @@ -703,6 +702,27 @@ RSpec.describe QuickActions::InterpretService do end end + shared_examples 'attention command' do + it 'updates reviewers attention status' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Requested attention from #{developer.to_reference}.") + + reviewer.reload + + expect(reviewer).to be_attention_requested + end + end + + shared_examples 'remove attention command' do + it 'updates reviewers attention status' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Removed attention from #{developer.to_reference}.") + expect(reviewer).not_to be_attention_requested + end + end + it_behaves_like 'reopen command' do let(:content) { '/reopen' } let(:issuable) { issue } @@ -887,9 +907,10 @@ RSpec.describe QuickActions::InterpretService do end end - it_behaves_like 'failed command', "Failed to assign a user because no user was found." do + it_behaves_like 'failed command', 'a parse error' do let(:content) { '/assign @abcd1234' } let(:issuable) { issue } + let(:match_msg) { eq "Could not apply assign command. Failed to find users for '@abcd1234'." } end it_behaves_like 'failed command', "Failed to assign a user because no user was found." do @@ -950,10 +971,38 @@ RSpec.describe QuickActions::InterpretService do it_behaves_like 'assign_reviewer command' end + context 'with a private user' do + let(:ref) { create(:user, :unconfirmed).to_reference } + let(:content) { "/assign_reviewer #{ref}" } + + it_behaves_like 'failed command', 'a parse error' do + let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for '#{ref}'." } + end + end + + context 'with a private user, bare username' do + let(:ref) { create(:user, :unconfirmed).username } + let(:content) { "/assign_reviewer #{ref}" } + + it_behaves_like 'failed command', 'a parse error' do + let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for '#{ref}'." } + end + end + + context 'with @all' do + let(:content) { "/assign_reviewer @all" } + + it_behaves_like 'failed command', 'a parse error' do + let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for '@all'." } + end + end + context 'with an incorrect user' do let(:content) { '/assign_reviewer @abcd1234' } - it_behaves_like 'failed command', "Failed to assign a reviewer because no user was found." + it_behaves_like 'failed command', 'a parse error' do + let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for '@abcd1234'." } + end end context 'with the "reviewer" alias' do @@ -971,13 +1020,15 @@ RSpec.describe QuickActions::InterpretService do context 'with no user' do let(:content) { '/assign_reviewer' } - it_behaves_like 'failed command', "Failed to assign a reviewer because no user was found." + it_behaves_like 'failed command', "Failed to assign a reviewer because no user was specified." end - context 'includes only the user reference with extra text' do - let(:content) { "/assign_reviewer @#{developer.username} do it!" } + context 'with extra text' do + let(:content) { "/assign_reviewer #{developer.to_reference} do it!" } - it_behaves_like 'assign_reviewer command' + it_behaves_like 'failed command', 'a parse error' do + let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for 'do' and 'it!'." } + end end end @@ -2279,6 +2330,82 @@ RSpec.describe QuickActions::InterpretService do expect(message).to eq('One or more contacts were successfully removed.') end end + + describe 'attention command' do + let(:issuable) { create(:merge_request, reviewers: [developer], source_project: project) } + let(:reviewer) { issuable.merge_request_reviewers.find_by(user_id: developer.id) } + let(:content) { "/attention @#{developer.username}" } + + context 'with one user' do + before do + reviewer.update!(state: :reviewed) + end + + it_behaves_like 'attention command' + end + + context 'with no user' do + let(:content) { "/attention" } + + it_behaves_like 'failed command', 'Failed to request attention because no user was found.' + end + + context 'with incorrect permissions' do + let(:service) { described_class.new(project, create(:user)) } + + it_behaves_like 'failed command', 'Could not apply attention command.' + end + + context 'with feature flag disabled' do + before do + stub_feature_flags(mr_attention_requests: false) + end + + it_behaves_like 'failed command', 'Could not apply attention command.' + end + + context 'with an issue instead of a merge request' do + let(:issuable) { issue } + + it_behaves_like 'failed command', 'Could not apply attention command.' + end + end + + describe 'remove attention command' do + let(:issuable) { create(:merge_request, reviewers: [developer], source_project: project) } + let(:reviewer) { issuable.merge_request_reviewers.find_by(user_id: developer.id) } + let(:content) { "/remove_attention @#{developer.username}" } + + context 'with one user' do + it_behaves_like 'remove attention command' + end + + context 'with no user' do + let(:content) { "/remove_attention" } + + it_behaves_like 'failed command', 'Failed to remove attention because no user was found.' + end + + context 'with incorrect permissions' do + let(:service) { described_class.new(project, create(:user)) } + + it_behaves_like 'failed command', 'Could not apply remove_attention command.' + end + + context 'with feature flag disabled' do + before do + stub_feature_flags(mr_attention_requests: false) + end + + it_behaves_like 'failed command', 'Could not apply remove_attention command.' + end + + context 'with an issue instead of a merge request' do + let(:issuable) { issue } + + it_behaves_like 'failed command', 'Could not apply remove_attention command.' + end + end end describe '#explain' do @@ -2317,12 +2444,42 @@ RSpec.describe QuickActions::InterpretService do end describe 'assign command' do - let(:content) { "/assign @#{developer.username} do it!" } + shared_examples 'assigns developer' do + it 'tells us we will assign the developer' do + _, explanations = service.explain(content, merge_request) - it 'includes only the user reference' do - _, explanations = service.explain(content, merge_request) + expect(explanations).to eq(["Assigns @#{developer.username}."]) + end + end + + context 'when using a reference' do + let(:content) { "/assign @#{developer.username}" } - expect(explanations).to eq(["Assigns @#{developer.username}."]) + include_examples 'assigns developer' + end + + context 'when using a bare username' do + let(:content) { "/assign #{developer.username}" } + + include_examples 'assigns developer' + end + + context 'when using me' do + let(:content) { "/assign me" } + + include_examples 'assigns developer' + end + + context 'when there are unparseable arguments' do + let(:arg) { "#{developer.username} to this issue" } + let(:content) { "/assign #{arg}" } + + it 'tells us why we cannot do that' do + _, explanations = service.explain(content, merge_request) + + expect(explanations) + .to contain_exactly "Problem with assign command: Failed to find users for 'to', 'this', and 'issue'." + end end end @@ -2598,6 +2755,45 @@ RSpec.describe QuickActions::InterpretService do expect(service.commands_executed_count).to eq(3) end end + + describe 'crm commands' do + let(:add_contacts) { '/add_contacts' } + let(:remove_contacts) { '/remove_contacts' } + + before_all do + group.add_developer(developer) + end + + context 'when group has no contacts' do + it '/add_contacts is not available' do + _, explanations = service.explain(add_contacts, issue) + + expect(explanations).to be_empty + end + + it '/remove_contacts is not available' do + _, explanations = service.explain(remove_contacts, issue) + + expect(explanations).to be_empty + end + end + + context 'when group has contacts' do + let!(:contact) { create(:contact, group: group) } + + it '/add_contacts is available' do + _, explanations = service.explain(add_contacts, issue) + + expect(explanations).to contain_exactly("Add customer relation contact(s).") + end + + it '/remove_contacts is available' do + _, explanations = service.explain(remove_contacts, issue) + + expect(explanations).to contain_exactly("Remove customer relation contact(s).") + end + end + end end describe '#available_commands' do diff --git a/spec/services/releases/create_service_spec.rb b/spec/services/releases/create_service_spec.rb index 7287825a0be..d53fc968e2a 100644 --- a/spec/services/releases/create_service_spec.rb +++ b/spec/services/releases/create_service_spec.rb @@ -146,7 +146,7 @@ RSpec.describe Releases::CreateService do end end - context 'when multiple miletone titles are passed in but one of them does not exist' do + context 'when multiple milestone titles are passed in but one of them does not exist' do let(:title) { 'v1.0' } let(:inexistent_title) { 'v111.0' } let!(:milestone) { create(:milestone, :active, project: project, title: title) } diff --git a/spec/services/security/ci_configuration/container_scanning_create_service_spec.rb b/spec/services/security/ci_configuration/container_scanning_create_service_spec.rb new file mode 100644 index 00000000000..df76750efc8 --- /dev/null +++ b/spec/services/security/ci_configuration/container_scanning_create_service_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::CiConfiguration::ContainerScanningCreateService, :snowplow do + subject(:result) { described_class.new(project, user).execute } + + let(:branch_name) { 'set-container-scanning-config-1' } + + let(:snowplow_event) do + { + category: 'Security::CiConfiguration::ContainerScanningCreateService', + action: 'create', + label: '' + } + end + + include_examples 'services security ci configuration create service', true +end diff --git a/spec/services/service_ping/submit_service_ping_service_spec.rb b/spec/services/service_ping/submit_service_ping_service_spec.rb index 2971c9a9309..73be8f000a9 100644 --- a/spec/services/service_ping/submit_service_ping_service_spec.rb +++ b/spec/services/service_ping/submit_service_ping_service_spec.rb @@ -55,7 +55,7 @@ RSpec.describe ServicePing::SubmitService do shared_examples 'does not run' do it do expect(Gitlab::HTTP).not_to receive(:post) - expect(Gitlab::UsageData).not_to receive(:data) + expect(Gitlab::Usage::ServicePingReport).not_to receive(:for) subject.execute end @@ -63,7 +63,7 @@ RSpec.describe ServicePing::SubmitService do shared_examples 'does not send a blank usage ping payload' do it do - expect(Gitlab::HTTP).not_to receive(:post) + expect(Gitlab::HTTP).not_to receive(:post).with(subject.url, any_args) expect { subject.execute }.to raise_error(described_class::SubmissionError) do |error| expect(error.message).to include('Usage data is blank') @@ -118,7 +118,7 @@ RSpec.describe ServicePing::SubmitService do it 'generates service ping' do stub_response(body: with_dev_ops_score_params) - expect(Gitlab::UsageData).to receive(:data).with(force_refresh: true).and_call_original + expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_call_original subject.execute end @@ -129,6 +129,7 @@ RSpec.describe ServicePing::SubmitService do stub_usage_data_connections stub_database_flavor_check stub_application_setting(usage_ping_enabled: true) + stub_response(body: nil, url: subject.error_url, status: 201) end context 'and user requires usage stats consent' do @@ -150,7 +151,7 @@ RSpec.describe ServicePing::SubmitService do it 'forces a refresh of usage data statistics before submitting' do stub_response(body: with_dev_ops_score_params) - expect(Gitlab::UsageData).to receive(:data).with(force_refresh: true).and_call_original + expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_call_original subject.execute end @@ -166,7 +167,7 @@ RSpec.describe ServicePing::SubmitService do recorded_at = Time.current usage_data = { uuid: 'uuid', recorded_at: recorded_at } - expect(Gitlab::UsageData).to receive(:data).with(force_refresh: true).and_return(usage_data) + expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_return(usage_data) subject.execute @@ -189,7 +190,7 @@ RSpec.describe ServicePing::SubmitService do recorded_at = Time.current usage_data = { uuid: 'uuid', recorded_at: recorded_at } - expect(Gitlab::UsageData).to receive(:data).with(force_refresh: true).and_return(usage_data) + expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_return(usage_data) subject.execute @@ -234,7 +235,7 @@ RSpec.describe ServicePing::SubmitService do recorded_at = Time.current usage_data = { uuid: 'uuid', recorded_at: recorded_at } - expect(Gitlab::UsageData).to receive(:data).with(force_refresh: true).and_return(usage_data) + expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_return(usage_data) subject.execute @@ -259,7 +260,7 @@ RSpec.describe ServicePing::SubmitService do context 'and usage data is empty string' do before do - allow(Gitlab::UsageData).to receive(:data).and_return({}) + allow(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_return({}) end it_behaves_like 'does not send a blank usage ping payload' @@ -268,7 +269,7 @@ RSpec.describe ServicePing::SubmitService do context 'and usage data is nil' do before do allow(ServicePing::BuildPayloadService).to receive(:execute).and_return(nil) - allow(Gitlab::UsageData).to receive(:data).and_return(nil) + allow(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_return(nil) end it_behaves_like 'does not send a blank usage ping payload' @@ -277,13 +278,23 @@ RSpec.describe ServicePing::SubmitService do context 'if payload service fails' do before do stub_response(body: with_dev_ops_score_params) - allow(ServicePing::BuildPayloadService).to receive(:execute).and_raise(described_class::SubmissionError, 'SubmissionError') + allow(ServicePing::BuildPayloadService).to receive_message_chain(:new, :execute) + .and_raise(described_class::SubmissionError, 'SubmissionError') end - it 'calls UsageData .data method' do + it 'calls Gitlab::Usage::ServicePingReport .for method' do usage_data = build_usage_data - expect(Gitlab::UsageData).to receive(:data).and_return(usage_data) + expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_return(usage_data) + + subject.execute + end + + it 'submits error' do + expect(Gitlab::HTTP).to receive(:post).with(subject.url, any_args) + .and_call_original + expect(Gitlab::HTTP).to receive(:post).with(subject.error_url, any_args) + .and_call_original subject.execute end @@ -315,10 +326,10 @@ RSpec.describe ServicePing::SubmitService do end end - it 'calls UsageData .data method' do + it 'calls Gitlab::Usage::ServicePingReport .for method' do usage_data = build_usage_data - expect(Gitlab::UsageData).to receive(:data).and_return(usage_data) + expect(Gitlab::Usage::ServicePingReport).to receive(:for).with(output: :all_metrics_values).and_return(usage_data) # SubmissionError is raised as a result of 404 in response from HTTP Request expect { subject.execute }.to raise_error(described_class::SubmissionError) diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 3ec2c71b20c..a719487a219 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -77,6 +77,19 @@ RSpec.describe SystemNoteService do end end + describe '.change_issuable_contacts' do + let(:added_count) { 5 } + let(:removed_count) { 3 } + + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_issuable_contacts).with(added_count, removed_count) + end + + described_class.change_issuable_contacts(noteable, project, author, added_count, removed_count) + end + end + describe '.close_after_error_tracking_resolve' do it 'calls IssuableService' do expect_next_instance_of(::SystemNotes::IssuablesService) do |service| @@ -572,12 +585,26 @@ RSpec.describe SystemNoteService do describe '.change_alert_status' do let(:alert) { build(:alert_management_alert) } - it 'calls AlertManagementService' do - expect_next_instance_of(SystemNotes::AlertManagementService) do |service| - expect(service).to receive(:change_alert_status).with(alert) + context 'with status change reason' do + let(:reason) { 'reason for status change' } + + it 'calls AlertManagementService' do + expect_next_instance_of(SystemNotes::AlertManagementService) do |service| + expect(service).to receive(:change_alert_status).with(reason) + end + + described_class.change_alert_status(alert, author, reason) end + end + + context 'without status change reason' do + it 'calls AlertManagementService' do + expect_next_instance_of(SystemNotes::AlertManagementService) do |service| + expect(service).to receive(:change_alert_status).with(nil) + end - described_class.change_alert_status(alert, author) + described_class.change_alert_status(alert, author) + end end end @@ -618,15 +645,29 @@ RSpec.describe SystemNoteService do end end - describe '.resolve_incident_status' do - let(:incident) { build(:incident, :closed) } + describe '.change_incident_status' do + let(:incident) { instance_double('Issue', project: project) } - it 'calls IncidentService' do - expect_next_instance_of(SystemNotes::IncidentService) do |service| - expect(service).to receive(:resolve_incident_status) + context 'with status change reason' do + let(:reason) { 'reason for status change' } + + it 'calls IncidentService' do + expect_next_instance_of(SystemNotes::IncidentService) do |service| + expect(service).to receive(:change_incident_status).with(reason) + end + + described_class.change_incident_status(incident, author, reason) end + end - described_class.resolve_incident_status(incident, author) + context 'without status change reason' do + it 'calls IncidentService' do + expect_next_instance_of(SystemNotes::IncidentService) do |service| + expect(service).to receive(:change_incident_status).with(nil) + end + + described_class.change_incident_status(incident, author) + end end end diff --git a/spec/services/system_notes/alert_management_service_spec.rb b/spec/services/system_notes/alert_management_service_spec.rb index 6e6bfeaa205..039975c1bf6 100644 --- a/spec/services/system_notes/alert_management_service_spec.rb +++ b/spec/services/system_notes/alert_management_service_spec.rb @@ -21,14 +21,26 @@ RSpec.describe ::SystemNotes::AlertManagementService do end describe '#change_alert_status' do - subject { described_class.new(noteable: noteable, project: project, author: author).change_alert_status(noteable) } + subject { described_class.new(noteable: noteable, project: project, author: author).change_alert_status(reason) } - it_behaves_like 'a system note' do - let(:action) { 'status' } + context 'with no specified reason' do + let(:reason) { nil } + + it_behaves_like 'a system note' do + let(:action) { 'status' } + end + + it 'has the appropriate message' do + expect(subject.note).to eq("changed the status to **Acknowledged**") + end end - it 'has the appropriate message' do - expect(subject.note).to eq("changed the status to **Acknowledged**") + context 'with reason provided' do + let(:reason) { ' by changing incident status' } + + it 'has the appropriate message' do + expect(subject.note).to eq("changed the status to **Acknowledged** by changing incident status") + end end end @@ -42,21 +54,7 @@ RSpec.describe ::SystemNotes::AlertManagementService do end it 'has the appropriate message' do - expect(subject.note).to eq("created issue #{issue.to_reference(project)} for this alert") - end - end - - describe '#closed_alert_issue' do - let_it_be(:issue) { noteable.issue } - - subject { described_class.new(noteable: noteable, project: project, author: author).closed_alert_issue(issue) } - - it_behaves_like 'a system note' do - let(:action) { 'status' } - end - - it 'has the appropriate message' do - expect(subject.note).to eq("changed the status to **Resolved** by closing issue #{issue.to_reference(project)}") + expect(subject.note).to eq("created incident #{issue.to_reference(project)} for this alert") end end diff --git a/spec/services/system_notes/incident_service_spec.rb b/spec/services/system_notes/incident_service_spec.rb index 669e357b7a4..5de352ad8fa 100644 --- a/spec/services/system_notes/incident_service_spec.rb +++ b/spec/services/system_notes/incident_service_spec.rb @@ -57,13 +57,27 @@ RSpec.describe ::SystemNotes::IncidentService do end end - describe '#resolve_incident_status' do - subject(:resolve_incident_status) { described_class.new(noteable: noteable, project: project, author: author).resolve_incident_status } + describe '#change_incident_status' do + let_it_be(:escalation_status) { create(:incident_management_issuable_escalation_status, issue: noteable) } - it 'creates a new note about resolved incident', :aggregate_failures do - expect { resolve_incident_status }.to change { noteable.notes.count }.by(1) + let(:service) { described_class.new(noteable: noteable, project: project, author: author) } - expect(noteable.notes.last.note).to eq('changed the status to **Resolved** by closing the incident') + context 'with a provided reason' do + subject(:change_incident_status) { service.change_incident_status(' by changing the alert status') } + + it 'creates a new note for an incident status change', :aggregate_failures do + expect { change_incident_status }.to change { noteable.notes.count }.by(1) + expect(noteable.notes.last.note).to eq("changed the incident status to **Triggered** by changing the alert status") + end + end + + context 'without provided reason' do + subject(:change_incident_status) { service.change_incident_status(nil) } + + it 'creates a new note for an incident status change', :aggregate_failures do + expect { change_incident_status }.to change { noteable.notes.count }.by(1) + expect(noteable.notes.last.note).to eq("changed the incident status to **Triggered**") + end end end end diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index 7e53e66303b..e1c97026418 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -188,6 +188,54 @@ RSpec.describe ::SystemNotes::IssuablesService do end end + describe '#change_issuable_contacts' do + subject { service.change_issuable_contacts(1, 1) } + + let_it_be(:noteable) { create(:issue, project: project) } + + it_behaves_like 'a system note' do + let(:action) { 'contact' } + end + + def build_note(added_count, removed_count) + service.change_issuable_contacts(added_count, removed_count).note + end + + it 'builds a correct phrase when one contact is added' do + expect(build_note(1, 0)).to eq "added 1 contact" + end + + it 'builds a correct phrase when one contact is removed' do + expect(build_note(0, 1)).to eq "removed 1 contact" + end + + it 'builds a correct phrase when one contact is added and one contact is removed' do + expect(build_note(1, 1)).to( + eq("added 1 contact and removed 1 contact") + ) + end + + it 'builds a correct phrase when three contacts are added and one removed' do + expect(build_note(3, 1)).to( + eq("added 3 contacts and removed 1 contact") + ) + end + + it 'builds a correct phrase when three contacts are removed and one added' do + expect(build_note(1, 3)).to( + eq("added 1 contact and removed 3 contacts") + ) + end + + it 'builds a correct phrase when the locale is different' do + Gitlab::I18n.with_locale('pt-BR') do + expect(build_note(1, 3)).to( + eq("added 1 contact and removed 3 contacts") + ) + end + end + end + describe '#change_status' do subject { service.change_status(status, source) } diff --git a/spec/services/test_hooks/project_service_spec.rb b/spec/services/test_hooks/project_service_spec.rb index cd6284b4a87..d97a6f15270 100644 --- a/spec/services/test_hooks/project_service_spec.rb +++ b/spec/services/test_hooks/project_service_spec.rb @@ -37,7 +37,7 @@ RSpec.describe TestHooks::ProjectService do it 'executes hook' do allow(Gitlab::DataBuilder::Push).to receive(:build_sample).and_return(sample_data) - expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) + expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result) expect(service.execute).to include(success_result) end end @@ -49,7 +49,7 @@ RSpec.describe TestHooks::ProjectService do it 'executes hook' do allow(Gitlab::DataBuilder::Push).to receive(:build_sample).and_return(sample_data) - expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) + expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result) expect(service.execute).to include(success_result) end end @@ -69,7 +69,7 @@ RSpec.describe TestHooks::ProjectService do allow(Gitlab::DataBuilder::Note).to receive(:build).and_return(sample_data) allow_next(NotesFinder).to receive(:execute).and_return(Note.all) - expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) + expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result) expect(service.execute).to include(success_result) end end @@ -86,7 +86,7 @@ RSpec.describe TestHooks::ProjectService do allow(issue).to receive(:to_hook_data).and_return(sample_data) allow_next(IssuesFinder).to receive(:execute).and_return([issue]) - expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) + expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result) expect(service.execute).to include(success_result) end end @@ -119,7 +119,7 @@ RSpec.describe TestHooks::ProjectService do allow(merge_request).to receive(:to_hook_data).and_return(sample_data) allow_next(MergeRequestsFinder).to receive(:execute).and_return([merge_request]) - expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) + expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result) expect(service.execute).to include(success_result) end end @@ -138,7 +138,7 @@ RSpec.describe TestHooks::ProjectService do allow(Gitlab::DataBuilder::Build).to receive(:build).and_return(sample_data) allow_next(Ci::JobsFinder).to receive(:execute).and_return([ci_job]) - expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) + expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result) expect(service.execute).to include(success_result) end end @@ -157,7 +157,7 @@ RSpec.describe TestHooks::ProjectService do allow(Gitlab::DataBuilder::Pipeline).to receive(:build).and_return(sample_data) allow_next(Ci::PipelinesFinder).to receive(:execute).and_return([pipeline]) - expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) + expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result) expect(service.execute).to include(success_result) end end @@ -184,7 +184,7 @@ RSpec.describe TestHooks::ProjectService do create(:wiki_page, wiki: project.wiki) allow(Gitlab::DataBuilder::WikiPage).to receive(:build).and_return(sample_data) - expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) + expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result) expect(service.execute).to include(success_result) end end @@ -203,7 +203,7 @@ RSpec.describe TestHooks::ProjectService do allow(release).to receive(:to_hook_data).and_return(sample_data) allow_next(ReleasesFinder).to receive(:execute).and_return([release]) - expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) + expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result) expect(service.execute).to include(success_result) end end diff --git a/spec/services/test_hooks/system_service_spec.rb b/spec/services/test_hooks/system_service_spec.rb index 48c8c24212a..66a1218d123 100644 --- a/spec/services/test_hooks/system_service_spec.rb +++ b/spec/services/test_hooks/system_service_spec.rb @@ -32,7 +32,7 @@ RSpec.describe TestHooks::SystemService do it 'executes hook' do expect(Gitlab::DataBuilder::Push).to receive(:sample_data).and_call_original - expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Push::SAMPLE_DATA, trigger_key).and_return(success_result) + expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Push::SAMPLE_DATA, trigger_key, force: true).and_return(success_result) expect(service.execute).to include(success_result) end end @@ -45,7 +45,7 @@ RSpec.describe TestHooks::SystemService do allow(project.repository).to receive(:tags).and_return(['tag']) expect(Gitlab::DataBuilder::Push).to receive(:sample_data).and_call_original - expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Push::SAMPLE_DATA, trigger_key).and_return(success_result) + expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Push::SAMPLE_DATA, trigger_key, force: true).and_return(success_result) expect(service.execute).to include(success_result) end end @@ -57,7 +57,7 @@ RSpec.describe TestHooks::SystemService do it 'executes hook' do expect(Gitlab::DataBuilder::Repository).to receive(:sample_data).and_call_original - expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Repository::SAMPLE_DATA, trigger_key).and_return(success_result) + expect(hook).to receive(:execute).with(Gitlab::DataBuilder::Repository::SAMPLE_DATA, trigger_key, force: true).and_return(success_result) expect(service.execute).to include(success_result) end end @@ -76,7 +76,7 @@ RSpec.describe TestHooks::SystemService do it 'executes hook' do expect(MergeRequest).to receive(:of_projects).and_return([merge_request]) expect(merge_request).to receive(:to_hook_data).and_return(sample_data) - expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) + expect(hook).to receive(:execute).with(sample_data, trigger_key, force: true).and_return(success_result) expect(service.execute).to include(success_result) end end diff --git a/spec/services/update_container_registry_info_service_spec.rb b/spec/services/update_container_registry_info_service_spec.rb index 740e53b0472..64071e79508 100644 --- a/spec/services/update_container_registry_info_service_spec.rb +++ b/spec/services/update_container_registry_info_service_spec.rb @@ -48,6 +48,7 @@ RSpec.describe UpdateContainerRegistryInfoService do before do stub_registry_info({}) + stub_supports_gitlab_api(false) end it 'uses a token with no access permissions' do @@ -63,6 +64,7 @@ RSpec.describe UpdateContainerRegistryInfoService do context 'when unabled to detect the container registry type' do it 'sets the application settings to their defaults' do stub_registry_info({}) + stub_supports_gitlab_api(false) subject @@ -76,20 +78,23 @@ RSpec.describe UpdateContainerRegistryInfoService do context 'when able to detect the container registry type' do context 'when using the GitLab container registry' do it 'updates application settings accordingly' do - stub_registry_info(vendor: 'gitlab', version: '2.9.1-gitlab', features: %w[a,b,c]) + stub_registry_info(vendor: 'gitlab', version: '2.9.1-gitlab', features: %w[a b c]) + stub_supports_gitlab_api(true) subject application_settings.reload expect(application_settings.container_registry_vendor).to eq('gitlab') expect(application_settings.container_registry_version).to eq('2.9.1-gitlab') - expect(application_settings.container_registry_features).to eq(%w[a,b,c]) + expect(application_settings.container_registry_features) + .to match_array(%W[a b c #{ContainerRegistry::GitlabApiClient::REGISTRY_GITLAB_V1_API_FEATURE}]) end end context 'when using a third-party container registry' do it 'updates application settings accordingly' do stub_registry_info(vendor: 'other', version: nil, features: nil) + stub_supports_gitlab_api(false) subject @@ -112,4 +117,10 @@ RSpec.describe UpdateContainerRegistryInfoService do allow(client).to receive(:registry_info).and_return(output) end end + + def stub_supports_gitlab_api(output) + allow_next_instance_of(ContainerRegistry::GitlabApiClient) do |client| + allow(client).to receive(:supports_gitlab_api?).and_return(output) + end + end end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 7d933ea9c5c..64371f97908 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -52,6 +52,25 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state end end + describe '#disabled?' do + using RSpec::Parameterized::TableSyntax + + subject { described_class.new(hook, data, :push_hooks, force: forced) } + + let(:hook) { double(executable?: executable, allow_local_requests?: false) } + + where(:forced, :executable, :disabled) do + false | true | false + false | false | true + true | true | false + true | false | false + end + + with_them do + it { is_expected.to have_attributes(disabled?: disabled) } + end + end + describe '#execute' do let!(:uuid) { SecureRandom.uuid } let(:headers) do @@ -129,7 +148,7 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state end it 'does not execute disabled hooks' do - project_hook.update!(recent_failures: 4) + allow(service_instance).to receive(:disabled?).and_return(true) expect(service_instance.execute).to eq({ status: :error, message: 'Hook disabled' }) end @@ -149,13 +168,13 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state .once end - it 'executes and logs if a recursive web hook is detected', :aggregate_failures do + it 'blocks and logs if a recursive web hook is detected', :aggregate_failures do stub_full_request(project_hook.url, method: :post) Gitlab::WebHooks::RecursionDetection.register!(project_hook) expect(Gitlab::AuthLogger).to receive(:error).with( include( - message: 'Webhook recursion detected and will be blocked in future', + message: 'Recursive webhook blocked from executing', hook_id: project_hook.id, hook_type: 'ProjectHook', hook_name: 'push_hooks', @@ -166,12 +185,10 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state service_instance.execute - expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)) - .with(headers: headers) - .once + expect(WebMock).not_to have_requested(:post, stubbed_hostname(project_hook.url)) end - it 'executes and logs if the recursion count limit would be exceeded', :aggregate_failures do + it 'blocks and logs if the recursion count limit would be exceeded', :aggregate_failures do stub_full_request(project_hook.url, method: :post) stub_const("#{Gitlab::WebHooks::RecursionDetection.name}::COUNT_LIMIT", 3) previous_hooks = create_list(:project_hook, 3) @@ -179,7 +196,7 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state expect(Gitlab::AuthLogger).to receive(:error).with( include( - message: 'Webhook recursion detected and will be blocked in future', + message: 'Recursive webhook blocked from executing', hook_id: project_hook.id, hook_type: 'ProjectHook', hook_name: 'push_hooks', @@ -190,9 +207,7 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state service_instance.execute - expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)) - .with(headers: headers) - .once + expect(WebMock).not_to have_requested(:post, stubbed_hostname(project_hook.url)) end it 'handles exceptions' do @@ -255,6 +270,20 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success') end + context 'when forced' do + let(:service_instance) { described_class.new(project_hook, data, :push_hooks, force: true) } + + it 'logs execution inline' do + expect(::WebHooks::LogExecutionWorker).not_to receive(:perform_async) + expect(::WebHooks::LogExecutionService) + .to receive(:new) + .with(hook: project_hook, log_data: Hash, response_category: :ok) + .and_return(double(execute: nil)) + + run_service + end + end + it 'log successful execution' do run_service @@ -408,7 +437,7 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state describe '#async_execute' do def expect_to_perform_worker(hook) - expect(WebHookWorker).to receive(:perform_async).with(hook.id, data, 'push_hooks') + expect(WebHookWorker).to receive(:perform_async).with(hook.id, data, 'push_hooks', an_instance_of(Hash)) end def expect_to_rate_limit(hook, threshold:, throttled: false) @@ -496,15 +525,15 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state Gitlab::WebHooks::RecursionDetection.set_request_uuid(SecureRandom.uuid) end - it 'queues a worker and logs an error if the call chain limit would be exceeded' do + it 'does not queue a worker and logs an error if the call chain limit would be exceeded' do stub_const("#{Gitlab::WebHooks::RecursionDetection.name}::COUNT_LIMIT", 3) previous_hooks = create_list(:project_hook, 3) previous_hooks.each { Gitlab::WebHooks::RecursionDetection.register!(_1) } - expect(WebHookWorker).to receive(:perform_async) + expect(WebHookWorker).not_to receive(:perform_async) expect(Gitlab::AuthLogger).to receive(:error).with( include( - message: 'Webhook recursion detected and will be blocked in future', + message: 'Recursive webhook blocked from executing', hook_id: project_hook.id, hook_type: 'ProjectHook', hook_name: 'push_hooks', @@ -519,13 +548,13 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state service_instance.async_execute end - it 'queues a worker and logs an error if a recursive call chain is detected' do + it 'does not queue a worker and logs an error if a recursive call chain is detected' do Gitlab::WebHooks::RecursionDetection.register!(project_hook) - expect(WebHookWorker).to receive(:perform_async) + expect(WebHookWorker).not_to receive(:perform_async) expect(Gitlab::AuthLogger).to receive(:error).with( include( - message: 'Webhook recursion detected and will be blocked in future', + message: 'Recursive webhook blocked from executing', hook_id: project_hook.id, hook_type: 'ProjectHook', hook_name: 'push_hooks', diff --git a/spec/services/work_items/create_service_spec.rb b/spec/services/work_items/create_service_spec.rb index 2c054ae59a0..f495e967b26 100644 --- a/spec/services/work_items/create_service_spec.rb +++ b/spec/services/work_items/create_service_spec.rb @@ -5,34 +5,46 @@ require 'spec_helper' RSpec.describe WorkItems::CreateService do include AfterNextHelpers - let_it_be(:group) { create(:group) } - let_it_be_with_reload(:project) { create(:project, group: group) } - let_it_be(:user) { create(:user) } + let_it_be_with_reload(:project) { create(:project) } + let_it_be(:guest) { create(:user) } + let_it_be(:user_with_no_access) { create(:user) } let(:spam_params) { double } + let(:current_user) { guest } + let(:opts) do + { + title: 'Awesome work_item', + description: 'please fix' + } + end + + before_all do + project.add_guest(guest) + end describe '#execute' do - let(:work_item) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute } + subject(:service_result) { described_class.new(project: project, current_user: current_user, params: opts, spam_params: spam_params).execute } before do stub_spam_services end - context 'when params are valid' do - before_all do - project.add_guest(user) - end + context 'when user is not allowed to create a work item in the project' do + let(:current_user) { user_with_no_access } + + it { is_expected.to be_error } - let(:opts) do - { - title: 'Awesome work_item', - description: 'please fix' - } + it 'returns an access error' do + expect(service_result.errors).to contain_exactly('Operation not allowed') end + end + context 'when params are valid' do it 'created instance is a WorkItem' do expect(Issuable::CommonSystemNotesService).to receive_message_chain(:new, :execute) + work_item = service_result[:work_item] + expect(work_item).to be_persisted expect(work_item).to be_a(::WorkItem) expect(work_item.title).to eq('Awesome work_item') @@ -41,17 +53,17 @@ RSpec.describe WorkItems::CreateService do end end - context 'checking spam' do - let(:params) do - { - title: 'Spam work_item' - } - end + context 'when params are invalid' do + let(:opts) { { title: '' } } - subject do - described_class.new(project: project, current_user: user, params: params, spam_params: spam_params) + it { is_expected.to be_error } + + it 'returns validation errors' do + expect(service_result.errors).to contain_exactly("Title can't be blank") end + end + context 'checking spam' do it 'executes SpamActionService' do expect_next_instance_of( Spam::SpamActionService, @@ -65,7 +77,7 @@ RSpec.describe WorkItems::CreateService do expect(instance).to receive(:execute) end - subject.execute + service_result end end end diff --git a/spec/services/work_items/delete_service_spec.rb b/spec/services/work_items/delete_service_spec.rb new file mode 100644 index 00000000000..6cca5018852 --- /dev/null +++ b/spec/services/work_items/delete_service_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::DeleteService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:guest) { create(:user) } + let_it_be(:work_item, refind: true) { create(:work_item, project: project, author: guest) } + + let(:user) { guest } + + before_all do + project.add_guest(guest) + # note necessary to test note removal as part of work item deletion + create(:note, project: project, noteable: work_item) + end + + describe '#execute' do + subject(:result) { described_class.new(project: project, current_user: user).execute(work_item) } + + context 'when user can delete the work item' do + it { is_expected.to be_success } + + # currently we don't expect destroy to fail. Mocking here for coverage and keeping + # the service's return type consistent + context 'when there are errors preventing to delete the work item' do + before do + allow(work_item).to receive(:destroy).and_return(false) + work_item.errors.add(:title) + end + + it { is_expected.to be_error } + + it 'returns error messages' do + expect(result.errors).to contain_exactly('Title is invalid') + end + end + end + + context 'when user cannot delete the work item' do + let(:user) { create(:user) } + + it { is_expected.to be_error } + + it 'returns error messages' do + expect(result.errors).to contain_exactly('User not authorized to delete work item') + end + end + end +end diff --git a/spec/services/work_items/update_service_spec.rb b/spec/services/work_items/update_service_spec.rb new file mode 100644 index 00000000000..f71f1060e40 --- /dev/null +++ b/spec/services/work_items/update_service_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::UpdateService do + let_it_be(:developer) { create(:user) } + let_it_be(:project) { create(:project).tap { |proj| proj.add_developer(developer) } } + let_it_be_with_reload(:work_item) { create(:work_item, project: project, assignees: [developer]) } + + let(:spam_params) { double } + let(:opts) { {} } + let(:current_user) { developer } + + describe '#execute' do + subject(:update_work_item) { described_class.new(project: project, current_user: current_user, params: opts, spam_params: spam_params).execute(work_item) } + + before do + stub_spam_services + end + + context 'when title is changed' do + let(:opts) { { title: 'changed' } } + + it 'triggers issuable_title_updated graphql subscription' do + expect(GraphqlTriggers).to receive(:issuable_title_updated).with(work_item).and_call_original + + update_work_item + end + end + + context 'when title is not changed' do + let(:opts) { { description: 'changed' } } + + it 'does not trigger issuable_title_updated graphql subscription' do + expect(GraphqlTriggers).not_to receive(:issuable_title_updated) + + update_work_item + end + end + + context 'when updating state_event' do + context 'when state_event is close' do + let(:opts) { { state_event: 'close' } } + + it 'closes the work item' do + expect do + update_work_item + work_item.reload + end.to change(work_item, :state).from('opened').to('closed') + end + end + + context 'when state_event is reopen' do + let(:opts) { { state_event: 'reopen' } } + + before do + work_item.close! + end + + it 'reopens the work item' do + expect do + update_work_item + work_item.reload + end.to change(work_item, :state).from('closed').to('opened') + end + end + end + end +end -- cgit v1.2.1