diff options
Diffstat (limited to 'spec/services')
63 files changed, 1993 insertions, 520 deletions
diff --git a/spec/services/alert_management/metric_images/upload_service_spec.rb b/spec/services/alert_management/metric_images/upload_service_spec.rb new file mode 100644 index 00000000000..527d9db0fd9 --- /dev/null +++ b/spec/services/alert_management/metric_images/upload_service_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AlertManagement::MetricImages::UploadService do + subject(:service) { described_class.new(alert, current_user, params) } + + let_it_be_with_refind(:project) { create(:project) } + let_it_be_with_refind(:alert) { create(:alert_management_alert, project: project) } + let_it_be_with_refind(:current_user) { create(:user) } + + let(:params) do + { + file: fixture_file_upload('spec/fixtures/rails_sample.jpg', 'image/jpg'), + url: 'https://www.gitlab.com' + } + end + + describe '#execute' do + subject { service.execute } + + shared_examples 'uploads the metric' do + it 'uploads the metric and returns a success' do + expect { subject }.to change(AlertManagement::MetricImage, :count).by(1) + expect(subject.success?).to eq(true) + expect(subject.payload).to match({ metric: instance_of(AlertManagement::MetricImage), alert: alert }) + end + end + + shared_examples 'no metric saved, an error given' do |message| + it 'returns an error and does not upload', :aggregate_failures do + expect(subject.success?).to eq(false) + expect(subject.message).to match(a_string_matching(message)) + expect(AlertManagement::MetricImage.count).to eq(0) + end + end + + context 'user does not have permissions' do + it_behaves_like 'no metric saved, an error given', 'You are not authorized to upload metric images' + end + + context 'user has permissions' do + before_all do + project.add_developer(current_user) + end + + it_behaves_like 'uploads the metric' + + context 'no url given' do + let(:params) do + { + file: fixture_file_upload('spec/fixtures/rails_sample.jpg', 'image/jpg') + } + end + + it_behaves_like 'uploads the metric' + end + + context 'record invalid' do + let(:params) do + { + file: fixture_file_upload('spec/fixtures/doc_sample.txt', 'text/plain'), + url: 'https://www.gitlab.com' + } + end + + it_behaves_like 'no metric saved, an error given', /File does not have a supported extension. Only png, jpg, jpeg, gif, bmp, tiff, ico, and webp are supported/ # rubocop: disable Layout/LineLength + end + + context 'user is guest' do + before_all do + project.add_guest(current_user) + end + + it_behaves_like 'no metric saved, an error given', 'You are not authorized to upload metric images' + end + end + end +end diff --git a/spec/services/audit_event_service_spec.rb b/spec/services/audit_event_service_spec.rb index 0379fd3f05c..6963515ba5c 100644 --- a/spec/services/audit_event_service_spec.rb +++ b/spec/services/audit_event_service_spec.rb @@ -17,7 +17,8 @@ RSpec.describe AuditEventService do author_name: user.name, entity_id: project.id, entity_type: "Project", - action: :destroy) + action: :destroy, + created_at: anything) expect { service.security_event }.to change(AuditEvent, :count).by(1) end @@ -39,7 +40,8 @@ RSpec.describe AuditEventService do from: 'true', to: 'false', action: :create, - target_id: 1) + target_id: 1, + created_at: anything) expect { service.security_event }.to change(AuditEvent, :count).by(1) @@ -50,6 +52,25 @@ RSpec.describe AuditEventService do expect(details[:target_id]).to eq(1) end + context 'when defining created_at manually' do + let(:service) { described_class.new(user, project, { action: :destroy }, :database, 3.weeks.ago) } + + it 'is overridden successfully' do + freeze_time do + expect(service).to receive(:file_logger).and_return(logger) + expect(logger).to receive(:info).with(author_id: user.id, + author_name: user.name, + entity_id: project.id, + entity_type: "Project", + action: :destroy, + created_at: 3.weeks.ago) + + expect { service.security_event }.to change(AuditEvent, :count).by(1) + expect(AuditEvent.last.created_at).to eq(3.weeks.ago) + end + end + end + context 'authentication event' do let(:audit_service) { described_class.new(user, user, with: 'standard') } @@ -110,7 +131,8 @@ RSpec.describe AuditEventService do author_name: user.name, entity_type: 'Project', entity_id: project.id, - action: :destroy) + action: :destroy, + created_at: anything) service.log_security_event_to_file end diff --git a/spec/services/bulk_imports/relation_export_service_spec.rb b/spec/services/bulk_imports/relation_export_service_spec.rb index 27a6ca60515..f0f85217d2e 100644 --- a/spec/services/bulk_imports/relation_export_service_spec.rb +++ b/spec/services/bulk_imports/relation_export_service_spec.rb @@ -88,6 +88,18 @@ RSpec.describe BulkImports::RelationExportService do subject.execute end + + context 'when export is recently finished' do + it 'returns recently finished export instead of re-exporting' do + updated_at = 5.seconds.ago + export.update!(status: 1, updated_at: updated_at) + + expect { subject.execute }.not_to change { export.updated_at } + + expect(export.status).to eq(1) + expect(export.updated_at).to eq(updated_at) + end + end end context 'when exception occurs during export' do diff --git a/spec/services/bulk_update_integration_service_spec.rb b/spec/services/bulk_update_integration_service_spec.rb index 5e521b98482..dcc8d2df36d 100644 --- a/spec/services/bulk_update_integration_service_spec.rb +++ b/spec/services/bulk_update_integration_service_spec.rb @@ -9,7 +9,13 @@ RSpec.describe BulkUpdateIntegrationService do stub_jira_integration_test end - let(:excluded_attributes) { %w[id project_id group_id inherit_from_id instance template created_at updated_at] } + let(:excluded_attributes) do + %w[ + id project_id group_id inherit_from_id instance template + created_at updated_at encrypted_properties encrypted_properties_iv + ] + end + let(:batch) do Integration.inherited_descendants_from_self_or_ancestors_from(subgroup_integration).where(id: group_integration.id..integration.id) end @@ -50,7 +56,9 @@ RSpec.describe BulkUpdateIntegrationService do end context 'with integration with data fields' do - let(:excluded_attributes) { %w[id service_id created_at updated_at] } + let(:excluded_attributes) do + %w[id service_id created_at updated_at encrypted_properties encrypted_properties_iv] + end it 'updates the data fields from the integration', :aggregate_failures do described_class.new(subgroup_integration, batch).execute diff --git a/spec/services/ci/after_requeue_job_service_spec.rb b/spec/services/ci/after_requeue_job_service_spec.rb index 2f2baa15945..c9bd44f78e2 100644 --- a/spec/services/ci/after_requeue_job_service_spec.rb +++ b/spec/services/ci/after_requeue_job_service_spec.rb @@ -85,7 +85,7 @@ RSpec.describe Ci::AfterRequeueJobService, :sidekiq_inline do c2: 'skipped' ) - new_a1 = Ci::RetryBuildService.new(project, user).clone!(a1) + new_a1 = Ci::RetryJobService.new(project, user).clone!(a1) new_a1.enqueue! check_jobs_statuses( a1: 'pending', @@ -172,7 +172,7 @@ RSpec.describe Ci::AfterRequeueJobService, :sidekiq_inline do c2: 'skipped' ) - new_a1 = Ci::RetryBuildService.new(project, user).clone!(a1) + new_a1 = Ci::RetryJobService.new(project, user).clone!(a1) new_a1.enqueue! check_jobs_statuses( a1: 'pending', @@ -196,25 +196,6 @@ RSpec.describe Ci::AfterRequeueJobService, :sidekiq_inline do c2: 'created' ) end - - context 'when the FF ci_fix_order_of_subsequent_jobs is disabled' do - before do - stub_feature_flags(ci_fix_order_of_subsequent_jobs: false) - end - - it 'does not mark b1 as processable' do - execute_after_requeue_service(a1) - - check_jobs_statuses( - a1: 'pending', - a2: 'created', - b1: 'skipped', - b2: 'created', - c1: 'created', - c2: 'created' - ) - end - end end private diff --git a/spec/services/ci/create_pipeline_service/rate_limit_spec.rb b/spec/services/ci/create_pipeline_service/rate_limit_spec.rb new file mode 100644 index 00000000000..caea165cc6c --- /dev/null +++ b/spec/services/ci/create_pipeline_service/rate_limit_spec.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Ci::CreatePipelineService, :freeze_time, :clean_gitlab_redis_rate_limiting do + describe 'rate limiting' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { project.first_owner } + + let(:ref) { 'refs/heads/master' } + + before do + stub_ci_pipeline_yaml_file(gitlab_ci_yaml) + stub_feature_flags(ci_throttle_pipelines_creation_dry_run: false) + + allow(Gitlab::ApplicationRateLimiter).to receive(:rate_limits) + .and_return(pipelines_create: { threshold: 1, interval: 1.minute }) + end + + context 'when user is under the limit' do + let(:pipeline) { create_pipelines(count: 1) } + + it 'allows pipeline creation' do + expect(pipeline).to be_created_successfully + expect(pipeline.statuses).not_to be_empty + end + end + + context 'when user is over the limit' do + let(:pipeline) { create_pipelines } + + it 'blocks pipeline creation' do + throttle_message = 'Too many pipelines created in the last minute. Try again later.' + + expect(pipeline).not_to be_persisted + expect(pipeline.statuses).to be_empty + expect(pipeline.errors.added?(:base, throttle_message)).to be_truthy + end + end + + context 'with different users' do + let(:other_user) { create(:user) } + + before do + project.add_maintainer(other_user) + end + + it 'allows other members to create pipelines' do + blocked_pipeline = create_pipelines(user: user) + allowed_pipeline = create_pipelines(count: 1, user: other_user) + + expect(blocked_pipeline).not_to be_persisted + expect(allowed_pipeline).to be_created_successfully + end + end + + context 'with different commits' do + it 'allows user to create pipeline' do + blocked_pipeline = create_pipelines(ref: ref) + allowed_pipeline = create_pipelines(count: 1, ref: 'refs/heads/feature') + + expect(blocked_pipeline).not_to be_persisted + expect(allowed_pipeline).to be_created_successfully + end + end + + context 'with different projects' do + let_it_be(:other_project) { create(:project, :repository) } + + before do + other_project.add_maintainer(user) + end + + it 'allows user to create pipeline' do + blocked_pipeline = create_pipelines(project: project) + allowed_pipeline = create_pipelines(count: 1, project: other_project) + + expect(blocked_pipeline).not_to be_persisted + expect(allowed_pipeline).to be_created_successfully + end + end + end + + def create_pipelines(attrs = {}) + attrs.reverse_merge!(user: user, ref: ref, project: project, count: 2) + + service = described_class.new(attrs[:project], attrs[:user], { ref: attrs[:ref] }) + + attrs[:count].pred.times { service.execute(:push) } + service.execute(:push).payload + end +end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index a7026f5062e..943d70ba142 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -12,6 +12,10 @@ RSpec.describe Ci::CreatePipelineService do before do stub_ci_pipeline_to_return_yaml_file + + # Disable rate limiting for pipeline creation + allow(Gitlab::ApplicationRateLimiter).to receive(:rate_limits) + .and_return(pipelines_create: { threshold: 0, interval: 1.minute }) end describe '#execute' do @@ -526,7 +530,7 @@ RSpec.describe Ci::CreatePipelineService do let(:ci_yaml) do <<-EOS image: - name: ruby:2.7 + name: image:1.0 ports: - 80 EOS @@ -538,12 +542,12 @@ RSpec.describe Ci::CreatePipelineService do context 'in the job image' do let(:ci_yaml) do <<-EOS - image: ruby:2.7 + image: image:1.0 test: script: rspec image: - name: ruby:2.7 + name: image:1.0 ports: - 80 EOS @@ -555,11 +559,11 @@ RSpec.describe Ci::CreatePipelineService do context 'in the service' do let(:ci_yaml) do <<-EOS - image: ruby:2.7 + image: image:1.0 test: script: rspec - image: ruby:2.7 + image: image:1.0 services: - name: test ports: diff --git a/spec/services/ci/create_web_ide_terminal_service_spec.rb b/spec/services/ci/create_web_ide_terminal_service_spec.rb index 0804773442d..3462b48cfe7 100644 --- a/spec/services/ci/create_web_ide_terminal_service_spec.rb +++ b/spec/services/ci/create_web_ide_terminal_service_spec.rb @@ -60,7 +60,7 @@ RSpec.describe Ci::CreateWebIdeTerminalService do <<-EOS terminal: image: - name: ruby:2.7 + name: image:1.0 ports: - 80 script: rspec diff --git a/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb b/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb index e95a449d615..1c6963e4a31 100644 --- a/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb +++ b/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb @@ -19,8 +19,23 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s let!(:artifact) { create(:ci_job_artifact, :expired, job: job, locked: job.pipeline.locked) } context 'with preloaded relationships' do + let(:second_artifact) { create(:ci_job_artifact, :expired, :junit, job: job) } + + let(:more_artifacts) do + [ + create(:ci_job_artifact, :expired, :sast, job: job), + create(:ci_job_artifact, :expired, :metadata, job: job), + create(:ci_job_artifact, :expired, :codequality, job: job), + create(:ci_job_artifact, :expired, :accessibility, job: job) + ] + end + before do - stub_const("#{described_class}::LARGE_LOOP_LIMIT", 1) + stub_const("#{described_class}::LOOP_LIMIT", 1) + + # This artifact-with-file is created before the control execution to ensure + # that the DeletedObject operations are accounted for in the query count. + second_artifact end context 'with ci_destroy_unlocked_job_artifacts feature flag disabled' do @@ -28,19 +43,12 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s stub_feature_flags(ci_destroy_unlocked_job_artifacts: false) end - it 'performs the smallest number of queries for job_artifacts' do - log = ActiveRecord::QueryRecorder.new { subject } + it 'performs a consistent number of queries' do + control = ActiveRecord::QueryRecorder.new { service.execute } - # SELECT expired ci_job_artifacts - 3 queries from each_batch - # PRELOAD projects, routes, project_statistics - # BEGIN - # INSERT into ci_deleted_objects - # DELETE loaded ci_job_artifacts - # DELETE security_findings -- for EE - # COMMIT - # SELECT next expired ci_job_artifacts + more_artifacts - expect(log.count).to be_within(1).of(10) + expect { subject }.not_to exceed_query_limit(control.count) end end @@ -49,9 +57,12 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s stub_feature_flags(ci_destroy_unlocked_job_artifacts: true) end - it 'performs the smallest number of queries for job_artifacts' do - log = ActiveRecord::QueryRecorder.new { subject } - expect(log.count).to be_within(1).of(8) + it 'performs a consistent number of queries' do + control = ActiveRecord::QueryRecorder.new { service.execute } + + more_artifacts + + expect { subject }.not_to exceed_query_limit(control.count) end end end @@ -119,7 +130,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s let!(:artifact) { create(:ci_job_artifact, :expired, job: job, locked: job.pipeline.locked) } before do - stub_const("#{described_class}::LARGE_LOOP_LIMIT", 10) + stub_const("#{described_class}::LOOP_LIMIT", 10) end context 'when the import fails' do @@ -189,8 +200,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s context 'when loop reached loop limit' do before do - stub_feature_flags(ci_artifact_fast_removal_large_loop_limit: false) - stub_const("#{described_class}::SMALL_LOOP_LIMIT", 1) + stub_const("#{described_class}::LOOP_LIMIT", 1) end it 'destroys one artifact' do 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 67d664a617b..5e77041a632 100644 --- a/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb +++ b/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Ci::JobArtifacts::DestroyBatchService do - let(:artifacts) { Ci::JobArtifact.where(id: [artifact_with_file.id, artifact_without_file.id]) } + let(:artifacts) { Ci::JobArtifact.where(id: [artifact_with_file.id, artifact_without_file.id, trace_artifact.id]) } let(:service) { described_class.new(artifacts, pick_up_at: Time.current) } let_it_be(:artifact_with_file, refind: true) do @@ -18,6 +18,10 @@ RSpec.describe Ci::JobArtifacts::DestroyBatchService do create(:ci_job_artifact) end + let_it_be(:trace_artifact, refind: true) do + create(:ci_job_artifact, :trace, :expired) + end + describe '.execute' do subject(:execute) { service.execute } @@ -42,6 +46,12 @@ RSpec.describe Ci::JobArtifacts::DestroyBatchService do execute end + it 'preserves trace artifacts and removes any timestamp' do + expect { subject } + .to change { trace_artifact.reload.expire_at }.from(trace_artifact.expire_at).to(nil) + .and not_change { Ci::JobArtifact.exists?(trace_artifact.id) } + end + context 'ProjectStatistics' do it 'resets project statistics' do expect(ProjectStatistics).to receive(:increment_statistic).once diff --git a/spec/services/ci/job_artifacts/update_unknown_locked_status_service_spec.rb b/spec/services/ci/job_artifacts/update_unknown_locked_status_service_spec.rb new file mode 100644 index 00000000000..67412e41fb8 --- /dev/null +++ b/spec/services/ci/job_artifacts/update_unknown_locked_status_service_spec.rb @@ -0,0 +1,145 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::JobArtifacts::UpdateUnknownLockedStatusService, :clean_gitlab_redis_shared_state do + include ExclusiveLeaseHelpers + + let(:service) { described_class.new } + + describe '.execute' do + subject { service.execute } + + let_it_be(:locked_pipeline) { create(:ci_pipeline, :artifacts_locked) } + let_it_be(:pipeline) { create(:ci_pipeline, :unlocked) } + let_it_be(:locked_job) { create(:ci_build, :success, pipeline: locked_pipeline) } + let_it_be(:job) { create(:ci_build, :success, pipeline: pipeline) } + + let!(:unknown_unlocked_artifact) do + create(:ci_job_artifact, :junit, expire_at: 1.hour.ago, job: job, locked: Ci::JobArtifact.lockeds[:unknown]) + end + + let!(:unknown_locked_artifact) do + create(:ci_job_artifact, :lsif, + expire_at: 1.day.ago, + job: locked_job, + locked: Ci::JobArtifact.lockeds[:unknown] + ) + end + + let!(:unlocked_artifact) do + create(:ci_job_artifact, :archive, expire_at: 1.hour.ago, job: job, locked: Ci::JobArtifact.lockeds[:unlocked]) + end + + let!(:locked_artifact) do + create(:ci_job_artifact, :sast, :raw, + expire_at: 1.day.ago, + job: locked_job, + locked: Ci::JobArtifact.lockeds[:artifacts_locked] + ) + end + + context 'when artifacts are expired' do + it 'sets artifact_locked when the pipeline is locked' do + expect { service.execute } + .to change { unknown_locked_artifact.reload.locked }.from('unknown').to('artifacts_locked') + .and not_change { Ci::JobArtifact.exists?(locked_artifact.id) } + end + + it 'destroys the artifact when the pipeline is unlocked' do + expect { subject }.to change { Ci::JobArtifact.exists?(unknown_unlocked_artifact.id) }.from(true).to(false) + end + + it 'does not update ci_job_artifact rows with known locked values' do + expect { service.execute } + .to not_change(locked_artifact, :attributes) + .and not_change { Ci::JobArtifact.exists?(locked_artifact.id) } + .and not_change(unlocked_artifact, :attributes) + .and not_change { Ci::JobArtifact.exists?(unlocked_artifact.id) } + end + + it 'logs the counts of affected artifacts' do + expect(subject).to eq({ removed: 1, locked: 1 }) + end + end + + context 'in a single iteration' do + before do + stub_const("#{described_class}::BATCH_SIZE", 1) + end + + context 'due to the LOOP_TIMEOUT' do + before do + stub_const("#{described_class}::LOOP_TIMEOUT", 0.seconds) + end + + it 'affects the earliest expired artifact first' do + subject + + expect(unknown_locked_artifact.reload.locked).to eq('artifacts_locked') + expect(unknown_unlocked_artifact.reload.locked).to eq('unknown') + end + + it 'reports the number of destroyed artifacts' do + is_expected.to eq({ removed: 0, locked: 1 }) + end + end + + context 'due to @loop_limit' do + before do + stub_const("#{described_class}::LARGE_LOOP_LIMIT", 1) + end + + it 'affects the most recently expired artifact first' do + subject + + expect(unknown_locked_artifact.reload.locked).to eq('artifacts_locked') + expect(unknown_unlocked_artifact.reload.locked).to eq('unknown') + end + + it 'reports the number of destroyed artifacts' do + is_expected.to eq({ removed: 0, locked: 1 }) + end + end + end + + context 'when artifact is not expired' do + let!(:unknown_unlocked_artifact) do + create(:ci_job_artifact, :junit, + expire_at: 1.year.from_now, + job: job, + locked: Ci::JobArtifact.lockeds[:unknown] + ) + end + + it 'does not change the locked status' do + expect { service.execute }.not_to change { unknown_unlocked_artifact.locked } + expect(Ci::JobArtifact.exists?(unknown_unlocked_artifact.id)).to eq(true) + end + end + + context 'when exclusive lease has already been taken by the other instance' do + before do + stub_exclusive_lease_taken(described_class::EXCLUSIVE_LOCK_KEY, timeout: described_class::LOCK_TIMEOUT) + end + + it 'raises an error and' do + expect { subject }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) + end + end + + context 'when there are no unknown status artifacts' do + before do + Ci::JobArtifact.update_all(locked: :unlocked) + end + + it 'does not raise error' do + expect { subject }.not_to raise_error + end + + it 'reports the number of destroyed artifacts' do + is_expected.to eq({ removed: 0, locked: 0 }) + end + end + end +end diff --git a/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb b/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb index 7365ad162d2..5bc508447c1 100644 --- a/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb +++ b/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb @@ -725,7 +725,7 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService do expect(builds_names).to eq ['build:1', 'build:2', 'test:1', 'test:2'] - Ci::Build.retry(pipeline.builds.find_by(name: 'test:2'), user).reset.success! + Ci::RetryJobService.new(pipeline.project, user).execute(pipeline.builds.find_by(name: 'test:2'))[:job].reset.success! expect(builds_names).to eq ['build:1', 'build:2', 'test:1', 'test:2', 'test:2', 'deploy:1', 'deploy:2'] @@ -1111,11 +1111,11 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService do end def enqueue_scheduled(name) - builds.scheduled.find_by(name: name).enqueue_scheduled + builds.scheduled.find_by(name: name).enqueue! end def retry_build(name) - Ci::Build.retry(builds.find_by(name: name), user) + Ci::RetryJobService.new(project, user).execute(builds.find_by(name: name)) end def manual_actions diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 245118e71fa..74adbc4efc8 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -103,6 +103,20 @@ module Ci pending_job.create_queuing_entry! end + context 'when build owner has been blocked' do + let(:user) { create(:user, :blocked) } + + before do + pending_job.update!(user: user) + end + + it 'does not pick the build and drops the build' do + expect(execute(shared_runner)).to be_falsey + + expect(pending_job.reload).to be_user_blocked + end + end + context 'for multiple builds' do let!(:project2) { create :project, shared_runners_enabled: true } let!(:pipeline2) { create :ci_pipeline, project: project2 } diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_job_service_spec.rb index 2421fd56c47..25aab73ab01 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_job_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::RetryBuildService do +RSpec.describe Ci::RetryJobService do let_it_be(:reporter) { create(:user) } let_it_be(:developer) { create(:user) } let_it_be(:project) { create(:project, :repository) } @@ -17,7 +17,7 @@ RSpec.describe Ci::RetryBuildService do name: 'test') end - let_it_be_with_refind(:build) { create(:ci_build, pipeline: pipeline, stage_id: stage.id) } + let_it_be_with_refind(:build) { create(:ci_build, :success, pipeline: pipeline, stage_id: stage.id) } let(:user) { developer } @@ -30,7 +30,7 @@ RSpec.describe Ci::RetryBuildService do project.add_reporter(reporter) end - clone_accessors = described_class.clone_accessors.without(described_class.extra_accessors) + clone_accessors = ::Ci::Build.clone_accessors.without(::Ci::Build.extra_accessors) reject_accessors = %i[id status user token token_encrypted coverage trace runner @@ -94,6 +94,10 @@ RSpec.describe Ci::RetryBuildService do create(:terraform_state_version, build: build) end + before do + build.update!(retried: false, status: :success) + end + describe 'clone accessors' do let(:forbidden_associations) do Ci::Build.reflect_on_all_associations.each_with_object(Set.new) do |assoc, memo| @@ -156,8 +160,8 @@ RSpec.describe Ci::RetryBuildService do Ci::Build.attribute_aliases.keys.map(&:to_sym) + Ci::Build.reflect_on_all_associations.map(&:name) + [:tag_list, :needs_attributes, :job_variables_attributes] - - # ee-specific accessors should be tested in ee/spec/services/ci/retry_build_service_spec.rb instead - described_class.extra_accessors - + # ee-specific accessors should be tested in ee/spec/services/ci/retry_job_service_spec.rb instead + Ci::Build.extra_accessors - [:dast_site_profiles_build, :dast_scanner_profiles_build] # join tables current_accessors.uniq! @@ -170,7 +174,7 @@ RSpec.describe Ci::RetryBuildService do describe '#execute' do let(:new_build) do travel_to(1.second.from_now) do - service.execute(build) + service.execute(build)[:job] end end @@ -248,7 +252,7 @@ RSpec.describe Ci::RetryBuildService do context 'when build has scheduling_type' do it 'does not call populate_scheduling_type!' do - expect_any_instance_of(Ci::Pipeline).not_to receive(:ensure_scheduling_type!) + expect_any_instance_of(Ci::Pipeline).not_to receive(:ensure_scheduling_type!) # rubocop: disable RSpec/AnyInstanceOf expect(new_build.scheduling_type).to eq('stage') end @@ -286,6 +290,18 @@ RSpec.describe Ci::RetryBuildService do expect { service.execute(build) } .to raise_error Gitlab::Access::AccessDeniedError end + + context 'when the job is not retryable' do + let(:build) { create(:ci_build, :created, pipeline: pipeline) } + + it 'returns a ServiceResponse error' do + response = service.execute(build) + + expect(response).to be_a(ServiceResponse) + expect(response).to be_error + expect(response.message).to eq("Job cannot be retried") + end + end end end @@ -342,7 +358,7 @@ RSpec.describe Ci::RetryBuildService do end shared_examples_for 'when build with dynamic environment is retried' do - let_it_be(:other_developer) { create(:user).tap { |u| project.add_developer(other_developer) } } + let_it_be(:other_developer) { create(:user).tap { |u| project.add_developer(u) } } let(:environment_name) { 'review/$CI_COMMIT_REF_SLUG-$GITLAB_USER_ID' } diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb index df1e159b5c0..24272801480 100644 --- a/spec/services/ci/retry_pipeline_service_spec.rb +++ b/spec/services/ci/retry_pipeline_service_spec.rb @@ -340,7 +340,7 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do context 'when user is not allowed to retry build' do before do build = create(:ci_build, pipeline: pipeline, status: :failed) - allow_next_instance_of(Ci::RetryBuildService) do |service| + allow_next_instance_of(Ci::RetryJobService) do |service| allow(service).to receive(:can?).with(user, :update_build, build).and_return(false) end end diff --git a/spec/services/database/consistency_check_service_spec.rb b/spec/services/database/consistency_check_service_spec.rb new file mode 100644 index 00000000000..2e642451432 --- /dev/null +++ b/spec/services/database/consistency_check_service_spec.rb @@ -0,0 +1,154 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Database::ConsistencyCheckService do + let(:batch_size) { 5 } + let(:max_batches) { 2 } + + before do + stub_const("Gitlab::Database::ConsistencyChecker::BATCH_SIZE", batch_size) + stub_const("Gitlab::Database::ConsistencyChecker::MAX_BATCHES", max_batches) + end + + after do + redis_shared_state_cleanup! + end + + subject(:consistency_check_service) do + described_class.new( + source_model: Namespace, + target_model: Ci::NamespaceMirror, + source_columns: %w[id traversal_ids], + target_columns: %w[namespace_id traversal_ids] + ) + end + + describe '#random_start_id' do + let(:batch_size) { 5 } + + before do + create_list(:namespace, 50) # This will also create Ci::NameSpaceMirror objects + end + + it 'generates a random start_id within the records ids' do + 10.times do + start_id = subject.send(:random_start_id) + expect(start_id).to be_between(Namespace.first.id, Namespace.last.id).inclusive + end + end + end + + describe '#execute' do + let(:empty_results) do + { batches: 0, matches: 0, mismatches: 0, mismatches_details: [] } + end + + context 'when empty tables' do + it 'returns results with zero counters' do + result = consistency_check_service.execute + + expect(result).to eq(empty_results) + end + + it 'does not call the ConsistencyCheckService' do + expect(Gitlab::Database::ConsistencyChecker).not_to receive(:new) + consistency_check_service.execute + end + end + + context 'no cursor has been saved before' do + let(:selected_start_id) { Namespace.order(:id).limit(5).pluck(:id).last } + let(:expected_next_start_id) { selected_start_id + batch_size * max_batches } + + before do + create_list(:namespace, 50) # This will also create Ci::NameSpaceMirror objects + expect(consistency_check_service).to receive(:random_start_id).and_return(selected_start_id) + end + + it 'picks a random start_id' do + expected_result = { + batches: 2, + matches: 10, + mismatches: 0, + mismatches_details: [], + start_id: selected_start_id, + next_start_id: expected_next_start_id + } + expect(consistency_check_service.execute).to eq(expected_result) + end + + it 'calls the ConsistencyCheckService with the expected parameters' do + allow_next_instance_of(Gitlab::Database::ConsistencyChecker) do |instance| + expect(instance).to receive(:execute).with(start_id: selected_start_id).and_return({ + batches: 2, + next_start_id: expected_next_start_id, + matches: 10, + mismatches: 0, + mismatches_details: [] + }) + end + + expect(Gitlab::Database::ConsistencyChecker).to receive(:new).with( + source_model: Namespace, + target_model: Ci::NamespaceMirror, + source_columns: %w[id traversal_ids], + target_columns: %w[namespace_id traversal_ids] + ).and_call_original + + expected_result = { + batches: 2, + start_id: selected_start_id, + next_start_id: expected_next_start_id, + matches: 10, + mismatches: 0, + mismatches_details: [] + } + expect(consistency_check_service.execute).to eq(expected_result) + end + + it 'saves the next_start_id in Redis for he next iteration' do + expect(consistency_check_service).to receive(:save_next_start_id).with(expected_next_start_id).and_call_original + consistency_check_service.execute + end + end + + context 'cursor saved in Redis and moving' do + let(:first_namespace_id) { Namespace.order(:id).first.id } + let(:second_namespace_id) { Namespace.order(:id).second.id } + + before do + create_list(:namespace, 30) # This will also create Ci::NameSpaceMirror objects + end + + it "keeps moving the cursor with each call to the service" do + expect(consistency_check_service).to receive(:random_start_id).at_most(:once).and_return(first_namespace_id) + + allow_next_instance_of(Gitlab::Database::ConsistencyChecker) do |instance| + expect(instance).to receive(:execute).ordered.with(start_id: first_namespace_id).and_call_original + expect(instance).to receive(:execute).ordered.with(start_id: first_namespace_id + 10).and_call_original + expect(instance).to receive(:execute).ordered.with(start_id: first_namespace_id + 20).and_call_original + # Gets back to the start of the table + expect(instance).to receive(:execute).ordered.with(start_id: first_namespace_id).and_call_original + end + + 4.times do + consistency_check_service.execute + end + end + + it "keeps moving the cursor from any start point" do + expect(consistency_check_service).to receive(:random_start_id).at_most(:once).and_return(second_namespace_id) + + allow_next_instance_of(Gitlab::Database::ConsistencyChecker) do |instance| + expect(instance).to receive(:execute).ordered.with(start_id: second_namespace_id).and_call_original + expect(instance).to receive(:execute).ordered.with(start_id: second_namespace_id + 10).and_call_original + end + + 2.times do + consistency_check_service.execute + end + end + end + end +end diff --git a/spec/services/deployments/update_environment_service_spec.rb b/spec/services/deployments/update_environment_service_spec.rb index 6996563fdb8..0859aa2c9d1 100644 --- a/spec/services/deployments/update_environment_service_spec.rb +++ b/spec/services/deployments/update_environment_service_spec.rb @@ -286,6 +286,37 @@ RSpec.describe Deployments::UpdateEnvironmentService do end end + context 'when environment url uses a nested variable' do + let(:yaml_variables) do + [ + { key: 'MAIN_DOMAIN', value: '${STACK_NAME}.example.com' }, + { key: 'STACK_NAME', value: 'appname-${ENVIRONMENT_NAME}' }, + { key: 'ENVIRONMENT_NAME', value: '${CI_COMMIT_REF_SLUG}' } + ] + end + + let(:job) do + create(:ci_build, + :with_deployment, + pipeline: pipeline, + ref: 'master', + environment: 'production', + project: project, + yaml_variables: yaml_variables, + options: { environment: { name: 'production', url: 'http://$MAIN_DOMAIN' } }) + end + + it { is_expected.to eq('http://appname-master.example.com') } + + context 'when the FF ci_expand_environment_name_and_url is disabled' do + before do + stub_feature_flags(ci_expand_environment_name_and_url: false) + end + + it { is_expected.to eq('http://${STACK_NAME}.example.com') } + end + end + context 'when yaml environment does not have url' do let(:job) { create(:ci_build, :with_deployment, pipeline: pipeline, environment: 'staging', project: project) } diff --git a/spec/services/emails/create_service_spec.rb b/spec/services/emails/create_service_spec.rb index 2fabf4ae66a..b13197f21b8 100644 --- a/spec/services/emails/create_service_spec.rb +++ b/spec/services/emails/create_service_spec.rb @@ -25,5 +25,34 @@ RSpec.describe Emails::CreateService do expect(user.emails).to include(Email.find_by(opts)) end + + it 'sends a notification to the user' do + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive(:new_email_address_added) + end + + service.execute + end + + it 'does not send a notification when the email is not persisted' do + allow_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).not_to receive(:new_email_address_added) + end + + service.execute(email: 'invalid@@example.com') + end + + it 'does not send a notification email when the email is the primary, because we are creating the user' do + allow_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).not_to receive(:new_email_address_added) + end + + # This is here to ensure that the service is actually called. + allow_next_instance_of(described_class) do |create_service| + expect(create_service).to receive(:execute).and_call_original + end + + create(:user) + end end end diff --git a/spec/services/environments/stop_service_spec.rb b/spec/services/environments/stop_service_spec.rb index 362071c1c26..9e9ef127c67 100644 --- a/spec/services/environments/stop_service_spec.rb +++ b/spec/services/environments/stop_service_spec.rb @@ -198,6 +198,31 @@ RSpec.describe Environments::StopService do expect(pipeline.environments_in_self_and_descendants.first).to be_stopped end + + context 'with environment related jobs ' do + let!(:environment) { create(:environment, :available, name: 'staging', project: project) } + let!(:prepare_staging_job) { create(:ci_build, :prepare_staging, pipeline: pipeline, project: project) } + let!(:start_staging_job) { create(:ci_build, :start_staging, :with_deployment, :manual, pipeline: pipeline, project: project) } + let!(:stop_staging_job) { create(:ci_build, :stop_staging, :manual, pipeline: pipeline, project: project) } + + it 'does not stop environments that was not started by the merge request' do + subject + + expect(prepare_staging_job.persisted_environment.state).to eq('available') + end + + context 'when fix_related_environments_for_merge_requests feature flag is disabled' do + before do + stub_feature_flags(fix_related_environments_for_merge_requests: false) + end + + it 'stops unrelated environments too' do + subject + + expect(prepare_staging_job.persisted_environment.state).to eq('stopped') + end + end + end end context 'when user is a reporter' do diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index 611e821f3e5..c22099fe410 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redis_shared_state do + include SnowplowHelpers + let(:service) { described_class.new } let_it_be(:user, reload: true) { create :user } @@ -18,6 +20,28 @@ RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redi end end + shared_examples 'Snowplow event' do + it 'is not emitted if FF is disabled' do + stub_feature_flags(route_hll_to_snowplow: false) + + subject + + expect_no_snowplow_event + end + + it 'is emitted' do + subject + + expect_snowplow_event( + category: described_class.to_s, + action: 'action_active_users_project_repo', + namespace: project.namespace, + user: user, + project: project + ) + end + end + describe 'Issues' do describe '#open_issue' do let(:issue) { create(:issue) } @@ -247,7 +271,7 @@ RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redi end end - describe '#push' do + describe '#push', :snowplow do let(:push_data) do { commits: [ @@ -270,9 +294,11 @@ 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::PUSH_ACTION } end + + it_behaves_like 'Snowplow event' end - describe '#bulk_push' do + describe '#bulk_push', :snowplow do let(:push_data) do { action: :created, @@ -288,6 +314,8 @@ 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::PUSH_ACTION } end + + it_behaves_like 'Snowplow event' end describe 'Project' do diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index 5a637b0956b..57c130f76a4 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -721,4 +721,14 @@ RSpec.describe Git::BranchPushService, services: true do it_behaves_like 'does not enqueue Jira sync worker' end end + + describe 'project target platforms detection' do + subject(:execute) { execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) } + + it 'calls enqueue_record_project_target_platforms on the project' do + expect(project).to receive(:enqueue_record_project_target_platforms) + + execute + end + end end diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index 819569d6e67..6e074f451c4 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -263,43 +263,6 @@ RSpec.describe Groups::CreateService, '#execute' do end end - describe 'invite team email' do - let(:service) { described_class.new(user, group_params) } - - before do - allow(Namespaces::InviteTeamEmailWorker).to receive(:perform_in) - end - - it 'is sent' do - group = service.execute - delay = Namespaces::InviteTeamEmailService::DELIVERY_DELAY_IN_MINUTES - expect(Namespaces::InviteTeamEmailWorker).to have_received(:perform_in).with(delay, group.id, user.id) - end - - context 'when group has not been persisted' do - let(:service) { described_class.new(user, group_params.merge(name: '<script>alert("Attack!")</script>')) } - - it 'not sent' do - expect(Namespaces::InviteTeamEmailWorker).not_to receive(:perform_in) - service.execute - end - end - - context 'when group is not root' do - let(:parent_group) { create :group } - let(:service) { described_class.new(user, group_params.merge(parent_id: parent_group.id)) } - - before do - parent_group.add_owner(user) - end - - it 'not sent' do - expect(Namespaces::InviteTeamEmailWorker).not_to receive(:perform_in) - service.execute - end - end - end - describe 'logged_out_marketing_header experiment', :experiment do let(:service) { described_class.new(user, group_params) } diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index 3a696228382..1c4b7aac87e 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -17,7 +17,7 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do end let_it_be(:user) { create(:user) } - let_it_be(:new_parent_group) { create(:group, :public) } + let_it_be(:new_parent_group) { create(:group, :public, :crm_enabled) } let!(:group_member) { create(:group_member, :owner, group: group, user: user) } let(:transfer_service) { described_class.new(group, user) } @@ -252,23 +252,6 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do expect(transfer_service.execute(new_parent_group)).to be_falsy expect(transfer_service.error).to eq('Transfer failed: The parent group already has a subgroup or a project with the same path.') end - - # currently when a project is created it gets a corresponding project namespace - # so we test the case where a project without a project namespace is transferred - # for backward compatibility - context 'without project namespace' do - before do - project_namespace = project.project_namespace - project.update_column(:project_namespace_id, nil) - project_namespace.delete - end - - it 'adds an error on group' do - expect(project.reload.project_namespace).to be_nil - expect(transfer_service.execute(new_parent_group)).to be_falsy - expect(transfer_service.error).to eq('Transfer failed: Validation failed: Group URL has already been taken') - end - end end context 'when projects have project namespaces' do @@ -876,5 +859,108 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do end end end + + context 'crm' do + let(:root_group) { create(:group, :public) } + let(:subgroup) { create(:group, :public, parent: root_group) } + let(:another_subgroup) { create(:group, :public, parent: root_group) } + let(:subsubgroup) { create(:group, :public, parent: subgroup) } + + let(:root_project) { create(:project, group: root_group) } + let(:sub_project) { create(:project, group: subgroup) } + let(:another_project) { create(:project, group: another_subgroup) } + let(:subsub_project) { create(:project, group: subsubgroup) } + + let!(:contacts) { create_list(:contact, 4, group: root_group) } + let!(:organizations) { create_list(:organization, 2, group: root_group) } + + before do + create(:issue_customer_relations_contact, contact: contacts[0], issue: create(:issue, project: root_project)) + create(:issue_customer_relations_contact, contact: contacts[1], issue: create(:issue, project: sub_project)) + create(:issue_customer_relations_contact, contact: contacts[2], issue: create(:issue, project: another_project)) + create(:issue_customer_relations_contact, contact: contacts[3], issue: create(:issue, project: subsub_project)) + root_group.add_owner(user) + end + + context 'moving up' do + let(:group) { subsubgroup } + + it 'retains issue contacts' do + expect { transfer_service.execute(root_group) } + .not_to change { CustomerRelations::IssueContact.count } + end + end + + context 'moving down' do + let(:group) { subgroup } + + it 'retains issue contacts' do + expect { transfer_service.execute(another_subgroup) } + .not_to change { CustomerRelations::IssueContact.count } + end + end + + context 'moving sideways' do + let(:group) { subsubgroup } + + it 'retains issue contacts' do + expect { transfer_service.execute(another_subgroup) } + .not_to change { CustomerRelations::IssueContact.count } + end + end + + context 'moving to new root group' do + let(:group) { root_group } + + before do + new_parent_group.add_owner(user) + end + + it 'moves all crm objects' do + expect { transfer_service.execute(new_parent_group) } + .to change { root_group.contacts.count }.by(-4) + .and change { root_group.organizations.count }.by(-2) + end + + it 'retains issue contacts' do + expect { transfer_service.execute(new_parent_group) } + .not_to change { CustomerRelations::IssueContact.count } + end + end + + context 'moving to a subgroup within a new root group' do + let(:group) { root_group } + let(:subgroup_in_new_parent_group) { create(:group, parent: new_parent_group) } + + context 'with permission on the root group' do + before do + new_parent_group.add_owner(user) + end + + it 'moves all crm objects' do + expect { transfer_service.execute(subgroup_in_new_parent_group) } + .to change { root_group.contacts.count }.by(-4) + .and change { root_group.organizations.count }.by(-2) + end + + it 'retains issue contacts' do + expect { transfer_service.execute(subgroup_in_new_parent_group) } + .not_to change { CustomerRelations::IssueContact.count } + end + end + + context 'with permission on the subgroup' do + before do + subgroup_in_new_parent_group.add_owner(user) + end + + it 'raises error' do + transfer_service.execute(subgroup_in_new_parent_group) + + expect(transfer_service.error).to eq("Transfer failed: Group contains contacts/organizations and you don't have enough permissions to move them to the new root group.") + end + end + end + end end end diff --git a/spec/services/import/github_service_spec.rb b/spec/services/import/github_service_spec.rb index 04a94d96f67..58afae1e647 100644 --- a/spec/services/import/github_service_spec.rb +++ b/spec/services/import/github_service_spec.rb @@ -34,11 +34,11 @@ RSpec.describe Import::GithubService do subject.execute(access_params, :github) end - it 'returns an error' do + it 'returns an error with message and code' do result = subject.execute(access_params, :github) expect(result).to include( - message: 'Import failed due to a GitHub error: Not Found', + message: 'Import failed due to a GitHub error: Not Found (HTTP 404)', status: :error, http_status: :unprocessable_entity ) 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 new file mode 100644 index 00000000000..c20a0688ac2 --- /dev/null +++ b/spec/services/incident_management/issuable_escalation_statuses/build_service_spec.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IncidentManagement::IssuableEscalationStatuses::BuildService do + let_it_be(:project) { create(:project) } + let_it_be(:incident, reload: true) { create(:incident, project: project) } + + let(:service) { described_class.new(incident) } + + 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 8fbab361ec4..2c7d330766c 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 @@ -8,12 +8,12 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::CreateService do let(:incident) { create(:incident, project: project) } let(:service) { described_class.new(incident) } - subject(:execute) { service.execute} + subject(:execute) { service.execute } it 'creates an escalation status for the incident with no policy set' do - expect { execute }.to change { incident.reload.incident_management_issuable_escalation_status }.from(nil) + expect { execute }.to change { incident.reload.escalation_status }.from(nil) - status = incident.incident_management_issuable_escalation_status + status = incident.escalation_status expect(status.policy_id).to eq(nil) expect(status.escalations_started_at).to eq(nil) @@ -24,7 +24,22 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::CreateService do let!(:existing_status) { create(:incident_management_issuable_escalation_status, issue: incident) } it 'exits without changing anything' do - expect { execute }.not_to change { incident.reload.incident_management_issuable_escalation_status } + 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 b30b3a69ae6..25164df40ca 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 @@ -71,7 +71,12 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::PrepareUpdateServ context 'when an IssuableEscalationStatus record for the issue does not exist' do let(:issue) { create(:incident) } - it_behaves_like 'availability error response' + it_behaves_like 'successful response', { status_event: :acknowledge } + + it 'initializes an issuable escalation status record' do + expect { result }.not_to change(::IncidentManagement::IssuableEscalationStatus, :count) + expect(issue.escalation_status).to be_present + end end context 'when called without params' do diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 6d3c3dd4e39..d496857bb25 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -9,8 +9,8 @@ RSpec.describe Issues::UpdateService, :mailer do let_it_be(:guest) { create(:user) } let_it_be(:group) { create(:group, :public, :crm_enabled) } let_it_be(:project, reload: true) { create(:project, :repository, group: group) } - let_it_be(:label) { create(:label, project: project) } - let_it_be(:label2) { create(:label, project: project) } + let_it_be(:label) { create(:label, title: 'a', project: project) } + let_it_be(:label2) { create(:label, title: 'b', project: project) } let_it_be(:milestone) { create(:milestone, project: project) } let(:issue) do @@ -1224,6 +1224,18 @@ RSpec.describe Issues::UpdateService, :mailer do end context 'without an escalation status record' do + it 'creates a new record' do + expect { update_issue(opts) }.to change(::IncidentManagement::IssuableEscalationStatus, :count).by(1) + end + + it_behaves_like 'updates the escalation status record', :acknowledged + end + + context 'with :incident_escalations feature flag disabled' do + before do + stub_feature_flags(incident_escalations: false) + end + it_behaves_like 'does not change the status record' end end @@ -1349,6 +1361,19 @@ RSpec.describe Issues::UpdateService, :mailer do end end + context 'labels are updated' do + let(:label_a) { label } + let(:label_b) { label2 } + let(:issuable) { issue } + + it_behaves_like 'keeps issuable labels sorted after update' + it_behaves_like 'broadcasting issuable labels updates' + + def update_issuable(update_params) + update_issue(update_params) + end + end + it_behaves_like 'issuable record that supports quick actions' do let(:existing_issue) { create(:issue, project: project) } let(:issuable) { described_class.new(project: project, current_user: user, params: params).execute(existing_issue) } diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb index 4396a0d3ec3..25437be1e78 100644 --- a/spec/services/members/create_service_spec.rb +++ b/spec/services/members/create_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ let_it_be(:source, reload: true) { create(:project) } let_it_be(:user) { create(:user) } let_it_be(:member) { create(:user) } + let_it_be(:user_invited_by_id) { create(:user) } let_it_be(:user_ids) { member.id.to_s } let_it_be(:access_level) { Gitlab::Access::GUEST } @@ -49,6 +50,36 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true) end + context 'when user_id is passed as an integer' do + let(:user_ids) { member.id } + + it 'successfully creates member' do + expect(execute_service[:status]).to eq(:success) + expect(source.users).to include member + expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true) + end + end + + context 'with user_ids as an array of integers' do + let(:user_ids) { [member.id, user_invited_by_id.id] } + + it 'successfully creates members' do + expect(execute_service[:status]).to eq(:success) + expect(source.users).to include(member, user_invited_by_id) + expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true) + end + end + + context 'with user_ids as an array of strings' do + let(:user_ids) { [member.id.to_s, user_invited_by_id.id.to_s] } + + it 'successfully creates members' do + expect(execute_service[:status]).to eq(:success) + expect(source.users).to include(member, user_invited_by_id) + expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true) + end + end + context 'when executing on a group' do let_it_be(:source) { create(:group) } @@ -112,6 +143,32 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ end end + context 'when adding a project_bot' do + let_it_be(:project_bot) { create(:user, :project_bot) } + + let(:user_ids) { project_bot.id } + + context 'when project_bot is already a member' do + before do + source.add_developer(project_bot) + end + + it 'does not update the member' do + expect(execute_service[:status]).to eq(:error) + expect(execute_service[:message]).to eq("#{project_bot.username}: not authorized to update member") + expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(false) + end + end + + context 'when project_bot is not already a member' do + it 'adds the member' do + expect(execute_service[:status]).to eq(:success) + expect(source.users).to include project_bot + expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true) + end + end + end + context 'when tracking the invite source', :snowplow do context 'when invite_source is not passed' do let(:additional_params) { {} } @@ -191,6 +248,15 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ ) end + context 'when it is an invite by email passed to user_ids' do + let(:user_ids) { 'email@example.org' } + + it 'does not create task issues' do + expect(TasksToBeDone::CreateWorker).not_to receive(:perform_async) + execute_service + end + end + context 'when passing many user ids' do before do stub_licensed_features(multiple_issue_assignees: false) diff --git a/spec/services/members/creator_service_spec.rb b/spec/services/members/creator_service_spec.rb new file mode 100644 index 00000000000..ff5bf705b6c --- /dev/null +++ b/spec/services/members/creator_service_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Members::CreatorService do + let_it_be(:source, reload: true) { create(:group, :public) } + let_it_be(:member_type) { GroupMember } + let_it_be(:user) { create(:user) } + let_it_be(:current_user) { create(:user) } + + describe '#execute' do + it 'raises error for new member on authorization check implementation' do + expect do + described_class.new(source, user, :maintainer, current_user: current_user).execute + end.to raise_error(NotImplementedError) + end + + it 'raises error for an existing member on authorization check implementation' do + source.add_developer(user) + + expect do + described_class.new(source, user, :maintainer, current_user: current_user).execute + end.to raise_error(NotImplementedError) + end + end +end diff --git a/spec/services/members/invite_service_spec.rb b/spec/services/members/invite_service_spec.rb index 8ceb9979f33..ab740138a8b 100644 --- a/spec/services/members/invite_service_spec.rb +++ b/spec/services/members/invite_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ let_it_be(:project, reload: true) { create(:project) } let_it_be(:user) { project.first_owner } let_it_be(:project_user) { create(:user) } + let_it_be(:user_invited_by_id) { create(:user) } let_it_be(:namespace) { project.namespace } let(:params) { {} } @@ -41,148 +42,422 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end end - context 'when email is not a valid email' do - let(:params) { { email: '_bogus_' } } + context 'when invites are passed as array' do + context 'with emails' do + let(:params) { { email: %w[email@example.org email2@example.org] } } - it 'returns an error' do - expect_not_to_create_members - expect(result[:message]['_bogus_']).to eq("Invite email is invalid") + it 'successfully creates members' do + expect_to_create_members(count: 2) + expect(result[:status]).to eq(:success) + end + end + + context 'with user_ids as integers' do + let(:params) { { user_ids: [project_user.id, user_invited_by_id.id] } } + + it 'successfully creates members' do + expect_to_create_members(count: 2) + expect(result[:status]).to eq(:success) + end end - it_behaves_like 'does not record an onboarding progress action' + context 'with user_ids as strings' do + let(:params) { { user_ids: [project_user.id.to_s, user_invited_by_id.id.to_s] } } + + it 'successfully creates members' do + expect_to_create_members(count: 2) + expect(result[:status]).to eq(:success) + end + end + + context 'with a mixture of emails and user_ids' do + let(:params) do + { user_ids: [project_user.id, user_invited_by_id.id], email: %w[email@example.org email2@example.org] } + end + + it 'successfully creates members' do + expect_to_create_members(count: 4) + expect(result[:status]).to eq(:success) + end + end end - context 'when emails are passed as an array' do - let(:params) { { email: %w[email@example.org email2@example.org] } } + context 'with multiple invites passed as strings' do + context 'with emails' do + let(:params) { { email: 'email@example.org,email2@example.org' } } - it 'successfully creates members' do - expect_to_create_members(count: 2) - expect(result[:status]).to eq(:success) + it 'successfully creates members' do + expect_to_create_members(count: 2) + expect(result[:status]).to eq(:success) + end + end + + context 'with user_ids' do + let(:params) { { user_ids: "#{project_user.id},#{user_invited_by_id.id}" } } + + it 'successfully creates members' do + expect_to_create_members(count: 2) + expect(result[:status]).to eq(:success) + end + end + + context 'with a mixture of emails and user_ids' do + let(:params) do + { user_ids: "#{project_user.id},#{user_invited_by_id.id}", email: 'email@example.org,email2@example.org' } + end + + it 'successfully creates members' do + expect_to_create_members(count: 4) + expect(result[:status]).to eq(:success) + end end end - context 'when emails are passed as an empty string' do - let(:params) { { email: '' } } + context 'when invites formats are mixed' do + context 'when user_ids is an array and emails is a string' do + let(:params) do + { user_ids: [project_user.id, user_invited_by_id.id], email: 'email@example.org,email2@example.org' } + end + + it 'successfully creates members' do + expect_to_create_members(count: 4) + expect(result[:status]).to eq(:success) + end + end + + context 'when user_ids is a string and emails is an array' do + let(:params) do + { user_ids: "#{project_user.id},#{user_invited_by_id.id}", email: %w[email@example.org email2@example.org] } + end - it 'returns an error' do - expect_not_to_create_members - expect(result[:message]).to eq('Emails cannot be blank') + it 'successfully creates members' do + expect_to_create_members(count: 4) + expect(result[:status]).to eq(:success) + end end end - context 'when email param is not included' do - it 'returns an error' do - expect_not_to_create_members - expect(result[:message]).to eq('Emails cannot be blank') + context 'when invites are passed in different formats' do + context 'when emails are passed as an empty string' do + let(:params) { { email: '' } } + + it 'returns an error' do + expect_not_to_create_members + expect(result[:message]).to eq('Invites cannot be blank') + end + end + + context 'when user_ids are passed as an empty string' do + let(:params) { { user_ids: '' } } + + it 'returns an error' do + expect_not_to_create_members + expect(result[:message]).to eq('Invites cannot be blank') + end + end + + context 'when user_ids and emails are both passed as empty strings' do + let(:params) { { user_ids: '', email: '' } } + + it 'returns an error' do + expect_not_to_create_members + expect(result[:message]).to eq('Invites cannot be blank') + end + end + + context 'when user_id is passed as an integer' do + let(:params) { { user_ids: project_user.id } } + + it 'successfully creates member' do + expect_to_create_members(count: 1) + expect(result[:status]).to eq(:success) + end + end + + context 'when invite params are not included' do + it 'returns an error' do + expect_not_to_create_members + expect(result[:message]).to eq('Invites cannot be blank') + end end end context 'when email is not a valid email format' do - let(:params) { { email: '_bogus_' } } + context 'with singular email' do + let(:params) { { email: '_bogus_' } } - it 'returns an error' do - expect { result }.not_to change(ProjectMember, :count) - expect(result[:status]).to eq(:error) - expect(result[:message][params[:email]]).to eq("Invite email is invalid") + it 'returns an error' do + expect_not_to_create_members + expect(result[:status]).to eq(:error) + expect(result[:message][params[:email]]).to eq("Invite email is invalid") + end + + it_behaves_like 'does not record an onboarding progress action' + end + + context 'with user_id and singular invalid email' do + let(:params) { { user_ids: project_user.id, email: '_bogus_' } } + + it 'has partial success' do + expect_to_create_members(count: 1) + expect(project.users).to include project_user + + expect(result[:status]).to eq(:error) + expect(result[:message][params[:email]]).to eq("Invite email is invalid") + end end end - context 'when duplicate email addresses are passed' do - let(:params) { { email: 'email@example.org,email@example.org' } } + context 'with duplicate invites' do + context 'with duplicate emails' do + let(:params) { { email: 'email@example.org,email@example.org' } } - it 'only creates one member per unique address' do - expect_to_create_members(count: 1) - expect(result[:status]).to eq(:success) + it 'only creates one member per unique invite' do + expect_to_create_members(count: 1) + expect(result[:status]).to eq(:success) + end + end + + context 'with duplicate user ids' do + let(:params) { { user_ids: "#{project_user.id},#{project_user.id}" } } + + it 'only creates one member per unique invite' do + expect_to_create_members(count: 1) + expect(result[:status]).to eq(:success) + end + end + + context 'with duplicate member by adding as user id and email' do + let(:params) { { user_ids: project_user.id, email: project_user.email } } + + it 'only creates one member per unique invite' do + expect_to_create_members(count: 1) + expect(result[:status]).to eq(:success) + end end end - context 'when observing email limits' do - let_it_be(:emails) { Array(1..101).map { |n| "email#{n}@example.com" } } + context 'when observing invite limits' do + context 'with emails and in general' do + let_it_be(:emails) { Array(1..101).map { |n| "email#{n}@example.com" } } - context 'when over the allowed default limit of emails' do - let(:params) { { email: emails } } + context 'when over the allowed default limit of emails' do + let(:params) { { email: emails } } - it 'limits the number of emails to 100' do - expect_not_to_create_members - expect(result[:message]).to eq('Too many users specified (limit is 100)') + it 'limits the number of emails to 100' do + expect_not_to_create_members + expect(result[:message]).to eq('Too many users specified (limit is 100)') + end + end + + context 'when over the allowed custom limit of emails' do + let(:params) { { email: 'email@example.org,email2@example.org', limit: 1 } } + + it 'limits the number of emails to the limit supplied' do + expect_not_to_create_members + expect(result[:message]).to eq('Too many users specified (limit is 1)') + end + end + + context 'when limit allowed is disabled via limit param' do + let(:params) { { email: emails, limit: -1 } } + + it 'does not limit number of emails' do + expect_to_create_members(count: 101) + expect(result[:status]).to eq(:success) + end end end - context 'when over the allowed custom limit of emails' do - let(:params) { { email: 'email@example.org,email2@example.org', limit: 1 } } + context 'with user_ids' do + let(:user_ids) { 1.upto(101).to_a.join(',') } + let(:params) { { user_ids: user_ids } } - it 'limits the number of emails to the limit supplied' do + it 'limits the number of users to 100' do expect_not_to_create_members - expect(result[:message]).to eq('Too many users specified (limit is 1)') + expect(result[:message]).to eq('Too many users specified (limit is 100)') end end + end - context 'when limit allowed is disabled via limit param' do - let(:params) { { email: emails, limit: -1 } } + context 'with an existing user' do + context 'with email' do + let(:params) { { email: project_user.email } } - it 'does not limit number of emails' do - expect_to_create_members(count: 101) + it 'adds an existing user to members' do + expect_to_create_members(count: 1) expect(result[:status]).to eq(:success) + expect(project.users).to include project_user end end - end - context 'when email belongs to an existing user' do - let(:params) { { email: project_user.email } } + context 'with user_id' do + let(:params) { { user_ids: project_user.id } } - it 'adds an existing user to members' do - expect_to_create_members(count: 1) - expect(result[:status]).to eq(:success) - expect(project.users).to include project_user + it_behaves_like 'records an onboarding progress action', :user_added + + it 'adds an existing user to members' do + expect_to_create_members(count: 1) + expect(result[:status]).to eq(:success) + expect(project.users).to include project_user + end + + context 'when assigning tasks to be done' do + let(:params) do + { user_ids: project_user.id, tasks_to_be_done: %w(ci code), tasks_project_id: project.id } + end + + it 'creates 2 task issues', :aggregate_failures do + expect(TasksToBeDone::CreateWorker) + .to receive(:perform_async) + .with(anything, user.id, [project_user.id]) + .once + .and_call_original + expect { result }.to change { project.issues.count }.by(2) + + expect(project.issues).to all have_attributes(project: project, author: user) + end + end end end context 'when access level is not valid' do - let(:params) { { email: project_user.email, access_level: -1 } } + context 'with email' do + let(:params) { { email: project_user.email, access_level: -1 } } - it 'returns an error' do - expect_not_to_create_members - expect(result[:message][project_user.email]) - .to eq("Access level is not included in the list") + it 'returns an error' do + expect_not_to_create_members + expect(result[:message][project_user.email]).to eq("Access level is not included in the list") + end end - end - context 'when invite already exists for an included email' do - let!(:invited_member) { create(:project_member, :invited, project: project) } - let(:params) { { email: "#{invited_member.invite_email},#{project_user.email}" } } + context 'with user_id' do + let(:params) { { user_ids: user_invited_by_id.id, access_level: -1 } } - it 'adds new email and returns an error for the already invited email' 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(project.users).to include project_user + it 'returns an error' do + expect_not_to_create_members + expect(result[:message][user_invited_by_id.username]).to eq("Access level is not included in the list") + end end - end - context 'when access request already exists for an included email' do - let!(:requested_member) { create(:project_member, :access_request, project: project) } - let(:params) { { email: "#{requested_member.user.email},#{project_user.email}" } } + context 'with a mix of user_id and email' do + let(:params) { { user_ids: user_invited_by_id.id, email: project_user.email, access_level: -1 } } - it 'adds new email and returns an error for the already invited email' do - expect_to_create_members(count: 1) - expect(result[:status]).to eq(:error) - expect(result[:message][requested_member.user.email]) - .to eq("User already exists in source") - expect(project.users).to include project_user + it 'returns errors' do + expect_not_to_create_members + expect(result[:message][project_user.email]).to eq("Access level is not included in the list") + expect(result[:message][user_invited_by_id.username]).to eq("Access level is not included in the list") + end end end - context 'when email is already a member on the project' do - let!(:existing_member) { create(:project_member, :guest, project: project) } - let(:params) { { email: "#{existing_member.user.email},#{project_user.email}" } } + 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}" } } + + it 'adds new email and returns an error for the already invited email' 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(project.users).to include project_user + end + end - it 'adds new email and returns an error for the already invited email' do - expect_to_create_members(count: 1) - expect(result[:status]).to eq(:error) - expect(result[:message][existing_member.user.email]) - .to eq("User already exists in source") - expect(project.users).to include project_user + context 'when email is already a member with a user on the project' do + let!(:existing_member) { create(:project_member, :guest, project: project) } + let(:params) { { email: "#{existing_member.user.email}" } } + + it 'returns an error for the already invited email' do + expect_not_to_create_members + expect(result[:message][existing_member.user.email]).to eq("User already exists in source") + end + + context 'when email belongs to an existing user as a secondary email' do + let(:secondary_email) { create(:email, email: 'secondary@example.com', user: existing_member.user) } + let(:params) { { email: "#{secondary_email.email}" } } + + it 'returns an error for the already invited email' do + expect_not_to_create_members + expect(result[:message][secondary_email.email]).to eq("User already exists in source") + end + end + end + + context 'with user_id that already exists' do + let!(:existing_member) { create(:project_member, project: project, user: project_user) } + let(:params) { { user_ids: existing_member.user_id } } + + it 'does not add the member again and is successful' do + expect_to_create_members(count: 0) + expect(project.users).to include project_user + end + end + + context 'with user_id that already exists with a lower access_level' do + let!(:existing_member) { create(:project_member, :developer, project: project, user: project_user) } + let(:params) { { user_ids: existing_member.user_id, access_level: ProjectMember::MAINTAINER } } + + it 'does not add the member again and updates the access_level' do + expect_to_create_members(count: 0) + expect(project.users).to include project_user + expect(existing_member.reset.access_level).to eq ProjectMember::MAINTAINER + end + end + + context 'with user_id that already exists with a higher access_level' do + let!(:existing_member) { create(:project_member, :developer, project: project, user: project_user) } + let(:params) { { user_ids: existing_member.user_id, access_level: ProjectMember::GUEST } } + + it 'does not add the member again and updates the access_level' do + expect_to_create_members(count: 0) + expect(project.users).to include project_user + expect(existing_member.reset.access_level).to eq ProjectMember::GUEST + end + end + + context 'with user_id that already exists in a parent group' do + let_it_be(:group) { create(:group) } + let_it_be(:group_member) { create(:group_member, :developer, source: group, user: project_user) } + let_it_be(:project, reload: true) { create(:project, group: group) } + let_it_be(:user) { project.creator } + + before_all do + project.add_maintainer(user) + end + + context 'when access_level is lower than inheriting member' do + let(:params) { { user_ids: group_member.user_id, access_level: ProjectMember::GUEST }} + + it 'does not add the member and returns an error' do + msg = "Access level should be greater than or equal " \ + "to Developer inherited membership from group #{group.name}" + + expect_not_to_create_members + expect(result[:message][project_user.username]).to eq msg + end + end + + context 'when access_level is the same as the inheriting member' do + let(:params) { { user_ids: group_member.user_id, access_level: ProjectMember::DEVELOPER }} + + it 'adds the member with correct access_level' do + expect_to_create_members(count: 1) + expect(project.users).to include project_user + expect(project.project_members.last.access_level).to eq ProjectMember::DEVELOPER + end + end + + context 'when access_level is greater than the inheriting member' do + let(:params) { { user_ids: group_member.user_id, access_level: ProjectMember::MAINTAINER }} + + it 'adds the member with correct access_level' do + expect_to_create_members(count: 1) + expect(project.users).to include project_user + expect(project.project_members.last.access_level).to eq ProjectMember::MAINTAINER + end + end end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index eb587797201..30095ebeb50 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do let(:user) { create(:user) } let(:user2) { create(:user) } let(:user3) { create(:user) } - let(:label) { create(:label, project: project) } + let(:label) { create(:label, title: 'a', project: project) } let(:label2) { create(:label) } let(:milestone) { create(:milestone, project: project) } @@ -1192,5 +1192,18 @@ RSpec.describe MergeRequests::UpdateService, :mailer do let(:existing_merge_request) { create(:merge_request, source_project: project) } let(:issuable) { described_class.new(project: project, current_user: user, params: params).execute(existing_merge_request) } end + + context 'labels are updated' do + let(:label_a) { label } + let(:label_b) { create(:label, title: 'b', project: project) } + let(:issuable) { merge_request } + + it_behaves_like 'keeps issuable labels sorted after update' + it_behaves_like 'broadcasting issuable labels updates' + + def update_issuable(update_params) + update_merge_request(update_params) + end + end end end diff --git a/spec/services/metrics/dashboard/custom_dashboard_service_spec.rb b/spec/services/metrics/dashboard/custom_dashboard_service_spec.rb index 5dc30c156ac..afeb1646005 100644 --- a/spec/services/metrics/dashboard/custom_dashboard_service_spec.rb +++ b/spec/services/metrics/dashboard/custom_dashboard_service_spec.rb @@ -15,7 +15,7 @@ RSpec.describe Metrics::Dashboard::CustomDashboardService, :use_clean_rails_memo subject { described_class.new(*service_params) } before do - project.add_maintainer(user) + project.add_maintainer(user) if user end describe '#raw_dashboard' do diff --git a/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb b/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb index 82321dbc822..127cec6275c 100644 --- a/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe Metrics::Dashboard::CustomMetricEmbedService do let_it_be(:environment) { create(:environment, project: project) } before do - project.add_maintainer(user) + project.add_maintainer(user) if user end let(:dashboard_path) { system_dashboard_path } diff --git a/spec/services/metrics/dashboard/default_embed_service_spec.rb b/spec/services/metrics/dashboard/default_embed_service_spec.rb index 2ce10eac026..647778eadc1 100644 --- a/spec/services/metrics/dashboard/default_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/default_embed_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe Metrics::Dashboard::DefaultEmbedService, :use_clean_rails_memory_ let_it_be(:environment) { create(:environment, project: project) } before do - project.add_maintainer(user) + project.add_maintainer(user) if user end describe '.valid_params?' do diff --git a/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb b/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb index 3c533b0c464..5eb8f24266c 100644 --- a/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe Metrics::Dashboard::DynamicEmbedService, :use_clean_rails_memory_ let_it_be(:environment) { create(:environment, project: project) } before do - project.add_maintainer(user) + project.add_maintainer(user) if user end let(:dashboard_path) { '.gitlab/dashboards/test.yml' } diff --git a/spec/services/metrics/dashboard/self_monitoring_dashboard_service_spec.rb b/spec/services/metrics/dashboard/self_monitoring_dashboard_service_spec.rb index 33b7e3c85cd..d0cefdbeb30 100644 --- a/spec/services/metrics/dashboard/self_monitoring_dashboard_service_spec.rb +++ b/spec/services/metrics/dashboard/self_monitoring_dashboard_service_spec.rb @@ -12,7 +12,7 @@ RSpec.describe Metrics::Dashboard::SelfMonitoringDashboardService, :use_clean_ra let(:service_params) { [project, user, { environment: environment }] } before do - project.add_maintainer(user) + project.add_maintainer(user) if user stub_application_setting(self_monitoring_project_id: project.id) end diff --git a/spec/services/metrics/dashboard/system_dashboard_service_spec.rb b/spec/services/metrics/dashboard/system_dashboard_service_spec.rb index ced7c29b507..e1c6aaeec66 100644 --- a/spec/services/metrics/dashboard/system_dashboard_service_spec.rb +++ b/spec/services/metrics/dashboard/system_dashboard_service_spec.rb @@ -15,7 +15,7 @@ RSpec.describe Metrics::Dashboard::SystemDashboardService, :use_clean_rails_memo subject { described_class.new(*service_params) } before do - project.add_maintainer(user) + project.add_maintainer(user) if user end describe '#raw_dashboard' do diff --git a/spec/services/metrics/dashboard/transient_embed_service_spec.rb b/spec/services/metrics/dashboard/transient_embed_service_spec.rb index 3fd0c97d909..53ea83c33d6 100644 --- a/spec/services/metrics/dashboard/transient_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/transient_embed_service_spec.rb @@ -8,7 +8,7 @@ RSpec.describe Metrics::Dashboard::TransientEmbedService, :use_clean_rails_memor let_it_be(:environment) { create(:environment, project: project) } before do - project.add_maintainer(user) + project.add_maintainer(user) if user end describe '.valid_params?' do diff --git a/spec/services/namespaces/in_product_marketing_email_records_spec.rb b/spec/services/namespaces/in_product_marketing_email_records_spec.rb index e5f1b275f9c..d80e20135d5 100644 --- a/spec/services/namespaces/in_product_marketing_email_records_spec.rb +++ b/spec/services/namespaces/in_product_marketing_email_records_spec.rb @@ -15,7 +15,7 @@ RSpec.describe Namespaces::InProductMarketingEmailRecords do before do allow(Users::InProductMarketingEmail).to receive(:bulk_insert!) - records.add(user, :invite_team, 0) + records.add(user, :team_short, 0) records.add(user, :create, 1) end @@ -33,13 +33,13 @@ RSpec.describe Namespaces::InProductMarketingEmailRecords do describe '#add' do it 'adds a Users::InProductMarketingEmail record to its records' do freeze_time do - records.add(user, :invite_team, 0) + records.add(user, :team_short, 0) records.add(user, :create, 1) first, second = records.records expect(first).to be_a Users::InProductMarketingEmail - expect(first.track.to_sym).to eq :invite_team + expect(first.track.to_sym).to eq :team_short expect(first.series).to eq 0 expect(first.created_at).to eq Time.zone.now expect(first.updated_at).to eq Time.zone.now 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 58ba577b7e7..de84666ca1d 100644 --- a/spec/services/namespaces/in_product_marketing_emails_service_spec.rb +++ b/spec/services/namespaces/in_product_marketing_emails_service_spec.rb @@ -183,7 +183,7 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do expect( Users::InProductMarketingEmail.where( user: user, - track: Users::InProductMarketingEmail.tracks[:create], + track: Users::InProductMarketingEmail::ACTIVE_TRACKS[:create], series: 0 ) ).to exist diff --git a/spec/services/namespaces/invite_team_email_service_spec.rb b/spec/services/namespaces/invite_team_email_service_spec.rb deleted file mode 100644 index 60ba91f433d..00000000000 --- a/spec/services/namespaces/invite_team_email_service_spec.rb +++ /dev/null @@ -1,128 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Namespaces::InviteTeamEmailService do - let_it_be(:user) { create(:user, email_opted_in: true) } - - let(:track) { described_class::TRACK } - let(:series) { 0 } - - let(:setup_for_company) { true } - let(:parent_group) { nil } - let(:group) { create(:group, parent: parent_group) } - - subject(:action) { described_class.send_email(user, group) } - - before do - group.add_owner(user) - allow(group).to receive(:setup_for_company).and_return(setup_for_company) - allow(Notify).to receive(:in_product_marketing_email).and_return(double(deliver_later: nil)) - end - - RSpec::Matchers.define :send_invite_team_email do |*args| - match do - expect(Notify).to have_received(:in_product_marketing_email).with(*args).once - end - - match_when_negated do - expect(Notify).not_to have_received(:in_product_marketing_email) - end - end - - shared_examples 'unexperimented' do - it { is_expected.not_to send_invite_team_email } - - it 'does not record sent email' do - expect { subject }.not_to change { Users::InProductMarketingEmail.count } - end - end - - shared_examples 'candidate' do - it { is_expected.to send_invite_team_email(user.id, group.id, track, 0) } - - it 'records sent email' do - expect { subject }.to change { Users::InProductMarketingEmail.count }.by(1) - - expect( - Users::InProductMarketingEmail.where( - user: user, - track: track, - series: 0 - ) - ).to exist - end - - it_behaves_like 'tracks assignment and records the subject', :invite_team_email, :group do - subject { group } - end - end - - context 'when group is in control path' do - before do - stub_experiments(invite_team_email: :control) - end - - it { is_expected.not_to send_invite_team_email } - - it 'does not record sent email' do - expect { subject }.not_to change { Users::InProductMarketingEmail.count } - end - - it_behaves_like 'tracks assignment and records the subject', :invite_team_email, :group do - subject { group } - end - end - - context 'when group is in candidate path' do - before do - stub_experiments(invite_team_email: :candidate) - end - - it_behaves_like 'candidate' - - context 'when the user has not opted into marketing emails' do - let(:user) { create(:user, email_opted_in: false ) } - - it_behaves_like 'unexperimented' - end - - context 'when group is not top level' do - it_behaves_like 'unexperimented' do - let(:parent_group) do - create(:group).tap { |g| g.add_owner(user) } - end - end - end - - context 'when group is not set up for a company' do - it_behaves_like 'unexperimented' do - let(:setup_for_company) { nil } - end - end - - context 'when other users have already been added to the group' do - before do - group.add_developer(create(:user)) - end - - it_behaves_like 'unexperimented' - end - - context 'when other users have already been invited to the group' do - before do - group.add_developer('not_a_user_yet@example.com') - end - - it_behaves_like 'unexperimented' - end - - context 'when the user already got sent the email' do - before do - create(:in_product_marketing_email, user: user, track: track, series: 0) - end - - it_behaves_like 'unexperimented' - end - end -end diff --git a/spec/services/notes/build_service_spec.rb b/spec/services/notes/build_service_spec.rb index b7b08390dcd..0e2bbcc8c66 100644 --- a/spec/services/notes/build_service_spec.rb +++ b/spec/services/notes/build_service_spec.rb @@ -5,12 +5,14 @@ require 'spec_helper' RSpec.describe Notes::BuildService do include AdminModeHelper - let(:note) { create(:discussion_note_on_issue) } - let(:project) { note.project } - let(:author) { note.author } - let(:user) { author } - let(:merge_request) { create(:merge_request, source_project: project) } - let(:mr_note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project, author: note.author) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:note) { create(:discussion_note_on_issue, project: project) } + let_it_be(:author) { note.author } + let_it_be(:user) { author } + let_it_be(:noteable_author) { create(:user) } + let_it_be(:other_user) { create(:user) } + let_it_be(:external) { create(:user, :external) } + let(:base_params) { { note: 'Test' } } let(:params) { {} } @@ -28,11 +30,10 @@ RSpec.describe Notes::BuildService do end context 'when discussion is resolved' do - let(:params) { { in_reply_to_discussion_id: mr_note.discussion_id } } + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + let_it_be(:mr_note) { create(:discussion_note_on_merge_request, :resolved, noteable: merge_request, project: project, author: author) } - before do - mr_note.resolve!(author) - end + let(:params) { { in_reply_to_discussion_id: mr_note.discussion_id } } it 'resolves the note' do expect(new_note).to be_valid @@ -57,7 +58,7 @@ RSpec.describe Notes::BuildService do end context 'when user has no access to discussion' do - let(:user) { create(:user) } + let(:user) { other_user } it 'sets an error' do expect(new_note.errors[:base]).to include('Discussion to reply to cannot be found') @@ -65,16 +66,14 @@ RSpec.describe Notes::BuildService do end context 'personal snippet note' do - def reply(note, user = nil) - user ||= create(:user) - + def reply(note, user = other_user) described_class.new(nil, user, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute end - let(:snippet_author) { create(:user) } + let_it_be(:snippet_author) { noteable_author } context 'when a snippet is public' do it 'creates a reply note' do @@ -89,8 +88,8 @@ RSpec.describe Notes::BuildService do end context 'when a snippet is private' do - let(:snippet) { create(:personal_snippet, :private, author: snippet_author) } - let(:note) { create(:discussion_note_on_personal_snippet, noteable: snippet) } + let_it_be(:snippet) { create(:personal_snippet, :private, author: snippet_author) } + let_it_be(:note) { create(:discussion_note_on_personal_snippet, noteable: snippet) } it 'creates a reply note when the author replies' do new_note = reply(note, snippet_author) @@ -107,8 +106,8 @@ RSpec.describe Notes::BuildService do end context 'when a snippet is internal' do - let(:snippet) { create(:personal_snippet, :internal, author: snippet_author) } - let(:note) { create(:discussion_note_on_personal_snippet, noteable: snippet) } + let_it_be(:snippet) { create(:personal_snippet, :internal, author: snippet_author) } + let_it_be(:note) { create(:discussion_note_on_personal_snippet, noteable: snippet) } it 'creates a reply note when the author replies' do new_note = reply(note, snippet_author) @@ -125,7 +124,7 @@ RSpec.describe Notes::BuildService do end it 'sets an error when an external user replies' do - new_note = reply(note, create(:user, :external)) + new_note = reply(note, external) expect(new_note.errors[:base]).to include('Discussion to reply to cannot be found') end @@ -134,7 +133,8 @@ RSpec.describe Notes::BuildService do end context 'when replying to individual note' do - let(:note) { create(:note_on_issue) } + let_it_be(:note) { create(:note_on_issue, project: project) } + let(:params) { { in_reply_to_discussion_id: note.discussion_id } } it 'sets the note up to be in reply to that note' do @@ -144,7 +144,7 @@ RSpec.describe Notes::BuildService do end context 'when noteable does not support replies' do - let(:note) { create(:note_on_commit) } + let_it_be(:note) { create(:note_on_commit, project: project) } it 'builds another individual note' do expect(new_note).to be_valid @@ -155,87 +155,137 @@ RSpec.describe Notes::BuildService do end context 'confidential comments' do + let_it_be(:project) { create(:project, :public) } + let_it_be(:guest) { create(:user) } + let_it_be(:reporter) { create(:user) } + let_it_be(:admin) { create(:admin) } + let_it_be(:issuable_assignee) { other_user } + let_it_be(:issue) do + create(:issue, project: project, author: noteable_author, assignees: [issuable_assignee]) + end + before do - project.add_reporter(author) + project.add_guest(guest) + project.add_reporter(reporter) end - context 'when replying to a confidential comment' do - let(:note) { create(:note_on_issue, confidential: true) } - let(:params) { { in_reply_to_discussion_id: note.discussion_id, confidential: false } } + context 'when creating a new confidential comment' do + let(:params) { { confidential: true, noteable: issue } } - context 'when the user can read confidential comments' do - it '`confidential` param is ignored and set to `true`' do - expect(new_note.confidential).to be_truthy - end + shared_examples 'user allowed to set comment as confidential' do + it { expect(new_note.confidential).to be_truthy } end - context 'when the user cannot read confidential comments' do - let(:user) { create(:user) } + shared_examples 'user not allowed to set comment as confidential' do + it { expect(new_note.confidential).to be_falsey } + end - it 'returns `Discussion to reply to cannot be found` error' do - expect(new_note.errors.added?(:base, "Discussion to reply to cannot be found")).to be true + context 'reporter' do + let(:user) { reporter } + + it_behaves_like 'user allowed to set comment as confidential' + end + + context 'issuable author' do + let(:user) { noteable_author } + + it_behaves_like 'user allowed to set comment as confidential' + end + + context 'issuable assignee' do + let(:user) { issuable_assignee } + + it_behaves_like 'user allowed to set comment as confidential' + end + + context 'admin' do + before do + enable_admin_mode!(admin) end + + let(:user) { admin } + + it_behaves_like 'user allowed to set comment as confidential' end - end - context 'when replying to a public comment' do - let(:note) { create(:note_on_issue, confidential: false) } - let(:params) { { in_reply_to_discussion_id: note.discussion_id, confidential: true } } + context 'external' do + let(:user) { external } - it '`confidential` param is ignored and set to `false`' do - expect(new_note.confidential).to be_falsey + it_behaves_like 'user not allowed to set comment as confidential' + end + + context 'guest' do + let(:user) { guest } + + it_behaves_like 'user not allowed to set comment as confidential' end end - context 'when creating a new comment' do - context 'when the `confidential` note flag is set to `true`' do - context 'when the user is allowed (reporter)' do - let(:params) { { confidential: true, noteable: merge_request } } + context 'when replying to a confidential comment' do + let_it_be(:note) { create(:note_on_issue, confidential: true, noteable: issue, project: project) } - it 'note `confidential` flag is set to `true`' do - expect(new_note.confidential).to be_truthy - end - end + let(:params) { { in_reply_to_discussion_id: note.discussion_id, confidential: false } } - context 'when the user is allowed (issuable author)' do - let(:user) { create(:user) } - let(:issue) { create(:issue, author: user) } - let(:params) { { confidential: true, noteable: issue } } + shared_examples 'returns `Discussion to reply to cannot be found` error' do + it do + expect(new_note.errors.added?(:base, "Discussion to reply to cannot be found")).to be true + end + end - it 'note `confidential` flag is set to `true`' do - expect(new_note.confidential).to be_truthy - end + shared_examples 'confidential set to `true`' do + it '`confidential` param is ignored to match the parent note confidentiality' do + expect(new_note.confidential).to be_truthy end + end - context 'when the user is allowed (admin)' do - before do - enable_admin_mode!(admin) - end + context 'with reporter access' do + let(:user) { reporter } + + it_behaves_like 'confidential set to `true`' + end - let(:admin) { create(:admin) } - let(:params) { { confidential: true, noteable: merge_request } } + context 'with admin access' do + let(:user) { admin } - it 'note `confidential` flag is set to `true`' do - expect(new_note.confidential).to be_truthy - end + before do + enable_admin_mode!(admin) end - context 'when the user is not allowed' do - let(:user) { create(:user) } - let(:params) { { confidential: true, noteable: merge_request } } + it_behaves_like 'confidential set to `true`' + end + + context 'with noteable author' do + let(:user) { note.noteable.author } - it 'note `confidential` flag is set to `false`' do - expect(new_note.confidential).to be_falsey - end - end + it_behaves_like 'confidential set to `true`' end - context 'when the `confidential` note flag is set to `false`' do - let(:params) { { confidential: false, noteable: merge_request } } + context 'with noteable assignee' do + let(:user) { issuable_assignee } - it 'note `confidential` flag is set to `false`' do - expect(new_note.confidential).to be_falsey - end + it_behaves_like 'confidential set to `true`' + end + + context 'with guest access' do + let(:user) { guest } + + it_behaves_like 'returns `Discussion to reply to cannot be found` error' + end + + context 'with external user' do + let(:user) { external } + + it_behaves_like 'returns `Discussion to reply to cannot be found` error' + end + end + + context 'when replying to a public comment' do + let_it_be(:note) { create(:note_on_issue, confidential: false, noteable: issue, project: project) } + + let(:params) { { in_reply_to_discussion_id: note.discussion_id, confidential: true } } + + it '`confidential` param is ignored and set to `false`' do + expect(new_note.confidential).to be_falsey end end end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index babbd44a9f1..b0410123630 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -106,7 +106,8 @@ RSpec.describe Notes::CreateService do type: 'DiffNote', noteable_type: 'MergeRequest', noteable_id: merge_request.id, - position: position.to_h) + position: position.to_h, + confidential: false) end before do @@ -141,7 +142,8 @@ RSpec.describe Notes::CreateService do type: 'DiffNote', noteable_type: 'MergeRequest', noteable_id: merge_request.id, - position: position.to_h) + position: position.to_h, + confidential: false) expect(merge_request).not_to receive(:diffs) @@ -173,7 +175,8 @@ RSpec.describe Notes::CreateService do type: 'DiffNote', noteable_type: 'MergeRequest', noteable_id: merge_request.id, - position: position.to_h) + position: position.to_h, + confidential: false) end it 'note is associated with a note diff file' do @@ -201,7 +204,8 @@ RSpec.describe Notes::CreateService do type: 'DiffNote', noteable_type: 'MergeRequest', noteable_id: merge_request.id, - position: position.to_h) + position: position.to_h, + confidential: false) end it 'note is not associated with a note diff file' do @@ -230,7 +234,8 @@ RSpec.describe Notes::CreateService do type: 'DiffNote', noteable_type: 'MergeRequest', noteable_id: merge_request.id, - position: image_position.to_h) + position: image_position.to_h, + confidential: false) end it 'note is not associated with a note diff file' do @@ -306,7 +311,7 @@ RSpec.describe Notes::CreateService do let_it_be(:merge_request) { create(:merge_request, source_project: project, labels: [bug_label]) } let(:issuable) { merge_request } - let(:note_params) { opts.merge(noteable_type: 'MergeRequest', noteable_id: merge_request.id) } + let(:note_params) { opts.merge(noteable_type: 'MergeRequest', noteable_id: merge_request.id, confidential: false) } let(:merge_request_quick_actions) do [ QuickAction.new( diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index 71ac1641ca5..ae7bea30944 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -138,45 +138,6 @@ RSpec.describe Notes::UpdateService do end end - context 'setting confidentiality' do - let(:opts) { { confidential: true } } - - context 'simple note' do - it 'updates the confidentiality' do - expect { update_note(opts) }.to change { note.reload.confidential }.from(nil).to(true) - end - end - - context 'discussion notes' do - let(:note) { create(:discussion_note, project: project, noteable: issue, author: user, note: "Old note #{user2.to_reference}") } - let!(:response_note_1) { create(:discussion_note, project: project, noteable: issue, in_reply_to: note) } - let!(:response_note_2) { create(:discussion_note, project: project, noteable: issue, in_reply_to: note, confidential: false) } - let!(:other_note) { create(:note, project: project, noteable: issue) } - - context 'when updating the root note' do - it 'updates the confidentiality of the root note and all the responses' do - update_note(opts) - - expect(note.reload.confidential).to be_truthy - expect(response_note_1.reload.confidential).to be_truthy - expect(response_note_2.reload.confidential).to be_truthy - expect(other_note.reload.confidential).to be_falsey - end - end - - context 'when updating one of the response notes' do - it 'updates only the confidentiality of the note that is being updated' do - Notes::UpdateService.new(project, user, opts).execute(response_note_1) - - expect(note.reload.confidential).to be_falsey - expect(response_note_1.reload.confidential).to be_truthy - expect(response_note_2.reload.confidential).to be_falsey - expect(other_note.reload.confidential).to be_falsey - end - end - end - end - context 'todos' do shared_examples 'does not update todos' do it 'keep todos' do diff --git a/spec/services/notification_recipients/builder/default_spec.rb b/spec/services/notification_recipients/builder/default_spec.rb index c142cc11384..4d0ddc7c4f7 100644 --- a/spec/services/notification_recipients/builder/default_spec.rb +++ b/spec/services/notification_recipients/builder/default_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe NotificationRecipients::Builder::Default do describe '#build!' do let_it_be(:group) { create(:group, :public) } - let_it_be(:project) { create(:project, :public, group: group).tap { |p| p.add_developer(project_watcher) } } + let_it_be(:project) { create(:project, :public, group: group).tap { |p| p.add_developer(project_watcher) if project_watcher } } let_it_be(:target) { create(:issue, project: project) } let_it_be(:current_user) { create(:user) } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 399b2b4be2d..d2d55c5ab79 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -376,6 +376,17 @@ RSpec.describe NotificationService, :mailer do end end + describe '#new_email_address_added' do + let_it_be(:user) { create(:user) } + let_it_be(:email) { create(:email, user: user) } + + subject { notification.new_email_address_added(user, email) } + + it 'sends email to the user' do + expect { subject }.to have_enqueued_email(user, email, mail: 'new_email_address_added_email') + end + end + describe 'Notes' do context 'issue note' do let_it_be(:project) { create(:project, :private) } @@ -2090,6 +2101,70 @@ RSpec.describe NotificationService, :mailer do should_not_email(@u_lazy_participant) end + describe 'triggers push_to_merge_request_email with corresponding email' do + let_it_be(:merge_request) { create(:merge_request, author: author, source_project: project) } + + def mock_commits(length) + Array.new(length) { |i| double(:commit, short_id: SecureRandom.hex(4), title: "This is commit #{i}") } + end + + def commit_to_hash(commit) + { short_id: commit.short_id, title: commit.title } + end + + let(:existing_commits) { mock_commits(50) } + let(:expected_existing_commits) { [commit_to_hash(existing_commits.first), commit_to_hash(existing_commits.last)] } + + before do + allow(::Notify).to receive(:push_to_merge_request_email).and_call_original + end + + where(:number_of_new_commits, :number_of_new_commits_displayed) do + limit = described_class::NEW_COMMIT_EMAIL_DISPLAY_LIMIT + [ + [0, 0], + [limit - 2, limit - 2], + [limit - 1, limit - 1], + [limit, limit], + [limit + 1, limit], + [limit + 2, limit] + ] + end + + with_them do + let(:new_commits) { mock_commits(number_of_new_commits) } + let(:expected_new_commits) { new_commits.first(number_of_new_commits_displayed).map(&method(:commit_to_hash)) } + + it 'triggers the corresponding mailer method with list of stripped commits' do + notification.push_to_merge_request( + merge_request, merge_request.author, + new_commits: new_commits, existing_commits: existing_commits + ) + + expect(Notify).to have_received(:push_to_merge_request_email).at_least(:once).with( + @subscriber.id, merge_request.id, merge_request.author.id, "subscribed", + new_commits: expected_new_commits, total_new_commits_count: number_of_new_commits, + existing_commits: expected_existing_commits, total_existing_commits_count: 50 + ) + end + end + + context 'there is only one existing commit' do + let(:new_commits) { mock_commits(10) } + let(:expected_new_commits) { new_commits.map(&method(:commit_to_hash)) } + + it 'triggers corresponding mailer method with only one existing commit' do + notification.push_to_merge_request(merge_request, merge_request.author, new_commits: new_commits, existing_commits: existing_commits.first(1)) + + expect(Notify).to have_received(:push_to_merge_request_email).at_least(:once).with( + @subscriber.id, merge_request.id, merge_request.author.id, "subscribed", + new_commits: expected_new_commits, total_new_commits_count: 10, + existing_commits: expected_existing_commits.first(1), total_existing_commits_count: 1 + ) + end + end + end + it_behaves_like 'participating notifications' do let(:participant) { create(:user, username: 'user-participant') } let(:issuable) { merge_request } diff --git a/spec/services/packages/rubygems/metadata_extraction_service_spec.rb b/spec/services/packages/rubygems/metadata_extraction_service_spec.rb index b308daad8f5..bbd5b6f3d59 100644 --- a/spec/services/packages/rubygems/metadata_extraction_service_spec.rb +++ b/spec/services/packages/rubygems/metadata_extraction_service_spec.rb @@ -46,5 +46,13 @@ RSpec.describe Packages::Rubygems::MetadataExtractionService do expect(metadata.requirements).to eq(gemspec.requirements.to_json) expect(metadata.rubygems_version).to eq(gemspec.rubygems_version) end + + context 'with an existing metadatum' do + let_it_be(:metadatum) { create(:rubygems_metadatum, package: package) } + + it 'updates it' do + expect { subject }.not_to change { Packages::Rubygems::Metadatum.count } + end + end end end diff --git a/spec/services/projects/apple_target_platform_detector_service_spec.rb b/spec/services/projects/apple_target_platform_detector_service_spec.rb new file mode 100644 index 00000000000..6391161824c --- /dev/null +++ b/spec/services/projects/apple_target_platform_detector_service_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::AppleTargetPlatformDetectorService do + let_it_be(:project) { build(:project) } + + subject { described_class.new(project).execute } + + context 'when project is not an xcode project' do + before do + allow(Gitlab::FileFinder).to receive(:new) { instance_double(Gitlab::FileFinder, find: []) } + end + + it 'returns an empty array' do + is_expected.to match_array [] + end + end + + context 'when project is an xcode project' do + using RSpec::Parameterized::TableSyntax + + let(:finder) { instance_double(Gitlab::FileFinder) } + + before do + allow(Gitlab::FileFinder).to receive(:new) { finder } + end + + def search_query(sdk, filename) + "SDKROOT = #{sdk} filename:#{filename}" + end + + context 'when setting string is found' do + where(:sdk, :filename, :result) do + 'iphoneos' | 'project.pbxproj' | [:ios] + 'iphoneos' | '*.xcconfig' | [:ios] + end + + with_them do + before do + allow(finder).to receive(:find).with(anything) { [] } + allow(finder).to receive(:find).with(search_query(sdk, filename)) { [instance_double(Gitlab::Search::FoundBlob)] } + end + + it 'returns an array of unique detected targets' do + is_expected.to match_array result + end + end + end + + context 'when setting string is not found' do + before do + allow(finder).to receive(:find).with(anything) { [] } + end + + it 'returns an empty array' do + is_expected.to match_array [] + end + end + end +end diff --git a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb index 38a3e00c8e7..86c0ba4222c 100644 --- a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb @@ -13,7 +13,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService, :clean_gitlab_ let(:tags) { %w[latest A Ba Bb C D E] } before do - project.add_maintainer(user) + project.add_maintainer(user) if user stub_container_registry_config(enabled: true) diff --git a/spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb b/spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb index 7fc963949eb..22cada7816b 100644 --- a/spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb @@ -58,7 +58,19 @@ RSpec.describe Projects::ContainerRepository::ThirdParty::DeleteTagsService do stub_put_manifest_request('Ba', 500, {}) end - it { is_expected.to eq(status: :error, message: 'could not delete tags') } + it { is_expected.to eq(status: :error, message: "could not delete tags: #{tags.join(', ')}")} + + context 'when a large list of tag updates fails' do + let(:tags) { Array.new(1000) { |i| "tag_#{i}" } } + + before do + expect(service).to receive(:replace_tag_manifests).and_return({}) + end + + it 'truncates the log message' do + expect(subject).to eq(status: :error, message: "could not delete tags: #{tags.join(', ')}".truncate(1000)) + end + end end context 'a single tag update fails' do diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 96a50b26871..c5c5af3cb01 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -135,10 +135,22 @@ RSpec.describe Projects::CreateService, '#execute' do create_project(user, opts) end - it 'builds associated project settings' do + it 'creates associated project settings' do project = create_project(user, opts) - expect(project.project_setting).to be_new_record + expect(project.project_setting).to be_persisted + end + + context 'create_project_settings feature flag is disabled' do + before do + stub_feature_flags(create_project_settings: false) + end + + it 'builds associated project settings' do + project = create_project(user, opts) + + expect(project.project_setting).to be_new_record + end end it_behaves_like 'storing arguments in the application context' do @@ -376,6 +388,18 @@ RSpec.describe Projects::CreateService, '#execute' do imported_project end + + describe 'import scheduling' do + context 'when project import type is gitlab project migration' do + it 'does not schedule project import' do + opts[:import_type] = 'gitlab_project_migration' + + project = create_project(user, opts) + + expect(project.import_state.status).to eq('none') + end + end + end end context 'builds_enabled global setting' do diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb index b64f2d1e7d6..3ee867ba6f2 100644 --- a/spec/services/projects/operations/update_service_spec.rb +++ b/spec/services/projects/operations/update_service_spec.rb @@ -407,10 +407,11 @@ RSpec.describe Projects::Operations::UpdateService do context 'prometheus integration' do context 'prometheus params were passed into service' do - let(:prometheus_integration) do - build_stubbed(:prometheus_integration, project: project, properties: { + let!(:prometheus_integration) do + create(:prometheus_integration, :instance, properties: { api_url: "http://example.prometheus.com", - manual_configuration: "0" + manual_configuration: "0", + google_iap_audience_client_id: 123 }) end @@ -424,21 +425,23 @@ RSpec.describe Projects::Operations::UpdateService do end it 'uses Project#find_or_initialize_integration to include instance defined defaults and pass them to Projects::UpdateService', :aggregate_failures do - project_update_service = double(Projects::UpdateService) - - expect(project) - .to receive(:find_or_initialize_integration) - .with('prometheus') - .and_return(prometheus_integration) expect(Projects::UpdateService).to receive(:new) do |project_arg, user_arg, update_params_hash| + prometheus_attrs = update_params_hash[:prometheus_integration_attributes] + expect(project_arg).to eq project expect(user_arg).to eq user - expect(update_params_hash[:prometheus_integration_attributes]).to include('properties' => { 'api_url' => 'http://new.prometheus.com', 'manual_configuration' => '1' }) - expect(update_params_hash[:prometheus_integration_attributes]).not_to include(*%w(id project_id created_at updated_at)) - end.and_return(project_update_service) - expect(project_update_service).to receive(:execute) + expect(prometheus_attrs).to have_key('encrypted_properties') + expect(prometheus_attrs.keys).not_to include(*%w(id project_id created_at updated_at properties)) + expect(prometheus_attrs['encrypted_properties']).not_to eq(prometheus_integration.encrypted_properties) + end.and_call_original - subject.execute + expect { subject.execute }.to change(Integrations::Prometheus, :count).by(1) + + expect(Integrations::Prometheus.last).to have_attributes( + api_url: 'http://new.prometheus.com', + manual_configuration: true, + google_iap_audience_client_id: 123 + ) end end diff --git a/spec/services/projects/record_target_platforms_service_spec.rb b/spec/services/projects/record_target_platforms_service_spec.rb new file mode 100644 index 00000000000..85311f36428 --- /dev/null +++ b/spec/services/projects/record_target_platforms_service_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::RecordTargetPlatformsService, '#execute' do + let_it_be(:project) { create(:project) } + + subject(:execute) { described_class.new(project).execute } + + context 'when project is an XCode project' do + before do + double = instance_double(Projects::AppleTargetPlatformDetectorService, execute: [:ios, :osx]) + allow(Projects::AppleTargetPlatformDetectorService).to receive(:new) { double } + end + + it 'creates a new setting record for the project', :aggregate_failures do + expect { execute }.to change { ProjectSetting.count }.from(0).to(1) + expect(ProjectSetting.last.target_platforms).to match_array(%w(ios osx)) + end + + it 'returns array of detected target platforms' do + expect(execute).to match_array %w(ios osx) + end + + context 'when a project has an existing setting record' do + before do + create(:project_setting, project: project, target_platforms: saved_target_platforms) + end + + def project_setting + ProjectSetting.find_by_project_id(project.id) + end + + context 'when target platforms changed' do + let(:saved_target_platforms) { %w(tvos) } + + it 'updates' do + expect { execute }.to change { project_setting.target_platforms }.from(%w(tvos)).to(%w(ios osx)) + end + + it { is_expected.to match_array %w(ios osx) } + end + + context 'when target platforms are the same' do + let(:saved_target_platforms) { %w(osx ios) } + + it 'does not update' do + expect { execute }.not_to change { project_setting.updated_at } + end + end + end + end + + context 'when project is not an XCode project' do + before do + double = instance_double(Projects::AppleTargetPlatformDetectorService, execute: []) + allow(Projects::AppleTargetPlatformDetectorService).to receive(:new).with(project) { double } + end + + it 'does nothing' do + expect { execute }.not_to change { ProjectSetting.count } + end + + it { is_expected.to be_nil } + end +end diff --git a/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb b/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb index 41de8c6bdbb..41487e9ea48 100644 --- a/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb +++ b/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb @@ -10,7 +10,8 @@ RSpec.describe Projects::RefreshBuildArtifactsSizeStatisticsService, :clean_gitl let_it_be(:artifact_1) { create(:ci_job_artifact, project: project, size: 1, created_at: 14.days.ago) } let_it_be(:artifact_2) { create(:ci_job_artifact, project: project, size: 2, created_at: 13.days.ago) } - let_it_be(:artifact_3) { create(:ci_job_artifact, project: project, size: 5, created_at: 12.days.ago) } + let_it_be(:artifact_3) { create(:ci_job_artifact, project: project, size: nil, created_at: 13.days.ago) } + let_it_be(:artifact_4) { create(:ci_job_artifact, project: project, size: 5, created_at: 12.days.ago) } # This should not be included in the recalculation as it is created later than the refresh start time let_it_be(:future_artifact) { create(:ci_job_artifact, project: project, size: 8, created_at: 2.days.from_now) } @@ -33,7 +34,7 @@ RSpec.describe Projects::RefreshBuildArtifactsSizeStatisticsService, :clean_gitl end before do - stub_const("#{described_class}::BATCH_SIZE", 2) + stub_const("#{described_class}::BATCH_SIZE", 3) stats = create(:project_statistics, project: project, build_artifacts_size: 120) stats.increment_counter(:build_artifacts_size, 30) @@ -48,7 +49,7 @@ RSpec.describe Projects::RefreshBuildArtifactsSizeStatisticsService, :clean_gitl end it 'updates the last_job_artifact_id to the ID of the last artifact from the batch' do - expect { service.execute }.to change { refresh.reload.last_job_artifact_id.to_i }.to(artifact_2.id) + expect { service.execute }.to change { refresh.reload.last_job_artifact_id.to_i }.to(artifact_3.id) end it 'requeues the refresh job' do @@ -62,7 +63,7 @@ RSpec.describe Projects::RefreshBuildArtifactsSizeStatisticsService, :clean_gitl :project_build_artifacts_size_refresh, :pending, project: project, - last_job_artifact_id: artifact_2.id + last_job_artifact_id: artifact_3.id ) end @@ -73,7 +74,7 @@ RSpec.describe Projects::RefreshBuildArtifactsSizeStatisticsService, :clean_gitl end it 'keeps the last_job_artifact_id unchanged' do - expect(refresh.reload.last_job_artifact_id).to eq(artifact_2.id) + expect(refresh.reload.last_job_artifact_id).to eq(artifact_3.id) end it 'keeps the state of the refresh record at running' do @@ -89,7 +90,7 @@ RSpec.describe Projects::RefreshBuildArtifactsSizeStatisticsService, :clean_gitl project: project, updated_at: 2.days.ago, refresh_started_at: now, - last_job_artifact_id: artifact_3.id + last_job_artifact_id: artifact_4.id ) end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index fb94e94fd18..e547ace1d9f 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -11,8 +11,9 @@ RSpec.describe Projects::TransferService do let(:project) { create(:project, :repository, :legacy_storage, namespace: user.namespace) } let(:target) { group } + let(:executor) { user } - subject(:execute_transfer) { described_class.new(project, user).execute(target).tap { project.reload } } + subject(:execute_transfer) { described_class.new(project, executor).execute(target).tap { project.reload } } context 'with npm packages' do before do @@ -92,6 +93,55 @@ RSpec.describe Projects::TransferService do end end + context 'project in a group -> a personal namespace', :enable_admin_mode do + let(:project) { create(:project, :repository, :legacy_storage, group: group) } + let(:target) { user.namespace } + # We need to use an admin user as the executor because + # only an admin user has required permissions to transfer projects + # under _all_ the different circumstances specified below. + let(:executor) { create(:user, :admin) } + + it 'executes the transfer to personal namespace successfully' do + execute_transfer + + expect(project.namespace).to eq(user.namespace) + end + + context 'the owner of the namespace does not have a direct membership in the project residing in the group' do + it 'creates a project membership record for the owner of the namespace, with OWNER access level, after the transfer' do + execute_transfer + + expect(project.members.owners.find_by(user_id: user.id)).to be_present + end + end + + context 'the owner of the namespace has a direct membership in the project residing in the group' do + context 'that membership has an access level of OWNER' do + before do + project.add_owner(user) + end + + it 'retains the project membership record for the owner of the namespace, with OWNER access level, after the transfer' do + execute_transfer + + expect(project.members.owners.find_by(user_id: user.id)).to be_present + end + end + + context 'that membership has an access level that is not OWNER' do + before do + project.add_developer(user) + end + + it 'updates the project membership record for the owner of the namespace, to OWNER access level, after the transfer' do + execute_transfer + + expect(project.members.owners.find_by(user_id: user.id)).to be_present + end + end + end + end + context 'when transfer succeeds' do before do group.add_owner(user) @@ -148,23 +198,23 @@ RSpec.describe Projects::TransferService do context 'with a project integration' do let_it_be_with_reload(:project) { create(:project, namespace: user.namespace) } - let_it_be(:instance_integration) { create(:integrations_slack, :instance, webhook: 'http://project.slack.com') } + let_it_be(:instance_integration) { create(:integrations_slack, :instance) } + let_it_be(:project_integration) { create(:integrations_slack, project: project) } - context 'with an inherited integration' do - let_it_be(:project_integration) { create(:integrations_slack, project: project, webhook: 'http://project.slack.com', inherit_from_id: instance_integration.id) } + context 'when it inherits from instance_integration' do + before do + project_integration.update!(inherit_from_id: instance_integration.id, webhook: instance_integration.webhook) + end it 'replaces inherited integrations', :aggregate_failures do - execute_transfer - - expect(project.slack_integration.webhook).to eq(group_integration.webhook) - expect(Integration.count).to eq(3) + expect { execute_transfer } + .to change(Integration, :count).by(0) + .and change { project.slack_integration.webhook }.to eq(group_integration.webhook) end end context 'with a custom integration' do - let_it_be(:project_integration) { create(:integrations_slack, project: project, webhook: 'http://project.slack.com') } - - it 'does not updates the integrations' do + it 'does not update the integrations' do expect { execute_transfer }.not_to change { project.slack_integration.webhook } end end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 94e0e8a9ea1..85dbc39edcf 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -671,6 +671,19 @@ RSpec.describe QuickActions::InterpretService do end shared_examples 'assign command' do + it 'assigns to users with escaped underscores' do + user = create(:user) + base = user.username + user.update!(username: "#{base}_") + issuable.project.add_developer(user) + + cmd = "/assign @#{base}\\_" + + _, updates, _ = service.execute(cmd, issuable) + + expect(updates).to eq(assignee_ids: [user.id]) + end + it 'assigns to a single user' do _, updates, _ = service.execute(content, issuable) @@ -726,6 +739,17 @@ RSpec.describe QuickActions::InterpretService do expect(reviewer).to be_attention_requested end + + it 'supports attn alias' do + attn_cmd = content.gsub(/attention/, 'attn') + _, _, message = service.execute(attn_cmd, issuable) + + expect(message).to eq("Requested attention from #{developer.to_reference}.") + + reviewer.reload + + expect(reviewer).to be_attention_requested + end end shared_examples 'remove attention command' do @@ -800,7 +824,7 @@ RSpec.describe QuickActions::InterpretService do let(:project) { repository_project } let(:service) { described_class.new(project, developer, {}) } - it_behaves_like 'failed command', 'Merge request diff sha parameter is required for the merge quick action.' do + it_behaves_like 'failed command', 'The `/merge` quick action requires the SHA of the head of the branch.' do let(:content) { "/merge" } let(:issuable) { merge_request } end diff --git a/spec/services/service_ping/build_payload_service_spec.rb b/spec/services/service_ping/build_payload_service_spec.rb index b90e5e66518..cd2685069c9 100644 --- a/spec/services/service_ping/build_payload_service_spec.rb +++ b/spec/services/service_ping/build_payload_service_spec.rb @@ -4,10 +4,6 @@ require 'spec_helper' RSpec.describe ServicePing::BuildPayloadService do describe '#execute', :without_license do - before do - stub_feature_flags(merge_service_ping_instrumented_metrics: false) - end - subject(:service_ping_payload) { described_class.new.execute } include_context 'stubbed service ping metrics definitions' do diff --git a/spec/services/task_list_toggle_service_spec.rb b/spec/services/task_list_toggle_service_spec.rb index 81f80ee926a..f889f298213 100644 --- a/spec/services/task_list_toggle_service_spec.rb +++ b/spec/services/task_list_toggle_service_spec.rb @@ -16,6 +16,8 @@ RSpec.describe TaskListToggleService do - [ ] loose list with an embedded paragraph + + + [ ] No-break space (U+00A0) EOT end @@ -40,12 +42,17 @@ RSpec.describe TaskListToggleService do </ul> </li> </ol> - <ul data-sourcepos="9:1-11:28" class="task-list" dir="auto"> - <li data-sourcepos="9:1-11:28" class="task-list-item"> + <ul data-sourcepos="9:1-12:0" class="task-list" dir="auto"> + <li data-sourcepos="9:1-12:0" class="task-list-item"> <p data-sourcepos="9:3-9:16"><input type="checkbox" class="task-list-item-checkbox" disabled=""> loose list</p> <p data-sourcepos="11:3-11:28">with an embedded paragraph</p> </li> </ul> + <ul data-sourcepos="13:1-13:21" class="task-list" dir="auto"> + <li data-sourcepos="13:1-13:21" class="task-list-item"> + <input type="checkbox" class="task-list-item-checkbox" disabled=""> No-break space (U+00A0) + </li> + </ul> EOT end @@ -79,6 +86,16 @@ RSpec.describe TaskListToggleService do expect(toggler.updated_markdown_html).to include('disabled checked> loose list') end + it 'checks task with no-break space' do + toggler = described_class.new(markdown, markdown_html, + toggle_as_checked: true, + line_source: '+ [ ] No-break space (U+00A0)', line_number: 13) + + expect(toggler.execute).to be_truthy + expect(toggler.updated_markdown.lines[12]).to eq "+ [x] No-break space (U+00A0)" + expect(toggler.updated_markdown_html).to include('disabled checked> No-break space (U+00A0)') + end + it 'returns false if line_source does not match the text' do toggler = described_class.new(markdown, markdown_html, toggle_as_checked: false, diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index 602db66dba1..80a506bb1d6 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -332,4 +332,39 @@ RSpec.describe Users::DestroyService do expect(User.exists?(other_user.id)).to be(false) end end + + context 'batched nullify' do + let(:other_user) { create(:user) } + + context 'when :nullify_in_batches_on_user_deletion feature flag is enabled' do + it 'nullifies related associations in batches' do + expect(other_user).to receive(:nullify_dependent_associations_in_batches).and_call_original + + described_class.new(user).execute(other_user, skip_authorization: true) + end + + it 'nullifies last_updated_issues and closed_issues' do + issue = create(:issue, closed_by: other_user, updated_by: other_user) + + described_class.new(user).execute(other_user, skip_authorization: true) + + issue.reload + + expect(issue.closed_by).to be_nil + expect(issue.updated_by).to be_nil + end + end + + context 'when :nullify_in_batches_on_user_deletion feature flag is disabled' do + before do + stub_feature_flags(nullify_in_batches_on_user_deletion: false) + end + + it 'does not use batching' do + expect(other_user).not_to receive(:nullify_dependent_associations_in_batches) + + described_class.new(user).execute(other_user, skip_authorization: true) + end + end + end end diff --git a/spec/services/users/saved_replies/destroy_service_spec.rb b/spec/services/users/saved_replies/destroy_service_spec.rb new file mode 100644 index 00000000000..cb97fac7b7c --- /dev/null +++ b/spec/services/users/saved_replies/destroy_service_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::SavedReplies::DestroyService do + describe '#execute' do + let!(:saved_reply) { create(:saved_reply) } + + subject { described_class.new(saved_reply: saved_reply).execute } + + context 'when destroy fails' do + before do + allow(saved_reply).to receive(:destroy).and_return(false) + end + + it 'does not remove Saved Reply from database' do + expect { subject }.not_to change(::Users::SavedReply, :count) + end + + it { is_expected.not_to be_success } + end + + context 'when destroy succeeds' do + it { is_expected.to be_success } + + it 'removes Saved Reply from database' do + expect { subject }.to change(::Users::SavedReply, :count).by(-1) + end + + it 'returns saved reply' do + expect(subject[:saved_reply]).to eq(saved_reply) + end + end + end +end diff --git a/spec/services/users/saved_replies/update_service_spec.rb b/spec/services/users/saved_replies/update_service_spec.rb index b67d09977c6..bdb54d7c8f7 100644 --- a/spec/services/users/saved_replies/update_service_spec.rb +++ b/spec/services/users/saved_replies/update_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Users::SavedReplies::UpdateService do let_it_be(:other_saved_reply) { create(:saved_reply, user: current_user) } let_it_be(:saved_reply_from_other_user) { create(:saved_reply) } - subject { described_class.new(current_user: current_user, saved_reply: saved_reply, name: name, content: content).execute } + subject { described_class.new(saved_reply: saved_reply, name: name, content: content).execute } context 'when update fails' do let(:name) { other_saved_reply.name } diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index c938ad9ee39..b99bc860523 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -107,6 +107,21 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state ).once end + context 'when the data is a Gitlab::DataBuilder::Pipeline' do + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:data) { ::Gitlab::DataBuilder::Pipeline.new(pipeline) } + + it 'can log the request payload' do + stub_full_request(project_hook.url, method: :post) + + # we call this with force to ensure that the logs are written inline, + # which tests that we can serialize the data to the DB correctly. + service = described_class.new(project_hook, data, :push_hooks, force: true) + + expect { service.execute }.to change(::WebHookLog, :count).by(1) + end + end + context 'when auth credentials are present' do let_it_be(:url) {'https://example.org'} let_it_be(:project_hook) { create(:project_hook, url: 'https://demo:demo@example.org/') } |