diff options
Diffstat (limited to 'spec/services')
112 files changed, 2646 insertions, 1788 deletions
diff --git a/spec/services/alert_management/alerts/update_service_spec.rb b/spec/services/alert_management/alerts/update_service_spec.rb index 9bdc9970807..8375c8cdf7d 100644 --- a/spec/services/alert_management/alerts/update_service_spec.rb +++ b/spec/services/alert_management/alerts/update_service_spec.rb @@ -249,57 +249,6 @@ RSpec.describe AlertManagement::Alerts::UpdateService do it_behaves_like 'adds a system note' end - - context 'with an associated issue' do - let_it_be(:issue, reload: true) { create(:issue, project: project) } - - before do - alert.update!(issue: issue) - end - - shared_examples 'does not sync with the incident status' do - specify do - expect(::Issues::UpdateService).not_to receive(:new) - expect { response }.to change { alert.acknowledged? }.to(true) - end - end - - it_behaves_like 'does not sync with the incident status' - - context 'when the issue is an incident' do - before do - issue.update!(issue_type: Issue.issue_types[:incident]) - end - - it_behaves_like 'does not sync with the incident status' - - context 'when the incident has an escalation status' do - let_it_be(:escalation_status, reload: true) { create(:incident_management_issuable_escalation_status, issue: issue) } - - it 'updates the incident escalation status with the new alert status' do - expect(::Issues::UpdateService).to receive(:new).once.and_call_original - expect(described_class).to receive(:new).once.and_call_original - - expect { response }.to change { escalation_status.reload.acknowledged? }.to(true) - .and change { alert.reload.acknowledged? }.to(true) - end - - context 'when the statuses match' do - before do - escalation_status.update!(status_event: :acknowledge) - end - - it_behaves_like 'does not sync with the incident status' - end - 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/ci/abort_pipelines_service_spec.rb b/spec/services/ci/abort_pipelines_service_spec.rb index 9f9519d6829..e43faf0af51 100644 --- a/spec/services/ci/abort_pipelines_service_spec.rb +++ b/spec/services/ci/abort_pipelines_service_spec.rb @@ -12,13 +12,13 @@ RSpec.describe Ci::AbortPipelinesService do let_it_be(:cancelable_build, reload: true) { create(:ci_build, :running, pipeline: cancelable_pipeline) } let_it_be(:non_cancelable_build, reload: true) { create(:ci_build, :success, pipeline: cancelable_pipeline) } - let_it_be(:cancelable_stage, reload: true) { create(:ci_stage_entity, name: 'stageA', status: :running, pipeline: cancelable_pipeline, project: project) } - let_it_be(:non_cancelable_stage, reload: true) { create(:ci_stage_entity, name: 'stageB', status: :success, pipeline: cancelable_pipeline, project: project) } + let_it_be(:cancelable_stage, reload: true) { create(:ci_stage, name: 'stageA', status: :running, pipeline: cancelable_pipeline, project: project) } + let_it_be(:non_cancelable_stage, reload: true) { create(:ci_stage, name: 'stageB', status: :success, pipeline: cancelable_pipeline, project: project) } let_it_be(:manual_pipeline_cancelable_build, reload: true) { create(:ci_build, :created, pipeline: manual_pipeline) } let_it_be(:manual_pipeline_non_cancelable_build, reload: true) { create(:ci_build, :manual, pipeline: manual_pipeline) } - let_it_be(:manual_pipeline_cancelable_stage, reload: true) { create(:ci_stage_entity, name: 'stageA', status: :created, pipeline: manual_pipeline, project: project) } - let_it_be(:manual_pipeline_non_cancelable_stage, reload: true) { create(:ci_stage_entity, name: 'stageB', status: :success, pipeline: manual_pipeline, project: project) } + let_it_be(:manual_pipeline_cancelable_stage, reload: true) { create(:ci_stage, name: 'stageA', status: :created, pipeline: manual_pipeline, project: project) } + let_it_be(:manual_pipeline_non_cancelable_stage, reload: true) { create(:ci_stage, name: 'stageB', status: :success, pipeline: manual_pipeline, project: project) } describe '#execute' do def expect_correct_pipeline_cancellations diff --git a/spec/services/ci/create_pipeline_service/include_spec.rb b/spec/services/ci/create_pipeline_service/include_spec.rb index 3116801d50c..849eb5885f6 100644 --- a/spec/services/ci/create_pipeline_service/include_spec.rb +++ b/spec/services/ci/create_pipeline_service/include_spec.rb @@ -126,5 +126,51 @@ RSpec.describe Ci::CreatePipelineService do it_behaves_like 'not including the file' end end + + context 'with ci_increase_includes_to_250 enabled on root project' do + let_it_be(:included_project) do + create(:project, :repository).tap { |p| p.add_developer(user) } + end + + before do + stub_const('::Gitlab::Ci::Config::External::Context::MAX_INCLUDES', 0) + stub_const('::Gitlab::Ci::Config::External::Context::TRIAL_MAX_INCLUDES', 3) + + stub_feature_flags(ci_increase_includes_to_250: false) + stub_feature_flags(ci_increase_includes_to_250: project) + + allow(Project) + .to receive(:find_by_full_path) + .with(included_project.full_path) + .and_return(included_project) + + allow(included_project.repository) + .to receive(:blob_data_at).with(included_project.commit.id, '.gitlab-ci.yml') + .and_return(local_config) + + allow(included_project.repository) + .to receive(:blob_data_at).with(included_project.commit.id, file_location) + .and_return(File.read(Rails.root.join(file_location))) + end + + let(:config) do + <<~EOY + include: + - project: #{included_project.full_path} + file: .gitlab-ci.yml + EOY + end + + let(:local_config) do + <<~EOY + include: #{file_location} + + job: + script: exit 0 + EOY + end + + it_behaves_like 'including the file' + end end end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index aac059f2104..9cef7f7dadb 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -2087,6 +2087,12 @@ RSpec.describe Ci::CreatePipelineService do rules: - changes: - $CI_JOB_NAME* + + changes-paths: + script: "I am using a new syntax!" + rules: + - changes: + paths: [README.md] EOY end @@ -2098,8 +2104,9 @@ RSpec.describe Ci::CreatePipelineService do it 'creates five jobs' do expect(pipeline).to be_persisted - expect(build_names) - .to contain_exactly('regular-job', 'rules-job', 'delayed-job', 'negligible-job', 'README') + expect(build_names).to contain_exactly( + 'regular-job', 'rules-job', 'delayed-job', 'negligible-job', 'README', 'changes-paths' + ) end it 'sets when: for all jobs' do diff --git a/spec/services/ci/ensure_stage_service_spec.rb b/spec/services/ci/ensure_stage_service_spec.rb index 3ede214cdd4..026814edda6 100644 --- a/spec/services/ci/ensure_stage_service_spec.rb +++ b/spec/services/ci/ensure_stage_service_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Ci::EnsureStageService, '#execute' do let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } - let(:stage) { create(:ci_stage_entity) } + let(:stage) { create(:ci_stage) } let(:job) { build(:ci_build) } let(:service) { described_class.new(project, user) } diff --git a/spec/services/ci/generate_coverage_reports_service_spec.rb b/spec/services/ci/generate_coverage_reports_service_spec.rb index d12a9268e7e..212e6be9d07 100644 --- a/spec/services/ci/generate_coverage_reports_service_spec.rb +++ b/spec/services/ci/generate_coverage_reports_service_spec.rb @@ -3,8 +3,9 @@ require 'spec_helper' RSpec.describe Ci::GenerateCoverageReportsService do + let_it_be(:project) { create(:project, :repository) } + let(:service) { described_class.new(project) } - let(:project) { create(:project, :repository) } describe '#execute' do subject { service.execute(base_pipeline, head_pipeline) } @@ -52,4 +53,41 @@ RSpec.describe Ci::GenerateCoverageReportsService do end end end + + describe '#latest?' do + subject { service.latest?(base_pipeline, head_pipeline, data) } + + let!(:base_pipeline) { nil } + let!(:head_pipeline) { create(:ci_pipeline, :with_coverage_reports, project: project) } + let!(:child_pipeline) { create(:ci_pipeline, child_of: head_pipeline) } + let!(:key) { service.send(:key, base_pipeline, head_pipeline) } + + let(:data) { { key: key } } + + context 'when cache key is latest' do + it { is_expected.to be_truthy } + end + + context 'when head pipeline has been updated' do + before do + head_pipeline.update_column(:updated_at, 1.minute.from_now) + end + + it { is_expected.to be_falsy } + end + + context 'when cache key is empty' do + let(:data) { { key: nil } } + + it { is_expected.to be_falsy } + end + + context 'when the pipeline has a child that is updated' do + before do + child_pipeline.update_column(:updated_at, 1.minute.from_now) + end + + it { is_expected.to be_falsy } + end + end end diff --git a/spec/services/ci/job_artifacts/create_service_spec.rb b/spec/services/ci/job_artifacts/create_service_spec.rb index 01f240805f5..b7a810ce47e 100644 --- a/spec/services/ci/job_artifacts/create_service_spec.rb +++ b/spec/services/ci/job_artifacts/create_service_spec.rb @@ -30,14 +30,6 @@ RSpec.describe Ci::JobArtifacts::CreateService do UploadedFile.new(upload.path, **params) end - def unique_metrics_report_uploaders - Gitlab::UsageDataCounters::HLLRedisCounter.unique_events( - event_names: described_class::METRICS_REPORT_UPLOAD_EVENT_NAME, - start_date: 2.weeks.ago, - end_date: 2.weeks.from_now - ) - end - describe '#execute' do subject { service.execute(artifacts_file, params, metadata_file: metadata_file) } @@ -61,12 +53,6 @@ RSpec.describe Ci::JobArtifacts::CreateService do expect(new_artifact.locked).to eq(job.pipeline.locked) end - it 'does not track the job user_id' do - subject - - expect(unique_metrics_report_uploaders).to eq(0) - end - context 'when metadata file is also uploaded' do let(:metadata_file) do file_to_upload('spec/fixtures/ci_build_artifacts_metadata.gz', sha256: artifacts_sha256) @@ -188,20 +174,6 @@ RSpec.describe Ci::JobArtifacts::CreateService do end end - context 'when artifact_type is metrics' do - before do - allow(job).to receive(:user_id).and_return(123) - end - - let(:params) { { 'artifact_type' => 'metrics', 'artifact_format' => 'gzip' }.with_indifferent_access } - - it 'tracks the job user_id' do - subject - - expect(unique_metrics_report_uploaders).to eq(1) - end - end - shared_examples 'rescues object storage error' do |klass, message, expected_message| it "handles #{klass}" do allow_next_instance_of(JobArtifactUploader) do |uploader| diff --git a/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb b/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb index 3a04a3af03e..05069054483 100644 --- a/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb +++ b/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb @@ -181,6 +181,26 @@ RSpec.describe Ci::JobArtifacts::DestroyBatchService do end end + context 'when artifact belongs to a project not undergoing refresh' do + context 'and skip_projects_on_refresh is set to false (default)' do + it 'does not log any warnings', :aggregate_failures do + expect(Gitlab::ProjectStatsRefreshConflictsLogger).not_to receive(:warn_artifact_deletion_during_stats_refresh) + + expect { subject }.to change { Ci::JobArtifact.count }.by(-2) + end + end + + context 'and skip_projects_on_refresh is set to true' do + let(:skip_projects_on_refresh) { true } + + it 'does not log any warnings', :aggregate_failures do + expect(Gitlab::ProjectStatsRefreshConflictsLogger).not_to receive(:warn_skipped_artifact_deletion_during_stats_refresh) + + expect { subject }.to change { Ci::JobArtifact.count }.by(-2) + end + end + end + context 'ProjectStatistics' do it 'resets project statistics' do expect(ProjectStatistics).to receive(:increment_statistic).once diff --git a/spec/services/ci/pipeline_artifacts/coverage_report_service_spec.rb b/spec/services/ci/pipeline_artifacts/coverage_report_service_spec.rb index 403afde5da3..31548793bac 100644 --- a/spec/services/ci/pipeline_artifacts/coverage_report_service_spec.rb +++ b/spec/services/ci/pipeline_artifacts/coverage_report_service_spec.rb @@ -2,16 +2,18 @@ require 'spec_helper' -RSpec.describe ::Ci::PipelineArtifacts::CoverageReportService do +RSpec.describe Ci::PipelineArtifacts::CoverageReportService do describe '#execute' do let_it_be(:project) { create(:project, :repository) } subject { described_class.new(pipeline).execute } - shared_examples 'creating a pipeline coverage report' do + shared_examples 'creating or updating a pipeline coverage report' do context 'when pipeline is finished' do - it 'creates a pipeline artifact' do - expect { subject }.to change { Ci::PipelineArtifact.count }.from(0).to(1) + it 'creates or updates a pipeline artifact' do + subject + + expect(pipeline.reload.pipeline_artifacts.count).to eq(1) end it 'persists the default file name' do @@ -22,7 +24,7 @@ RSpec.describe ::Ci::PipelineArtifacts::CoverageReportService do expect(file.filename).to eq('code_coverage.json') end - it 'sets expire_at to 1 week' do + it 'sets expire_at to 1 week from now' do freeze_time do subject @@ -31,13 +33,16 @@ RSpec.describe ::Ci::PipelineArtifacts::CoverageReportService do expect(pipeline_artifact.expire_at).to eq(1.week.from_now) end end - end - context 'when pipeline artifact has already been created' do - it 'does not raise an error and does not persist the same artifact twice' do - expect { 2.times { described_class.new(pipeline).execute } }.not_to raise_error + it 'logs relevant information' do + expect(Gitlab::AppLogger).to receive(:info).with({ + project_id: project.id, + pipeline_id: pipeline.id, + pipeline_artifact_id: kind_of(Numeric), + message: kind_of(String) + }) - expect(Ci::PipelineArtifact.count).to eq(1) + subject end end end @@ -45,21 +50,32 @@ RSpec.describe ::Ci::PipelineArtifacts::CoverageReportService do context 'when pipeline has coverage report' do let!(:pipeline) { create(:ci_pipeline, :with_coverage_reports, project: project) } - it_behaves_like 'creating a pipeline coverage report' + it_behaves_like 'creating or updating a pipeline coverage report' end context 'when pipeline has coverage report from child pipeline' do let!(:pipeline) { create(:ci_pipeline, :success, project: project) } let!(:child_pipeline) { create(:ci_pipeline, :with_coverage_reports, project: project, child_of: pipeline) } - it_behaves_like 'creating a pipeline coverage report' + it_behaves_like 'creating or updating a pipeline coverage report' + end + + context 'when pipeline has existing pipeline artifact for coverage report' do + let!(:pipeline) { create(:ci_pipeline, :with_coverage_reports, project: project) } + let!(:child_pipeline) { create(:ci_pipeline, :with_coverage_reports, project: project, child_of: pipeline) } + + let!(:pipeline_artifact) do + create(:ci_pipeline_artifact, :with_coverage_report, pipeline: pipeline, expire_at: 1.day.from_now) + end + + it_behaves_like 'creating or updating a pipeline coverage report' end context 'when pipeline is running and coverage report does not exist' do let(:pipeline) { create(:ci_pipeline, :running) } it 'does not persist data' do - expect { subject }.not_to change { Ci::PipelineArtifact.count } + expect { subject }.not_to change { Ci::PipelineArtifact.count }.from(0) end end end diff --git a/spec/services/ci/play_manual_stage_service_spec.rb b/spec/services/ci/play_manual_stage_service_spec.rb index 3e2a95ee975..b3ae92aa787 100644 --- a/spec/services/ci/play_manual_stage_service_spec.rb +++ b/spec/services/ci/play_manual_stage_service_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Ci::PlayManualStageService, '#execute' do let(:stage_status) { 'manual' } let(:stage) do - create(:ci_stage_entity, + create(:ci_stage, pipeline: pipeline, project: project, name: 'test') diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 74adbc4efc8..2316575f164 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -750,41 +750,7 @@ module Ci end context 'when using pending builds table' do - before do - stub_feature_flags(ci_pending_builds_queue_source: true) - end - - context 'with ci_queuing_use_denormalized_data_strategy enabled' do - before do - stub_feature_flags(ci_queuing_use_denormalized_data_strategy: true) - end - - include_examples 'handles runner assignment' - end - - 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 - - around do |example| - allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/332952') do - example.run - end - end - - include_examples 'handles runner assignment' - end - - context 'with ci_queuing_use_denormalized_data_strategy enabled' do - before do - stub_feature_flags(ci_queuing_use_denormalized_data_strategy: true) - end - - include_examples 'handles runner assignment' - end + include_examples 'handles runner assignment' context 'when a conflicting data is stored in denormalized table' do let!(:specific_runner) { create(:ci_runner, :project, projects: [project], tag_list: %w[conflict]) } @@ -805,22 +771,6 @@ module Ci end end end - - 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 - - around do |example| - allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/332952') do - example.run - end - end - - include_examples 'handles runner assignment' - end end describe '#register_success' do @@ -888,14 +838,6 @@ module Ci shared_examples 'metrics collector' do it_behaves_like 'attempt counter collector' it_behaves_like 'jobs queueing time histogram collector' - - context 'when using denormalized data is disabled' do - before do - stub_feature_flags(ci_pending_builds_maintain_denormalized_data: false) - end - - it_behaves_like 'jobs queueing time histogram collector' - end end context 'when shared runner is used' do diff --git a/spec/services/ci/retry_job_service_spec.rb b/spec/services/ci/retry_job_service_spec.rb index acc7a99637b..f042471bd1f 100644 --- a/spec/services/ci/retry_job_service_spec.rb +++ b/spec/services/ci/retry_job_service_spec.rb @@ -12,7 +12,7 @@ RSpec.describe Ci::RetryJobService do end let_it_be(:stage) do - create(:ci_stage_entity, project: project, + create(:ci_stage, project: project, pipeline: pipeline, name: 'test') end @@ -154,7 +154,7 @@ RSpec.describe Ci::RetryJobService do end context 'when the pipeline has other jobs' do - let!(:stage2) { create(:ci_stage_entity, project: project, pipeline: pipeline, name: 'deploy') } + let!(:stage2) { create(:ci_stage, project: project, pipeline: pipeline, name: 'deploy') } let!(:build2) { create(:ci_build, pipeline: pipeline, stage_id: stage.id ) } let!(:deploy) { create(:ci_build, pipeline: pipeline, stage_id: stage2.id) } let!(:deploy_needs_build2) { create(:ci_build_need, build: deploy, name: build2.name) } diff --git a/spec/services/ci/runners/reconcile_existing_runner_versions_service_spec.rb b/spec/services/ci/runners/reconcile_existing_runner_versions_service_spec.rb new file mode 100644 index 00000000000..f8313eaab90 --- /dev/null +++ b/spec/services/ci/runners/reconcile_existing_runner_versions_service_spec.rb @@ -0,0 +1,149 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::Runners::ReconcileExistingRunnerVersionsService, '#execute' do + subject(:execute) { described_class.new.execute } + + let_it_be(:runner_14_0_1) { create(:ci_runner, version: '14.0.1') } + let_it_be(:runner_version_14_0_1) do + create(:ci_runner_version, version: '14.0.1', status: :not_available) + end + + context 'with RunnerUpgradeCheck recommending 14.0.2' do + before do + stub_const('Ci::Runners::ReconcileExistingRunnerVersionsService::VERSION_BATCH_SIZE', 1) + + allow(::Gitlab::Ci::RunnerUpgradeCheck.instance) + .to receive(:check_runner_upgrade_status) + .and_return({ recommended: ::Gitlab::VersionInfo.new(14, 0, 2) }) + end + + context 'with runner with new version' do + let!(:runner_14_0_2) { create(:ci_runner, version: '14.0.2') } + let!(:runner_version_14_0_0) { create(:ci_runner_version, version: '14.0.0', status: :not_available) } + let!(:runner_14_0_0) { create(:ci_runner, version: '14.0.0') } + + before do + allow(::Gitlab::Ci::RunnerUpgradeCheck.instance) + .to receive(:check_runner_upgrade_status) + .with('14.0.2') + .and_return({ not_available: ::Gitlab::VersionInfo.new(14, 0, 2) }) + .once + end + + it 'creates and updates expected ci_runner_versions entries', :aggregate_failures do + expect(Ci::RunnerVersion).to receive(:insert_all) + .ordered + .with([{ version: '14.0.2' }], anything) + .once + .and_call_original + + result = nil + expect { result = execute } + .to change { runner_version_14_0_0.reload.status }.from('not_available').to('recommended') + .and change { runner_version_14_0_1.reload.status }.from('not_available').to('recommended') + .and change { ::Ci::RunnerVersion.find_by(version: '14.0.2')&.status }.from(nil).to('not_available') + + expect(result).to eq({ + status: :success, + total_inserted: 1, # 14.0.2 is inserted + total_updated: 3, # 14.0.0, 14.0.1 are updated, and newly inserted 14.0.2's status is calculated + total_deleted: 0 + }) + end + end + + context 'with orphan ci_runner_version' do + let!(:runner_version_14_0_2) { create(:ci_runner_version, version: '14.0.2', status: :not_available) } + + before do + allow(::Gitlab::Ci::RunnerUpgradeCheck.instance) + .to receive(:check_runner_upgrade_status) + .and_return({ not_available: ::Gitlab::VersionInfo.new(14, 0, 2) }) + end + + it 'deletes orphan ci_runner_versions entry', :aggregate_failures do + result = nil + expect { result = execute } + .to change { ::Ci::RunnerVersion.find_by_version('14.0.2')&.status }.from('not_available').to(nil) + .and not_change { runner_version_14_0_1.reload.status }.from('not_available') + + expect(result).to eq({ + status: :success, + total_inserted: 0, + total_updated: 0, + total_deleted: 1 # 14.0.2 is deleted + }) + end + end + + context 'with no runner version changes' do + before do + allow(::Gitlab::Ci::RunnerUpgradeCheck.instance) + .to receive(:check_runner_upgrade_status) + .and_return({ not_available: ::Gitlab::VersionInfo.new(14, 0, 1) }) + end + + it 'does not modify ci_runner_versions entries', :aggregate_failures do + result = nil + expect { result = execute }.not_to change { runner_version_14_0_1.reload.status }.from('not_available') + + expect(result).to eq({ + status: :success, + total_inserted: 0, + total_updated: 0, + total_deleted: 0 + }) + end + end + + context 'with failing version check' do + before do + allow(::Gitlab::Ci::RunnerUpgradeCheck.instance) + .to receive(:check_runner_upgrade_status) + .and_return({ error: ::Gitlab::VersionInfo.new(14, 0, 1) }) + end + + it 'makes no changes to ci_runner_versions', :aggregate_failures do + result = nil + expect { result = execute }.not_to change { runner_version_14_0_1.reload.status }.from('not_available') + + expect(result).to eq({ + status: :success, + total_inserted: 0, + total_updated: 0, + total_deleted: 0 + }) + end + end + end + + context 'integration testing with Gitlab::Ci::RunnerUpgradeCheck' do + let(:available_runner_releases) do + %w[14.0.0 14.0.1] + end + + before do + url = ::Gitlab::CurrentSettings.current_application_settings.public_runner_releases_url + + WebMock.stub_request(:get, url).to_return( + body: available_runner_releases.map { |v| { name: v } }.to_json, + status: 200, + headers: { 'Content-Type' => 'application/json' } + ) + end + + it 'does not modify ci_runner_versions entries', :aggregate_failures do + result = nil + expect { result = execute }.not_to change { runner_version_14_0_1.reload.status }.from('not_available') + + expect(result).to eq({ + status: :success, + total_inserted: 0, + total_updated: 0, + total_deleted: 0 + }) + end + end +end diff --git a/spec/services/ci/runners/register_runner_service_spec.rb b/spec/services/ci/runners/register_runner_service_spec.rb index f43fd823078..03dcf851e53 100644 --- a/spec/services/ci/runners/register_runner_service_spec.rb +++ b/spec/services/ci/runners/register_runner_service_spec.rb @@ -13,7 +13,7 @@ RSpec.describe ::Ci::Runners::RegisterRunnerService, '#execute' do stub_application_setting(valid_runner_registrars: ApplicationSetting::VALID_RUNNER_REGISTRAR_TYPES) end - subject { described_class.new.execute(token, args) } + subject(:runner) { described_class.new.execute(token, args) } context 'when no token is provided' do let(:token) { '' } @@ -83,6 +83,9 @@ RSpec.describe ::Ci::Runners::RegisterRunnerService, '#execute' do expect(subject.platform).to eq args[:platform] expect(subject.architecture).to eq args[:architecture] expect(subject.ip_address).to eq args[:ip_address] + + expect(Ci::Runner.tagged_with('tag1')).to include(subject) + expect(Ci::Runner.tagged_with('tag2')).to include(subject) end end @@ -230,5 +233,41 @@ RSpec.describe ::Ci::Runners::RegisterRunnerService, '#execute' do end end end + + context 'when tags are provided' do + let(:token) { registration_token } + + let(:args) do + { tag_list: %w(tag1 tag2) } + end + + it 'creates runner with tags' do + expect(runner).to be_persisted + + expect(runner.tags).to contain_exactly( + an_object_having_attributes(name: 'tag1'), + an_object_having_attributes(name: 'tag2') + ) + end + + it 'creates tags in bulk' do + expect(Gitlab::Ci::Tags::BulkInsert).to receive(:bulk_insert_tags!).and_call_original + + expect(runner).to be_persisted + end + + context 'and tag list exceeds limit' do + let(:args) do + { tag_list: (1..Ci::Runner::TAG_LIST_MAX_LENGTH + 1).map { |i| "tag#{i}" } } + end + + it 'does not create any tags' do + expect(Gitlab::Ci::Tags::BulkInsert).not_to receive(:bulk_insert_tags!) + + expect(runner).not_to be_persisted + expect(runner.tags).to be_empty + end + end + end end end diff --git a/spec/services/ci/unlock_artifacts_service_spec.rb b/spec/services/ci/unlock_artifacts_service_spec.rb index 8ee07fc44c8..94d39fc9f14 100644 --- a/spec/services/ci/unlock_artifacts_service_spec.rb +++ b/spec/services/ci/unlock_artifacts_service_spec.rb @@ -130,7 +130,7 @@ RSpec.describe Ci::UnlockArtifactsService do WHERE "ci_pipelines"."ci_ref_id" = #{ci_ref.id} AND "ci_pipelines"."locked" = 1 - AND (ci_pipelines.id < #{before_pipeline.id}) + AND "ci_pipelines"."id" < #{before_pipeline.id} AND "ci_pipelines"."id" NOT IN (WITH RECURSIVE "base_and_descendants" diff --git a/spec/services/ci/update_pending_build_service_spec.rb b/spec/services/ci/update_pending_build_service_spec.rb index 2bb0aded24a..e49b22299f0 100644 --- a/spec/services/ci/update_pending_build_service_spec.rb +++ b/spec/services/ci/update_pending_build_service_spec.rb @@ -42,19 +42,6 @@ RSpec.describe Ci::UpdatePendingBuildService do expect(pending_build_1.instance_runners_enabled).to be_truthy expect(pending_build_2.instance_runners_enabled).to be_truthy end - - context 'when ci_pending_builds_maintain_denormalized_data is disabled' do - before do - stub_feature_flags(ci_pending_builds_maintain_denormalized_data: false) - end - - it 'does not update all pending builds', :aggregate_failures do - update_pending_builds - - expect(pending_build_1.instance_runners_enabled).to be_falsey - expect(pending_build_2.instance_runners_enabled).to be_truthy - end - end end context 'when model is a project with pending builds' do @@ -66,19 +53,6 @@ RSpec.describe Ci::UpdatePendingBuildService do expect(pending_build_1.instance_runners_enabled).to be_truthy expect(pending_build_2.instance_runners_enabled).to be_truthy end - - context 'when ci_pending_builds_maintain_denormalized_data is disabled' do - before do - stub_feature_flags(ci_pending_builds_maintain_denormalized_data: false) - end - - it 'does not update all pending builds', :aggregate_failures do - update_pending_builds - - expect(pending_build_1.instance_runners_enabled).to be_falsey - expect(pending_build_2.instance_runners_enabled).to be_truthy - end - end end end end diff --git a/spec/services/clusters/applications/create_service_spec.rb b/spec/services/clusters/applications/create_service_spec.rb index eb907377ca8..00a67a9b2ef 100644 --- a/spec/services/clusters/applications/create_service_spec.rb +++ b/spec/services/clusters/applications/create_service_spec.rb @@ -168,29 +168,6 @@ RSpec.describe Clusters::Applications::CreateService do subject end end - - context 'elastic stack application' do - let(:params) do - { - application: 'elastic_stack' - } - end - - before do - create(:clusters_applications_ingress, :installed, external_ip: "127.0.0.0", cluster: cluster) - expect_any_instance_of(Clusters::Applications::ElasticStack) - .to receive(:make_scheduled!) - .and_call_original - end - - it 'creates the application' do - expect do - subject - - cluster.reload - end.to change(cluster, :application_elastic_stack) - end - end end context 'invalid application' do diff --git a/spec/services/clusters/integrations/create_service_spec.rb b/spec/services/clusters/integrations/create_service_spec.rb index 6dac97ebf8f..016511a3c01 100644 --- a/spec/services/clusters/integrations/create_service_spec.rb +++ b/spec/services/clusters/integrations/create_service_spec.rb @@ -61,7 +61,6 @@ RSpec.describe Clusters::Integrations::CreateService, '#execute' do end it_behaves_like 'a cluster integration', 'prometheus' - it_behaves_like 'a cluster integration', 'elastic_stack' context 'when application_type is invalid' do let(:params) do diff --git a/spec/services/deployments/create_for_build_service_spec.rb b/spec/services/deployments/create_for_build_service_spec.rb index 6fc7c9e56a6..38d94580512 100644 --- a/spec/services/deployments/create_for_build_service_spec.rb +++ b/spec/services/deployments/create_for_build_service_spec.rb @@ -25,6 +25,7 @@ RSpec.describe Deployments::CreateForBuildService do expect(build.deployment.deployable).to eq(build) expect(build.deployment.deployable_type).to eq('CommitStatus') expect(build.deployment.environment).to eq(build.persisted_environment) + expect(build.deployment.valid?).to be_truthy end context 'when creation failure occures' do diff --git a/spec/services/deployments/create_service_spec.rb b/spec/services/deployments/create_service_spec.rb index f6f4c68a6f1..0f2a6ce32e1 100644 --- a/spec/services/deployments/create_service_spec.rb +++ b/spec/services/deployments/create_service_spec.rb @@ -21,34 +21,11 @@ RSpec.describe Deployments::CreateService do expect(Deployments::UpdateEnvironmentWorker).to receive(:perform_async) expect(Deployments::LinkMergeRequestWorker).to receive(:perform_async) - expect_next_instance_of(Deployment) do |deployment| - expect(deployment).to receive(:execute_hooks) - end + expect(Deployments::HooksWorker).to receive(:perform_async) expect(service.execute).to be_persisted end - context 'when `deployment_hooks_skip_worker` flag is disabled' do - before do - stub_feature_flags(deployment_hooks_skip_worker: false) - end - - it 'executes Deployments::HooksWorker asynchronously' do - service = described_class.new( - environment, - user, - sha: 'b83d6e391c22777fca1ed3012fce84f633d7fed0', - ref: 'master', - tag: false, - status: 'success' - ) - - expect(Deployments::HooksWorker).to receive(:perform_async) - - service.execute - end - end - it 'does not change the status if no status is given' do service = described_class.new( environment, @@ -60,9 +37,7 @@ RSpec.describe Deployments::CreateService do expect(Deployments::UpdateEnvironmentWorker).not_to receive(:perform_async) expect(Deployments::LinkMergeRequestWorker).not_to receive(:perform_async) - expect_next_instance_of(Deployment) do |deployment| - expect(deployment).not_to receive(:execute_hooks) - end + expect(Deployments::HooksWorker).not_to receive(:perform_async) expect(service.execute).to be_persisted end @@ -80,9 +55,11 @@ RSpec.describe Deployments::CreateService do it 'does not create a new deployment' do described_class.new(environment, user, params).execute - expect do - described_class.new(environment.reload, user, params).execute - end.not_to change { Deployment.count } + expect(Deployments::UpdateEnvironmentWorker).not_to receive(:perform_async) + expect(Deployments::LinkMergeRequestWorker).not_to receive(:perform_async) + expect(Deployments::HooksWorker).not_to receive(:perform_async) + + described_class.new(environment.reload, user, params).execute end end end diff --git a/spec/services/deployments/update_environment_service_spec.rb b/spec/services/deployments/update_environment_service_spec.rb index e2d7a80fde3..8ab53a37a33 100644 --- a/spec/services/deployments/update_environment_service_spec.rb +++ b/spec/services/deployments/update_environment_service_spec.rb @@ -33,7 +33,7 @@ RSpec.describe Deployments::UpdateEnvironmentService do before do allow(Deployments::LinkMergeRequestWorker).to receive(:perform_async) - allow(deployment).to receive(:execute_hooks) + allow(Deployments::HooksWorker).to receive(:perform_async) job.success! # Create/Succeed deployment end diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb index 51ef30c91c0..81443eed7d3 100644 --- a/spec/services/draft_notes/publish_service_spec.rb +++ b/spec/services/draft_notes/publish_service_spec.rb @@ -168,7 +168,7 @@ RSpec.describe DraftNotes::PublishService do # NOTE: This should be reduced as we work on reducing Gitaly calls. # Gitaly requests shouldn't go above this threshold as much as possible # as it may add more to the Gitaly N+1 issue we are experiencing. - expect { publish }.to change { Gitlab::GitalyClient.get_request_count }.by(21) + expect { publish }.to change { Gitlab::GitalyClient.get_request_count }.by(20) end end diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index 56da85cc4a0..e66b413a5c9 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -379,10 +379,14 @@ RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redi end end - describe 'design events' do + describe 'design events', :snowplow do let_it_be(:design) { create(:design, project: project) } let_it_be(:author) { user } + before do + allow(Gitlab::Tracking).to receive(:event) # rubocop:disable RSpec/ExpectGitlabTracking + end + describe '#save_designs' do let_it_be(:updated) { create_list(:design, 5) } let_it_be(:created) { create_list(:design, 3) } @@ -411,6 +415,44 @@ RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redi it_behaves_like "it records the event in the event counter" do let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::DESIGN_ACTION } end + + it 'records correct create payload with Snowplow event' do + service.save_designs(author, create: [design]) + + expect_snowplow_event( + category: Gitlab::UsageDataCounters::TrackUniqueEvents::DESIGN_ACTION.to_s, + action: 'create', + namespace: design.project.namespace, + user: author, + project: design.project, + label: 'design_users' + ) + end + + it 'records correct update payload with Snowplow event' do + service.save_designs(author, update: [design]) + + expect_snowplow_event( + category: Gitlab::UsageDataCounters::TrackUniqueEvents::DESIGN_ACTION.to_s, + action: 'update', + namespace: design.project.namespace, + user: author, + project: design.project, + label: 'design_users' + ) + end + + context 'when FF is disabled' do + before do + stub_feature_flags(route_hll_to_snowplow_phase2: false) + end + + it 'doesnt emit snowwplow events', :snowplow do + subject + + expect_no_snowplow_event + end + end end describe '#destroy_designs' do @@ -434,6 +476,31 @@ RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redi it_behaves_like "it records the event in the event counter" do let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::DESIGN_ACTION } end + + it 'records correct payload with Snowplow event' do + service.destroy_designs([design], author) + + expect_snowplow_event( + category: Gitlab::UsageDataCounters::TrackUniqueEvents::DESIGN_ACTION.to_s, + action: 'destroy', + namespace: design.project.namespace, + user: author, + project: design.project, + label: 'design_users' + ) + end + + context 'when FF is disabled' do + before do + stub_feature_flags(route_hll_to_snowplow_phase2: false) + end + + it 'doesnt emit snowwplow events', :snowplow do + subject + + expect_no_snowplow_event + end + end end end diff --git a/spec/services/feature_flags/create_service_spec.rb b/spec/services/feature_flags/create_service_spec.rb index e37d41562f9..1c9bde70af3 100644 --- a/spec/services/feature_flags/create_service_spec.rb +++ b/spec/services/feature_flags/create_service_spec.rb @@ -41,6 +41,8 @@ RSpec.describe FeatureFlags::CreateService do subject end + + it_behaves_like 'does not update feature flag client' end context 'when feature flag is saved correctly' do @@ -62,6 +64,8 @@ RSpec.describe FeatureFlags::CreateService do expect { subject }.to change { Operations::FeatureFlag.count }.by(1) end + it_behaves_like 'update feature flag client' + context 'when Jira Connect subscription does not exist' do it 'does not sync the feature flag to Jira' do expect(::JiraConnect::SyncFeatureFlagsWorker).not_to receive(:perform_async) diff --git a/spec/services/feature_flags/destroy_service_spec.rb b/spec/services/feature_flags/destroy_service_spec.rb index d3796ef6b4d..740923db9b6 100644 --- a/spec/services/feature_flags/destroy_service_spec.rb +++ b/spec/services/feature_flags/destroy_service_spec.rb @@ -36,6 +36,8 @@ RSpec.describe FeatureFlags::DestroyService do expect(audit_event_message).to eq("Deleted feature flag #{feature_flag.name}.") end + it_behaves_like 'update feature flag client' + context 'when user is reporter' do let(:user) { reporter } @@ -57,6 +59,8 @@ RSpec.describe FeatureFlags::DestroyService do it 'does not create audit log' do expect { subject }.not_to change { AuditEvent.count } end + + it_behaves_like 'does not update feature flag client' end end end diff --git a/spec/services/feature_flags/update_service_spec.rb b/spec/services/feature_flags/update_service_spec.rb index f5e94c4af0f..8f985d34961 100644 --- a/spec/services/feature_flags/update_service_spec.rb +++ b/spec/services/feature_flags/update_service_spec.rb @@ -58,6 +58,8 @@ RSpec.describe FeatureFlags::UpdateService do ) end + it_behaves_like 'update feature flag client' + context 'with invalid params' do let(:params) { { name: nil } } @@ -79,6 +81,8 @@ RSpec.describe FeatureFlags::UpdateService do subject end + + it_behaves_like 'does not update feature flag client' end context 'when user is reporter' do diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb index 79c2cb1fca3..5de1c0e27be 100644 --- a/spec/services/git/branch_hooks_service_spec.rb +++ b/spec/services/git/branch_hooks_service_spec.rb @@ -387,6 +387,27 @@ RSpec.describe Git::BranchHooksService, :clean_gitlab_redis_shared_state do expect(commits_count).to eq(project.repository.commit_count_for_ref(newrev)) end + + it 'collects the related metrics' do + expect(Gitlab::Metrics).to receive(:add_event).with(:push_commit, { branch: 'master' }) + expect(Gitlab::Metrics).to receive(:add_event).with(:push_branch, {}) + expect(Gitlab::Metrics).to receive(:add_event).with(:change_default_branch, {}) + expect(Gitlab::Metrics).to receive(:add_event).with(:process_commit_limit_overflow) + + service.execute + end + + context 'when limit is not hit' do + before do + stub_const("::Git::BaseHooksService::PROCESS_COMMIT_LIMIT", 100) + end + + it 'does not collect the corresponding metric' do + expect(Gitlab::Metrics).not_to receive(:add_event).with(:process_commit_limit_overflow) + + service.execute + end + end end context 'updating the default branch' do diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index befa9598964..8d41b20c8a9 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -19,11 +19,13 @@ RSpec.describe Git::BranchPushService, services: true do project.add_maintainer(user) end - describe 'Push branches' do - subject do - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref, push_options: push_options) - end + subject(:execute_service) do + described_class + .new(project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref }, push_options: push_options) + .execute + end + describe 'Push branches' do context 'new branch' do let(:oldrev) { blankrev } @@ -72,8 +74,6 @@ RSpec.describe Git::BranchPushService, services: true do end describe "Pipelines" do - subject { execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) } - before do stub_ci_pipeline_to_return_yaml_file end @@ -117,7 +117,7 @@ RSpec.describe Git::BranchPushService, services: true do end context 'with push options' do - let(:push_options) { ['mr.create'] } + let(:push_options) { { 'mr' => { 'create' => true } } } it 'sanitizes push options' do allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(true) @@ -148,27 +148,34 @@ RSpec.describe Git::BranchPushService, services: true do end describe "Updates merge requests" do + let(:oldrev) { blankrev } + it "when pushing a new branch for the first time" do expect(UpdateMergeRequestsWorker) .to receive(:perform_async) - .with(project.id, user.id, blankrev, newrev, ref) + .with(project.id, user.id, blankrev, newrev, ref, { 'push_options' => nil }) + .ordered - execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) + subject end end describe "Updates git attributes" do context "for default branch" do - it "calls the copy attributes method for the first push to the default branch" do - expect(project.repository).to receive(:copy_gitattributes).with('master') + context "when first push" do + let(:oldrev) { blankrev } + + it "calls the copy attributes method for the first push to the default branch" do + expect(project.repository).to receive(:copy_gitattributes).with('master') - execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) + subject + end end it "calls the copy attributes method for changes to the default branch" do expect(project.repository).to receive(:copy_gitattributes).with(ref) - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end end @@ -181,49 +188,53 @@ RSpec.describe Git::BranchPushService, services: true do it "does not call copy attributes method" do expect(project.repository).not_to receive(:copy_gitattributes) - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end end end describe "Webhooks" do - context "execute webhooks" do - before do - create(:project_hook, push_events: true, project: project) - end + before do + create(:project_hook, push_events: true, project: project) + end - it "when pushing a branch for the first time" do + context "when pushing a branch for the first time" do + let(:oldrev) { blankrev } + + it "executes webhooks" do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) + + subject + expect(project.protected_branches).not_to be_empty expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER]) expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER]) end - it "when pushing a branch for the first time with default branch protection disabled" do + it "with default branch protection disabled" do expect(project.namespace).to receive(:default_branch_protection).and_return(Gitlab::Access::PROTECTION_NONE) expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) + subject expect(project.protected_branches).to be_empty end - it "when pushing a branch for the first time with default branch protection set to 'developers can push'" do + it "with default branch protection set to 'developers can push'" do expect(project.namespace).to receive(:default_branch_protection).and_return(Gitlab::Access::PROTECTION_DEV_CAN_PUSH) expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) + subject expect(project.protected_branches).not_to be_empty expect(project.protected_branches.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER]) expect(project.protected_branches.last.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER]) end - it "when pushing a branch for the first time with an existing branch permission configured" do + it "with an existing branch permission configured" do expect(project.namespace).to receive(:default_branch_protection).and_return(Gitlab::Access::PROTECTION_DEV_CAN_PUSH) create(:protected_branch, :no_one_can_push, :developers_can_merge, project: project, name: 'master') @@ -231,27 +242,29 @@ RSpec.describe Git::BranchPushService, services: true do expect(project.default_branch).to eq("master") expect(ProtectedBranches::CreateService).not_to receive(:new) - execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) + subject expect(project.protected_branches).not_to be_empty expect(project.protected_branches.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::NO_ACCESS]) expect(project.protected_branches.last.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER]) end - it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do + it "with default branch protection set to 'developers can merge'" do expect(project.namespace).to receive(:default_branch_protection).and_return(Gitlab::Access::PROTECTION_DEV_CAN_MERGE) expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) + subject expect(project.protected_branches).not_to be_empty expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER]) expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER]) end + end - it "when pushing new commits to existing branch" do + context "when pushing new commits to existing branch" do + it "executes webhooks" do expect(project).to receive(:execute_hooks) - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end end end @@ -281,7 +294,7 @@ RSpec.describe Git::BranchPushService, services: true do it "creates a note if a pushed commit mentions an issue", :sidekiq_might_not_need_inline do expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end it "only creates a cross-reference note if one doesn't already exist" do @@ -289,7 +302,7 @@ RSpec.describe Git::BranchPushService, services: true do expect(SystemNoteService).not_to receive(:cross_reference).with(issue, commit, commit_author) - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end it "defaults to the pushing user if the commit's author is not known", :sidekiq_inline, :use_clean_rails_redis_caching do @@ -299,16 +312,21 @@ RSpec.describe Git::BranchPushService, services: true do ) expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, user) - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end - it "finds references in the first push to a non-default branch", :sidekiq_might_not_need_inline do - allow(project.repository).to receive(:commits_between).with(blankrev, newrev).and_return([]) - allow(project.repository).to receive(:commits_between).with("master", newrev).and_return([commit]) + context "when first push on a non-default branch" do + let(:oldrev) { blankrev } + let(:ref) { 'refs/heads/other' } - expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) + it "finds references", :sidekiq_might_not_need_inline do + allow(project.repository).to receive(:commits_between).with(blankrev, newrev).and_return([]) + allow(project.repository).to receive(:commits_between).with("master", newrev).and_return([commit]) - execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: 'refs/heads/other') + expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) + + subject + end end end @@ -338,14 +356,14 @@ RSpec.describe Git::BranchPushService, services: true do context "while saving the 'first_mentioned_in_commit_at' metric for an issue" do it 'sets the metric for referenced issues', :sidekiq_inline, :use_clean_rails_redis_caching do - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject expect(issue.reload.metrics.first_mentioned_in_commit_at).to be_like_time(commit_time) end it 'does not set the metric for non-referenced issues' do non_referenced_issue = create(:issue, project: project) - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject expect(non_referenced_issue.reload.metrics.first_mentioned_in_commit_at).to be_nil end @@ -376,19 +394,21 @@ RSpec.describe Git::BranchPushService, services: true do end context "to default branches" do + let(:user) { commit_author } + it "closes issues", :sidekiq_might_not_need_inline do - execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) + subject expect(Issue.find(issue.id)).to be_closed end it "adds a note indicating that the issue is now closed", :sidekiq_might_not_need_inline do expect(SystemNoteService).to receive(:change_status).with(issue, project, commit_author, "closed", closing_commit) - execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) + subject end it "doesn't create additional cross-reference notes" do expect(SystemNoteService).not_to receive(:cross_reference) - execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) + subject end end @@ -400,11 +420,11 @@ RSpec.describe Git::BranchPushService, services: true do it "creates cross-reference notes", :sidekiq_inline, :use_clean_rails_redis_caching do expect(SystemNoteService).to receive(:cross_reference).with(issue, closing_commit, commit_author) - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end it "doesn't close issues" do - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject expect(Issue.find(issue.id)).to be_opened end end @@ -441,7 +461,7 @@ RSpec.describe Git::BranchPushService, services: true do let(:message) { "this is some work.\n\nrelated to JIRA-1" } it "initiates one api call to jira server to mention the issue", :sidekiq_inline, :use_clean_rails_redis_caching do - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject expect(WebMock).to have_requested(:post, jira_api_comment_url('JIRA-1')).with( body: /mentioned this issue in/ @@ -468,37 +488,43 @@ RSpec.describe Git::BranchPushService, services: true do end context "using right markdown", :sidekiq_might_not_need_inline do + let(:user) { commit_author } + it "initiates one api call to jira server to close the issue" do - execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) + subject expect(WebMock).to have_requested(:post, jira_api_transition_url('JIRA-1')).once end it "initiates one api call to jira server to comment on the issue" do - execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) + subject - expect(WebMock).to have_requested(:post, jira_api_comment_url('JIRA-1')).with( - body: comment_body - ).once + expect(WebMock) + .to have_requested(:post, jira_api_comment_url('JIRA-1')) + .with(body: comment_body) + .once end end context "using internal issue reference" do + let(:user) { commit_author } + context 'when internal issues are disabled' do before do project.issues_enabled = false project.save! end + let(:message) { "this is some work.\n\ncloses #1" } it "does not initiates one api call to jira server to close the issue" do - execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) + subject expect(WebMock).not_to have_requested(:post, jira_api_transition_url('JIRA-1')) end it "does not initiates one api call to jira server to comment on the issue" do - execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) + subject expect(WebMock).not_to have_requested(:post, jira_api_comment_url('JIRA-1')).with( body: comment_body @@ -511,13 +537,13 @@ RSpec.describe Git::BranchPushService, services: true do let(:message) { "this is some work.\n\ncloses JIRA-1 \n\n closes #{issue.to_reference}" } it "initiates one api call to jira server to close the jira issue" do - execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) + subject expect(WebMock).to have_requested(:post, jira_api_transition_url('JIRA-1')).once end it "initiates one api call to jira server to comment on the jira issue" do - execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) + subject expect(WebMock).to have_requested(:post, jira_api_comment_url('JIRA-1')).with( body: comment_body @@ -525,14 +551,14 @@ RSpec.describe Git::BranchPushService, services: true do end it "closes the internal issue" do - execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) + subject expect(issue.reload).to be_closed end it "adds a note indicating that the issue is now closed" do expect(SystemNoteService).to receive(:change_status) .with(issue, project, commit_author, "closed", closing_commit) - execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) + subject end end end @@ -542,7 +568,8 @@ RSpec.describe Git::BranchPushService, services: true do describe "empty project" do let(:project) { create(:project_empty_repo) } - let(:new_ref) { 'refs/heads/feature' } + let(:ref) { 'refs/heads/feature' } + let(:oldrev) { blankrev } before do allow(project).to receive(:default_branch).and_return('feature') @@ -550,7 +577,7 @@ RSpec.describe Git::BranchPushService, services: true do end it 'push to first branch updates HEAD' do - execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: new_ref) + subject end end @@ -561,7 +588,7 @@ RSpec.describe Git::BranchPushService, services: true do it 'does nothing' do expect(::Environments::StopService).not_to receive(:new) - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end end @@ -569,7 +596,7 @@ RSpec.describe Git::BranchPushService, services: true do it 'does nothing' do expect(::Environments::StopService).not_to receive(:new) - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end end @@ -583,7 +610,7 @@ RSpec.describe Git::BranchPushService, services: true do expect(stop_service).to receive(:execute_for_branch).with(branch) end - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end end end @@ -595,7 +622,7 @@ RSpec.describe Git::BranchPushService, services: true do it 'does nothing' do expect(::Ci::RefDeleteUnlockArtifactsWorker).not_to receive(:perform_async) - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end end @@ -603,7 +630,7 @@ RSpec.describe Git::BranchPushService, services: true do it 'does nothing' do expect(::Ci::RefDeleteUnlockArtifactsWorker).not_to receive(:perform_async) - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end end @@ -614,7 +641,7 @@ RSpec.describe Git::BranchPushService, services: true do expect(::Ci::RefDeleteUnlockArtifactsWorker) .to receive(:perform_async).with(project.id, user.id, "refs/heads/#{branch}") - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end end end @@ -636,7 +663,7 @@ RSpec.describe Git::BranchPushService, services: true do expect(hooks_service).to receive(:execute) end - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end end @@ -646,38 +673,24 @@ RSpec.describe Git::BranchPushService, services: true do it 'does nothing' do expect(::Git::BranchHooksService).not_to receive(:new) - execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + subject end end end - def execute_service(project, user, change, push_options = {}) - service = described_class.new(project, user, change: change, push_options: push_options) - service.execute - service - end - context 'Jira Connect hooks' do - let_it_be(:project) { create(:project, :repository) } - let(:branch_to_sync) { nil } let(:commits_to_sync) { [] } - let(:params) do - { change: { oldrev: oldrev, newrev: newrev, ref: ref } } - end - - subject do - described_class.new(project, user, params) - end shared_examples 'enqueues Jira sync worker' do specify :aggregate_failures do Sidekiq::Testing.fake! do - expect(JiraConnect::SyncBranchWorker).to receive(:perform_async) - .with(project.id, branch_to_sync, commits_to_sync, kind_of(Numeric)) - .and_call_original + expect(JiraConnect::SyncBranchWorker) + .to receive(:perform_async) + .with(project.id, branch_to_sync, commits_to_sync, kind_of(Numeric)) + .and_call_original - expect { subject.execute }.to change(JiraConnect::SyncBranchWorker.jobs, :size).by(1) + expect { subject }.to change(JiraConnect::SyncBranchWorker.jobs, :size).by(1) end end end @@ -685,7 +698,7 @@ RSpec.describe Git::BranchPushService, services: true do shared_examples 'does not enqueue Jira sync worker' do specify do Sidekiq::Testing.fake! do - expect { subject.execute }.not_to change(JiraConnect::SyncBranchWorker.jobs, :size) + expect { subject }.not_to change(JiraConnect::SyncBranchWorker.jobs, :size) end end end @@ -723,12 +736,12 @@ RSpec.describe Git::BranchPushService, services: true do end describe 'project target platforms detection' do - subject(:execute) { execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) } + let(:oldrev) { blankrev } it 'calls enqueue_record_project_target_platforms on the project' do expect(project).to receive(:enqueue_record_project_target_platforms) - execute + subject end end end diff --git a/spec/services/git/process_ref_changes_service_spec.rb b/spec/services/git/process_ref_changes_service_spec.rb index 05c1f898cab..8d2da4a899e 100644 --- a/spec/services/git/process_ref_changes_service_spec.rb +++ b/spec/services/git/process_ref_changes_service_spec.rb @@ -243,14 +243,37 @@ RSpec.describe Git::ProcessRefChangesService do end it 'schedules job for existing merge requests' do - expect(UpdateMergeRequestsWorker).to receive(:perform_async) - .with(project.id, user.id, Gitlab::Git::BLANK_SHA, '789012', "#{ref_prefix}/create1").ordered - expect(UpdateMergeRequestsWorker).to receive(:perform_async) - .with(project.id, user.id, Gitlab::Git::BLANK_SHA, '789013', "#{ref_prefix}/create2").ordered - expect(UpdateMergeRequestsWorker).to receive(:perform_async) - .with(project.id, user.id, '789015', '789016', "#{ref_prefix}/changed1").ordered - expect(UpdateMergeRequestsWorker).to receive(:perform_async) - .with(project.id, user.id, '789020', Gitlab::Git::BLANK_SHA, "#{ref_prefix}/removed2").ordered + expect(UpdateMergeRequestsWorker).to receive(:perform_async).with( + project.id, + user.id, + Gitlab::Git::BLANK_SHA, + '789012', + "#{ref_prefix}/create1", + { 'push_options' => nil }).ordered + + expect(UpdateMergeRequestsWorker).to receive(:perform_async).with( + project.id, + user.id, + Gitlab::Git::BLANK_SHA, + '789013', + "#{ref_prefix}/create2", + { 'push_options' => nil }).ordered + + expect(UpdateMergeRequestsWorker).to receive(:perform_async).with( + project.id, + user.id, + '789015', + '789016', + "#{ref_prefix}/changed1", + { 'push_options' => nil }).ordered + + expect(UpdateMergeRequestsWorker).to receive(:perform_async).with( + project.id, + user.id, + '789020', + Gitlab::Git::BLANK_SHA, + "#{ref_prefix}/removed2", + { 'push_options' => nil }).ordered subject.execute end diff --git a/spec/services/git/tag_hooks_service_spec.rb b/spec/services/git/tag_hooks_service_spec.rb index dae2f63f2f9..2d50c64d63c 100644 --- a/spec/services/git/tag_hooks_service_spec.rb +++ b/spec/services/git/tag_hooks_service_spec.rb @@ -138,7 +138,7 @@ RSpec.describe Git::TagHooksService, :service do before do # Create the lightweight tag - rugged_repo(project.repository).tags.create(tag_name, newrev) + project.repository.write_ref("refs/tags/#{tag_name}", newrev) # Clear tag list cache project.repository.expire_tags_cache diff --git a/spec/services/google_cloud/gcp_region_add_or_replace_service_spec.rb b/spec/services/google_cloud/gcp_region_add_or_replace_service_spec.rb index e2f5a2e719e..b2cd5632be0 100644 --- a/spec/services/google_cloud/gcp_region_add_or_replace_service_spec.rb +++ b/spec/services/google_cloud/gcp_region_add_or_replace_service_spec.rb @@ -11,7 +11,7 @@ RSpec.describe GoogleCloud::GcpRegionAddOrReplaceService do service.execute('env_2', 'loc_2') service.execute('env_1', 'loc_3') - list = project.variables.reload.filter { |variable| variable.key == Projects::GoogleCloudController::GCP_REGION_CI_VAR_KEY } + list = project.variables.reload.filter { |variable| variable.key == Projects::GoogleCloud::GcpRegionsController::GCP_REGION_CI_VAR_KEY } list = list.sort_by(&:environment_scope) aggregate_failures 'testing list of gcp regions' do diff --git a/spec/services/google_cloud/setup_cloudsql_instance_service_spec.rb b/spec/services/google_cloud/setup_cloudsql_instance_service_spec.rb new file mode 100644 index 00000000000..55553097423 --- /dev/null +++ b/spec/services/google_cloud/setup_cloudsql_instance_service_spec.rb @@ -0,0 +1,158 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GoogleCloud::SetupCloudsqlInstanceService do + let(:random_user) { create(:user) } + let(:project) { create(:project) } + + context 'when unauthorized user triggers worker' do + subject do + params = { + gcp_project_id: :gcp_project_id, + instance_name: :instance_name, + database_version: :database_version, + environment_name: :environment_name, + is_protected: :is_protected + } + described_class.new(project, random_user, params).execute + end + + it 'raises unauthorized error' do + message = subject[:message] + status = subject[:status] + + expect(status).to eq(:error) + expect(message).to eq('Unauthorized user') + end + end + + context 'when authorized user triggers worker' do + subject do + user = project.creator + params = { + gcp_project_id: :gcp_project_id, + instance_name: :instance_name, + database_version: :database_version, + environment_name: :environment_name, + is_protected: :is_protected + } + described_class.new(project, user, params).execute + end + + context 'when instance is not RUNNABLE' do + let(:get_instance_response_pending) do + Google::Apis::SqladminV1beta4::DatabaseInstance.new(state: 'PENDING') + end + + it 'raises error' do + allow_next_instance_of(GoogleApi::CloudPlatform::Client) do |google_api_client| + expect(google_api_client).to receive(:get_cloudsql_instance).and_return(get_instance_response_pending) + end + + message = subject[:message] + status = subject[:status] + + expect(status).to eq(:error) + expect(message).to eq('CloudSQL instance not RUNNABLE: {"state":"PENDING"}') + end + end + + context 'when instance is RUNNABLE' do + let(:get_instance_response_runnable) do + Google::Apis::SqladminV1beta4::DatabaseInstance.new( + connection_name: 'mock-connection-name', + ip_addresses: [Struct.new(:ip_address).new('1.2.3.4')], + state: 'RUNNABLE' + ) + end + + let(:operation_fail) { Google::Apis::SqladminV1beta4::Operation.new(status: 'FAILED') } + + let(:operation_done) { Google::Apis::SqladminV1beta4::Operation.new(status: 'DONE') } + + context 'when database creation fails' do + it 'raises error' do + allow_next_instance_of(GoogleApi::CloudPlatform::Client) do |google_api_client| + expect(google_api_client).to receive(:get_cloudsql_instance).and_return(get_instance_response_runnable) + expect(google_api_client).to receive(:create_cloudsql_database).and_return(operation_fail) + end + + message = subject[:message] + status = subject[:status] + + expect(status).to eq(:error) + expect(message).to eq('Database creation failed: {"status":"FAILED"}') + end + end + + context 'when user creation fails' do + it 'raises error' do + allow_next_instance_of(GoogleApi::CloudPlatform::Client) do |google_api_client| + expect(google_api_client).to receive(:get_cloudsql_instance).and_return(get_instance_response_runnable) + expect(google_api_client).to receive(:create_cloudsql_database).and_return(operation_done) + expect(google_api_client).to receive(:create_cloudsql_user).and_return(operation_fail) + end + + message = subject[:message] + status = subject[:status] + + expect(status).to eq(:error) + expect(message).to eq('User creation failed: {"status":"FAILED"}') + end + end + + context 'when database and user creation succeeds' do + it 'stores project CI vars' do + allow_next_instance_of(GoogleApi::CloudPlatform::Client) do |google_api_client| + expect(google_api_client).to receive(:get_cloudsql_instance).and_return(get_instance_response_runnable) + expect(google_api_client).to receive(:create_cloudsql_database).and_return(operation_done) + expect(google_api_client).to receive(:create_cloudsql_user).and_return(operation_done) + end + + subject + + aggregate_failures 'test generated vars' do + variables = project.reload.variables + + expect(variables.count).to eq(8) + expect(variables.find_by(key: 'GCP_PROJECT_ID').value).to eq("gcp_project_id") + expect(variables.find_by(key: 'GCP_CLOUDSQL_INSTANCE_NAME').value).to eq("instance_name") + expect(variables.find_by(key: 'GCP_CLOUDSQL_CONNECTION_NAME').value).to eq("mock-connection-name") + expect(variables.find_by(key: 'GCP_CLOUDSQL_PRIMARY_IP_ADDRESS').value).to eq("1.2.3.4") + expect(variables.find_by(key: 'GCP_CLOUDSQL_VERSION').value).to eq("database_version") + expect(variables.find_by(key: 'GCP_CLOUDSQL_DATABASE_NAME').value).to eq("main_db") + expect(variables.find_by(key: 'GCP_CLOUDSQL_DATABASE_USER').value).to eq("main_user") + expect(variables.find_by(key: 'GCP_CLOUDSQL_DATABASE_PASS').value).to be_present + end + end + + context 'when the ci variable already exists' do + before do + create( + :ci_variable, + project: project, + key: 'GCP_PROJECT_ID', + value: 'previous_gcp_project_id', + environment_scope: :environment_name + ) + end + + it 'overwrites existing GCP_PROJECT_ID var' do + allow_next_instance_of(GoogleApi::CloudPlatform::Client) do |google_api_client| + expect(google_api_client).to receive(:get_cloudsql_instance).and_return(get_instance_response_runnable) + expect(google_api_client).to receive(:create_cloudsql_database).and_return(operation_done) + expect(google_api_client).to receive(:create_cloudsql_user).and_return(operation_done) + end + + subject + + variables = project.reload.variables + value = variables.find_by(key: 'GCP_PROJECT_ID', environment_scope: :environment_name).value + expect(value).to eq("gcp_project_id") + end + end + end + end + end +end diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index 628943e40ff..57a151efda6 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -12,7 +12,7 @@ RSpec.describe Groups::DestroyService do let(:remove_path) { group.path + "+#{group.id}+deleted" } before do - group.add_user(user, Gitlab::Access::OWNER) + group.add_member(user, Gitlab::Access::OWNER) end def destroy_group(group, user, async) @@ -168,8 +168,8 @@ RSpec.describe Groups::DestroyService do let(:group2_user) { create(:user) } before do - group1.add_user(group1_user, Gitlab::Access::OWNER) - group2.add_user(group2_user, Gitlab::Access::OWNER) + group1.add_member(group1_user, Gitlab::Access::OWNER) + group2.add_member(group2_user, Gitlab::Access::OWNER) end context 'when a project is shared with a group' do @@ -203,7 +203,7 @@ RSpec.describe Groups::DestroyService do let(:group3_user) { create(:user) } before do - group3.add_user(group3_user, Gitlab::Access::OWNER) + group3.add_member(group3_user, Gitlab::Access::OWNER) create(:group_group_link, shared_group: group2, shared_with_group: group3) group3.refresh_members_authorized_projects @@ -290,7 +290,7 @@ RSpec.describe Groups::DestroyService do let!(:shared_with_group_user) { create(:user) } before do - shared_with_group.add_user(shared_with_group_user, Gitlab::Access::MAINTAINER) + shared_with_group.add_member(shared_with_group_user, Gitlab::Access::MAINTAINER) create(:group_group_link, shared_group: shared_group, shared_with_group: shared_with_group) shared_with_group.refresh_members_authorized_projects diff --git a/spec/services/groups/group_links/destroy_service_spec.rb b/spec/services/groups/group_links/destroy_service_spec.rb index 6aaf5f45069..03de7175edd 100644 --- a/spec/services/groups/group_links/destroy_service_spec.rb +++ b/spec/services/groups/group_links/destroy_service_spec.rb @@ -24,11 +24,29 @@ RSpec.describe Groups::GroupLinks::DestroyService, '#execute' do expect { subject.execute(link) }.to change { shared_group.shared_with_group_links.count }.from(1).to(0) end - it 'revokes project authorization', :sidekiq_inline do - group.add_developer(user) + context 'with skip_group_share_unlink_auth_refresh feature flag disabled' do + before do + stub_feature_flags(skip_group_share_unlink_auth_refresh: false) + end - expect { subject.execute(link) }.to( - change { Ability.allowed?(user, :read_project, project) }.from(true).to(false)) + it 'revokes project authorization', :sidekiq_inline do + group.add_developer(user) + + expect { subject.execute(link) }.to( + change { Ability.allowed?(user, :read_project, project) }.from(true).to(false)) + end + end + + context 'with skip_group_share_unlink_auth_refresh feature flag enabled' do + before do + stub_feature_flags(skip_group_share_unlink_auth_refresh: true) + end + + it 'maintains project authorization', :sidekiq_inline do + group.add_developer(user) + + expect(Ability.allowed?(user, :read_project, project)).to be_truthy + end end end @@ -45,12 +63,32 @@ RSpec.describe Groups::GroupLinks::DestroyService, '#execute' do ] end - it 'updates project authorization once per group' do - expect(GroupGroupLink).to receive(:delete).and_call_original - expect(group).to receive(:refresh_members_authorized_projects).with(direct_members_only: true, blocking: false).once - expect(another_group).to receive(:refresh_members_authorized_projects).with(direct_members_only: true, blocking: false).once + context 'with skip_group_share_unlink_auth_refresh feature flag disabled' do + before do + stub_feature_flags(skip_group_share_unlink_auth_refresh: false) + end + + it 'updates project authorization once per group' do + expect(GroupGroupLink).to receive(:delete).and_call_original + expect(group).to receive(:refresh_members_authorized_projects).with(direct_members_only: true, blocking: false).once + expect(another_group).to receive(:refresh_members_authorized_projects).with(direct_members_only: true, blocking: false).once + + subject.execute(links) + end + end + + context 'with skip_group_share_unlink_auth_refresh feature flag enabled' do + before do + stub_feature_flags(skip_group_share_unlink_auth_refresh: true) + end + + it 'does not update project authorization once per group' do + expect(GroupGroupLink).to receive(:delete).and_call_original + expect(group).not_to receive(:refresh_members_authorized_projects) + expect(another_group).not_to receive(:refresh_members_authorized_projects) - subject.execute(links) + subject.execute(links) + end end end end diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index 20ea8b2bf1b..fbcca215282 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -439,6 +439,7 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do before do TestEnv.clean_test_path create(:group_member, :owner, group: new_parent_group, user: user) + allow(transfer_service).to receive(:update_project_settings) transfer_service.execute(new_parent_group) end @@ -478,6 +479,11 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do end end + it 'invokes #update_project_settings' do + expect(transfer_service).to have_received(:update_project_settings) + .with(group.projects.pluck(:id)) + end + it_behaves_like 'project namespace path is in sync with project path' do let(:group_full_path) { "#{new_parent_group.path}/#{group.path}" } let(:projects_with_project_namespace) { [project1, project2] } @@ -601,8 +607,8 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do }.from(0).to(1) end - it 'performs authorizations job immediately' do - expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).to receive(:bulk_perform_inline) + it 'performs authorizations job' do + expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).to receive(:bulk_perform_async) transfer_service.execute(new_parent_group) end @@ -659,7 +665,7 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do it 'schedules authorizations job' do expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).to receive(:bulk_perform_async) - .with(array_including(group.all_projects.ids.map { |id| [id, anything] })) + .with(array_including(group.all_projects.ids.map { |id| [id] })) transfer_service.execute(new_parent_group) end diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 46c5e2a9818..c0e1691fe26 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -58,7 +58,7 @@ RSpec.describe Groups::UpdateService do let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL) } before do - public_group.add_user(user, Gitlab::Access::OWNER) + public_group.add_member(user, Gitlab::Access::OWNER) create(:project, :public, group: public_group) expect(TodosDestroyer::GroupPrivateWorker).not_to receive(:perform_in) @@ -119,7 +119,7 @@ RSpec.describe Groups::UpdateService do let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) } before do - internal_group.add_user(user, Gitlab::Access::OWNER) + internal_group.add_member(user, Gitlab::Access::OWNER) create(:project, :internal, group: internal_group) expect(TodosDestroyer::GroupPrivateWorker).not_to receive(:perform_in) @@ -135,7 +135,7 @@ RSpec.describe Groups::UpdateService do let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) } before do - internal_group.add_user(user, Gitlab::Access::OWNER) + internal_group.add_member(user, Gitlab::Access::OWNER) create(:project, :private, group: internal_group) expect(TodosDestroyer::GroupPrivateWorker).to receive(:perform_in) @@ -233,7 +233,7 @@ RSpec.describe Groups::UpdateService do let!(:service) { described_class.new(internal_group, user, visibility_level: 99) } before do - internal_group.add_user(user, Gitlab::Access::MAINTAINER) + internal_group.add_member(user, Gitlab::Access::MAINTAINER) end it "does not change permission level" do @@ -246,7 +246,7 @@ RSpec.describe Groups::UpdateService do let(:service) { described_class.new(internal_group, user, emails_disabled: true) } it 'updates the attribute' do - internal_group.add_user(user, Gitlab::Access::OWNER) + internal_group.add_member(user, Gitlab::Access::OWNER) expect { service.execute }.to change { internal_group.emails_disabled }.to(true) end @@ -280,7 +280,7 @@ RSpec.describe Groups::UpdateService do let!(:service) { described_class.new(internal_group, user, path: SecureRandom.hex) } before do - internal_group.add_user(user, Gitlab::Access::MAINTAINER) + internal_group.add_member(user, Gitlab::Access::MAINTAINER) create(:project, :internal, group: internal_group) 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 731406613dd..4b0c8d9113c 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 @@ -7,14 +7,11 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::AfterUpdateServic let_it_be(:escalation_status, reload: true) { create(:incident_management_issuable_escalation_status, :triggered) } let_it_be(:issue, reload: true) { escalation_status.issue } let_it_be(:project) { issue.project } - let_it_be(:alert) { create(:alert_management_alert, issue: issue, project: project) } - let(:status_event) { :acknowledge } - let(:update_params) { { incident_management_issuable_escalation_status_attributes: { status_event: status_event } } } let(:service) { IncidentManagement::IssuableEscalationStatuses::AfterUpdateService.new(issue, current_user) } subject(:result) do - issue.update!(update_params) + issue.update!(incident_management_issuable_escalation_status_attributes: update_params) service.execute end @@ -22,46 +19,31 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::AfterUpdateServic issue.project.add_developer(current_user) end - shared_examples 'does not attempt to update the alert' do - specify do - expect(::AlertManagement::Alerts::UpdateService).not_to receive(:new) - - expect(result).to be_success - 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 + let(:status_event) { :acknowledge } + let(:update_params) { { status_event: status_event } } - expect { result }.to change { escalation_status.reload.acknowledged? }.to(true) - .and change { alert.reload.acknowledged? }.to(true) + it 'adds a status change system note' do + expect { result }.to change { issue.reload.notes.count }.by(1) end - context 'when incident is not associated with an alert' do - before do - alert.destroy! - end + it 'adds a status change timeline event' do + expect(IncidentManagement::TimelineEvents::CreateService) + .to receive(:change_incident_status) + .with(issue, current_user, escalation_status) + .and_call_original - it_behaves_like 'does not attempt to update the alert' - it_behaves_like 'adds a status change system note' + expect { result }.to change { issue.reload.incident_management_timeline_events.count }.by(1) end + end - context 'when new status matches the current status' do - let(:status_event) { :trigger } - - it_behaves_like 'does not attempt to update the alert' + context 'with non-status attributes' do + let(:update_params) { { updated_at: Time.current } } - specify { expect { result }.not_to change { issue.reload.notes.count } } + it 'does not add a status change system note or timeline event' do + expect { result } + .to not_change { issue.reload.notes.count } + .and not_change { issue.reload.incident_management_timeline_events.count } end end end diff --git a/spec/services/incident_management/issuable_escalation_statuses/build_service_spec.rb b/spec/services/incident_management/issuable_escalation_statuses/build_service_spec.rb index c20a0688ac2..b5c5238d483 100644 --- a/spec/services/incident_management/issuable_escalation_statuses/build_service_spec.rb +++ b/spec/services/incident_management/issuable_escalation_statuses/build_service_spec.rb @@ -11,10 +11,4 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::BuildService do subject(:execute) { service.execute } it_behaves_like 'initializes new escalation status with expected attributes' - - context 'with associated alert' do - let_it_be(:alert) { create(:alert_management_alert, :acknowledged, project: project, issue: incident) } - - it_behaves_like 'initializes new escalation status with expected attributes', { status_event: :acknowledge } - end end diff --git a/spec/services/incident_management/issuable_escalation_statuses/create_service_spec.rb b/spec/services/incident_management/issuable_escalation_statuses/create_service_spec.rb index 2c7d330766c..b6ae03a19fe 100644 --- a/spec/services/incident_management/issuable_escalation_statuses/create_service_spec.rb +++ b/spec/services/incident_management/issuable_escalation_statuses/create_service_spec.rb @@ -27,19 +27,4 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::CreateService do expect { execute }.not_to change { incident.reload.escalation_status } end end - - context 'with associated alert' do - before do - create(:alert_management_alert, :acknowledged, project: project, issue: incident) - end - - it 'creates an escalation status matching the alert attributes' do - expect { execute }.to change { incident.reload.escalation_status }.from(nil) - expect(incident.escalation_status).to have_attributes( - status_name: :acknowledged, - policy_id: nil, - escalations_started_at: nil - ) - 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 6c99631fcb0..761cc5c92ea 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 @@ -102,10 +102,4 @@ 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/incident_management/timeline_events/create_service_spec.rb b/spec/services/incident_management/timeline_events/create_service_spec.rb index 133a644f243..a4e928b98f4 100644 --- a/spec/services/incident_management/timeline_events/create_service_spec.rb +++ b/spec/services/incident_management/timeline_events/create_service_spec.rb @@ -132,6 +132,40 @@ RSpec.describe IncidentManagement::TimelineEvents::CreateService do it 'creates a system note' do expect { execute }.to change { incident.notes.reload.count }.by(1) end + + context 'with auto_created param' do + let(:args) do + { + note: 'note', + occurred_at: Time.current, + action: 'new comment', + promoted_from_note: comment, + auto_created: auto_created + } + end + + context 'when auto_created is true' do + let(:auto_created) { true } + + it 'does not create a system note' do + expect { execute }.not_to change { incident.notes.reload.count } + end + + context 'when user does not have permissions' do + let(:current_user) { user_without_permissions } + + it_behaves_like 'success response' + end + end + + context 'when auto_created is false' do + let(:auto_created) { false } + + it 'creates a system note' do + expect { execute }.to change { incident.notes.reload.count }.by(1) + end + end + end end context 'when incident_timeline feature flag is disabled' do @@ -144,4 +178,71 @@ RSpec.describe IncidentManagement::TimelineEvents::CreateService do end end end + + describe 'automatically created timeline events' do + shared_examples 'successfully created timeline event' do + it 'creates a timeline event', :aggregate_failures do + expect(execute).to be_success + + result = execute.payload[:timeline_event] + expect(result).to be_a(::IncidentManagement::TimelineEvent) + expect(result.author).to eq(current_user) + expect(result.incident).to eq(incident) + expect(result.project).to eq(project) + expect(result.note).to eq(expected_note) + expect(result.editable).to eq(false) + expect(result.action).to eq(expected_action) + end + + it_behaves_like 'an incident management tracked event', :incident_management_timeline_event_created + + it 'successfully creates a database record', :aggregate_failures do + expect { execute }.to change { ::IncidentManagement::TimelineEvent.count }.by(1) + end + + it 'does not create a system note' do + expect { execute }.not_to change { incident.notes.reload.count } + end + end + + describe '.create_incident' do + subject(:execute) { described_class.create_incident(incident, current_user) } + + let(:expected_note) { "@#{current_user.username} created the incident" } + let(:expected_action) { 'issues' } + + it_behaves_like 'successfully created timeline event' + end + + describe '.reopen_incident' do + subject(:execute) { described_class.reopen_incident(incident, current_user) } + + let(:expected_note) { "@#{current_user.username} reopened the incident" } + let(:expected_action) { 'issues' } + + it_behaves_like 'successfully created timeline event' + end + + describe '.resolve_incident' do + subject(:execute) { described_class.resolve_incident(incident, current_user) } + + let(:expected_note) { "@#{current_user.username} resolved the incident" } + let(:expected_action) { 'status' } + + it_behaves_like 'successfully created timeline event' + end + + describe '.change_incident_status' do + subject(:execute) { described_class.change_incident_status(incident, current_user, escalation_status) } + + let(:escalation_status) do + instance_double('IncidentManagement::IssuableEscalationStatus', status_name: 'acknowledged') + end + + let(:expected_note) { "@#{current_user.username} changed the incident status to **Acknowledged**" } + let(:expected_action) { 'status' } + + it_behaves_like 'successfully created timeline event' + end + end end diff --git a/spec/services/incident_management/timeline_events/update_service_spec.rb b/spec/services/incident_management/timeline_events/update_service_spec.rb index 3da533fb2a6..728f2fa3e9d 100644 --- a/spec/services/incident_management/timeline_events/update_service_spec.rb +++ b/spec/services/incident_management/timeline_events/update_service_spec.rb @@ -146,7 +146,8 @@ RSpec.describe IncidentManagement::TimelineEvents::UpdateService do create(:incident_management_timeline_event, :non_editable, project: project, incident: incident) end - it_behaves_like 'error response', 'You cannot edit this timeline event.' + it_behaves_like 'error response', + 'You have insufficient permissions to manage timeline events for this incident' end end @@ -155,7 +156,8 @@ RSpec.describe IncidentManagement::TimelineEvents::UpdateService do project.add_reporter(user) end - it_behaves_like 'error response', 'You have insufficient permissions to manage timeline events for this incident' + it_behaves_like 'error response', + 'You have insufficient permissions to manage timeline events for this incident' end end end diff --git a/spec/services/issuable/clone/attributes_rewriter_spec.rb b/spec/services/issuable/clone/attributes_rewriter_spec.rb deleted file mode 100644 index 7f434b8b246..00000000000 --- a/spec/services/issuable/clone/attributes_rewriter_spec.rb +++ /dev/null @@ -1,140 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Issuable::Clone::AttributesRewriter do - let(:user) { create(:user) } - let(:group) { create(:group) } - let(:project1) { create(:project, :public, group: group) } - let(:project2) { create(:project, :public, group: group) } - let(:original_issue) { create(:issue, project: project1) } - let(:new_issue) { create(:issue, project: project2) } - - subject { described_class.new(user, original_issue, new_issue) } - - context 'setting labels' do - it 'sets labels present in the new project and group labels' do - project1_label_1 = create(:label, title: 'label1', project: project1) - project1_label_2 = create(:label, title: 'label2', project: project1) - project2_label_1 = create(:label, title: 'label1', project: project2) - group_label = create(:group_label, title: 'group_label', group: group) - create(:label, title: 'label3', project: project2) - - original_issue.update!(labels: [project1_label_1, project1_label_2, group_label]) - - subject.execute - - expect(new_issue.reload.labels).to match_array([project2_label_1, group_label]) - end - - it 'does not set any labels when not used on the original issue' do - subject.execute - - expect(new_issue.reload.labels).to be_empty - end - - it 'copies the resource label events' do - resource_label_events = create_list(:resource_label_event, 2, issue: original_issue) - - subject.execute - - expected = resource_label_events.map(&:label_id) - - expect(new_issue.resource_label_events.map(&:label_id)).to match_array(expected) - end - end - - context 'setting milestones' do - it 'sets milestone to nil when old issue milestone is not in the new project' do - milestone = create(:milestone, title: 'milestone', project: project1) - - original_issue.update!(milestone: milestone) - - subject.execute - - expect(new_issue.reload.milestone).to be_nil - end - - it 'copies the milestone when old issue milestone title is in the new project' do - milestone_project1 = create(:milestone, title: 'milestone', project: project1) - milestone_project2 = create(:milestone, title: 'milestone', project: project2) - - original_issue.update!(milestone: milestone_project1) - - subject.execute - - expect(new_issue.reload.milestone).to eq(milestone_project2) - end - - it 'copies the milestone when old issue milestone is a group milestone' do - milestone = create(:milestone, title: 'milestone', group: group) - - original_issue.update!(milestone: milestone) - - subject.execute - - expect(new_issue.reload.milestone).to eq(milestone) - end - - context 'with existing milestone events' do - let!(:milestone1_project1) { create(:milestone, title: 'milestone1', project: project1) } - let!(:milestone2_project1) { create(:milestone, title: 'milestone2', project: project1) } - let!(:milestone3_project1) { create(:milestone, title: 'milestone3', project: project1) } - - let!(:milestone1_project2) { create(:milestone, title: 'milestone1', project: project2) } - let!(:milestone2_project2) { create(:milestone, title: 'milestone2', project: project2) } - - before do - original_issue.update!(milestone: milestone2_project1) - - create_event(milestone1_project1) - create_event(milestone2_project1) - create_event(nil, 'remove') - create_event(milestone3_project1) - end - - it 'copies existing resource milestone events' do - subject.execute - - new_issue_milestone_events = new_issue.reload.resource_milestone_events - expect(new_issue_milestone_events.count).to eq(3) - - expect_milestone_event(new_issue_milestone_events.first, milestone: milestone1_project2, action: 'add', state: 'opened') - expect_milestone_event(new_issue_milestone_events.second, milestone: milestone2_project2, action: 'add', state: 'opened') - expect_milestone_event(new_issue_milestone_events.third, milestone: nil, action: 'remove', state: 'opened') - end - - def create_event(milestone, action = 'add') - create(:resource_milestone_event, issue: original_issue, milestone: milestone, action: action) - end - - def expect_milestone_event(event, expected_attrs) - expect(event.milestone_id).to eq(expected_attrs[:milestone]&.id) - expect(event.action).to eq(expected_attrs[:action]) - expect(event.state).to eq(expected_attrs[:state]) - end - end - - context 'with existing state events' do - let!(:event1) { create(:resource_state_event, issue: original_issue, state: 'opened') } - let!(:event2) { create(:resource_state_event, issue: original_issue, state: 'closed') } - let!(:event3) { create(:resource_state_event, issue: original_issue, state: 'reopened') } - - it 'copies existing state events as expected' do - subject.execute - - state_events = new_issue.reload.resource_state_events - expect(state_events.size).to eq(3) - - expect_state_event(state_events.first, issue: new_issue, state: 'opened') - expect_state_event(state_events.second, issue: new_issue, state: 'closed') - expect_state_event(state_events.third, issue: new_issue, state: 'reopened') - end - - def expect_state_event(event, expected_attrs) - expect(event.issue_id).to eq(expected_attrs[:issue]&.id) - expect(event.state).to eq(expected_attrs[:state]) - end - end - end -end diff --git a/spec/services/issues/clone_service_spec.rb b/spec/services/issues/clone_service_spec.rb index abbcb1c1d48..858dfc4ab3a 100644 --- a/spec/services/issues/clone_service_spec.rb +++ b/spec/services/issues/clone_service_spec.rb @@ -82,12 +82,14 @@ RSpec.describe Issues::CloneService do expect(new_issue.iid).to be_present end - it 'preserves create time' do - expect(old_issue.created_at.strftime('%D')).to eq new_issue.created_at.strftime('%D') - end + it 'sets created_at of new issue to the time of clone' do + future_time = 5.days.from_now - it 'does not copy system notes' do - expect(new_issue.notes.count).to eq(1) + travel_to(future_time) do + new_issue = clone_service.execute(old_issue, new_project, with_notes: with_notes) + + expect(new_issue.created_at).to be_like_time(future_time) + end end it 'does not set moved_issue' do @@ -105,6 +107,24 @@ RSpec.describe Issues::CloneService do end end + context 'issue with system notes and resource events' do + before do + create(:note, :system, noteable: old_issue, project: old_project) + create(:resource_label_event, label: create(:label, project: old_project), issue: old_issue) + create(:resource_state_event, issue: old_issue, state: :reopened) + create(:resource_milestone_event, issue: old_issue, action: 'remove', milestone_id: nil) + end + + it 'does not copy system notes and resource events' do + new_issue = clone_service.execute(old_issue, new_project) + + # 1 here is for the "cloned from" system note + expect(new_issue.notes.count).to eq(1) + expect(new_issue.resource_state_events).to be_empty + expect(new_issue.resource_milestone_events).to be_empty + end + end + context 'issue with award emoji' do let!(:award_emoji) { create(:award_emoji, awardable: old_issue) } @@ -124,14 +144,27 @@ RSpec.describe Issues::CloneService do create(:issue, title: title, description: description, project: old_project, author: author, milestone: milestone) end - before do - create(:resource_milestone_event, issue: old_issue, milestone: milestone, action: :add) + it 'copies the milestone and creates a resource_milestone_event' do + new_issue = clone_service.execute(old_issue, new_project) + + expect(new_issue.milestone).to eq(milestone) + expect(new_issue.resource_milestone_events.count).to eq(1) + end + end + + context 'issue with label' do + let(:label) { create(:group_label, group: sub_group_1) } + let(:new_project) { create(:project, namespace: sub_group_1) } + + let(:old_issue) do + create(:issue, project: old_project, labels: [label]) end - it 'does not create extra milestone events' do + it 'copies the label and creates a resource_label_event' do new_issue = clone_service.execute(old_issue, new_project) - expect(new_issue.resource_milestone_events.count).to eq(old_issue.resource_milestone_events.count) + expect(new_issue.labels).to contain_exactly(label) + expect(new_issue.resource_label_events.count).to eq(1) end end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 344da5a6582..e88fe1b42f0 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -122,14 +122,29 @@ RSpec.describe Issues::CloseService do expect(new_note.author).to eq(user) end + it 'adds a timeline event', :aggregate_failures do + expect(IncidentManagement::TimelineEvents::CreateService) + .to receive(:resolve_incident) + .with(issue, user) + .and_call_original + + expect { service.execute(issue) }.to change { issue.incident_management_timeline_events.count }.by(1) + end + context 'when the escalation status did not change to resolved' do let(:escalation_status) { instance_double('IncidentManagement::IssuableEscalationStatus', resolve: false) } - it 'does not create a system note' do + before do allow(issue).to receive(:incident_management_issuable_escalation_status).and_return(escalation_status) + end + it 'does not create a system note' do expect { service.execute(issue) }.not_to change { issue.notes.count } end + + it 'does not create a timeline event' do + expect { service.execute(issue) }.not_to change { issue.incident_management_timeline_events.count } + end end end end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 9f006603f29..0bc8511e3e3 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -135,6 +135,14 @@ RSpec.describe Issues::CreateService do issue end + it 'calls IncidentManagement::TimelineEvents::CreateService.create_incident' do + expect(IncidentManagement::TimelineEvents::CreateService) + .to receive(:create_incident) + .with(a_kind_of(Issue), reporter) + + issue + end + context 'when invalid' do before do opts.merge!(title: '') @@ -489,6 +497,23 @@ RSpec.describe Issues::CreateService do end end end + + context 'with alert bot author' do + let_it_be(:user) { User.alert_bot } + let_it_be(:label) { create(:label, project: project) } + + let(:opts) do + { + title: 'Title', + description: %(/label #{label.to_reference(format: :name)}") + } + end + + it 'can apply labels' do + expect(issue).to be_persisted + expect(issue.labels).to eq([label]) + end + end end context 'resolving discussions' do diff --git a/spec/services/issues/import_csv_service_spec.rb b/spec/services/issues/import_csv_service_spec.rb index fa40b75190f..9ad1d7dba9f 100644 --- a/spec/services/issues/import_csv_service_spec.rb +++ b/spec/services/issues/import_csv_service_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe Issues::ImportCsvService do let(:project) { create(:project) } let(:user) { create(:user) } + let(:assignee) { create(:user, username: 'csv_assignee') } let(:service) do uploader = FileUploader.new(project) uploader.store!(file) @@ -16,4 +17,27 @@ RSpec.describe Issues::ImportCsvService do let(:issuables) { project.issues } let(:email_method) { :import_issues_csv_email } end + + describe '#execute' do + let(:file) { fixture_file_upload('spec/fixtures/csv_complex.csv') } + + subject { service.execute } + + it 'sets all issueable attributes and executes quick actions' do + project.add_developer(user) + project.add_developer(assignee) + + expect { subject }.to change { issuables.count }.by 3 + + expect(issuables.reload).to include( + have_attributes( + title: 'Title with quote"', + description: 'Description', + time_estimate: 3600, + assignees: include(assignee), + due_date: Date.new(2022, 6, 28) + ) + ) + end + end end diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index 56a3c22cd7f..5a1bb2e8b74 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -47,6 +47,7 @@ RSpec.describe Issues::MoveService do it 'creates a new issue in a new project' do expect(new_issue.project).to eq new_project + expect(new_issue.namespace_id).to eq new_project.project_namespace_id end it 'copies issue title' do diff --git a/spec/services/issues/related_branches_service_spec.rb b/spec/services/issues/related_branches_service_spec.rb index 7a4bae7f852..95d456c1b05 100644 --- a/spec/services/issues/related_branches_service_spec.rb +++ b/spec/services/issues/related_branches_service_spec.rb @@ -3,88 +3,47 @@ require 'spec_helper' RSpec.describe Issues::RelatedBranchesService do + let_it_be(:project) { create(:project, :repository, :public, public_builds: false) } let_it_be(:developer) { create(:user) } - let_it_be(:issue) { create(:issue) } + let_it_be(:issue) { create(:issue, project: project) } let(:user) { developer } - subject { described_class.new(project: issue.project, current_user: user) } + subject { described_class.new(project: project, current_user: user) } - before do - issue.project.add_developer(developer) + before_all do + project.add_developer(developer) end describe '#execute' do - let(:sha) { 'abcdef' } - let(:repo) { issue.project.repository } - let(:project) { issue.project } let(:branch_info) { subject.execute(issue) } - def make_branch - double('Branch', dereferenced_target: double('Target', sha: sha)) - end - - before do - allow(repo).to receive(:branch_names).and_return(branch_names) - end - - context 'no branches are available' do - let(:branch_names) { [] } - - it 'returns an empty array' do - expect(branch_info).to be_empty - end - end - context 'branches are available' do - let(:missing_branch) { "#{issue.to_branch_name}-missing" } - let(:unreadable_branch_name) { "#{issue.to_branch_name}-unreadable" } - let(:pipeline) { build(:ci_pipeline, :success, project: project) } - let(:unreadable_pipeline) { build(:ci_pipeline, :running) } - - let(:branch_names) do - [ - generate(:branch), - "#{issue.iid}doesnt-match", - issue.to_branch_name, - missing_branch, - unreadable_branch_name - ] - end + let_it_be(:pipeline) { create(:ci_pipeline, :success, project: project, ref: issue.to_branch_name) } - before do - { - issue.to_branch_name => pipeline, - unreadable_branch_name => unreadable_pipeline - }.each do |name, pipeline| - allow(repo).to receive(:find_branch).with(name).and_return(make_branch) - allow(project).to receive(:latest_pipeline).with(name, sha).and_return(pipeline) - end + before_all do + project.repository.create_branch(issue.to_branch_name, pipeline.sha) + project.repository.create_branch("#{issue.iid}doesnt-match", project.repository.root_ref) + project.repository.create_branch("#{issue.iid}-0-stable", project.repository.root_ref) - allow(repo).to receive(:find_branch).with(missing_branch).and_return(nil) + project.repository.add_tag(developer, issue.to_branch_name, pipeline.sha) end - it 'selects relevant branches, along with pipeline status where available' do - expect(branch_info).to contain_exactly( - { name: issue.to_branch_name, pipeline_status: an_instance_of(Gitlab::Ci::Status::Success) }, - { name: missing_branch, pipeline_status: be_nil }, - { name: unreadable_branch_name, pipeline_status: be_nil } - ) + context 'when user has access to pipelines' do + it 'selects relevant branches, along with pipeline status' do + expect(branch_info).to contain_exactly( + { name: issue.to_branch_name, pipeline_status: an_instance_of(Gitlab::Ci::Status::Success) } + ) + end end - context 'the user has access to otherwise unreadable pipelines' do - let(:user) { create(:admin) } + context 'when user does not have access to pipelines' do + let(:user) { create(:user) } - context 'when admin mode is enabled', :enable_admin_mode do - it 'returns info a developer could not see' do - expect(branch_info.pluck(:pipeline_status)).to include(an_instance_of(Gitlab::Ci::Status::Running)) - end - end - - context 'when admin mode is disabled' do - it 'does not return info a developer could not see' do - expect(branch_info.pluck(:pipeline_status)).not_to include(an_instance_of(Gitlab::Ci::Status::Running)) - end + it 'returns branches without pipeline status' do + expect(branch_info).to contain_exactly( + { name: issue.to_branch_name, pipeline_status: nil } + ) end end @@ -103,10 +62,10 @@ RSpec.describe Issues::RelatedBranchesService do end end - context 'one of the branches is stable' do - let(:branch_names) { ["#{issue.iid}-0-stable"] } + context 'no branches are available' do + let(:project) { create(:project, :empty_repo) } - it 'is excluded' do + it 'returns an empty array' do expect(branch_info).to be_empty end end diff --git a/spec/services/issues/reopen_service_spec.rb b/spec/services/issues/reopen_service_spec.rb index c9469b861ac..477b44f4c2c 100644 --- a/spec/services/issues/reopen_service_spec.rb +++ b/spec/services/issues/reopen_service_spec.rb @@ -33,6 +33,8 @@ RSpec.describe Issues::ReopenService do context 'when user is authorized to reopen issue' do let(:user) { create(:user) } + subject(:execute) { described_class.new(project: project, current_user: user).execute(issue) } + before do project.add_maintainer(user) end @@ -41,14 +43,12 @@ RSpec.describe Issues::ReopenService do issue.assignees << user expect_any_instance_of(User).to receive(:invalidate_issue_cache_counts) - described_class.new(project: project, current_user: user).execute(issue) + execute end it 'refreshes the number of opened issues' do - service = described_class.new(project: project, current_user: user) - expect do - service.execute(issue) + execute BatchLoader::Executor.clear_current end.to change { project.open_issues_count }.from(0).to(1) @@ -61,16 +61,27 @@ RSpec.describe Issues::ReopenService do expect(service).to receive(:delete_cache).and_call_original end - described_class.new(project: project, current_user: user).execute(issue) + execute + end + + it 'does not create timeline event' do + expect { execute }.not_to change { issue.incident_management_timeline_events.count } end context 'issue is incident type' do let(:issue) { create(:incident, :closed, project: project) } let(:current_user) { user } - subject { described_class.new(project: project, current_user: user).execute(issue) } - it_behaves_like 'an incident management tracked event', :incident_management_incident_reopened + + it 'creates a timeline event' do + expect(IncidentManagement::TimelineEvents::CreateService) + .to receive(:reopen_incident) + .with(issue, current_user) + .and_call_original + + expect { execute }.to change { issue.incident_management_timeline_events.count }.by(1) + end end context 'when issue is not confidential' do @@ -78,18 +89,18 @@ RSpec.describe Issues::ReopenService do expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :issue_hooks) - described_class.new(project: project, current_user: user).execute(issue) + execute end end context 'when issue is confidential' do - it 'executes confidential issue hooks' do - issue = create(:issue, :confidential, :closed, project: project) + let(:issue) { create(:issue, :confidential, :closed, project: project) } + it 'executes confidential issue hooks' do expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks) expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :confidential_issue_hooks) - described_class.new(project: project, current_user: user).execute(issue) + execute end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index d11fe772023..e2e8828ae89 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -1146,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, expected_reason = nil| + shared_examples 'updates the escalation status record' do |expected_status| let(:service_double) { instance_double(escalation_update_class) } it 'has correct value' do - expect(escalation_update_class).to receive(:new).with(issue, user, status_change_reason: expected_reason).and_return(service_double) + expect(escalation_update_class).to receive(:new).with(issue, user).and_return(service_double) expect(service_double).to receive(:execute) update_issue(opts) @@ -1193,23 +1193,6 @@ RSpec.describe Issues::UpdateService, :mailer do it_behaves_like 'updates the escalation status record', :acknowledged - context 'with associated alert' do - let!(:alert) { create(:alert_management_alert, issue: issue, project: project) } - - it 'syncs the update back to the alert' do - update_issue(opts) - - expect(issue.escalation_status.status_name).to eq(:acknowledged) - expect(alert.reload.status_name).to eq(:acknowledged) - 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' } } } diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb index e79e13af769..fe9f3ddc14d 100644 --- a/spec/services/members/create_service_spec.rb +++ b/spec/services/members/create_service_spec.rb @@ -146,12 +146,14 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ end context 'when passing an existing invite user id' do - let(:user_id) { create(:project_member, :invited, project: source).invite_email } + let(:invited_member) { create(:project_member, :guest, :invited, project: source) } + let(:user_id) { invited_member.invite_email } + let(:access_level) { ProjectMember::MAINTAINER } - it 'does not add a member' do - expect(execute_service[:status]).to eq(:error) - expect(execute_service[:message]).to eq("The member's email address has already been taken") - expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(false) + it 'allows already invited members to be re-invited by email and updates the member access' do + expect(execute_service[:status]).to eq(:success) + expect(invited_member.reset.access_level).to eq ProjectMember::MAINTAINER + expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true) end end diff --git a/spec/services/members/creator_service_spec.rb b/spec/services/members/creator_service_spec.rb index 8b1df2ab86d..ad4c649086b 100644 --- a/spec/services/members/creator_service_spec.rb +++ b/spec/services/members/creator_service_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Members::CreatorService do describe '#execute' do it 'raises error for new member on authorization check implementation' do expect do - described_class.add_user(source, user, :maintainer, current_user: current_user) + described_class.add_member(source, user, :maintainer, current_user: current_user) end.to raise_error(NotImplementedError) end @@ -19,7 +19,7 @@ RSpec.describe Members::CreatorService do source.add_developer(user) expect do - described_class.add_user(source, user, :maintainer, current_user: current_user) + described_class.add_member(source, user, :maintainer, current_user: current_user) end.to raise_error(NotImplementedError) end end diff --git a/spec/services/members/groups/creator_service_spec.rb b/spec/services/members/groups/creator_service_spec.rb index b80b7998eac..4130fbd44fa 100644 --- a/spec/services/members/groups/creator_service_spec.rb +++ b/spec/services/members/groups/creator_service_spec.rb @@ -14,13 +14,13 @@ RSpec.describe Members::Groups::CreatorService do it_behaves_like 'owner management' - describe '.add_users' do + describe '.add_members' do it_behaves_like 'bulk member creation' do let_it_be(:member_type) { GroupMember } end end - describe '.add_user' do + describe '.add_member' do it_behaves_like 'member creation' do let_it_be(:member_type) { GroupMember } end @@ -30,7 +30,7 @@ RSpec.describe Members::Groups::CreatorService do expect(AuthorizedProjectsWorker).to receive(:bulk_perform_and_wait).once 1.upto(3) do - described_class.add_user(source, user, :maintainer) + described_class.add_member(source, user, :maintainer) end end end diff --git a/spec/services/members/invite_member_builder_spec.rb b/spec/services/members/invite_member_builder_spec.rb new file mode 100644 index 00000000000..52de65364c4 --- /dev/null +++ b/spec/services/members/invite_member_builder_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Members::InviteMemberBuilder do + let_it_be(:source) { create(:group) } + let_it_be(:existing_member) { create(:group_member) } + + let(:existing_members) { { existing_member.user.id => existing_member } } + + describe '#execute' do + context 'when user record found by email' do + it 'returns member from existing members hash' do + expect(described_class.new(source, existing_member.user.email, existing_members).execute).to eq existing_member + end + + it 'builds a new member' do + user = create(:user) + + member = described_class.new(source, user.email, existing_members).execute + + expect(member).to be_new_record + expect(member.user).to eq user + end + end + end + + context 'when no existing users found by the email' do + it 'finds existing member' do + member = create(:group_member, :invited, source: source) + + expect(described_class.new(source, member.invite_email, existing_members).execute).to eq member + end + + it 'builds a new member' do + email = 'test@example.com' + + member = described_class.new(source, email, existing_members).execute + + expect(member).to be_new_record + expect(member.invite_email).to eq email + end + end +end diff --git a/spec/services/members/invite_service_spec.rb b/spec/services/members/invite_service_spec.rb index a948041479b..7a1512970b4 100644 --- a/spec/services/members/invite_service_spec.rb +++ b/spec/services/members/invite_service_spec.rb @@ -353,15 +353,16 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ context 'when member already exists' do context 'with email' do - let!(:invited_member) { create(:project_member, :invited, project: project) } - let(:params) { { email: "#{invited_member.invite_email},#{project_user.email}" } } + let!(:invited_member) { create(:project_member, :guest, :invited, project: project) } + let(:params) do + { email: "#{invited_member.invite_email},#{project_user.email}", access_level: ProjectMember::MAINTAINER } + end - it 'adds new email and returns an error for the already invited email' do + it 'adds new email and allows already invited members to be re-invited by email and updates the member access' do expect_to_create_members(count: 1) - expect(result[:status]).to eq(:error) - expect(result[:message][invited_member.invite_email]) - .to eq("The member's email address has already been taken") + expect(result[:status]).to eq(:success) expect(project.users).to include project_user + expect(invited_member.reset.access_level).to eq ProjectMember::MAINTAINER end end diff --git a/spec/services/members/projects/creator_service_spec.rb b/spec/services/members/projects/creator_service_spec.rb index 38955122ab0..8304bee2ffc 100644 --- a/spec/services/members/projects/creator_service_spec.rb +++ b/spec/services/members/projects/creator_service_spec.rb @@ -14,13 +14,13 @@ RSpec.describe Members::Projects::CreatorService do it_behaves_like 'owner management' - describe '.add_users' do + describe '.add_members' do it_behaves_like 'bulk member creation' do let_it_be(:member_type) { ProjectMember } end end - describe '.add_user' do + describe '.add_member' do it_behaves_like 'member creation' do let_it_be(:member_type) { ProjectMember } end @@ -30,7 +30,7 @@ RSpec.describe Members::Projects::CreatorService do expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to receive(:bulk_perform_in).once 1.upto(3) do - described_class.add_user(source, user, :maintainer) + described_class.add_member(source, user, :maintainer) end end end diff --git a/spec/services/members/standard_member_builder_spec.rb b/spec/services/members/standard_member_builder_spec.rb new file mode 100644 index 00000000000..16daff53d31 --- /dev/null +++ b/spec/services/members/standard_member_builder_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Members::StandardMemberBuilder do + let_it_be(:source) { create(:group) } + let_it_be(:existing_member) { create(:group_member) } + + let(:existing_members) { { existing_member.user.id => existing_member } } + + describe '#execute' do + it 'returns member from existing members hash' do + expect(described_class.new(source, existing_member.user, existing_members).execute).to eq existing_member + end + + it 'builds a new member' do + user = create(:user) + + member = described_class.new(source, user, existing_members).execute + + expect(member).to be_new_record + expect(member.user).to eq user + end + end +end diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb index e500102a00c..e1fbb945ee3 100644 --- a/spec/services/merge_requests/approval_service_spec.rb +++ b/spec/services/merge_requests/approval_service_spec.rb @@ -90,7 +90,7 @@ RSpec.describe MergeRequests::ApprovalService do it 'tracks merge request approve action' do expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) - .to receive(:track_approve_mr_action).with(user: user) + .to receive(:track_approve_mr_action).with(user: user, merge_request: merge_request) service.execute(merge_request) end diff --git a/spec/services/merge_requests/create_pipeline_service_spec.rb b/spec/services/merge_requests/create_pipeline_service_spec.rb index 08ad05b54da..03a37ea59a3 100644 --- a/spec/services/merge_requests/create_pipeline_service_spec.rb +++ b/spec/services/merge_requests/create_pipeline_service_spec.rb @@ -13,7 +13,6 @@ RSpec.describe MergeRequests::CreatePipelineService do let(:params) { {} } before do - stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false) project.add_developer(user) end @@ -92,16 +91,6 @@ RSpec.describe MergeRequests::CreatePipelineService do end end end - - context 'when ci_disallow_to_create_merge_request_pipelines_in_target_project feature flag is enabled' do - before do - stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: true) - end - - it 'creates a pipeline in the source project' do - expect(response.payload.project).to eq(source_project) - end - end end context 'when actor has permission to create pipelines in forked project' do diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index c0c56a72192..9c9bcb79990 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -212,7 +212,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end before do - stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false) target_project.add_developer(user2) target_project.add_maintainer(user) end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index eecf7c21cba..4b7dd84474a 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -243,6 +243,25 @@ RSpec.describe MergeRequests::RefreshService do end end + context 'when ci.skip push_options are passed' do + let(:params) { { push_options: { ci: { skip: true } } } } + let(:service_instance) { service.new(project: project, current_user: @user, params: params) } + + subject { service_instance.execute(@oldrev, @newrev, ref) } + + it 'creates a skipped detached merge request pipeline with commits' do + expect { subject } + .to change { @merge_request.pipelines_for_merge_request.count }.by(1) + .and change { @another_merge_request.pipelines_for_merge_request.count }.by(0) + + expect(@merge_request.has_commits?).to be_truthy + expect(@another_merge_request.has_commits?).to be_falsy + + pipeline = @merge_request.pipelines_for_merge_request.last + expect(pipeline).to be_skipped + end + end + it 'does not create detached merge request pipeline for forked project' do expect { subject } .not_to change { @fork_merge_request.pipelines_for_merge_request.count } @@ -267,10 +286,6 @@ RSpec.describe MergeRequests::RefreshService do context 'when service runs on forked project' do let(:project) { @fork_project } - before do - stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false) - end - it 'creates detached merge request pipeline for fork merge request' do expect { subject } .to change { @fork_merge_request.pipelines_for_merge_request.count }.by(1) diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 7164ba8fac0..212f75d853f 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -845,6 +845,8 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end context 'when the draft status is changed' do + let(:title) { 'New Title' } + let(:draft_title) { "Draft: #{title}" } let!(:non_subscriber) { create(:user) } let!(:subscriber) do create(:user) { |u| merge_request.toggle_subscription(u, project) } @@ -857,7 +859,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do context 'removing draft status' do before do - merge_request.update_attribute(:title, 'Draft: New Title') + merge_request.update_attribute(:title, draft_title) end it 'sends notifications for subscribers', :sidekiq_might_not_need_inline do @@ -870,9 +872,22 @@ RSpec.describe MergeRequests::UpdateService, :mailer do should_email(subscriber) should_not_email(non_subscriber) end + + context 'when removing through wip_event param' do + it 'removes Draft from the title' do + expect { update_merge_request({ wip_event: "ready" }) } + .to change { merge_request.title } + .from(draft_title) + .to(title) + end + end end context 'adding draft status' do + before do + merge_request.update_attribute(:title, title) + end + it 'does not send notifications', :sidekiq_might_not_need_inline do opts = { title: 'Draft: New title' } @@ -883,6 +898,15 @@ RSpec.describe MergeRequests::UpdateService, :mailer do should_not_email(subscriber) should_not_email(non_subscriber) end + + context 'when adding through wip_event param' do + it 'adds Draft to the title' do + expect { update_merge_request({ wip_event: "draft" }) } + .to change { merge_request.title } + .from(title) + .to(draft_title) + end + end end end diff --git a/spec/services/namespaces/in_product_marketing_emails_service_spec.rb b/spec/services/namespaces/in_product_marketing_emails_service_spec.rb index de84666ca1d..b44c256802f 100644 --- a/spec/services/namespaces/in_product_marketing_emails_service_spec.rb +++ b/spec/services/namespaces/in_product_marketing_emails_service_spec.rb @@ -54,7 +54,6 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do :team | 1 | { created_at: frozen_time - 2.days, git_write_at: frozen_time - 2.days, pipeline_created_at: frozen_time - 2.days, trial_started_at: frozen_time - 2.days } :team | 5 | { created_at: frozen_time - 6.days, git_write_at: frozen_time - 6.days, pipeline_created_at: frozen_time - 6.days, trial_started_at: frozen_time - 6.days } :team | 10 | { created_at: frozen_time - 11.days, git_write_at: frozen_time - 11.days, pipeline_created_at: frozen_time - 11.days, trial_started_at: frozen_time - 11.days } - :experience | 30 | { created_at: frozen_time - 31.days, git_write_at: frozen_time - 31.days } end with_them do diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 032f35cfc29..98fe8a40c61 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -147,6 +147,34 @@ RSpec.describe NotificationService, :mailer do end end + shared_examples 'participating by confidential note notification' do + context 'when user is mentioned on confidential note' do + let_it_be(:guest_1) { create(:user) } + let_it_be(:guest_2) { create(:user) } + let_it_be(:reporter) { create(:user) } + + before do + issuable.resource_parent.add_guest(guest_1) + issuable.resource_parent.add_guest(guest_2) + issuable.resource_parent.add_reporter(reporter) + end + + it 'only emails authorized users' do + confidential_note_text = "#{guest_1.to_reference} and #{guest_2.to_reference} and #{reporter.to_reference}" + note_text = "Mentions #{guest_2.to_reference}" + create(:note_on_issue, noteable: issuable, project_id: project.id, note: confidential_note_text, confidential: true) + create(:note_on_issue, noteable: issuable, project_id: project.id, note: note_text) + reset_delivered_emails! + + notification_trigger + + should_not_email(guest_1) + should_email(guest_2) + should_email(reporter) + end + end + end + shared_examples 'participating by assignee notification' do it 'emails the participant' do issuable.assignees << participant @@ -554,8 +582,8 @@ RSpec.describe NotificationService, :mailer do before do note.project.namespace_id = group.id - group.add_user(@u_watcher, GroupMember::MAINTAINER) - group.add_user(@u_custom_global, GroupMember::MAINTAINER) + group.add_member(@u_watcher, GroupMember::MAINTAINER) + group.add_member(@u_custom_global, GroupMember::MAINTAINER) note.project.save! @u_watcher.notification_settings_for(note.project).participating! @@ -736,6 +764,20 @@ RSpec.describe NotificationService, :mailer do let(:notification_target) { note } let(:notification_trigger) { notification.new_note(note) } end + + context 'when note is confidential' do + let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@all mentioned', confidential: true) } + let(:guest) { create(:user) } + + it 'does not notify users that cannot read note' do + project.add_guest(guest) + reset_delivered_emails! + + notification.new_note(note) + + should_not_email(guest) + end + end end end @@ -1376,6 +1418,11 @@ RSpec.describe NotificationService, :mailer do let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled, [assignee]) } end + it_behaves_like 'participating by confidential note notification' do + let(:issuable) { issue } + let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled, [assignee]) } + end + it_behaves_like 'project emails are disabled' do let(:notification_target) { issue } let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled, [assignee]) } @@ -1494,6 +1541,11 @@ RSpec.describe NotificationService, :mailer do let(:notification_target) { issue } let(:notification_trigger) { notification.removed_milestone_issue(issue, issue.author) } end + + it_behaves_like 'participating by confidential note notification' do + let(:issuable) { issue } + let(:notification_trigger) { notification.removed_milestone_issue(issue, issue.author) } + end end context 'confidential issues' do @@ -1616,6 +1668,11 @@ RSpec.describe NotificationService, :mailer do let(:notification_trigger) { notification.close_issue(issue, @u_disabled) } end + it_behaves_like 'participating by confidential note notification' do + let(:issuable) { issue } + let(:notification_trigger) { notification.close_issue(issue, @u_disabled) } + end + it 'adds "subscribed" reason to subscriber emails' do user_1 = create(:user) issue.subscribe(user_1) @@ -1658,6 +1715,11 @@ RSpec.describe NotificationService, :mailer do let(:notification_trigger) { notification.reopen_issue(issue, @u_disabled) } end + it_behaves_like 'participating by confidential note notification' do + let(:issuable) { issue } + let(:notification_trigger) { notification.reopen_issue(issue, @u_disabled) } + end + it_behaves_like 'project emails are disabled' do let(:notification_target) { issue } let(:notification_trigger) { notification.reopen_issue(issue, @u_disabled) } @@ -1689,6 +1751,11 @@ RSpec.describe NotificationService, :mailer do let(:notification_trigger) { notification.issue_moved(issue, new_issue, @u_disabled) } end + it_behaves_like 'participating by confidential note notification' do + let(:issuable) { issue } + let(:notification_trigger) { notification.issue_moved(issue, new_issue, @u_disabled) } + end + it_behaves_like 'project emails are disabled' do let(:notification_target) { issue } let(:notification_trigger) { notification.issue_moved(issue, new_issue, @u_disabled) } @@ -1720,6 +1787,11 @@ RSpec.describe NotificationService, :mailer do let(:notification_trigger) { notification.issue_cloned(issue, new_issue, @u_disabled) } end + it_behaves_like 'participating by confidential note notification' do + let(:issuable) { issue } + let(:notification_trigger) { notification.issue_cloned(issue, new_issue, @u_disabled) } + end + it_behaves_like 'project emails are disabled' do let(:notification_target) { issue } let(:notification_trigger) { notification.issue_cloned(issue, new_issue, @u_disabled) } @@ -1765,6 +1837,11 @@ RSpec.describe NotificationService, :mailer do let(:notification_trigger) { notification.issue_due(issue) } end + it_behaves_like 'participating by confidential note notification' do + let(:issuable) { issue } + let(:notification_trigger) { notification.issue_due(issue) } + end + it_behaves_like 'project emails are disabled' do let(:notification_target) { issue } let(:notification_trigger) { notification.issue_due(issue) } @@ -3773,7 +3850,7 @@ RSpec.describe NotificationService, :mailer do # Group member: global=watch, group=global @g_global_watcher ||= create_global_setting_for(create(:user), :watch) - group.add_users([@g_watcher, @g_global_watcher], :maintainer) + group.add_members([@g_watcher, @g_global_watcher], :maintainer) group end diff --git a/spec/services/packages/cleanup/execute_policy_service_spec.rb b/spec/services/packages/cleanup/execute_policy_service_spec.rb new file mode 100644 index 00000000000..93335c4a821 --- /dev/null +++ b/spec/services/packages/cleanup/execute_policy_service_spec.rb @@ -0,0 +1,163 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Packages::Cleanup::ExecutePolicyService do + let_it_be(:project) { create(:project) } + let_it_be_with_reload(:policy) { create(:packages_cleanup_policy, project: project) } + + let(:service) { described_class.new(policy) } + + describe '#execute' do + subject(:execute) { service.execute } + + context 'with the keep_n_duplicated_files parameter' do + let_it_be(:package1) { create(:package, project: project) } + let_it_be(:package2) { create(:package, project: project) } + let_it_be(:package3) { create(:package, project: project) } + let_it_be(:package4) { create(:package, :pending_destruction, project: project) } + + let_it_be(:package_file1_1) { create(:package_file, package: package1, file_name: 'file_name1') } + let_it_be(:package_file1_2) { create(:package_file, package: package1, file_name: 'file_name1') } + let_it_be(:package_file1_3) { create(:package_file, package: package1, file_name: 'file_name1') } + + let_it_be(:package_file1_4) { create(:package_file, package: package1, file_name: 'file_name2') } + let_it_be(:package_file1_5) { create(:package_file, package: package1, file_name: 'file_name2') } + let_it_be(:package_file1_6) { create(:package_file, package: package1, file_name: 'file_name2') } + let_it_be(:package_file1_7) do + create(:package_file, :pending_destruction, package: package1, file_name: 'file_name2') + end + + let_it_be(:package_file2_1) { create(:package_file, package: package2, file_name: 'file_name1') } + let_it_be(:package_file2_2) { create(:package_file, package: package2, file_name: 'file_name1') } + let_it_be(:package_file2_3) { create(:package_file, package: package2, file_name: 'file_name1') } + let_it_be(:package_file2_4) { create(:package_file, package: package2, file_name: 'file_name1') } + + let_it_be(:package_file3_1) { create(:package_file, package: package3, file_name: 'file_name_test') } + + let_it_be(:package_file4_1) { create(:package_file, package: package4, file_name: 'file_name1') } + let_it_be(:package_file4_2) { create(:package_file, package: package4, file_name: 'file_name1') } + + let(:package_files_1) { package1.package_files.installable } + let(:package_files_2) { package2.package_files.installable } + let(:package_files_3) { package3.package_files.installable } + + context 'set to less than the total number of duplicated files' do + before do + # for each package file duplicate, we keep only the most recent one + policy.update!(keep_n_duplicated_package_files: '1') + end + + shared_examples 'keeping the most recent package files' do + let(:response_payload) do + { + counts: { + marked_package_files_total_count: 7, + unique_package_id_and_file_name_total_count: 3 + }, + timeout: false + } + end + + it 'only keeps the most recent package files' do + expect { execute }.to change { ::Packages::PackageFile.installable.count }.by(-7) + + expect(package_files_1).to contain_exactly(package_file1_3, package_file1_6) + expect(package_files_2).to contain_exactly(package_file2_4) + expect(package_files_3).to contain_exactly(package_file3_1) + + expect(execute).to be_success + expect(execute.message).to eq("Packages cleanup policy executed for project #{project.id}") + expect(execute.payload).to eq(response_payload) + end + end + + it_behaves_like 'keeping the most recent package files' + + context 'when the service needs to loop' do + before do + stub_const("#{described_class.name}::DUPLICATED_FILES_BATCH_SIZE", 2) + end + + it_behaves_like 'keeping the most recent package files' do + before do + expect(::Packages::MarkPackageFilesForDestructionService) + .to receive(:new).exactly(3).times.and_call_original + end + end + + context 'when a timeout is hit' do + let(:response_payload) do + { + counts: { + marked_package_files_total_count: 4, + unique_package_id_and_file_name_total_count: 3 + }, + timeout: true + } + end + + let(:service_timeout_response) do + ServiceResponse.error( + message: 'Timeout while marking package files as pending destruction', + payload: { marked_package_files_count: 0 } + ) + end + + before do + mock_service_timeout(on_iteration: 3) + end + + it 'keeps part of the most recent package files' do + expect { execute } + .to change { ::Packages::PackageFile.installable.count }.by(-4) + .and not_change { package_files_2.count } # untouched because of the timeout + .and not_change { package_files_3.count } # untouched because of the timeout + + expect(package_files_1).to contain_exactly(package_file1_3, package_file1_6) + expect(execute).to be_success + expect(execute.message).to eq("Packages cleanup policy executed for project #{project.id}") + expect(execute.payload).to eq(response_payload) + end + + def mock_service_timeout(on_iteration:) + execute_call_count = 1 + expect_next_instances_of(::Packages::MarkPackageFilesForDestructionService, 3) do |service| + expect(service).to receive(:execute).and_wrap_original do |m, *args| + # timeout if we are on the right iteration + if execute_call_count == on_iteration + service_timeout_response + else + execute_call_count += 1 + m.call(*args) + end + end + end + end + end + end + end + + context 'set to more than the total number of duplicated files' do + before do + # using the biggest value for keep_n_duplicated_package_files + policy.update!(keep_n_duplicated_package_files: '50') + end + + it 'keeps all package files' do + expect { execute }.not_to change { ::Packages::PackageFile.installable.count } + end + end + + context 'set to all' do + before do + policy.update!(keep_n_duplicated_package_files: 'all') + end + + it 'skips the policy' do + expect(::Packages::MarkPackageFilesForDestructionService).not_to receive(:new) + expect { execute }.not_to change { ::Packages::PackageFile.installable.count } + end + end + end + end +end diff --git a/spec/services/packages/debian/create_package_file_service_spec.rb b/spec/services/packages/debian/create_package_file_service_spec.rb index 74b97a4f941..c8292b2d5c2 100644 --- a/spec/services/packages/debian/create_package_file_service_spec.rb +++ b/spec/services/packages/debian/create_package_file_service_spec.rb @@ -102,5 +102,13 @@ RSpec.describe Packages::Debian::CreatePackageFileService do expect { subject.execute }.to raise_error(ActiveRecord::RecordInvalid) end end + + context 'FIPS mode enabled', :fips_mode do + let(:file) { nil } + + it 'raises an error' do + expect { subject.execute }.to raise_error(::Packages::FIPS::DisabledError) + end + end end end diff --git a/spec/services/packages/debian/extract_changes_metadata_service_spec.rb b/spec/services/packages/debian/extract_changes_metadata_service_spec.rb index ced846866c2..4765e6c3bd4 100644 --- a/spec/services/packages/debian/extract_changes_metadata_service_spec.rb +++ b/spec/services/packages/debian/extract_changes_metadata_service_spec.rb @@ -13,6 +13,12 @@ RSpec.describe Packages::Debian::ExtractChangesMetadataService do subject { service.execute } + context 'with FIPS mode enabled', :fips_mode do + it 'raises an error' do + expect { subject }.to raise_error(::Packages::FIPS::DisabledError) + end + end + context 'with valid package file' do it 'extract metadata', :aggregate_failures do expected_fields = { 'Architecture' => 'source amd64', 'Binary' => 'libsample0 sample-dev sample-udeb' } diff --git a/spec/services/packages/debian/generate_distribution_service_spec.rb b/spec/services/packages/debian/generate_distribution_service_spec.rb index 53805d03655..fe5fbfbbe1f 100644 --- a/spec/services/packages/debian/generate_distribution_service_spec.rb +++ b/spec/services/packages/debian/generate_distribution_service_spec.rb @@ -15,6 +15,12 @@ RSpec.describe Packages::Debian::GenerateDistributionService do context "for #{container_type}" do include_context 'with Debian distribution', container_type + context 'with FIPS mode enabled', :fips_mode do + it 'raises an error' do + expect { subject }.to raise_error(::Packages::FIPS::DisabledError) + end + end + it_behaves_like 'Generate Debian Distribution and component files' end end diff --git a/spec/services/packages/mark_package_files_for_destruction_service_spec.rb b/spec/services/packages/mark_package_files_for_destruction_service_spec.rb index a836de1f7f6..66534338003 100644 --- a/spec/services/packages/mark_package_files_for_destruction_service_spec.rb +++ b/spec/services/packages/mark_package_files_for_destruction_service_spec.rb @@ -6,9 +6,11 @@ RSpec.describe Packages::MarkPackageFilesForDestructionService, :aggregate_failu let(:service) { described_class.new(package_files) } describe '#execute', :aggregate_failures do - subject { service.execute } + let(:batch_deadline) { nil } - shared_examples 'executing successfully' do + subject { service.execute(batch_deadline: batch_deadline) } + + shared_examples 'executing successfully' do |marked_package_files_count: 0| it 'marks package files for destruction' do expect { subject } .to change { ::Packages::PackageFile.pending_destruction.count }.by(package_files.size) @@ -17,6 +19,7 @@ RSpec.describe Packages::MarkPackageFilesForDestructionService, :aggregate_failu it 'executes successfully' do expect(subject).to be_success expect(subject.message).to eq('Package files are now pending destruction') + expect(subject.payload).to eq(marked_package_files_count: marked_package_files_count) end end @@ -30,13 +33,49 @@ RSpec.describe Packages::MarkPackageFilesForDestructionService, :aggregate_failu let_it_be(:package_file) { create(:package_file) } let_it_be(:package_files) { ::Packages::PackageFile.id_in(package_file.id) } - it_behaves_like 'executing successfully' + it_behaves_like 'executing successfully', marked_package_files_count: 1 end context 'with many package files' do let_it_be(:package_files) { ::Packages::PackageFile.id_in(create_list(:package_file, 3).map(&:id)) } - it_behaves_like 'executing successfully' + it_behaves_like 'executing successfully', marked_package_files_count: 3 + + context 'with a batch deadline' do + let_it_be(:batch_deadline) { 250.seconds.from_now } + + context 'when the deadline is not hit' do + before do + expect(Time.zone).to receive(:now).and_return(batch_deadline - 10.seconds) + end + + it_behaves_like 'executing successfully', marked_package_files_count: 3 + end + + context 'when the deadline is hit' do + it 'does not execute the batch loop' do + expect(Time.zone).to receive(:now).and_return(batch_deadline + 10.seconds) + expect { subject }.to not_change { ::Packages::PackageFile.pending_destruction.count } + expect(subject).to be_error + expect(subject.message).to eq('Timeout while marking package files as pending destruction') + expect(subject.payload).to eq(marked_package_files_count: 0) + end + end + end + + context 'when a batch size is defined' do + let_it_be(:batch_deadline) { 250.seconds.from_now } + + let(:batch_size) { 2 } + + subject { service.execute(batch_deadline: batch_deadline, batch_size: batch_size) } + + before do + expect(Time.zone).to receive(:now).twice.and_call_original + end + + it_behaves_like 'executing successfully', marked_package_files_count: 3 + end end context 'with an error during the update' do diff --git a/spec/services/packages/pypi/create_package_service_spec.rb b/spec/services/packages/pypi/create_package_service_spec.rb index 354ac92b99a..6794ab4d9d6 100644 --- a/spec/services/packages/pypi/create_package_service_spec.rb +++ b/spec/services/packages/pypi/create_package_service_spec.rb @@ -42,6 +42,21 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures do end end + context 'with FIPS mode', :fips_mode do + it 'does not generate file_md5' do + expect { subject }.to change { Packages::Package.pypi.count }.by(1) + + expect(created_package.name).to eq 'foo' + expect(created_package.version).to eq '1.0' + + expect(created_package.pypi_metadatum.required_python).to eq '>=2.7' + expect(created_package.package_files.size).to eq 1 + expect(created_package.package_files.first.file_name).to eq 'foo.tgz' + expect(created_package.package_files.first.file_sha256).to eq sha256 + expect(created_package.package_files.first.file_md5).to be_nil + end + end + context 'without required_python' do before do params.delete(:requires_python) diff --git a/spec/services/pages/delete_service_spec.rb b/spec/services/pages/delete_service_spec.rb index 0c0b2c0431b..29d9a47c72e 100644 --- a/spec/services/pages/delete_service_spec.rb +++ b/spec/services/pages/delete_service_spec.rb @@ -45,7 +45,11 @@ RSpec.describe Pages::DeleteService do end it 'publishes a ProjectDeleted event with project id and namespace id' do - expected_data = { project_id: project.id, namespace_id: project.namespace_id } + expected_data = { + project_id: project.id, + namespace_id: project.namespace_id, + root_namespace_id: project.root_namespace.id + } expect { service.execute }.to publish_event(Pages::PageDeletedEvent).with(expected_data) end diff --git a/spec/services/pod_logs/base_service_spec.rb b/spec/services/pod_logs/base_service_spec.rb deleted file mode 100644 index d2f6300ab65..00000000000 --- a/spec/services/pod_logs/base_service_spec.rb +++ /dev/null @@ -1,147 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ::PodLogs::BaseService do - include KubernetesHelpers - - let_it_be(:cluster) { create(:cluster, :provided_by_gcp, environment_scope: '*') } - - let(:namespace) { 'autodevops-deploy-9-production' } - - let(:pod_name) { 'pod-1' } - let(:pod_name_2) { 'pod-2' } - let(:container_name) { 'container-0' } - let(:params) { {} } - let(:raw_pods) do - [ - { - name: pod_name, - container_names: %w(container-0-0 container-0-1) - }, - { - name: pod_name_2, - container_names: %w(container-1-0 container-1-1) - } - ] - end - - subject { described_class.new(cluster, namespace, params: params) } - - describe '#initialize' do - let(:params) do - { - 'container_name' => container_name, - 'another_param' => 'foo' - } - end - - it 'filters the parameters' do - expect(subject.cluster).to eq(cluster) - expect(subject.namespace).to eq(namespace) - expect(subject.params).to eq({ - 'container_name' => container_name - }) - expect(subject.params.equal?(params)).to be(false) - end - end - - describe '#check_arguments' do - context 'when cluster and namespace are provided' do - it 'returns success' do - result = subject.send(:check_arguments, {}) - - expect(result[:status]).to eq(:success) - end - end - - context 'when cluster is nil' do - let(:cluster) { nil } - - it 'returns an error' do - result = subject.send(:check_arguments, {}) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Cluster does not exist') - end - end - - context 'when namespace is nil' do - let(:namespace) { nil } - - it 'returns an error' do - result = subject.send(:check_arguments, {}) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Namespace is empty') - end - end - - context 'when namespace is empty' do - let(:namespace) { '' } - - it 'returns an error' do - result = subject.send(:check_arguments, {}) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Namespace is empty') - end - end - - context 'when pod_name and container_name are provided' do - let(:params) do - { - 'pod_name' => pod_name, - 'container_name' => container_name - } - end - - it 'returns success' do - result = subject.send(:check_arguments, {}) - - expect(result[:status]).to eq(:success) - expect(result[:pod_name]).to eq(pod_name) - expect(result[:container_name]).to eq(container_name) - end - end - - context 'when pod_name is not a string' do - let(:params) do - { - 'pod_name' => { something_that_is: :not_a_string } - } - end - - it 'returns error' do - result = subject.send(:check_arguments, {}) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Invalid pod_name') - end - end - - context 'when container_name is not a string' do - let(:params) do - { - 'container_name' => { something_that_is: :not_a_string } - } - end - - it 'returns error' do - result = subject.send(:check_arguments, {}) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Invalid container_name') - end - end - end - - describe '#get_pod_names' do - it 'returns success with a list of pods' do - result = subject.send(:get_pod_names, raw_pods: raw_pods) - - expect(result[:status]).to eq(:success) - expect(result[:pods]).to eq([pod_name, pod_name_2]) - end - end -end diff --git a/spec/services/pod_logs/elasticsearch_service_spec.rb b/spec/services/pod_logs/elasticsearch_service_spec.rb deleted file mode 100644 index 1111d9b9307..00000000000 --- a/spec/services/pod_logs/elasticsearch_service_spec.rb +++ /dev/null @@ -1,309 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ::PodLogs::ElasticsearchService do - let_it_be(:cluster) { create(:cluster, :provided_by_gcp, environment_scope: '*') } - - let(:namespace) { 'autodevops-deploy-9-production' } - - let(:pod_name) { 'pod-1' } - let(:container_name) { 'container-1' } - let(:search) { 'foo -bar' } - let(:start_time) { '2019-01-02T12:13:14+02:00' } - let(:end_time) { '2019-01-03T12:13:14+02:00' } - let(:cursor) { '9999934,1572449784442' } - let(:params) { {} } - let(:expected_logs) do - [ - { message: "Log 1", timestamp: "2019-12-13T14:04:22.123456Z" }, - { message: "Log 2", timestamp: "2019-12-13T14:04:23.123456Z" }, - { message: "Log 3", timestamp: "2019-12-13T14:04:24.123456Z" } - ] - end - - let(:raw_pods) do - [ - { - name: pod_name, - container_names: [container_name, "#{container_name}-1"] - } - ] - end - - subject { described_class.new(cluster, namespace, params: params) } - - describe '#get_raw_pods' do - before do - create(:clusters_integrations_elastic_stack, cluster: cluster) - end - - it 'returns success with elasticsearch response' do - allow_any_instance_of(::Clusters::Integrations::ElasticStack) - .to receive(:elasticsearch_client) - .and_return(Elasticsearch::Transport::Client.new) - allow_any_instance_of(::Gitlab::Elasticsearch::Logs::Pods) - .to receive(:pods) - .with(namespace) - .and_return(raw_pods) - - result = subject.send(:get_raw_pods, {}) - - expect(result[:status]).to eq(:success) - expect(result[:raw_pods]).to eq(raw_pods) - end - - it 'returns an error when ES is unreachable' do - allow_any_instance_of(::Clusters::Integrations::ElasticStack) - .to receive(:elasticsearch_client) - .and_return(nil) - - result = subject.send(:get_raw_pods, {}) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Unable to connect to Elasticsearch') - end - - it 'handles server errors from elasticsearch' do - allow_any_instance_of(::Clusters::Integrations::ElasticStack) - .to receive(:elasticsearch_client) - .and_return(Elasticsearch::Transport::Client.new) - allow_any_instance_of(::Gitlab::Elasticsearch::Logs::Pods) - .to receive(:pods) - .and_raise(Elasticsearch::Transport::Transport::Errors::ServiceUnavailable.new) - - result = subject.send(:get_raw_pods, {}) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Elasticsearch returned status code: ServiceUnavailable') - end - end - - describe '#check_times' do - context 'with start and end provided and valid' do - let(:params) do - { - 'start_time' => start_time, - 'end_time' => end_time - } - end - - it 'returns success with times' do - result = subject.send(:check_times, {}) - - expect(result[:status]).to eq(:success) - expect(result[:start_time]).to eq(start_time) - expect(result[:end_time]).to eq(end_time) - end - end - - context 'with start and end not provided' do - let(:params) do - {} - end - - it 'returns success with nothing else' do - result = subject.send(:check_times, {}) - - expect(result.keys.length).to eq(1) - expect(result[:status]).to eq(:success) - end - end - - context 'with start valid and end invalid' do - let(:params) do - { - 'start_time' => start_time, - 'end_time' => 'invalid date' - } - end - - it 'returns error' do - result = subject.send(:check_times, {}) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Invalid start or end time format') - end - end - - context 'with start invalid and end valid' do - let(:params) do - { - 'start_time' => 'invalid date', - 'end_time' => end_time - } - end - - it 'returns error' do - result = subject.send(:check_times, {}) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Invalid start or end time format') - end - end - end - - describe '#check_search' do - context 'with search provided and valid' do - let(:params) do - { - 'search' => search - } - end - - it 'returns success with search' do - result = subject.send(:check_search, {}) - - expect(result[:status]).to eq(:success) - expect(result[:search]).to eq(search) - end - end - - context 'with search provided and invalid' do - let(:params) do - { - 'search' => { term: "foo-bar" } - } - end - - it 'returns error' do - result = subject.send(:check_search, {}) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq("Invalid search parameter") - end - end - - context 'with search not provided' do - let(:params) do - {} - end - - it 'returns success with nothing else' do - result = subject.send(:check_search, {}) - - expect(result.keys.length).to eq(1) - expect(result[:status]).to eq(:success) - end - end - end - - describe '#check_cursor' do - context 'with cursor provided and valid' do - let(:params) do - { - 'cursor' => cursor - } - end - - it 'returns success with cursor' do - result = subject.send(:check_cursor, {}) - - expect(result[:status]).to eq(:success) - expect(result[:cursor]).to eq(cursor) - end - end - - context 'with cursor provided and invalid' do - let(:params) do - { - 'cursor' => { term: "foo-bar" } - } - end - - it 'returns error' do - result = subject.send(:check_cursor, {}) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq("Invalid cursor parameter") - end - end - - context 'with cursor not provided' do - let(:params) do - {} - end - - it 'returns success with nothing else' do - result = subject.send(:check_cursor, {}) - - expect(result.keys.length).to eq(1) - expect(result[:status]).to eq(:success) - end - end - end - - describe '#pod_logs' do - let(:result_arg) do - { - pod_name: pod_name, - container_name: container_name, - search: search, - start_time: start_time, - end_time: end_time, - cursor: cursor - } - end - - let(:expected_cursor) { '9999934,1572449784442' } - - before do - create(:clusters_integrations_elastic_stack, cluster: cluster) - end - - it 'returns the logs' do - allow_any_instance_of(::Clusters::Integrations::ElasticStack) - .to receive(:elasticsearch_client) - .and_return(Elasticsearch::Transport::Client.new) - allow_any_instance_of(::Gitlab::Elasticsearch::Logs::Lines) - .to receive(:pod_logs) - .with(namespace, pod_name: pod_name, container_name: container_name, search: search, start_time: start_time, end_time: end_time, cursor: cursor, chart_above_v2: true) - .and_return({ logs: expected_logs, cursor: expected_cursor }) - - result = subject.send(:pod_logs, result_arg) - - expect(result[:status]).to eq(:success) - expect(result[:logs]).to eq(expected_logs) - expect(result[:cursor]).to eq(expected_cursor) - end - - it 'returns an error when ES is unreachable' do - allow_any_instance_of(::Clusters::Integrations::ElasticStack) - .to receive(:elasticsearch_client) - .and_return(nil) - - result = subject.send(:pod_logs, result_arg) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Unable to connect to Elasticsearch') - end - - it 'handles server errors from elasticsearch' do - allow_any_instance_of(::Clusters::Integrations::ElasticStack) - .to receive(:elasticsearch_client) - .and_return(Elasticsearch::Transport::Client.new) - allow_any_instance_of(::Gitlab::Elasticsearch::Logs::Lines) - .to receive(:pod_logs) - .and_raise(Elasticsearch::Transport::Transport::Errors::ServiceUnavailable.new) - - result = subject.send(:pod_logs, result_arg) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Elasticsearch returned status code: ServiceUnavailable') - end - - it 'handles cursor errors from elasticsearch' do - allow_any_instance_of(::Clusters::Integrations::ElasticStack) - .to receive(:elasticsearch_client) - .and_return(Elasticsearch::Transport::Client.new) - allow_any_instance_of(::Gitlab::Elasticsearch::Logs::Lines) - .to receive(:pod_logs) - .and_raise(::Gitlab::Elasticsearch::Logs::Lines::InvalidCursor.new) - - result = subject.send(:pod_logs, result_arg) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Invalid cursor value provided') - end - end -end diff --git a/spec/services/pod_logs/kubernetes_service_spec.rb b/spec/services/pod_logs/kubernetes_service_spec.rb deleted file mode 100644 index c06a87830ca..00000000000 --- a/spec/services/pod_logs/kubernetes_service_spec.rb +++ /dev/null @@ -1,310 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ::PodLogs::KubernetesService do - include KubernetesHelpers - - let_it_be(:cluster) { create(:cluster, :provided_by_gcp, environment_scope: '*') } - - let(:namespace) { 'autodevops-deploy-9-production' } - - let(:pod_name) { 'pod-1' } - let(:pod_name_2) { 'pod-2' } - let(:container_name) { 'container-0' } - let(:container_name_2) { 'foo-0' } - let(:params) { {} } - - let(:raw_logs) do - "2019-12-13T14:04:22.123456Z Log 1\n2019-12-13T14:04:23.123456Z Log 2\n" \ - "2019-12-13T14:04:24.123456Z Log 3" - end - - let(:raw_pods) do - [ - { - name: pod_name, - container_names: [container_name, "#{container_name}-1"] - }, - { - name: pod_name_2, - container_names: [container_name_2, "#{container_name_2}-1"] - } - ] - end - - subject { described_class.new(cluster, namespace, params: params) } - - describe '#get_raw_pods' do - let(:service) { create(:cluster_platform_kubernetes, :configured) } - - it 'returns success with passthrough k8s response' do - stub_kubeclient_pods(namespace) - - result = subject.send(:get_raw_pods, {}) - - expect(result[:status]).to eq(:success) - expect(result[:raw_pods]).to eq([{ - name: 'kube-pod', - container_names: %w(container-0 container-0-1) - }]) - end - end - - describe '#pod_logs' do - let(:result_arg) do - { - pod_name: pod_name, - container_name: container_name - } - end - - let(:expected_logs) { raw_logs } - let(:service) { create(:cluster_platform_kubernetes, :configured) } - - it 'returns the logs' do - stub_kubeclient_logs(pod_name, namespace, container: container_name) - - result = subject.send(:pod_logs, result_arg) - - expect(result[:status]).to eq(:success) - expect(result[:logs]).to eq(expected_logs) - end - - it 'handles Not Found errors from k8s' do - allow_any_instance_of(Gitlab::Kubernetes::KubeClient) - .to receive(:get_pod_log) - .with(any_args) - .and_raise(Kubeclient::ResourceNotFoundError.new(404, 'Not Found', {})) - - result = subject.send(:pod_logs, result_arg) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Pod not found') - end - - it 'handles HTTP errors from k8s' do - allow_any_instance_of(Gitlab::Kubernetes::KubeClient) - .to receive(:get_pod_log) - .with(any_args) - .and_raise(Kubeclient::HttpError.new(500, 'Error', {})) - - result = subject.send(:pod_logs, result_arg) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Kubernetes API returned status code: 500') - end - end - - describe '#encode_logs_to_utf8', :aggregate_failures do - let(:service) { create(:cluster_platform_kubernetes, :configured) } - let(:expected_logs) { '2019-12-13T14:04:22.123456Z ✔ Started logging errors to Sentry' } - let(:raw_logs) { expected_logs.dup.force_encoding(Encoding::ASCII_8BIT) } - let(:result) { subject.send(:encode_logs_to_utf8, result_arg) } - - let(:result_arg) do - { - pod_name: pod_name, - container_name: container_name, - logs: raw_logs - } - end - - it 'converts logs to utf-8' do - expect(result[:status]).to eq(:success) - expect(result[:logs]).to eq(expected_logs) - end - - it 'returns error if output of encoding helper is blank' do - allow(Gitlab::EncodingHelper).to receive(:encode_utf8).and_return('') - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Unable to convert Kubernetes logs encoding to UTF-8') - end - - it 'returns error if output of encoding helper is nil' do - allow(Gitlab::EncodingHelper).to receive(:encode_utf8).and_return(nil) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Unable to convert Kubernetes logs encoding to UTF-8') - end - - it 'returns error if output of encoding helper is not UTF-8' do - allow(Gitlab::EncodingHelper).to receive(:encode_utf8) - .and_return(expected_logs.encode(Encoding::UTF_16BE)) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Unable to convert Kubernetes logs encoding to UTF-8') - end - - context 'when logs are nil' do - let(:raw_logs) { nil } - let(:expected_logs) { nil } - - it 'returns nil' do - expect(result[:status]).to eq(:success) - expect(result[:logs]).to eq(expected_logs) - end - end - - context 'when logs are blank' do - let(:raw_logs) { (+'').force_encoding(Encoding::ASCII_8BIT) } - let(:expected_logs) { '' } - - it 'returns blank string' do - expect(result[:status]).to eq(:success) - expect(result[:logs]).to eq(expected_logs) - end - end - - context 'when logs are already in utf-8' do - let(:raw_logs) { expected_logs } - - it 'does not fail' do - expect(result[:status]).to eq(:success) - expect(result[:logs]).to eq(expected_logs) - end - end - end - - describe '#split_logs' do - let(:service) { create(:cluster_platform_kubernetes, :configured) } - - let(:expected_logs) do - [ - { message: "Log 1", pod: 'pod-1', timestamp: "2019-12-13T14:04:22.123456Z" }, - { message: "Log 2", pod: 'pod-1', timestamp: "2019-12-13T14:04:23.123456Z" }, - { message: "Log 3", pod: 'pod-1', timestamp: "2019-12-13T14:04:24.123456Z" } - ] - end - - let(:result_arg) do - { - pod_name: pod_name, - container_name: container_name, - logs: raw_logs - } - end - - it 'returns the logs' do - result = subject.send(:split_logs, result_arg) - - aggregate_failures do - expect(result[:status]).to eq(:success) - expect(result[:logs]).to eq(expected_logs) - end - end - end - - describe '#check_pod_name' do - it 'returns success if pod_name was specified' do - result = subject.send(:check_pod_name, pod_name: pod_name, pods: [pod_name]) - - expect(result[:status]).to eq(:success) - expect(result[:pod_name]).to eq(pod_name) - end - - it 'returns success if pod_name was not specified but there are pods' do - result = subject.send(:check_pod_name, pod_name: nil, pods: [pod_name]) - - expect(result[:status]).to eq(:success) - expect(result[:pod_name]).to eq(pod_name) - end - - it 'returns error if pod_name was not specified and there are no pods' do - result = subject.send(:check_pod_name, pod_name: nil, pods: []) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('No pods available') - end - - it 'returns error if pod_name was specified but does not exist' do - result = subject.send(:check_pod_name, pod_name: 'another-pod', pods: [pod_name]) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Pod does not exist') - end - - it 'returns error if pod_name is too long' do - result = subject.send(:check_pod_name, pod_name: "a very long string." * 15, pods: [pod_name]) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('pod_name cannot be larger than 253 chars') - end - - it 'returns error if pod_name is in invalid format' do - result = subject.send(:check_pod_name, pod_name: "Invalid_pod_name", pods: [pod_name]) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('pod_name can contain only lowercase letters, digits, \'-\', and \'.\' and must start and end with an alphanumeric character') - end - end - - describe '#check_container_name' do - it 'returns success if container_name was specified' do - result = subject.send(:check_container_name, - container_name: container_name, - pod_name: pod_name, - raw_pods: raw_pods - ) - - expect(result[:status]).to eq(:success) - expect(result[:container_name]).to eq(container_name) - end - - it 'returns success if container_name was not specified and there are containers' do - result = subject.send(:check_container_name, - pod_name: pod_name_2, - raw_pods: raw_pods - ) - - expect(result[:status]).to eq(:success) - expect(result[:container_name]).to eq(container_name_2) - end - - it 'returns error if container_name was not specified and there are no containers on the pod' do - raw_pods.first[:container_names] = [] - - result = subject.send(:check_container_name, - pod_name: pod_name, - raw_pods: raw_pods - ) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('No containers available') - end - - it 'returns error if container_name was specified but does not exist' do - result = subject.send(:check_container_name, - container_name: 'foo', - pod_name: pod_name, - raw_pods: raw_pods - ) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Container does not exist') - end - - it 'returns error if container_name is too long' do - result = subject.send(:check_container_name, - container_name: "a very long string." * 15, - pod_name: pod_name, - raw_pods: raw_pods - ) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('container_name cannot be larger than 253 chars') - end - - it 'returns error if container_name is in invalid format' do - result = subject.send(:check_container_name, - container_name: "Invalid_container_name", - pod_name: pod_name, - raw_pods: raw_pods - ) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('container_name can contain only lowercase letters, digits, \'-\', and \'.\' and must start and end with an alphanumeric character') - end - end -end diff --git a/spec/services/preview_markdown_service_spec.rb b/spec/services/preview_markdown_service_spec.rb index 53f8f5b7253..fe1ab6b1d58 100644 --- a/spec/services/preview_markdown_service_spec.rb +++ b/spec/services/preview_markdown_service_spec.rb @@ -172,4 +172,24 @@ RSpec.describe PreviewMarkdownService do expect(result[:commands]).to eq 'Tags this commit to v1.2.3 with "Stable release".' end end + + context 'note with multiple quick actions' do + let(:issue) { create(:issue, project: project) } + let(:params) do + { + text: "/confidential\n/due 2001-12-31\n/estimate 2y\n/assign #{user.to_reference}", + target_type: 'Issue', + target_id: issue.id + } + end + + let(:service) { described_class.new(project, user, params) } + + it 'renders quick actions on multiple lines' do + result = service.execute + + expect(result[:commands]).to eq "Makes this issue confidential.<br>Sets the due date to Dec 31, 2001.<br>" \ + "Sets time estimate to 2y.<br>Assigns #{user.to_reference}." + end + end end diff --git a/spec/services/projects/after_rename_service_spec.rb b/spec/services/projects/after_rename_service_spec.rb index a9329f092fa..9dc15131bc5 100644 --- a/spec/services/projects/after_rename_service_spec.rb +++ b/spec/services/projects/after_rename_service_spec.rb @@ -59,22 +59,6 @@ RSpec.describe Projects::AfterRenameService do end end - context 'gitlab pages' do - before do - allow(project_storage).to receive(:rename_repo) { true } - end - - context 'when the project does not have pages deployed' do - it 'does nothing with the pages directory' do - allow(project).to receive(:pages_deployed?).and_return(false) - - expect(PagesTransferWorker).not_to receive(:perform_async) - - service_execute - end - end - end - context 'attachments' do before do expect(project_storage).to receive(:rename_repo) { true } @@ -204,6 +188,22 @@ RSpec.describe Projects::AfterRenameService do ) end end + + context 'EventStore' do + let(:project) { create(:project, :repository, skip_disk_validation: true) } + + it 'publishes a ProjectPathChangedEvent' do + expect { service_execute } + .to publish_event(Projects::ProjectPathChangedEvent) + .with( + project_id: project.id, + namespace_id: project.namespace_id, + root_namespace_id: project.root_namespace.id, + old_path: full_path_before_rename, + new_path: full_path_after_rename + ) + end + end end def service_execute diff --git a/spec/services/projects/blame_service_spec.rb b/spec/services/projects/blame_service_spec.rb index 40b2bc869dc..54c4315d242 100644 --- a/spec/services/projects/blame_service_spec.rb +++ b/spec/services/projects/blame_service_spec.rb @@ -98,31 +98,21 @@ RSpec.describe Projects::BlameService, :aggregate_failures do end end - describe 'Current page' do - subject { service.pagination.current_page } - - context 'with page = 1' do - let(:page) { 1 } - - it { is_expected.to eq(1) } - end - - context 'with page = 2' do - let(:page) { 2 } - - it { is_expected.to eq(2) } + describe 'Pagination attributes' do + using RSpec::Parameterized::TableSyntax + + where(:page, :current_page, :total_pages) do + 1 | 1 | 2 + 2 | 2 | 2 + 3 | 1 | 2 # Overlimit + 0 | 1 | 2 # Incorrect end - context 'with page = 3 (overlimit)' do - let(:page) { 3 } - - it { is_expected.to eq(1) } - end - - context 'with page = 0 (incorrect)' do - let(:page) { 0 } - - it { is_expected.to eq(1) } + with_them do + it 'returns the correct pagination attributes' do + expect(subject.current_page).to eq(current_page) + expect(subject.total_pages).to eq(total_pages) + end end end end diff --git a/spec/services/projects/create_from_template_service_spec.rb b/spec/services/projects/create_from_template_service_spec.rb index 7e23daabcd3..fba6225b87a 100644 --- a/spec/services/projects/create_from_template_service_spec.rb +++ b/spec/services/projects/create_from_template_service_spec.rb @@ -49,7 +49,7 @@ RSpec.describe Projects::CreateFromTemplateService do end it 'is not scheduled' do - expect(project.import_scheduled?).to be_nil + expect(project.import_scheduled?).to be(false) end it 'repository is empty' do diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index cd1e629e1d2..59dee209ff9 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -152,6 +152,20 @@ RSpec.describe Projects::CreateService, '#execute' do create_project(user, opts) end + + it 'publishes a ProjectCreatedEvent' do + group = create(:group, :nested).tap do |group| + group.add_owner(user) + end + + expect { create_project(user, name: 'Project', path: 'project', namespace_id: group.id) } + .to publish_event(Projects::ProjectCreatedEvent) + .with( + project_id: kind_of(Numeric), + namespace_id: group.id, + root_namespace_id: group.parent_id + ) + end end context "admin creates project with other user's namespace_id" do @@ -543,15 +557,15 @@ RSpec.describe Projects::CreateService, '#execute' do end context 'with legacy storage' do - let(:fake_repo_path) { File.join(TestEnv.repos_path, user.namespace.full_path, 'existing.git') } + let(:raw_fake_repo) { Gitlab::Git::Repository.new('default', File.join(user.namespace.full_path, 'existing.git'), nil, nil) } before do stub_application_setting(hashed_storage_enabled: false) - TestEnv.create_bare_repository(fake_repo_path) + raw_fake_repo.create_repository end after do - FileUtils.rm_rf(fake_repo_path) + raw_fake_repo.remove end it 'does not allow to create a project when path matches existing repository on disk' do @@ -578,15 +592,15 @@ RSpec.describe Projects::CreateService, '#execute' do context 'with hashed storage' do let(:hash) { '6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b' } let(:hashed_path) { '@hashed/6b/86/6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b' } - let(:fake_repo_path) { File.join(TestEnv.repos_path, "#{hashed_path}.git") } + let(:raw_fake_repo) { Gitlab::Git::Repository.new('default', "#{hashed_path}.git", nil, nil) } before do allow(Digest::SHA2).to receive(:hexdigest) { hash } - TestEnv.create_bare_repository(fake_repo_path) + raw_fake_repo.create_repository end after do - FileUtils.rm_rf(fake_repo_path) + raw_fake_repo.remove end it 'does not allow to create a project when path matches existing repository on disk' do diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index c00438199fd..955384e518c 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -25,8 +25,12 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi expect(project.gitlab_shell.repository_exists?(project.repository_storage, 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 } + it 'publishes a ProjectDeletedEvent' do + expected_data = { + project_id: project.id, + namespace_id: project.namespace_id, + root_namespace_id: project.root_namespace.id + } expect { destroy_project(project, user, {}) }.to publish_event(Projects::ProjectDeletedEvent).with(expected_data) end @@ -119,14 +123,15 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi 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) + [MergeRequestDiffCommit, MergeRequestDiffFile].each do |model| + it "deletes #{model} records of the merge request" 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 + records_count = model.where(merge_request_diff_id: merge_request_diffs.first.id).count - expect { destroy_project(project, user, {}) } - .to change { MergeRequestDiffCommit.count }.by(-mrdc_count) + expect { destroy_project(project, user, {}) }.to change { model.count }.by(-records_count) + end end end @@ -220,7 +225,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi context 'when flushing caches fail due to Redis' do before do new_user = create(:user) - project.team.add_user(new_user, Gitlab::Access::DEVELOPER) + project.team.add_member(new_user, Gitlab::Access::DEVELOPER) allow_any_instance_of(described_class).to receive(:flush_caches).and_raise(::Redis::CannotConnectError) end @@ -454,10 +459,10 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi it 'deletes webhooks and logs related to project' do expect_next_instance_of(WebHooks::DestroyService, user) do |instance| - expect(instance).to receive(:sync_destroy).with(web_hook1).and_call_original + expect(instance).to receive(:execute).with(web_hook1).and_call_original end expect_next_instance_of(WebHooks::DestroyService, user) do |instance| - expect(instance).to receive(:sync_destroy).with(web_hook2).and_call_original + expect(instance).to receive(:execute).with(web_hook2).and_call_original end expect do @@ -468,7 +473,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi context 'when an error is raised deleting webhooks' do before do allow_next_instance_of(WebHooks::DestroyService) do |instance| - allow(instance).to receive(:sync_destroy).and_return(message: 'foo', status: :error) + allow(instance).to receive(:execute).and_return(message: 'foo', status: :error) end end diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index ce30a20edf4..48756cf774b 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -32,14 +32,14 @@ RSpec.describe Projects::ForkService do external_authorization_classification_label: 'classification-label') @to_user = create(:user) @to_namespace = @to_user.namespace - @from_project.add_user(@to_user, :developer) + @from_project.add_member(@to_user, :developer) end context 'fork project' do context 'when forker is a guest' do before do @guest = create(:user) - @from_project.add_user(@guest, :guest) + @from_project.add_member(@guest, :guest) end subject { fork_project(@from_project, @guest, using_service: true) } @@ -68,6 +68,9 @@ RSpec.describe Projects::ForkService do it { expect(to_project.avatar.file).to be_exists } it { expect(to_project.ci_config_path).to eq(@from_project.ci_config_path) } it { expect(to_project.external_authorization_classification_label).to eq(@from_project.external_authorization_classification_label) } + it { expect(to_project.suggestion_commit_message).to eq(@from_project.suggestion_commit_message) } + it { expect(to_project.merge_commit_template).to eq(@from_project.merge_commit_template) } + it { expect(to_project.squash_commit_template).to eq(@from_project.squash_commit_template) } # This test is here because we had a bug where the from-project lost its # avatar after being forked. @@ -156,16 +159,16 @@ RSpec.describe Projects::ForkService do end context 'repository in legacy storage already exists' do - let(:fake_repo_path) { File.join(TestEnv.repos_path, @to_user.namespace.full_path, "#{@from_project.path}.git") } + let(:raw_fake_repo) { Gitlab::Git::Repository.new('default', File.join(@to_user.namespace.full_path, "#{@from_project.path}.git"), nil, nil) } let(:params) { { namespace: @to_user.namespace, using_service: true } } before do stub_application_setting(hashed_storage_enabled: false) - TestEnv.create_bare_repository(fake_repo_path) + raw_fake_repo.create_repository end after do - FileUtils.rm_rf(fake_repo_path) + raw_fake_repo.remove end subject { fork_project(@from_project, @to_user, params) } @@ -261,10 +264,10 @@ RSpec.describe Projects::ForkService do description: 'Wow, such a cool project!', ci_config_path: 'debian/salsa-ci.yml') @group = create(:group) - @group.add_user(@group_owner, GroupMember::OWNER) - @group.add_user(@developer, GroupMember::DEVELOPER) - @project.add_user(@developer, :developer) - @project.add_user(@group_owner, :developer) + @group.add_member(@group_owner, GroupMember::OWNER) + @group.add_member(@developer, GroupMember::DEVELOPER) + @project.add_member(@developer, :developer) + @project.add_member(@group_owner, :developer) @opts = { namespace: @group, using_service: true } end diff --git a/spec/services/projects/group_links/update_service_spec.rb b/spec/services/projects/group_links/update_service_spec.rb index ff1618c3bbe..20616890ebd 100644 --- a/spec/services/projects/group_links/update_service_spec.rb +++ b/spec/services/projects/group_links/update_service_spec.rb @@ -15,7 +15,7 @@ RSpec.describe Projects::GroupLinks::UpdateService, '#execute' do expires_at: expiry_date } end - subject { described_class.new(link).execute(group_link_params) } + subject { described_class.new(link, user).execute(group_link_params) } before do group.add_developer(user) diff --git a/spec/services/projects/move_deploy_keys_projects_service_spec.rb b/spec/services/projects/move_deploy_keys_projects_service_spec.rb index bd93b80f712..59674a3a4ef 100644 --- a/spec/services/projects/move_deploy_keys_projects_service_spec.rb +++ b/spec/services/projects/move_deploy_keys_projects_service_spec.rb @@ -56,5 +56,22 @@ RSpec.describe Projects::MoveDeployKeysProjectsService do expect(project_with_deploy_keys.deploy_keys_projects.count).not_to eq 0 end end + + context 'when SHA256 fingerprint is missing' do + before do + create(:deploy_keys_project, project: target_project) + DeployKey.all.update_all(fingerprint_sha256: nil) + end + + it 'moves the user\'s deploy keys from one project to another' do + combined_keys = project_with_deploy_keys.deploy_keys + target_project.deploy_keys + + subject.execute(project_with_deploy_keys) + + expect(project_with_deploy_keys.deploy_keys.reload).to be_empty + expect(target_project.deploy_keys.reload).to match_array(combined_keys) + expect(DeployKey.all.select(:fingerprint_sha256)).to all(be_present) + end + end end end diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb index 3ee867ba6f2..bee91c358ce 100644 --- a/spec/services/projects/operations/update_service_spec.rb +++ b/spec/services/projects/operations/update_service_spec.rb @@ -462,93 +462,5 @@ RSpec.describe Projects::Operations::UpdateService do end end end - - context 'tracing setting' do - context 'with valid params' do - let(:params) do - { - tracing_setting_attributes: { - external_url: 'http://some-url.com' - } - } - end - - context 'with an existing setting' do - before do - create(:project_tracing_setting, project: project) - end - - shared_examples 'setting deletion' do - let!(:original_params) { params.deep_dup } - - it 'deletes the setting' do - expect(result[:status]).to eq(:success) - expect(project.reload.tracing_setting).to be_nil - end - - it 'does not modify original params' do - subject.execute - - expect(params).to eq(original_params) - end - end - - it 'updates the setting' do - expect(project.tracing_setting).not_to be_nil - - expect(result[:status]).to eq(:success) - expect(project.reload.tracing_setting.external_url) - .to eq('http://some-url.com') - end - - context 'with missing external_url' do - before do - params[:tracing_setting_attributes].delete(:external_url) - end - - it_behaves_like 'setting deletion' - end - - context 'with empty external_url' do - before do - params[:tracing_setting_attributes][:external_url] = '' - end - - it_behaves_like 'setting deletion' - end - - context 'with blank external_url' do - before do - params[:tracing_setting_attributes][:external_url] = ' ' - end - - it_behaves_like 'setting deletion' - end - end - - context 'without an existing setting' do - it 'creates a setting' do - expect(project.tracing_setting).to be_nil - - expect(result[:status]).to eq(:success) - expect(project.reload.tracing_setting.external_url) - .to eq('http://some-url.com') - end - end - end - - context 'with empty params' do - let(:params) { {} } - - let!(:tracing_setting) do - create(:project_tracing_setting, project: project) - end - - it 'does nothing' do - expect(result[:status]).to eq(:success) - expect(project.reload.tracing_setting).to eq(tracing_setting) - end - end - end end end diff --git a/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/spec/services/projects/prometheus/alerts/notify_service_spec.rb index 0d0bb317df2..6f760e6dbfa 100644 --- a/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -228,12 +228,14 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do context 'when payload exceeds max amount of processable alerts' do # We are defining 2 alerts in payload_raw above let(:max_alerts) { 1 } + let(:fingerprint) { prometheus_alert_payload_fingerprint(alert_resolved) } before do stub_const("#{described_class}::PROCESS_MAX_ALERTS", max_alerts) create(:prometheus_integration, project: project) create(:project_alerting_setting, project: project, token: token) + create(:alert_management_alert, project: project, fingerprint: fingerprint) allow(Gitlab::AppLogger).to receive(:warn) end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index bebe80b710b..ecf9f92d74f 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -372,16 +372,16 @@ RSpec.describe Projects::TransferService do end context 'namespace which contains orphan repository with same projects path name' do - let(:fake_repo_path) { File.join(TestEnv.repos_path, group.full_path, "#{project.path}.git") } + let(:raw_fake_repo) { Gitlab::Git::Repository.new('default', File.join(group.full_path, "#{project.path}.git"), nil, nil) } before do group.add_owner(user) - TestEnv.create_bare_repository(fake_repo_path) + raw_fake_repo.create_repository end after do - FileUtils.rm_rf(fake_repo_path) + raw_fake_repo.remove end it 'does not allow the project transfer' do @@ -715,20 +715,6 @@ RSpec.describe Projects::TransferService do end end - context 'moving pages' do - let_it_be(:project) { create(:project, namespace: user.namespace) } - - before do - group.add_owner(user) - end - - it 'does not schedule a job when no pages are deployed' do - expect(PagesTransferWorker).not_to receive(:perform_async) - - execute_transfer - end - end - context 'handling issue contacts' do let_it_be(:root_group) { create(:group) } diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index cbbed82aa0b..24b5e35e422 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -43,6 +43,16 @@ RSpec.describe Projects::UpdatePagesService do expect(project.pages_deployed?).to be_truthy end + it 'publishes a PageDeployedEvent event with project id and namespace id' do + expected_data = { + project_id: project.id, + namespace_id: project.namespace_id, + root_namespace_id: project.root_namespace.id + } + + expect { subject.execute }.to publish_event(Pages::PageDeployedEvent).with(expected_data) + end + it 'creates pages_deployment and saves it in the metadata' do expect do expect(execute).to eq(:success) @@ -161,16 +171,6 @@ RSpec.describe Projects::UpdatePagesService do end end - shared_examples 'fails with outdated reference message' do - it 'fails' do - expect(execute).not_to eq(:success) - expect(project.reload.pages_metadatum).not_to be_deployed - - expect(deploy_status).to be_failed - expect(deploy_status.description).to eq('build SHA is outdated for this ref') - end - end - shared_examples 'successfully deploys' do it 'succeeds' do expect do @@ -202,27 +202,29 @@ RSpec.describe Projects::UpdatePagesService do project.update_pages_deployment!(new_deployment) end - include_examples 'fails with outdated reference message' + it 'fails with outdated reference message' do + expect(execute).to eq(:error) + expect(project.reload.pages_metadatum).not_to be_deployed + + expect(deploy_status).to be_failed + expect(deploy_status.description).to eq('build SHA is outdated for this ref') + end end end - context 'when uploaded deployment size is wrong' do - it 'raises an error' do - allow_next_instance_of(PagesDeployment) do |deployment| - allow(deployment) - .to receive(:size) - .and_return(file.size + 1) - end + it 'fails when uploaded deployment size is wrong' do + allow_next_instance_of(PagesDeployment) do |deployment| + allow(deployment) + .to receive(:size) + .and_return(file.size + 1) + end - expect do - expect(execute).not_to eq(:success) + expect(execute).not_to eq(:success) - expect(GenericCommitStatus.last.description).to eq("Error: The uploaded artifact size does not match the expected value.") - project.pages_metadatum.reload - expect(project.pages_metadatum).not_to be_deployed - expect(project.pages_metadatum.pages_deployment).to be_ni - end.to raise_error(Projects::UpdatePagesService::WrongUploadedDeploymentSizeError) - end + expect(GenericCommitStatus.last.description).to eq('The uploaded artifact size does not match the expected value') + project.pages_metadatum.reload + expect(project.pages_metadatum).not_to be_deployed + expect(project.pages_metadatum.pages_deployment).to be_nil end end end diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 7b5bf1db030..f019434a4fe 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -289,6 +289,42 @@ RSpec.describe Projects::UpdateService do end end + context 'when changing operations feature visibility' do + let(:feature_params) { { operations_access_level: ProjectFeature::DISABLED } } + + it 'does not sync the changes to the related fields' do + result = update_project(project, user, project_feature_attributes: feature_params) + + expect(result).to eq({ status: :success }) + feature = project.project_feature + + expect(feature.operations_access_level).to eq(ProjectFeature::DISABLED) + expect(feature.monitor_access_level).not_to eq(ProjectFeature::DISABLED) + expect(feature.infrastructure_access_level).not_to eq(ProjectFeature::DISABLED) + expect(feature.feature_flags_access_level).not_to eq(ProjectFeature::DISABLED) + expect(feature.environments_access_level).not_to eq(ProjectFeature::DISABLED) + end + + context 'when split_operations_visibility_permissions feature is disabled' do + before do + stub_feature_flags(split_operations_visibility_permissions: false) + end + + it 'syncs the changes to the related fields' do + result = update_project(project, user, project_feature_attributes: feature_params) + + expect(result).to eq({ status: :success }) + feature = project.project_feature + + expect(feature.operations_access_level).to eq(ProjectFeature::DISABLED) + expect(feature.monitor_access_level).to eq(ProjectFeature::DISABLED) + expect(feature.infrastructure_access_level).to eq(ProjectFeature::DISABLED) + expect(feature.feature_flags_access_level).to eq(ProjectFeature::DISABLED) + expect(feature.environments_access_level).to eq(ProjectFeature::DISABLED) + end + end + end + context 'when updating a project that contains container images' do before do stub_container_registry_config(enabled: true) @@ -312,17 +348,17 @@ RSpec.describe Projects::UpdateService do end context 'when renaming a project' do - let(:fake_repo_path) { File.join(TestEnv.repos_path, user.namespace.full_path, 'existing.git') } + let(:raw_fake_repo) { Gitlab::Git::Repository.new('default', File.join(user.namespace.full_path, 'existing.git'), nil, nil) } context 'with legacy storage' do let(:project) { create(:project, :legacy_storage, :repository, creator: user, namespace: user.namespace) } before do - TestEnv.create_bare_repository(fake_repo_path) + raw_fake_repo.create_repository end after do - FileUtils.rm_rf(fake_repo_path) + raw_fake_repo.remove end it 'does not allow renaming when new path matches existing repository on disk' do @@ -388,7 +424,7 @@ RSpec.describe Projects::UpdateService do it 'does not update when not project owner' do maintainer = create(:user) - project.add_user(maintainer, :maintainer) + project.add_member(maintainer, :maintainer) expect { update_project(project, maintainer, emails_disabled: true) } .not_to change { project.emails_disabled } diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index f7ed6006099..3f11eaa7e93 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe QuickActions::InterpretService do + include AfterNextHelpers + let_it_be(:group) { create(:group, :crm_enabled) } let_it_be(:public_project) { create(:project, :public, group: group) } let_it_be(:repository_project) { create(:project, :repository) } @@ -17,8 +19,9 @@ RSpec.describe QuickActions::InterpretService do let(:milestone) { create(:milestone, project: project, title: '9.10') } let(:commit) { create(:commit, project: project) } + let(:current_user) { developer } - subject(:service) { described_class.new(project, developer) } + subject(:service) { described_class.new(project, current_user) } before_all do public_project.add_developer(developer) @@ -682,6 +685,58 @@ RSpec.describe QuickActions::InterpretService do end shared_examples 'assign command' do + it 'assigns to me' do + cmd = '/assign me' + + _, updates, _ = service.execute(cmd, issuable) + + expect(updates).to eq(assignee_ids: [current_user.id]) + end + + it 'does not assign to group members' do + grp = create(:group) + grp.add_developer(developer) + grp.add_developer(developer2) + + cmd = "/assign #{grp.to_reference}" + + _, updates, message = service.execute(cmd, issuable) + + expect(updates).to be_blank + expect(message).to include('Failed to find users') + end + + context 'when there are too many references' do + before do + stub_const('Gitlab::QuickActions::UsersExtractor::MAX_QUICK_ACTION_USERS', 2) + end + + it 'says what went wrong' do + cmd = '/assign her and you, me and them' + + _, updates, message = service.execute(cmd, issuable) + + expect(updates).to be_blank + expect(message).to include('Too many references. Quick actions are limited to at most 2 user references') + end + end + + context 'when the user extractor raises an uninticipated error' do + before do + allow_next(Gitlab::QuickActions::UsersExtractor) + .to receive(:execute).and_raise(Gitlab::QuickActions::UsersExtractor::Error) + end + + it 'tracks the exception in dev, and reports a generic message in production' do + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).twice + + _, updates, message = service.execute('/assign some text', issuable) + + expect(updates).to be_blank + expect(message).to include('Something went wrong') + end + end + it 'assigns to users with escaped underscores' do user = create(:user) base = user.username diff --git a/spec/services/repositories/changelog_service_spec.rb b/spec/services/repositories/changelog_service_spec.rb index 82546ae810b..3615747e191 100644 --- a/spec/services/repositories/changelog_service_spec.rb +++ b/spec/services/repositories/changelog_service_spec.rb @@ -194,6 +194,25 @@ RSpec.describe Repositories::ChangelogService do end end end + + context 'with specified changelog config file path' do + it 'return specified changelog content' do + config = Gitlab::Changelog::Config.from_hash(project, { 'template' => 'specified_changelog_content' }, creator) + + allow(Gitlab::Changelog::Config) + .to receive(:from_git) + .with(project, creator, 'specified_changelog_config.yml') + .and_return(config) + + described_class + .new(project, creator, version: '1.0.0', from: sha1, config_file: 'specified_changelog_config.yml') + .execute(commit_to_changelog: commit_to_changelog) + + changelog = project.repository.blob_at('master', 'CHANGELOG.md')&.data + + expect(changelog).to include('specified_changelog_content') + end + end end describe '#start_of_commit_range' do diff --git a/spec/services/search_service_spec.rb b/spec/services/search_service_spec.rb index d7a36ff370e..5edea13afa4 100644 --- a/spec/services/search_service_spec.rb +++ b/spec/services/search_service_spec.rb @@ -511,7 +511,7 @@ RSpec.describe SearchService do end context 'with :with_api_entity_associations' do - it_behaves_like "redaction limits N+1 queries", limit: 13 + it_behaves_like "redaction limits N+1 queries", limit: 14 end end @@ -599,25 +599,13 @@ RSpec.describe SearchService do let(:search_service) { double(:search_service) } before do - stub_feature_flags(prevent_abusive_searches: should_detect_abuse) expect(Gitlab::Search::Params).to receive(:new) - .with(raw_params, detect_abuse: should_detect_abuse).and_call_original + .with(raw_params, detect_abuse: true).and_call_original allow(subject).to receive(:search_service).and_return search_service end - context 'when abusive search but prevent_abusive_searches FF is disabled' do - let(:should_detect_abuse) { false } - let(:scope) { '1;drop%20table' } - - it 'executes search even if params are abusive' do - expect(search_service).to receive(:execute) - subject.search_results - end - end - context 'a search is abusive' do - let(:should_detect_abuse) { true } let(:scope) { '1;drop%20table' } it 'does NOT execute search service' do @@ -627,7 +615,6 @@ RSpec.describe SearchService do end context 'a search is NOT abusive' do - let(:should_detect_abuse) { true } let(:scope) { 'issues' } it 'executes search service' do 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 7a8bd1910fe..b863b2a46b0 100644 --- a/spec/services/service_ping/submit_service_ping_service_spec.rb +++ b/spec/services/service_ping/submit_service_ping_service_spec.rb @@ -377,12 +377,15 @@ RSpec.describe ServicePing::SubmitService do stub_database_flavor_check stub_application_setting(usage_ping_enabled: true) stub_response(body: with_conv_index_params) + allow_next_instance_of(ServicePing::BuildPayload) do |service| + allow(service).to receive(:execute).and_return(payload) + end end - context 'with feature flag measure_service_ping_metric_collection turned on' do - let(:metric_double) { instance_double(Gitlab::Usage::ServicePing::LegacyMetricTimingDecorator, duration: 123) } - let(:payload) do - { + let(:metric_double) { instance_double(Gitlab::Usage::ServicePing::LegacyMetricTimingDecorator, duration: 123) } + let(:payload) do + { + uuid: 'uuid', metric_a: metric_double, metric_group: { metric_b: metric_double @@ -390,49 +393,27 @@ RSpec.describe ServicePing::SubmitService do metric_without_timing: "value", recorded_at: Time.current } - end + end - let(:metadata_payload) do - { - metadata: { + let(:metadata_payload) do + { + metadata: { + uuid: 'uuid', metrics: [ { name: 'metric_a', time_elapsed: 123 }, { name: 'metric_group.metric_b', time_elapsed: 123 } ] } } - end - - before do - stub_feature_flags(measure_service_ping_metric_collection: true) - - allow_next_instance_of(ServicePing::BuildPayload) do |service| - allow(service).to receive(:execute).and_return(payload) - end - end - - it 'submits metadata' do - response = stub_full_request(service_ping_metadata_url, method: :post) - .with(body: metadata_payload) - - subject.execute - - expect(response).to have_been_requested - end end - context 'with feature flag measure_service_ping_metric_collection turned off' do - before do - stub_feature_flags(measure_service_ping_metric_collection: false) - end + it 'submits metadata' do + response = stub_full_request(service_ping_metadata_url, method: :post) + .with(body: metadata_payload) - it 'does NOT submit metadata' do - response = stub_full_request(service_ping_metadata_url, method: :post) - - subject.execute + subject.execute - expect(response).not_to have_been_requested - end + expect(response).to have_been_requested end end diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb index dc330a5546f..6052882813e 100644 --- a/spec/services/suggestions/apply_service_spec.rb +++ b/spec/services/suggestions/apply_service_spec.rb @@ -581,8 +581,7 @@ RSpec.describe Suggestions::ApplyService do let(:project) { create(:project, :public, :repository) } let(:forked_project) do - fork_project_with_submodules(project, - user, repository: project.repository) + fork_project_with_submodules(project, user) end let(:merge_request) do diff --git a/spec/services/system_notes/incidents_service_spec.rb b/spec/services/system_notes/incidents_service_spec.rb index d1b831e9c4c..6439f9fae93 100644 --- a/spec/services/system_notes/incidents_service_spec.rb +++ b/spec/services/system_notes/incidents_service_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' RSpec.describe SystemNotes::IncidentsService do - include Gitlab::Routing - let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } let_it_be(:author) { create(:user) } @@ -22,14 +20,12 @@ RSpec.describe SystemNotes::IncidentsService do end it 'posts the correct text to the system note' do - path = project_issues_incident_path(project, incident, anchor: "timeline_event_#{timeline_event.id}") - expect(subject.note).to match("added an [incident timeline event](#{path})") + expect(subject.note).to match("added an incident timeline event") end end describe '#edit_timeline_event' do let(:was_changed) { :unknown } - let(:path) { project_issues_incident_path(project, incident, anchor: "timeline_event_#{timeline_event.id}") } subject do described_class.new(noteable: incident).edit_timeline_event(timeline_event, author, was_changed: was_changed) @@ -44,7 +40,7 @@ RSpec.describe SystemNotes::IncidentsService do let(:was_changed) { :occurred_at } it 'posts the correct text to the system note' do - expect(subject.note).to match("edited the event time/date on [incident timeline event](#{path})") + expect(subject.note).to match("edited the event time/date on incident timeline event") end end @@ -52,7 +48,7 @@ RSpec.describe SystemNotes::IncidentsService do let(:was_changed) { :note } it 'posts the correct text to the system note' do - expect(subject.note).to match("edited the text on [incident timeline event](#{path})") + expect(subject.note).to match("edited the text on incident timeline event") end end @@ -60,7 +56,7 @@ RSpec.describe SystemNotes::IncidentsService do let(:was_changed) { :occurred_at_and_note } it 'posts the correct text to the system note' do - expect(subject.note).to match("edited the event time/date and text on [incident timeline event](#{path})") + expect(subject.note).to match("edited the event time/date and text on incident timeline event") end end @@ -68,7 +64,7 @@ RSpec.describe SystemNotes::IncidentsService do let(:was_changed) { :unknown } it 'posts the correct text to the system note' do - expect(subject.note).to match("edited [incident timeline event](#{path})") + expect(subject.note).to match("edited incident timeline event") end end end diff --git a/spec/services/terraform/states/trigger_destroy_service_spec.rb b/spec/services/terraform/states/trigger_destroy_service_spec.rb index 2e96331779c..459f4c3bdb9 100644 --- a/spec/services/terraform/states/trigger_destroy_service_spec.rb +++ b/spec/services/terraform/states/trigger_destroy_service_spec.rb @@ -9,7 +9,9 @@ RSpec.describe Terraform::States::TriggerDestroyService do describe '#execute', :aggregate_failures do let_it_be(:state) { create(:terraform_state, project: project) } - subject { described_class.new(state, current_user: user).execute } + let(:service) { described_class.new(state, current_user: user) } + + subject { service.execute } it 'marks the state as deleted and schedules a cleanup worker' do expect(Terraform::States::DestroyWorker).to receive(:perform_async).with(state.id).once @@ -18,6 +20,15 @@ RSpec.describe Terraform::States::TriggerDestroyService do expect(state.deleted_at).to be_like_time(Time.current) end + context 'within a database transaction' do + subject { state.with_lock { service.execute } } + + it 'does not raise an EnqueueFromTransactionError' do + expect { subject }.not_to raise_error + expect(state.deleted_at).to be_like_time(Time.current) + end + end + shared_examples 'unable to delete state' do it 'does not modify the state' do expect(Terraform::States::DestroyWorker).not_to receive(:perform_async) diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index e4582e19416..1cb44366457 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -186,8 +186,8 @@ RSpec.describe TodoService do before do group.add_owner(author) - group.add_user(member, Gitlab::Access::DEVELOPER) - group.add_user(john_doe, Gitlab::Access::DEVELOPER) + group.add_member(member, Gitlab::Access::DEVELOPER) + group.add_member(john_doe, Gitlab::Access::DEVELOPER) service.new_issue(issue, author) end diff --git a/spec/services/users/activity_service_spec.rb b/spec/services/users/activity_service_spec.rb index 092c5cd3e5e..47a4b943d83 100644 --- a/spec/services/users/activity_service_spec.rb +++ b/spec/services/users/activity_service_spec.rb @@ -34,6 +34,13 @@ RSpec.describe Users::ActivityService do subject.execute end + + it 'tracks RedisHLL event' do + expect(Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event) + .with('unique_active_user', values: user.id) + + subject.execute + end end context 'when a bad object is passed' do diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 068550ec234..339ffc44e4d 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -84,8 +84,74 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state Gitlab::WebHooks::RecursionDetection.set_request_uuid(uuid) end + context 'when there is an interpolation error' do + let(:error) { ::WebHook::InterpolationError.new('boom') } + + before do + stub_full_request(project_hook.url, method: :post) + allow(project_hook).to receive(:interpolated_url).and_raise(error) + end + + it 'logs the error' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with(error) + + expect(service_instance).to receive(:log_execution).with( + execution_duration: (be > 0), + response: have_attributes(code: 200) + ) + + service_instance.execute + end + end + + context 'when there are URL variables' do + before do + project_hook.update!( + url: 'http://example.com/{one}/{two}', + url_variables: { 'one' => 'a', 'two' => 'b' } + ) + end + + it 'POSTs to the interpolated URL, and logs the hook.url' do + stub_full_request(project_hook.interpolated_url, method: :post) + + expect(service_instance).to receive(:queue_log_execution_with_retry).with( + include(url: project_hook.url), + :ok + ) + + service_instance.execute + + expect(WebMock) + .to have_requested(:post, stubbed_hostname(project_hook.interpolated_url)).once + end + + context 'there is userinfo' do + before do + project_hook.update!(url: 'http://{one}:{two}@example.com') + stub_full_request('http://example.com', method: :post) + end + + it 'POSTs to the interpolated URL, and logs the hook.url' do + expect(service_instance).to receive(:queue_log_execution_with_retry).with( + include(url: project_hook.url), + :ok + ) + + service_instance.execute + + expect(WebMock) + .to have_requested(:post, stubbed_hostname('http://example.com')) + .with(headers: headers.merge('Authorization' => 'Basic YTpi')) + .once + end + end + end + context 'when token is defined' do - let_it_be(:project_hook) { create(:project_hook, :token) } + before do + project_hook.token = generate(:token) + end it 'POSTs to the webhook URL' do stub_full_request(project_hook.url, method: :post) diff --git a/spec/services/web_hooks/log_execution_service_spec.rb b/spec/services/web_hooks/log_execution_service_spec.rb index 0ba0372b99d..873f6adc8dc 100644 --- a/spec/services/web_hooks/log_execution_service_spec.rb +++ b/spec/services/web_hooks/log_execution_service_spec.rb @@ -35,6 +35,12 @@ RSpec.describe WebHooks::LogExecutionService do expect(WebHookLog.recent.first).to have_attributes(data) end + it 'updates the last failure' do + expect(project_hook).to receive(:update_last_failure) + + service.execute + end + context 'obtaining an exclusive lease' do let(:lease_key) { "web_hooks:update_hook_failure_state:#{project_hook.id}" } diff --git a/spec/services/work_items/create_and_link_service_spec.rb b/spec/services/work_items/create_and_link_service_spec.rb index 93c029bdab1..81be15f9e2f 100644 --- a/spec/services/work_items/create_and_link_service_spec.rb +++ b/spec/services/work_items/create_and_link_service_spec.rb @@ -7,13 +7,16 @@ RSpec.describe WorkItems::CreateAndLinkService do let_it_be(:project) { create(:project, group: group) } let_it_be(:user) { create(:user) } let_it_be(:related_work_item) { create(:work_item, project: project) } + let_it_be(:invalid_parent) { create(:work_item, :task, project: project) } let(:spam_params) { double } let(:link_params) { {} } + let(:params) do { title: 'Awesome work item', - description: 'please fix' + description: 'please fix', + work_item_type_id: WorkItems::Type.default_by_type(:task).id } end @@ -40,32 +43,32 @@ RSpec.describe WorkItems::CreateAndLinkService do end context 'when link params are valid' do - let(:link_params) { { issuable_references: [related_work_item.to_reference] } } + let(:link_params) { { parent_work_item: related_work_item } } it 'creates a work item successfully with links' do expect do service_result end.to change(WorkItem, :count).by(1).and( - change(IssueLink, :count).by(1) + change(WorkItems::ParentLink, :count).by(1) ) end end - context 'when link params are invalid' do - let(:link_params) { { issuable_references: ['invalid reference'] } } + context 'when link creation fails' do + let(:link_params) { { parent_work_item: invalid_parent } } it { is_expected.to be_error } it 'does not create a link and does not rollback transaction' do expect do service_result - end.to not_change(IssueLink, :count).and( + end.to not_change(WorkItems::ParentLink, :count).and( change(WorkItem, :count).by(1) ) end it 'returns a link creation error message' do - expect(service_result.errors).to contain_exactly('No matching issue found. Make sure that you are adding a valid issue URL.') + expect(service_result.errors).to contain_exactly(/only Issue and Incident can be parent of Task./) end end end @@ -84,7 +87,7 @@ RSpec.describe WorkItems::CreateAndLinkService do expect do service_result end.to not_change(WorkItem, :count).and( - not_change(IssueLink, :count) + not_change(WorkItems::ParentLink, :count) ) end diff --git a/spec/services/work_items/create_from_task_service_spec.rb b/spec/services/work_items/create_from_task_service_spec.rb index b4db925f053..7d2dab228b1 100644 --- a/spec/services/work_items/create_from_task_service_spec.rb +++ b/spec/services/work_items/create_from_task_service_spec.rb @@ -32,7 +32,7 @@ RSpec.describe WorkItems::CreateFromTaskService do expect do service_result end.to not_change(WorkItem, :count).and( - not_change(IssueLink, :count) + not_change(WorkItems::ParentLink, :count) ) end end @@ -47,12 +47,14 @@ RSpec.describe WorkItems::CreateFromTaskService do context 'when work item params are valid' do it { is_expected.to be_success } - it 'creates a work item and links it to the original work item successfully' do + it 'creates a work item and creates parent link to the original work item' do expect do service_result end.to change(WorkItem, :count).by(1).and( - change(IssueLink, :count) + change(WorkItems::ParentLink, :count).by(1) ) + + expect(work_item_to_update.reload.work_item_children).not_to be_empty end it 'replaces the original issue markdown description with new work item reference' do @@ -73,7 +75,7 @@ RSpec.describe WorkItems::CreateFromTaskService do expect do service_result end.to not_change(WorkItem, :count).and( - not_change(IssueLink, :count) + not_change(WorkItems::ParentLink, :count) ) end diff --git a/spec/services/work_items/create_service_spec.rb b/spec/services/work_items/create_service_spec.rb index f495e967b26..4009c85bacd 100644 --- a/spec/services/work_items/create_service_spec.rb +++ b/spec/services/work_items/create_service_spec.rb @@ -6,9 +6,12 @@ RSpec.describe WorkItems::CreateService do include AfterNextHelpers let_it_be_with_reload(:project) { create(:project) } + let_it_be(:parent) { create(:work_item, project: project) } let_it_be(:guest) { create(:user) } + let_it_be(:reporter) { create(:user) } let_it_be(:user_with_no_access) { create(:user) } + let(:widget_params) { {} } let(:spam_params) { double } let(:current_user) { guest } let(:opts) do @@ -20,10 +23,21 @@ RSpec.describe WorkItems::CreateService do before_all do project.add_guest(guest) + project.add_reporter(reporter) end describe '#execute' do - subject(:service_result) { described_class.new(project: project, current_user: current_user, params: opts, spam_params: spam_params).execute } + let(:service) do + described_class.new( + project: project, + current_user: current_user, + params: opts, + spam_params: spam_params, + widget_params: widget_params + ) + end + + subject(:service_result) { service.execute } before do stub_spam_services @@ -61,6 +75,14 @@ RSpec.describe WorkItems::CreateService do it 'returns validation errors' do expect(service_result.errors).to contain_exactly("Title can't be blank") end + + it 'does not execute after-create transaction widgets' do + expect(service).to receive(:create).and_call_original + expect(service).not_to receive(:execute_widgets) + .with(callback: :after_create_in_transaction, widget_params: widget_params) + + service_result + end end context 'checking spam' do @@ -80,5 +102,104 @@ RSpec.describe WorkItems::CreateService do service_result end end + + it_behaves_like 'work item widgetable service' do + let(:widget_params) do + { + hierarchy_widget: { parent: parent } + } + end + + let(:service) do + described_class.new( + project: project, + current_user: current_user, + params: opts, + spam_params: spam_params, + widget_params: widget_params + ) + end + + let(:service_execute) { service.execute } + + let(:supported_widgets) do + [ + { + klass: WorkItems::Widgets::HierarchyService::CreateService, + callback: :after_create_in_transaction, + params: { parent: parent } + } + ] + end + end + + describe 'hierarchy widget' do + let(:widget_params) { { hierarchy_widget: { parent: parent } } } + + shared_examples 'fails creating work item and returns errors' do + it 'does not create new work item if parent can not be set' do + expect { service_result }.not_to change(WorkItem, :count) + + expect(service_result[:status]).to be(:error) + expect(service_result[:message]).to match(error_message) + end + end + + context 'when user can admin parent link' do + let(:current_user) { reporter } + + context 'when parent is valid work item' do + let(:opts) do + { + title: 'Awesome work_item', + description: 'please fix', + work_item_type: create(:work_item_type, :task) + } + end + + it 'creates new work item and sets parent reference' do + expect { service_result }.to change( + WorkItem, :count).by(1).and(change( + WorkItems::ParentLink, :count).by(1)) + + expect(service_result[:status]).to be(:success) + end + end + + context 'when parent type is invalid' do + let_it_be(:parent) { create(:work_item, :task, project: project) } + + it_behaves_like 'fails creating work item and returns errors' do + let(:error_message) { 'only Issue and Incident can be parent of Task.'} + end + end + + context 'when hierarchy feature flag is disabled' do + before do + stub_feature_flags(work_items_hierarchy: false) + end + + it_behaves_like 'fails creating work item and returns errors' do + let(:error_message) { '`work_items_hierarchy` feature flag disabled for this project' } + end + end + end + + context 'when user cannot admin parent link' do + let(:current_user) { guest } + + let(:opts) do + { + title: 'Awesome work_item', + description: 'please fix', + work_item_type: create(:work_item_type, :task) + } + end + + it_behaves_like 'fails creating work item and returns errors' do + let(:error_message) { 'No matching task found. Make sure that you are adding a valid task ID.'} + end + end + end end end diff --git a/spec/services/work_items/delete_task_service_spec.rb b/spec/services/work_items/delete_task_service_spec.rb index 04944645c9b..07a0d8d6c1a 100644 --- a/spec/services/work_items/delete_task_service_spec.rb +++ b/spec/services/work_items/delete_task_service_spec.rb @@ -67,7 +67,7 @@ RSpec.describe WorkItems::DeleteTaskService do it 'removes the task list item with the work item reference' do expect do service_result - end.to change(list_work_item, :description).from(list_work_item.description).to('') + end.to change(list_work_item, :description).from(list_work_item.description).to("- [ ] #{task.title}") end end diff --git a/spec/services/work_items/parent_links/create_service_spec.rb b/spec/services/work_items/parent_links/create_service_spec.rb new file mode 100644 index 00000000000..85b0ee040cd --- /dev/null +++ b/spec/services/work_items/parent_links/create_service_spec.rb @@ -0,0 +1,173 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::ParentLinks::CreateService do + describe '#execute' do + let_it_be(:user) { create(:user) } + let_it_be(:guest) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:work_item) { create(:work_item, project: project) } + let_it_be(:task) { create(:work_item, :task, project: project) } + let_it_be(:task1) { create(:work_item, :task, project: project) } + let_it_be(:task2) { create(:work_item, :task, project: project) } + let_it_be(:guest_task) { create(:work_item, :task) } + let_it_be(:invalid_task) { build_stubbed(:work_item, :task, id: non_existing_record_id)} + let_it_be(:another_project) { (create :project) } + let_it_be(:other_project_task) { create(:work_item, :task, iid: 100, project: another_project) } + let_it_be(:existing_parent_link) { create(:parent_link, work_item: task, work_item_parent: work_item)} + + let(:parent_link_class) { WorkItems::ParentLink } + let(:issuable_type) { :task } + let(:params) { {} } + + before do + project.add_reporter(user) + project.add_guest(guest) + guest_task.project.add_guest(user) + another_project.add_reporter(user) + end + + shared_examples 'returns not found error' do + it 'returns error' do + error = "No matching #{issuable_type} found. Make sure that you are adding a valid #{issuable_type} ID." + + is_expected.to eq(service_error(error)) + end + + it 'no relationship is created' do + expect { subject }.not_to change(parent_link_class, :count) + end + end + + subject { described_class.new(work_item, user, params).execute } + + context 'when the reference list is empty' do + let(:params) { { issuable_references: [] } } + + it_behaves_like 'returns not found error' + end + + context 'when work item not found' do + let(:params) { { issuable_references: [invalid_task] } } + + it_behaves_like 'returns not found error' + end + + context 'when user has no permission to link work items' do + let(:params) { { issuable_references: [guest_task] } } + + it_behaves_like 'returns not found error' + end + + context 'child and parent are the same work item' do + let(:params) { { issuable_references: [work_item] } } + + it 'no relationship is created' do + expect { subject }.not_to change(parent_link_class, :count) + end + end + + context 'when there are tasks to relate' do + let(:params) { { issuable_references: [task1, task2] } } + + it 'creates relationships', :aggregate_failures do + expect { subject }.to change(parent_link_class, :count).by(2) + + tasks_parent = parent_link_class.where(work_item: [task1, task2]).map(&:work_item_parent).uniq + expect(tasks_parent).to match_array([work_item]) + end + + it 'returns success status and created links', :aggregate_failures do + expect(subject.keys).to match_array([:status, :created_references]) + expect(subject[:status]).to eq(:success) + expect(subject[:created_references].map(&:work_item_id)).to match_array([task1.id, task2.id]) + end + + context 'when task is already assigned' do + let(:params) { { issuable_references: [task, task2] } } + + it 'creates links only for non related tasks' do + expect { subject }.to change(parent_link_class, :count).by(1) + + expect(subject[:created_references].map(&:work_item_id)).to match_array([task2.id]) + end + end + + context 'when there are invalid children' do + let_it_be(:issue) { create(:work_item, project: project) } + + let(:params) { { issuable_references: [task1, issue, other_project_task] } } + + it 'creates links only for valid children' do + expect { subject }.to change { parent_link_class.count }.by(1) + end + + it 'returns error status' do + error = "#{issue.to_reference} cannot be added: only Task can be assigned as a child in hierarchy.. " \ + "#{other_project_task.to_reference} cannot be added: parent must be in the same project as child." + + is_expected.to eq(service_error(error, http_status: 422)) + end + end + + context 'when parent type is invalid' do + let(:work_item) { create :work_item, :task, project: project } + + let(:params) { { target_issuable: task1 } } + + it 'returns error status' do + error = "#{task1.to_reference} cannot be added: only Issue and Incident can be parent of Task." + + is_expected.to eq(service_error(error, http_status: 422)) + end + end + + context 'when max depth is reached' do + let(:params) { { issuable_references: [task2] } } + + before do + stub_const("#{parent_link_class}::MAX_CHILDREN", 1) + end + + it 'returns error status' do + error = "#{task2.to_reference} cannot be added: parent already has maximum number of children." + + is_expected.to eq(service_error(error, http_status: 422)) + end + end + + context 'when params include invalid ids' do + let(:params) { { issuable_references: [task1, invalid_task] } } + + it 'creates links only for valid IDs' do + expect { subject }.to change(parent_link_class, :count).by(1) + end + end + + context 'when user is a guest' do + let(:user) { guest } + + it_behaves_like 'returns not found error' + end + + context 'when user is a guest assigned to the work item' do + let(:user) { guest } + + before do + work_item.assignees = [guest] + end + + it_behaves_like 'returns not found error' + end + end + end + + def service_error(message, http_status: 404) + { + message: message, + status: :error, + http_status: http_status + } + end +end diff --git a/spec/services/work_items/parent_links/destroy_service_spec.rb b/spec/services/work_items/parent_links/destroy_service_spec.rb new file mode 100644 index 00000000000..574b70af397 --- /dev/null +++ b/spec/services/work_items/parent_links/destroy_service_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::ParentLinks::DestroyService do + describe '#execute' do + let_it_be(:reporter) { create(:user) } + let_it_be(:guest) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:work_item) { create(:work_item, project: project) } + let_it_be(:task) { create(:work_item, :task, project: project) } + let_it_be(:parent_link) { create(:parent_link, work_item: task, work_item_parent: work_item)} + + let(:parent_link_class) { WorkItems::ParentLink } + + subject { described_class.new(parent_link, user).execute } + + before do + project.add_reporter(reporter) + project.add_guest(guest) + end + + context 'when user has permissions to update work items' do + let(:user) { reporter } + + it 'removes relation' do + expect { subject }.to change(parent_link_class, :count).by(-1) + end + + it 'returns success message' do + is_expected.to eq(message: 'Relation was removed', status: :success) + end + end + + context 'when user has insufficient permissions' do + let(:user) { guest } + + it 'does not remove relation' do + expect { subject }.not_to change(parent_link_class, :count).from(1) + end + + it 'returns error message' do + is_expected.to eq(message: 'No Work Item Link found', status: :error, http_status: 404) + end + end + end +end diff --git a/spec/services/work_items/task_list_reference_removal_service_spec.rb b/spec/services/work_items/task_list_reference_removal_service_spec.rb index bca72da0efa..91b7814ae92 100644 --- a/spec/services/work_items/task_list_reference_removal_service_spec.rb +++ b/spec/services/work_items/task_list_reference_removal_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe WorkItems::TaskListReferenceRemovalService do let_it_be(:developer) { create(:user) } let_it_be(:project) { create(:project, :repository).tap { |project| project.add_developer(developer) } } - let_it_be(:task) { create(:work_item, project: project) } + let_it_be(:task) { create(:work_item, project: project, title: 'Task title') } let_it_be(:single_line_work_item, refind: true) do create(:work_item, project: project, description: "- [ ] #{task.to_reference}+ single line") end @@ -82,7 +82,7 @@ RSpec.describe WorkItems::TaskListReferenceRemovalService do let(:line_number_end) { 1 } let(:work_item) { single_line_work_item } - it_behaves_like 'successful work item task reference removal service', '' + it_behaves_like 'successful work item task reference removal service', '- [ ] Task title single line' context 'when description does not contain a task' do let_it_be(:no_matching_work_item) { create(:work_item, project: project, description: 'no matching task') } @@ -102,7 +102,8 @@ RSpec.describe WorkItems::TaskListReferenceRemovalService do end context 'when task mardown spans multiple lines' do - it_behaves_like 'successful work item task reference removal service', "Any text\n\n* [x] task\n\nMore text" + it_behaves_like 'successful work item task reference removal service', + "Any text\n\n* [ ] Item to be converted\n Task title second line\n third line\n* [x] task\n\nMore text" end context 'when updating the work item fails' do diff --git a/spec/services/work_items/task_list_reference_replacement_service_spec.rb b/spec/services/work_items/task_list_reference_replacement_service_spec.rb index e7914eb4a92..965c5f1d554 100644 --- a/spec/services/work_items/task_list_reference_replacement_service_spec.rb +++ b/spec/services/work_items/task_list_reference_replacement_service_spec.rb @@ -3,7 +3,8 @@ require 'spec_helper' RSpec.describe WorkItems::TaskListReferenceReplacementService do - let_it_be(:project) { create(:project, :repository) } + let_it_be(:developer) { create(:user) } + let_it_be(:project) { create(:project, :repository).tap { |project| project.add_developer(developer) } } let_it_be(:single_line_work_item, refind: true) { create(:work_item, project: project, description: '- [ ] single line', lock_version: 3) } let_it_be(:multiple_line_work_item, refind: true) { create(:work_item, project: project, description: "Any text\n\n* [ ] Item to be converted\n second line\n third line", lock_version: 3) } @@ -37,6 +38,7 @@ RSpec.describe WorkItems::TaskListReferenceReplacementService do subject(:result) do described_class.new( work_item: work_item, + current_user: developer, work_item_reference: reference, line_number_start: line_number_start, line_number_end: line_number_end, @@ -52,6 +54,12 @@ RSpec.describe WorkItems::TaskListReferenceReplacementService do let(:task_prefix) { '- [ ]' } it_behaves_like 'successful work item task reference replacement service' + + it 'creates description version note' do + expect { result }.to change(Note, :count).by(1) + expect(work_item.notes.last.note).to eq('changed the description') + expect(work_item.saved_description_version.id).to eq(work_item.notes.last.system_note_metadata.description_version_id) + end end context 'when task mardown spans multiple lines' do diff --git a/spec/services/work_items/update_service_spec.rb b/spec/services/work_items/update_service_spec.rb index 9030326dadb..b17c9ffb4fb 100644 --- a/spec/services/work_items/update_service_spec.rb +++ b/spec/services/work_items/update_service_spec.rb @@ -5,6 +5,7 @@ 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(:parent) { create(:work_item, project: project) } let_it_be_with_reload(:work_item) { create(:work_item, project: project, assignees: [developer]) } let(:spam_params) { double } @@ -13,7 +14,15 @@ RSpec.describe WorkItems::UpdateService do 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, widget_params: widget_params).execute(work_item) } + subject(:update_work_item) do + described_class.new( + project: project, + current_user: current_user, + params: opts, + spam_params: spam_params, + widget_params: widget_params + ).execute(work_item) + end before do stub_spam_services @@ -27,8 +36,7 @@ RSpec.describe WorkItems::UpdateService do expect(Gitlab::UsageDataCounters::WorkItemActivityUniqueCounter).to receive(:track_work_item_title_changed_action).with(author: current_user) # During the work item transition we also want to track work items as issues expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_title_changed_action) - - update_work_item + expect(update_work_item[:status]).to eq(:success) end end @@ -38,8 +46,7 @@ RSpec.describe WorkItems::UpdateService do it 'does not trigger issuable_title_updated graphql subscription' do expect(GraphqlTriggers).not_to receive(:issuable_title_updated) expect(Gitlab::UsageDataCounters::WorkItemActivityUniqueCounter).not_to receive(:track_work_item_title_changed_action) - - update_work_item + expect(update_work_item[:status]).to eq(:success) end end @@ -71,16 +78,104 @@ RSpec.describe WorkItems::UpdateService do end end + it_behaves_like 'work item widgetable service' do + let(:widget_params) do + { + hierarchy_widget: { parent: parent }, + description_widget: { description: 'foo' }, + weight_widget: { weight: 1 } + } + end + + let(:service) do + described_class.new( + project: project, + current_user: current_user, + params: opts, + spam_params: spam_params, + widget_params: widget_params + ) + end + + let(:service_execute) { service.execute(work_item) } + + let(:supported_widgets) do + [ + { klass: WorkItems::Widgets::DescriptionService::UpdateService, callback: :update, params: { description: 'foo' } }, + { klass: WorkItems::Widgets::WeightService::UpdateService, callback: :update, params: { weight: 1 } }, + { klass: WorkItems::Widgets::HierarchyService::UpdateService, callback: :before_update_in_transaction, params: { parent: parent } } + ] + end + end + context 'when updating widgets' do - context 'for the description widget' do - let(:widget_params) { { description_widget: { description: 'changed' } } } + let(:widget_service_class) { WorkItems::Widgets::DescriptionService::UpdateService } + let(:widget_params) { { description_widget: { description: 'changed' } } } + + context 'when widget service is not present' do + before do + allow(widget_service_class).to receive(:new).and_return(nil) + end + + it 'ignores widget param' do + expect { update_work_item }.not_to change(work_item, :description) + end + end + context 'when the widget does not support update callback' do + before do + allow_next_instance_of(widget_service_class) do |instance| + allow(instance) + .to receive(:update) + .with(params: { description: 'changed' }).and_return(nil) + end + end + + it 'ignores widget param' do + expect { update_work_item }.not_to change(work_item, :description) + end + end + + context 'for the description widget' do it 'updates the description of the work item' do update_work_item expect(work_item.description).to eq('changed') end end + + context 'for the hierarchy widget' do + let(:opts) { { title: 'changed' } } + let_it_be(:child_work_item) { create(:work_item, :task, project: project) } + + let(:widget_params) { { hierarchy_widget: { children: [child_work_item] } } } + + it 'updates the children of the work item' do + expect do + update_work_item + work_item.reload + end.to change(WorkItems::ParentLink, :count).by(1) + + expect(work_item.work_item_children).to include(child_work_item) + end + + context 'when child type is invalid' do + let_it_be(:child_work_item) { create(:work_item, project: project) } + + it 'returns error status' do + expect(subject[:status]).to be(:error) + expect(subject[:message]) + .to match("#{child_work_item.to_reference} cannot be added: only Task can be assigned as a child in hierarchy.") + end + + it 'does not update work item attributes' do + expect do + update_work_item + work_item.reload + end.to not_change(WorkItems::ParentLink, :count).and(not_change(work_item, :title)) + end + end + end end end end diff --git a/spec/services/work_items/widgets/description_service/update_service_spec.rb b/spec/services/work_items/widgets/description_service/update_service_spec.rb new file mode 100644 index 00000000000..a2eceb97f09 --- /dev/null +++ b/spec/services/work_items/widgets/description_service/update_service_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::Widgets::DescriptionService::UpdateService do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be_with_reload(:work_item) { create(:work_item, project: project, description: 'old description') } + + let(:widget) { work_item.widgets.find {|widget| widget.is_a?(WorkItems::Widgets::Description) } } + + describe '#update' do + subject { described_class.new(widget: widget, current_user: user).update(params: params) } # rubocop:disable Rails/SaveBang + + context 'when description param is present' do + let(:params) { { description: 'updated description' } } + + it 'correctly sets work item description value' do + subject + + expect(work_item.description).to eq('updated description') + end + end + + context 'when description param is not present' do + let(:params) { {} } + + it 'does not change work item description value' do + subject + + expect(work_item.description).to eq('old description') + end + end + end +end diff --git a/spec/services/work_items/widgets/hierarchy_service/update_service_spec.rb b/spec/services/work_items/widgets/hierarchy_service/update_service_spec.rb new file mode 100644 index 00000000000..4f6ff1b8676 --- /dev/null +++ b/spec/services/work_items/widgets/hierarchy_service/update_service_spec.rb @@ -0,0 +1,160 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::Widgets::HierarchyService::UpdateService do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + + let_it_be(:work_item) { create(:work_item, project: project) } + let_it_be(:parent_work_item) { create(:work_item, project: project) } + let_it_be(:child_work_item) { create(:work_item, :task, project: project) } + let_it_be(:existing_link) { create(:parent_link, work_item: child_work_item, work_item_parent: work_item) } + + let(:widget) { work_item.widgets.find {|widget| widget.is_a?(WorkItems::Widgets::Hierarchy) } } + let(:not_found_error) { 'No matching task found. Make sure that you are adding a valid task ID.' } + + shared_examples 'raises a WidgetError' do + it { expect { subject }.to raise_error(described_class::WidgetError, message) } + end + + describe '#update' do + subject { described_class.new(widget: widget, current_user: user).before_update_in_transaction(params: params) } + + context 'when parent and children params are present' do + let(:params) { { parent: parent_work_item, children: [child_work_item] } } + + it_behaves_like 'raises a WidgetError' do + let(:message) { 'A Work Item can be a parent or a child, but not both.' } + end + end + + context 'when updating children' do + let_it_be(:child_work_item2) { create(:work_item, :task, project: project) } + let_it_be(:child_work_item3) { create(:work_item, :task, project: project) } + let_it_be(:child_work_item4) { create(:work_item, :task, project: project) } + + context 'when work_items_hierarchy feature flag is disabled' do + let(:params) { { children: [child_work_item4] }} + + before do + stub_feature_flags(work_items_hierarchy: false) + end + + it_behaves_like 'raises a WidgetError' do + let(:message) { '`work_items_hierarchy` feature flag disabled for this project' } + end + end + + context 'when user has insufficient permissions to link work items' do + let(:params) { { children: [child_work_item4] }} + + it_behaves_like 'raises a WidgetError' do + let(:message) { not_found_error } + end + end + + context 'when user has sufficient permissions to link work item' do + before do + project.add_developer(user) + end + + context 'with valid params' do + let(:params) { { children: [child_work_item2, child_work_item3] }} + + it 'correctly sets work item parent' do + subject + + expect(work_item.reload.work_item_children) + .to contain_exactly(child_work_item, child_work_item2, child_work_item3) + end + end + + context 'when child is already assigned' do + let(:params) { { children: [child_work_item] }} + + it_behaves_like 'raises a WidgetError' do + let(:message) { 'Task(s) already assigned' } + end + end + + context 'when child type is invalid' do + let_it_be(:child_issue) { create(:work_item, project: project) } + + let(:params) { { children: [child_issue] }} + + it_behaves_like 'raises a WidgetError' do + let(:message) do + "#{child_issue.to_reference} cannot be added: only Task can be assigned as a child in hierarchy." + end + end + end + end + end + + context 'when updating parent' do + let_it_be(:work_item) { create(:work_item, :task, project: project) } + + let(:params) {{ parent: parent_work_item } } + + context 'when work_items_hierarchy feature flag is disabled' do + before do + stub_feature_flags(work_items_hierarchy: false) + end + + it_behaves_like 'raises a WidgetError' do + let(:message) { '`work_items_hierarchy` feature flag disabled for this project' } + end + end + + context 'when user has insufficient permissions to link work items' do + it_behaves_like 'raises a WidgetError' do + let(:message) { not_found_error } + end + end + + context 'when user has sufficient permissions to link work item' do + before do + project.add_developer(user) + end + + it 'correctly sets new parent' do + expect(subject[:status]).to eq(:success) + expect(work_item.work_item_parent).to eq(parent_work_item) + end + + context 'when parent is nil' do + let(:params) { { parent: nil } } + + it 'removes the work item parent if present' do + work_item.update!(work_item_parent: parent_work_item) + + expect do + subject + work_item.reload + end.to change(work_item, :work_item_parent).from(parent_work_item).to(nil) + end + + it 'returns success status if parent not present', :aggregate_failure do + work_item.update!(work_item_parent: nil) + + expect(subject[:status]).to eq(:success) + expect(work_item.reload.work_item_parent).to be_nil + end + end + + context 'when type is invalid' do + let_it_be(:parent_task) { create(:work_item, :task, project: project)} + + let(:params) {{ parent: parent_task } } + + it_behaves_like 'raises a WidgetError' do + let(:message) do + "#{work_item.to_reference} cannot be added: only Issue and Incident can be parent of Task." + end + end + end + end + end + end +end diff --git a/spec/services/work_items/widgets/weight_service/update_service_spec.rb b/spec/services/work_items/widgets/weight_service/update_service_spec.rb new file mode 100644 index 00000000000..97e17f1c526 --- /dev/null +++ b/spec/services/work_items/widgets/weight_service/update_service_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::Widgets::WeightService::UpdateService do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be_with_reload(:work_item) { create(:work_item, project: project, weight: 1) } + + let(:widget) { work_item.widgets.find {|widget| widget.is_a?(WorkItems::Widgets::Weight) } } + + describe '#update' do + subject { described_class.new(widget: widget, current_user: user).update(params: params) } # rubocop:disable Rails/SaveBang + + context 'when weight param is present' do + let(:params) { { weight: 2 } } + + it 'correctly sets work item weight value' do + subject + + expect(work_item.weight).to eq(2) + end + end + + context 'when weight param is not present' do + let(:params) { {} } + + it 'does not change work item weight value', :aggregate_failures do + expect { subject } + .to not_change { work_item.weight } + + expect(work_item.weight).to eq(1) + end + end + end +end |