diff options
Diffstat (limited to 'spec/services')
81 files changed, 3104 insertions, 1071 deletions
diff --git a/spec/services/admin/propagate_integration_service_spec.rb b/spec/services/admin/propagate_integration_service_spec.rb index 49d974b7154..5dfe39aebbc 100644 --- a/spec/services/admin/propagate_integration_service_spec.rb +++ b/spec/services/admin/propagate_integration_service_spec.rb @@ -10,9 +10,7 @@ RSpec.describe Admin::PropagateIntegrationService do stub_jira_service_test end - let(:excluded_attributes) { %w[id project_id group_id inherit_from_id instance created_at updated_at default] } - let!(:project) { create(:project) } - let!(:group) { create(:group) } + let_it_be(:project) { create(:project) } let!(:instance_integration) do JiraService.create!( instance: true, @@ -39,7 +37,7 @@ RSpec.describe Admin::PropagateIntegrationService do let!(:not_inherited_integration) do JiraService.create!( - project: create(:project), + project: project, inherit_from_id: nil, instance: false, active: true, @@ -52,7 +50,7 @@ RSpec.describe Admin::PropagateIntegrationService do let!(:different_type_inherited_integration) do BambooService.create!( - project: create(:project), + project: project, inherit_from_id: instance_integration.id, instance: false, active: true, @@ -64,75 +62,37 @@ RSpec.describe Admin::PropagateIntegrationService do ) end - shared_examples 'inherits settings from integration' do - it 'updates the inherited integrations' do - described_class.propagate(instance_integration) + context 'with inherited integration' do + let(:integration) { inherited_integration } - expect(integration.reload.inherit_from_id).to eq(instance_integration.id) - expect(integration.attributes.except(*excluded_attributes)) - .to eq(instance_integration.attributes.except(*excluded_attributes)) - end + it 'calls to PropagateIntegrationProjectWorker' do + expect(PropagateIntegrationInheritWorker).to receive(:perform_async) + .with(instance_integration.id, inherited_integration.id, inherited_integration.id) - context 'integration with data fields' do - let(:excluded_attributes) { %w[id service_id created_at updated_at] } - - it 'updates the data fields from inherited integrations' do - described_class.propagate(instance_integration) - - expect(integration.reload.data_fields.attributes.except(*excluded_attributes)) - .to eq(instance_integration.data_fields.attributes.except(*excluded_attributes)) - end - end - end - - shared_examples 'does not inherit settings from integration' do - it 'does not update the not inherited integrations' do described_class.propagate(instance_integration) - - expect(integration.reload.attributes.except(*excluded_attributes)) - .not_to eq(instance_integration.attributes.except(*excluded_attributes)) end end - context 'update only inherited integrations' do - it_behaves_like 'inherits settings from integration' do - let(:integration) { inherited_integration } - end - - it_behaves_like 'does not inherit settings from integration' do - let(:integration) { not_inherited_integration } - end - - it_behaves_like 'does not inherit settings from integration' do - let(:integration) { different_type_inherited_integration } - end + context 'with a project without integration' do + let!(:another_project) { create(:project) } - it_behaves_like 'inherits settings from integration' do - let(:integration) { project.jira_service } - end + it 'calls to PropagateIntegrationProjectWorker' do + expect(PropagateIntegrationProjectWorker).to receive(:perform_async) + .with(instance_integration.id, another_project.id, another_project.id) - it_behaves_like 'inherits settings from integration' do - let(:integration) { Service.find_by(group_id: group.id) } + described_class.propagate(instance_integration) end end - it 'updates project#has_external_issue_tracker for issue tracker services' do - described_class.propagate(instance_integration) + context 'with a group without integration' do + let!(:group) { create(:group) } - expect(project.reload.has_external_issue_tracker).to eq(true) - end + it 'calls to PropagateIntegrationProjectWorker' do + expect(PropagateIntegrationGroupWorker).to receive(:perform_async) + .with(instance_integration.id, group.id, group.id) - it 'updates project#has_external_wiki for external wiki services' do - instance_integration = ExternalWikiService.create!( - instance: true, - active: true, - push_events: false, - external_wiki_url: 'http://external-wiki-url.com' - ) - - described_class.propagate(instance_integration) - - expect(project.reload.has_external_wiki).to eq(true) + described_class.propagate(instance_integration) + end end end end diff --git a/spec/services/admin/propagate_service_template_spec.rb b/spec/services/admin/propagate_service_template_spec.rb index 15654653095..d95d31ceaea 100644 --- a/spec/services/admin/propagate_service_template_spec.rb +++ b/spec/services/admin/propagate_service_template_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Admin::PropagateServiceTemplate do describe '.propagate' do + let_it_be(:project) { create(:project) } let!(:service_template) do PushoverService.create!( template: true, @@ -19,124 +20,40 @@ RSpec.describe Admin::PropagateServiceTemplate do ) end - let!(:project) { create(:project) } - let(:excluded_attributes) { %w[id project_id template created_at updated_at default] } - - it 'creates services for projects' do - expect(project.pushover_service).to be_nil + it 'calls to PropagateIntegrationProjectWorker' do + expect(PropagateIntegrationProjectWorker).to receive(:perform_async) + .with(service_template.id, project.id, project.id) described_class.propagate(service_template) - - expect(project.reload.pushover_service).to be_present - end - - it 'creates services for a project that has another service' do - BambooService.create!( - active: true, - project: project, - properties: { - bamboo_url: 'http://gitlab.com', - username: 'mic', - password: 'password', - build_key: 'build' - } - ) - - expect(project.pushover_service).to be_nil - - described_class.propagate(service_template) - - expect(project.reload.pushover_service).to be_present - end - - it 'does not create the service if it exists already' do - other_service = BambooService.create!( - template: true, - active: true, - properties: { - bamboo_url: 'http://gitlab.com', - username: 'mic', - password: 'password', - build_key: 'build' - } - ) - - Service.build_from_integration(project.id, service_template).save! - Service.build_from_integration(project.id, other_service).save! - - expect { described_class.propagate(service_template) } - .not_to change { Service.count } end - it 'creates the service containing the template attributes' do - described_class.propagate(service_template) - - expect(project.pushover_service.properties).to eq(service_template.properties) - - expect(project.pushover_service.attributes.except(*excluded_attributes)) - .to eq(service_template.attributes.except(*excluded_attributes)) - end - - context 'service with data fields' do - include JiraServiceHelper - - let(:service_template) do - stub_jira_service_test - - JiraService.create!( - template: true, + context 'with a project that has another service' do + before do + BambooService.create!( active: true, - push_events: false, - url: 'http://jira.instance.com', - username: 'user', - password: 'secret' + project: project, + properties: { + bamboo_url: 'http://gitlab.com', + username: 'mic', + password: 'password', + build_key: 'build' + } ) end - it 'creates the service containing the template attributes' do - described_class.propagate(service_template) - - expect(project.jira_service.attributes.except(*excluded_attributes)) - .to eq(service_template.attributes.except(*excluded_attributes)) - - excluded_attributes = %w[id service_id created_at updated_at] - expect(project.jira_service.data_fields.attributes.except(*excluded_attributes)) - .to eq(service_template.data_fields.attributes.except(*excluded_attributes)) - end - end - - describe 'bulk update', :use_sql_query_cache do - let(:project_total) { 5 } - - before do - stub_const('Admin::PropagateServiceTemplate::BATCH_SIZE', 3) - - project_total.times { create(:project) } + it 'calls to PropagateIntegrationProjectWorker' do + expect(PropagateIntegrationProjectWorker).to receive(:perform_async) + .with(service_template.id, project.id, project.id) described_class.propagate(service_template) end - - it 'creates services for all projects' do - expect(Service.all.reload.count).to eq(project_total + 2) - end - end - - describe 'external tracker' do - it 'updates the project external tracker' do - service_template.update!(category: 'issue_tracker') - - expect { described_class.propagate(service_template) } - .to change { project.reload.has_external_issue_tracker }.to(true) - end end - describe 'external wiki' do - it 'updates the project external tracker' do - service_template.update!(type: 'ExternalWikiService') + it 'does not create the service if it exists already' do + Service.build_from_integration(service_template, project_id: project.id).save! - expect { described_class.propagate(service_template) } - .to change { project.reload.has_external_wiki }.to(true) - end + expect { described_class.propagate(service_template) } + .not_to change { Service.count } end end end diff --git a/spec/services/alert_management/process_prometheus_alert_service_spec.rb b/spec/services/alert_management/process_prometheus_alert_service_spec.rb index b14cc65506a..ad4ab26c198 100644 --- a/spec/services/alert_management/process_prometheus_alert_service_spec.rb +++ b/spec/services/alert_management/process_prometheus_alert_service_spec.rb @@ -148,28 +148,20 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do expect { execute }.to change { alert.reload.resolved? }.to(true) end - [true, false].each do |state_tracking_enabled| - context 'existing issue' do - before do - stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - end - - let!(:alert) { create(:alert_management_alert, :with_issue, project: project, fingerprint: fingerprint) } - - it 'closes the issue' do - issue = alert.issue - - expect { execute } - .to change { issue.reload.state } - .from('opened') - .to('closed') - end - - if state_tracking_enabled - specify { expect { execute }.to change(ResourceStateEvent, :count).by(1) } - else - specify { expect { execute }.to change(Note, :count).by(1) } - end + context 'existing issue' do + let!(:alert) { create(:alert_management_alert, :with_issue, project: project, fingerprint: fingerprint) } + + it 'closes the issue' do + issue = alert.issue + + expect { execute } + .to change { issue.reload.state } + .from('opened') + .to('closed') + end + + it 'creates a resource state event' do + expect { execute }.to change(ResourceStateEvent, :count).by(1) end end end diff --git a/spec/services/audit_event_service_spec.rb b/spec/services/audit_event_service_spec.rb index 93de2a23edc..3317fcf8444 100644 --- a/spec/services/audit_event_service_spec.rb +++ b/spec/services/audit_event_service_spec.rb @@ -57,7 +57,7 @@ RSpec.describe AuditEventService do let(:audit_service) { described_class.new(user, user, with: 'standard') } it 'creates an authentication event' do - expect(AuthenticationEvent).to receive(:create).with( + expect(AuthenticationEvent).to receive(:new).with( user: user, user_name: user.name, ip_address: user.current_sign_in_ip, @@ -67,6 +67,17 @@ RSpec.describe AuditEventService do audit_service.for_authentication.security_event end + + it 'tracks exceptions when the event cannot be created' do + allow(user).to receive_messages(current_sign_in_ip: 'invalid IP') + + expect(Gitlab::ErrorTracking).to( + receive(:track_exception) + .with(ActiveRecord::RecordInvalid, audit_event_type: 'AuthenticationEvent').and_call_original + ) + + audit_service.for_authentication.security_event + end end end diff --git a/spec/services/bulk_create_integration_service_spec.rb b/spec/services/bulk_create_integration_service_spec.rb new file mode 100644 index 00000000000..5d896f78b35 --- /dev/null +++ b/spec/services/bulk_create_integration_service_spec.rb @@ -0,0 +1,107 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe BulkCreateIntegrationService do + include JiraServiceHelper + + before do + stub_jira_service_test + end + + let(:excluded_attributes) { %w[id project_id group_id inherit_from_id instance template created_at updated_at] } + let!(:instance_integration) { create(:jira_service, :instance) } + let!(:template_integration) { create(:jira_service, :template) } + + shared_examples 'creates integration from batch ids' do + it 'updates the inherited integrations' do + described_class.new(integration, batch, association).execute + + expect(created_integration.attributes.except(*excluded_attributes)) + .to eq(integration.attributes.except(*excluded_attributes)) + end + + context 'integration with data fields' do + let(:excluded_attributes) { %w[id service_id created_at updated_at] } + + it 'updates the data fields from inherited integrations' do + described_class.new(integration, batch, association).execute + + expect(created_integration.reload.data_fields.attributes.except(*excluded_attributes)) + .to eq(integration.data_fields.attributes.except(*excluded_attributes)) + end + end + end + + shared_examples 'updates inherit_from_id' do + it 'updates inherit_from_id attributes' do + described_class.new(integration, batch, association).execute + + expect(created_integration.reload.inherit_from_id).to eq(integration.id) + end + end + + shared_examples 'runs project callbacks' do + it 'updates projects#has_external_issue_tracker for issue tracker services' do + described_class.new(integration, batch, association).execute + + expect(project.reload.has_external_issue_tracker).to eq(true) + end + + context 'with an external wiki integration' do + let(:integration) do + ExternalWikiService.create!( + instance: true, + active: true, + push_events: false, + external_wiki_url: 'http://external-wiki-url.com' + ) + end + + it 'updates projects#has_external_wiki for external wiki services' do + described_class.new(integration, batch, association).execute + + expect(project.reload.has_external_wiki).to eq(true) + end + end + end + + context 'with an instance-level integration' do + let(:integration) { instance_integration } + + context 'with a project association' do + let!(:project) { create(:project) } + let(:created_integration) { project.jira_service } + let(:batch) { Project.all } + let(:association) { 'project' } + + it_behaves_like 'creates integration from batch ids' + it_behaves_like 'updates inherit_from_id' + it_behaves_like 'runs project callbacks' + end + + context 'with a group association' do + let!(:group) { create(:group) } + let(:created_integration) { Service.find_by(group: group) } + let(:batch) { Group.all } + let(:association) { 'group' } + + it_behaves_like 'creates integration from batch ids' + it_behaves_like 'updates inherit_from_id' + end + end + + context 'with a template integration' do + let(:integration) { template_integration } + + context 'with a project association' do + let!(:project) { create(:project) } + let(:created_integration) { project.jira_service } + let(:batch) { Project.all } + let(:association) { 'project' } + + it_behaves_like 'creates integration from batch ids' + it_behaves_like 'runs project callbacks' + end + end +end diff --git a/spec/services/bulk_update_integration_service_spec.rb b/spec/services/bulk_update_integration_service_spec.rb new file mode 100644 index 00000000000..2f0bfd31600 --- /dev/null +++ b/spec/services/bulk_update_integration_service_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe BulkUpdateIntegrationService do + include JiraServiceHelper + + before do + stub_jira_service_test + end + + let(:excluded_attributes) { %w[id project_id group_id inherit_from_id instance template created_at updated_at] } + let!(:instance_integration) do + JiraService.create!( + instance: true, + active: true, + push_events: true, + url: 'http://update-jira.instance.com', + username: 'user', + password: 'secret' + ) + end + + let!(:integration) do + JiraService.create!( + project: create(:project), + inherit_from_id: instance_integration.id, + instance: false, + active: true, + push_events: false, + url: 'http://jira.instance.com', + username: 'user', + password: 'secret' + ) + end + + context 'with inherited integration' do + it 'updates the integration' do + described_class.new(instance_integration, Service.inherit_from_id(instance_integration.id)).execute + + expect(integration.reload.inherit_from_id).to eq(instance_integration.id) + expect(integration.attributes.except(*excluded_attributes)) + .to eq(instance_integration.attributes.except(*excluded_attributes)) + end + + context 'with integration with data fields' do + let(:excluded_attributes) { %w[id service_id created_at updated_at] } + + it 'updates the data fields from the integration' do + described_class.new(instance_integration, Service.inherit_from_id(instance_integration.id)).execute + + expect(integration.reload.data_fields.attributes.except(*excluded_attributes)) + .to eq(instance_integration.data_fields.attributes.except(*excluded_attributes)) + end + end + end +end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index e0893ed6de3..c28c3449485 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -731,30 +731,11 @@ RSpec.describe Ci::CreatePipelineService do .and_call_original end - context 'when ci_pipeline_rewind_iid is enabled' do - before do - stub_feature_flags(ci_pipeline_rewind_iid: true) - end - - it 'rewinds iid' do - result = execute_service - - expect(result).not_to be_persisted - expect(internal_id.last_value).to eq(0) - end - end - - context 'when ci_pipeline_rewind_iid is disabled' do - before do - stub_feature_flags(ci_pipeline_rewind_iid: false) - end - - it 'does not rewind iid' do - result = execute_service + it 'rewinds iid' do + result = execute_service - expect(result).not_to be_persisted - expect(internal_id.last_value).to eq(1) - end + expect(result).not_to be_persisted + expect(internal_id.last_value).to eq(0) end end end diff --git a/spec/services/ci/expire_pipeline_cache_service_spec.rb b/spec/services/ci/expire_pipeline_cache_service_spec.rb index b5d664947de..8df5d0bc159 100644 --- a/spec/services/ci/expire_pipeline_cache_service_spec.rb +++ b/spec/services/ci/expire_pipeline_cache_service_spec.rb @@ -26,9 +26,11 @@ RSpec.describe Ci::ExpirePipelineCacheService do project = merge_request.target_project merge_request_pipelines_path = "/#{project.full_path}/-/merge_requests/#{merge_request.iid}/pipelines.json" + merge_request_widget_path = "/#{project.full_path}/-/merge_requests/#{merge_request.iid}/cached_widget.json" allow_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch) expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(merge_request_pipelines_path) + expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(merge_request_widget_path) subject.execute(merge_request.all_pipelines.last) end diff --git a/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb b/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb index 77645298bc7..2936d6fae4d 100644 --- a/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb +++ b/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb @@ -43,12 +43,12 @@ RSpec.shared_context 'Pipeline Processing Service Tests With Yaml' do { pipeline: pipeline.status, stages: pipeline.stages.pluck(:name, :status).to_h, - jobs: pipeline.statuses.latest.pluck(:name, :status).to_h + jobs: pipeline.latest_statuses.pluck(:name, :status).to_h } end def event_on_jobs(event, job_names) - statuses = pipeline.statuses.latest.by_name(job_names).to_a + statuses = pipeline.latest_statuses.by_name(job_names).to_a expect(statuses.count).to eq(job_names.count) # ensure that we have the same counts statuses.each { |status| status.public_send("#{event}!") } diff --git a/spec/services/ci/pipelines/create_artifact_service_spec.rb b/spec/services/ci/pipelines/create_artifact_service_spec.rb index d5e9cf83a6d..6f177889ed3 100644 --- a/spec/services/ci/pipelines/create_artifact_service_spec.rb +++ b/spec/services/ci/pipelines/create_artifact_service_spec.rb @@ -35,16 +35,6 @@ RSpec.describe ::Ci::Pipelines::CreateArtifactService do end end - context 'when feature is disabled' do - it 'does not create a pipeline artifact' do - stub_feature_flags(coverage_report_view: false) - - subject - - expect(Ci::PipelineArtifact.count).to eq(0) - end - end - context 'when pipeline artifact has already been created' do it 'do not raise an error and do not persist the same artifact twice' do expect { 2.times { described_class.new.execute(pipeline) } }.not_to raise_error(ActiveRecord::RecordNotUnique) diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 51741440075..4c1e698d52d 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -154,7 +154,7 @@ RSpec.describe Ci::RetryBuildService do describe '#execute' do let(:new_build) do - Timecop.freeze(1.second.from_now) do + travel_to(1.second.from_now) do service.execute(build) end end @@ -257,7 +257,7 @@ RSpec.describe Ci::RetryBuildService do describe '#reprocess' do let(:new_build) do - Timecop.freeze(1.second.from_now) do + travel_to(1.second.from_now) do service.reprocess!(build) end end diff --git a/spec/services/ci/update_build_queue_service_spec.rb b/spec/services/ci/update_build_queue_service_spec.rb index 0f4c0fa5ecb..ebccfdc5140 100644 --- a/spec/services/ci/update_build_queue_service_spec.rb +++ b/spec/services/ci/update_build_queue_service_spec.rb @@ -45,21 +45,7 @@ RSpec.describe Ci::UpdateBuildQueueService do runner.update!(contacted_at: Ci::Runner.recent_queue_deadline) end - context 'when ci_update_queues_for_online_runners is enabled' do - before do - stub_feature_flags(ci_update_queues_for_online_runners: true) - end - - it_behaves_like 'does not refresh runner' - end - - context 'when ci_update_queues_for_online_runners is disabled' do - before do - stub_feature_flags(ci_update_queues_for_online_runners: false) - end - - it_behaves_like 'refreshes runner' - end + it_behaves_like 'does not refresh runner' end end diff --git a/spec/services/ci/update_build_state_service_spec.rb b/spec/services/ci/update_build_state_service_spec.rb index f5ad732bf7e..aa1de368154 100644 --- a/spec/services/ci/update_build_state_service_spec.rb +++ b/spec/services/ci/update_build_state_service_spec.rb @@ -83,9 +83,26 @@ RSpec.describe Ci::UpdateBuildStateService do { checksum: 'crc32:12345678', state: 'failed', failure_reason: 'script_failure' } end + context 'when build does not have associated trace chunks' do + it 'updates a build status' do + result = subject.execute + + expect(build).to be_failed + expect(result.status).to eq 200 + end + + it 'does not increment invalid trace metric' do + execute_with_stubbed_metrics! + + expect(metrics) + .not_to have_received(:increment_trace_operation) + .with(operation: :invalid) + end + end + context 'when build trace has been migrated' do before do - create(:ci_build_trace_chunk, :database_with_data, build: build) + create(:ci_build_trace_chunk, :persisted, build: build, initial_data: 'abcd') end it 'updates a build state' do @@ -100,6 +117,12 @@ RSpec.describe Ci::UpdateBuildStateService do expect(result.status).to eq 200 end + it 'does not set a backoff value' do + result = subject.execute + + expect(result.backoff).to be_nil + end + it 'increments trace finalized operation metric' do execute_with_stubbed_metrics! @@ -107,6 +130,48 @@ RSpec.describe Ci::UpdateBuildStateService do .to have_received(:increment_trace_operation) .with(operation: :finalized) end + + context 'when trace checksum is not valid' do + it 'increments invalid trace metric' do + execute_with_stubbed_metrics! + + expect(metrics) + .to have_received(:increment_trace_operation) + .with(operation: :invalid) + end + end + + context 'when trace checksum is valid' do + let(:params) { { checksum: 'crc32:ed82cd11', state: 'success' } } + + it 'does not increment invalid trace metric' do + execute_with_stubbed_metrics! + + expect(metrics) + .not_to have_received(:increment_trace_operation) + .with(operation: :invalid) + end + end + + context 'when failed to acquire a build trace lock' do + it 'accepts a state update request' do + build.trace.lock do + result = subject.execute + + expect(result.status).to eq 202 + end + end + + it 'increment locked trace metric' do + build.trace.lock do + execute_with_stubbed_metrics! + + expect(metrics) + .to have_received(:increment_trace_operation) + .with(operation: :locked) + end + end + end end context 'when build trace has not been migrated yet' do @@ -126,6 +191,12 @@ RSpec.describe Ci::UpdateBuildStateService do expect(result.status).to eq 202 end + it 'sets a request backoff value' do + result = subject.execute + + expect(result.backoff.to_i).to be > 0 + end + it 'schedules live chunks for migration' do expect(Ci::BuildTraceChunkFlushWorker) .to receive(:perform_async) @@ -134,14 +205,6 @@ RSpec.describe Ci::UpdateBuildStateService do subject.execute end - it 'increments trace accepted operation metric' do - execute_with_stubbed_metrics! - - expect(metrics) - .to have_received(:increment_trace_operation) - .with(operation: :accepted) - end - it 'creates a pending state record' do subject.execute @@ -153,6 +216,22 @@ RSpec.describe Ci::UpdateBuildStateService do end end + it 'increments trace accepted operation metric' do + execute_with_stubbed_metrics! + + expect(metrics) + .to have_received(:increment_trace_operation) + .with(operation: :accepted) + end + + it 'does not increment invalid trace metric' do + execute_with_stubbed_metrics! + + expect(metrics) + .not_to have_received(:increment_trace_operation) + .with(operation: :invalid) + end + context 'when build pending state is outdated' do before do build.create_pending_state( diff --git a/spec/services/clusters/gcp/finalize_creation_service_spec.rb b/spec/services/clusters/gcp/finalize_creation_service_spec.rb index be362dc6e23..a20cf90a770 100644 --- a/spec/services/clusters/gcp/finalize_creation_service_spec.rb +++ b/spec/services/clusters/gcp/finalize_creation_service_spec.rb @@ -84,11 +84,9 @@ RSpec.describe Clusters::Gcp::FinalizeCreationService, '#execute' do before do stub_cloud_platform_get_zone_cluster( provider.gcp_project_id, provider.zone, cluster.name, - { - endpoint: endpoint, - username: username, - password: password - } + endpoint: endpoint, + username: username, + password: password ) stub_kubeclient_discover(api_url) @@ -101,11 +99,9 @@ RSpec.describe Clusters::Gcp::FinalizeCreationService, '#execute' do stub_kubeclient_get_secret( api_url, - { - metadata_name: secret_name, - token: Base64.encode64(token), - namespace: 'default' - } + metadata_name: secret_name, + token: Base64.encode64(token), + namespace: 'default' ) stub_kubeclient_put_cluster_role_binding(api_url, 'gitlab-admin') diff --git a/spec/services/clusters/kubernetes/configure_istio_ingress_service_spec.rb b/spec/services/clusters/kubernetes/configure_istio_ingress_service_spec.rb index b4402aadc88..f26177a56d0 100644 --- a/spec/services/clusters/kubernetes/configure_istio_ingress_service_spec.rb +++ b/spec/services/clusters/kubernetes/configure_istio_ingress_service_spec.rb @@ -26,27 +26,21 @@ RSpec.describe Clusters::Kubernetes::ConfigureIstioIngressService, '#execute' do stub_kubeclient_get_secret( api_url, - { - metadata_name: "#{namespace}-token", - token: Base64.encode64('sample-token'), - namespace: namespace - } + metadata_name: "#{namespace}-token", + token: Base64.encode64('sample-token'), + namespace: namespace ) stub_kubeclient_get_secret( api_url, - { - metadata_name: 'istio-ingressgateway-ca-certs', - namespace: 'istio-system' - } + metadata_name: 'istio-ingressgateway-ca-certs', + namespace: 'istio-system' ) stub_kubeclient_get_secret( api_url, - { - metadata_name: 'istio-ingressgateway-certs', - namespace: 'istio-system' - } + metadata_name: 'istio-ingressgateway-certs', + namespace: 'istio-system' ) stub_kubeclient_put_secret(api_url, 'istio-ingressgateway-ca-certs', namespace: 'istio-system') diff --git a/spec/services/clusters/kubernetes/create_or_update_namespace_service_spec.rb b/spec/services/clusters/kubernetes/create_or_update_namespace_service_spec.rb index ee10c59390e..7e3f1fdb379 100644 --- a/spec/services/clusters/kubernetes/create_or_update_namespace_service_spec.rb +++ b/spec/services/clusters/kubernetes/create_or_update_namespace_service_spec.rb @@ -41,11 +41,9 @@ RSpec.describe Clusters::Kubernetes::CreateOrUpdateNamespaceService, '#execute' stub_kubeclient_get_secret( api_url, - { - metadata_name: "#{namespace}-token", - token: Base64.encode64('sample-token'), - namespace: namespace - } + metadata_name: "#{namespace}-token", + token: Base64.encode64('sample-token'), + namespace: namespace ) end diff --git a/spec/services/clusters/kubernetes/fetch_kubernetes_token_service_spec.rb b/spec/services/clusters/kubernetes/fetch_kubernetes_token_service_spec.rb index c4daae9dbf0..7a283a974d2 100644 --- a/spec/services/clusters/kubernetes/fetch_kubernetes_token_service_spec.rb +++ b/spec/services/clusters/kubernetes/fetch_kubernetes_token_service_spec.rb @@ -31,11 +31,9 @@ RSpec.describe Clusters::Kubernetes::FetchKubernetesTokenService do before do stub_kubeclient_get_secret( api_url, - { - metadata_name: service_account_token_name, - namespace: namespace, - token: token - } + metadata_name: service_account_token_name, + namespace: namespace, + token: token ) end @@ -54,11 +52,9 @@ RSpec.describe Clusters::Kubernetes::FetchKubernetesTokenService do before do stub_kubeclient_get_secret_not_found_then_found( api_url, - { - metadata_name: service_account_token_name, - namespace: namespace, - token: token - } + metadata_name: service_account_token_name, + namespace: namespace, + token: token ) end @@ -96,11 +92,9 @@ RSpec.describe Clusters::Kubernetes::FetchKubernetesTokenService do before do stub_kubeclient_get_secret( api_url, - { - metadata_name: service_account_token_name, - namespace: namespace, - token: nil - } + metadata_name: service_account_token_name, + namespace: namespace, + token: nil ) end diff --git a/spec/services/deployments/after_create_service_spec.rb b/spec/services/deployments/after_create_service_spec.rb index 6cdb4c88191..ee9b008c2f8 100644 --- a/spec/services/deployments/after_create_service_spec.rb +++ b/spec/services/deployments/after_create_service_spec.rb @@ -269,14 +269,14 @@ RSpec.describe Deployments::AfterCreateService do it "does not overwrite the older 'first_deployed_to_production_at' time" do # Previous deploy time = 5.minutes.from_now - Timecop.freeze(time) { service.execute } + travel_to(time) { service.execute } expect(merge_request.reload.metrics.merged_at).to be < merge_request.reload.metrics.first_deployed_to_production_at previous_time = merge_request.reload.metrics.first_deployed_to_production_at # Current deploy - Timecop.freeze(time + 12.hours) { service.execute } + travel_to(time + 12.hours) { service.execute } expect(merge_request.reload.metrics.first_deployed_to_production_at).to eq(previous_time) end diff --git a/spec/services/design_management/copy_design_collection/copy_service_spec.rb b/spec/services/design_management/copy_design_collection/copy_service_spec.rb new file mode 100644 index 00000000000..e93e5f13fea --- /dev/null +++ b/spec/services/design_management/copy_design_collection/copy_service_spec.rb @@ -0,0 +1,259 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe DesignManagement::CopyDesignCollection::CopyService, :clean_gitlab_redis_shared_state do + include DesignManagementTestHelpers + + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:issue, refind: true) { create(:issue, project: project) } + let(:target_issue) { create(:issue) } + + subject { described_class.new(project, user, issue: issue, target_issue: target_issue).execute } + + before do + enable_design_management + end + + shared_examples 'service error' do |message:| + it 'returns an error response', :aggregate_failures do + expect(subject).to be_kind_of(ServiceResponse) + expect(subject).to be_error + expect(subject.message).to eq(message) + end + end + + shared_examples 'service success' do + it 'returns a success response', :aggregate_failures do + expect(subject).to be_kind_of(ServiceResponse) + expect(subject).to be_success + end + end + + include_examples 'service error', message: 'User cannot copy design collection to issue' + + context 'when user has permission to read the design collection' do + before_all do + project.add_reporter(user) + end + + include_examples 'service error', message: 'User cannot copy design collection to issue' + + context 'when the user also has permission to admin the target issue' do + let(:target_repository) { target_issue.project.design_repository } + + before do + target_issue.project.add_reporter(user) + end + + include_examples 'service error', message: 'Target design collection must first be queued' + + context 'when the target design collection has been queued' do + before do + target_issue.design_collection.start_copy! + end + + include_examples 'service error', message: 'Design collection has no designs' + + context 'when design collection has designs' do + let_it_be(:designs) do + create_list(:design, 3, :with_lfs_file, :with_relative_position, issue: issue, project: project) + end + + context 'when target issue already has designs' do + before do + create(:design, issue: target_issue, project: target_issue.project) + end + + include_examples 'service error', message: 'Target design collection already has designs' + end + + include_examples 'service success' + + it 'creates a design repository for the target project' do + expect { subject }.to change { target_repository.exists? }.from(false).to(true) + end + + context 'when the target project already has a design repository' do + before do + target_repository.create_if_not_exists + end + + include_examples 'service success' + end + + it 'copies the designs correctly', :aggregate_failures do + expect { subject }.to change { target_issue.designs.count }.by(3) + + old_designs = issue.designs.ordered + new_designs = target_issue.designs.ordered + + new_designs.zip(old_designs).each do |new_design, old_design| + expect(new_design).to have_attributes( + filename: old_design.filename, + relative_position: old_design.relative_position, + issue: target_issue, + project: target_issue.project + ) + end + end + + it 'copies the design versions correctly', :aggregate_failures do + expect { subject }.to change { target_issue.design_versions.count }.by(3) + + old_versions = issue.design_versions.ordered + new_versions = target_issue.design_versions.ordered + + new_versions.zip(old_versions).each do |new_version, old_version| + expect(new_version).to have_attributes( + created_at: old_version.created_at, + author_id: old_version.author_id + ) + expect(new_version.designs.pluck(:filename)).to eq(old_version.designs.pluck(:filename)) + expect(new_version.actions.pluck(:event)).to eq(old_version.actions.pluck(:event)) + end + end + + it 'copies the design actions correctly', :aggregate_failures do + expect { subject }.to change { DesignManagement::Action.count }.by(3) + + old_actions = issue.design_versions.ordered.flat_map(&:actions) + new_actions = target_issue.design_versions.ordered.flat_map(&:actions) + + new_actions.zip(old_actions).each do |new_action, old_action| + # This is a way to identify if the versions linked to the actions + # are correct is to compare design filenames, as the SHA changes. + new_design_filenames = new_action.version.designs.ordered.pluck(:filename) + old_design_filenames = old_action.version.designs.ordered.pluck(:filename) + + expect(new_design_filenames).to eq(old_design_filenames) + expect(new_action.event).to eq(old_action.event) + expect(new_action.design.filename).to eq(old_action.design.filename) + end + end + + it 'copies design notes correctly', :aggregate_failures, :sidekiq_inline do + old_notes = [ + create(:diff_note_on_design, note: 'first note', noteable: designs.first, project: project, author: create(:user)), + create(:diff_note_on_design, note: 'second note', noteable: designs.first, project: project, author: create(:user)) + ] + matchers = old_notes.map do |note| + have_attributes( + note.attributes.slice( + :type, + :author_id, + :note, + :position + ) + ) + end + + expect { subject }.to change { Note.count }.by(2) + + new_notes = target_issue.designs.first.notes.fresh + + expect(new_notes).to match_array(matchers) + end + + it 'links the LfsObjects' do + expect { subject }.to change { target_issue.project.lfs_objects.count }.by(3) + end + + it 'copies the Git repository data', :aggregate_failures do + subject + + commit_shas = target_repository.commits('master', limit: 99).map(&:id) + + expect(commit_shas).to include(*target_issue.design_versions.ordered.pluck(:sha)) + end + + it 'creates a master branch if none previously existed' do + expect { subject }.to change { target_repository.branch_names }.from([]).to(['master']) + end + + it 'leaves the design collection in the correct copy state' do + subject + + expect(target_issue.design_collection).to be_copy_ready + end + + describe 'rollback' do + before do + # Ensure the very last step throws an error + expect_next_instance_of(described_class) do |service| + expect(service).to receive(:finalize!).and_raise + end + end + + include_examples 'service error', message: 'Designs were unable to be copied successfully' + + it 'rollsback all PostgreSQL data created', :aggregate_failures do + expect { subject }.not_to change { + [ + DesignManagement::Design.count, + DesignManagement::Action.count, + DesignManagement::Version.count, + Note.count + ] + } + + collections = [ + target_issue.design_collection, + target_issue.designs, + target_issue.design_versions + ] + + expect(collections).to all(be_empty) + end + + it 'does not alter master branch', :aggregate_failures do + # Add some Git data to the target_repository, so we are testing + # that any original data remains + issue_2 = create(:issue, project: target_issue.project) + create(:design, :with_file, issue: issue_2, project: target_issue.project) + + expect { subject }.not_to change { + expect(target_repository.commits('master', limit: 10).size).to eq(1) + } + end + + it 'sets the design collection copy state' do + subject + + expect(target_issue.design_collection).to be_copy_error + end + end + end + end + end + end + + describe 'Alert if schema changes', :aggregate_failures do + let_it_be(:config_file) { Rails.root.join('lib/gitlab/design_management/copy_design_collection_model_attributes.yml') } + let_it_be(:config) { YAML.load_file(config_file).symbolize_keys } + + %w(Design Action Version).each do |model| + specify do + attributes = config["#{model.downcase}_attributes".to_sym] || [] + ignored_attributes = config["ignore_#{model.downcase}_attributes".to_sym] + + expect(attributes + ignored_attributes).to contain_exactly( + *DesignManagement.const_get(model, false).column_names + ), failure_message(model) + end + end + + def failure_message(model) + <<-MSG + The schema of the `#{model}` model has changed. + + `#{described_class.name}` refers to specific lists of attributes of `#{model}` to either + copy or ignore, so that we continue to copy designs correctly after schema changes. + + Please update: + #{config_file} + to reflect the latest changes to `#{model}`. See that file for more information. + MSG + end + end +end diff --git a/spec/services/design_management/copy_design_collection/queue_service_spec.rb b/spec/services/design_management/copy_design_collection/queue_service_spec.rb new file mode 100644 index 00000000000..2d9ea4633a0 --- /dev/null +++ b/spec/services/design_management/copy_design_collection/queue_service_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe DesignManagement::CopyDesignCollection::QueueService, :clean_gitlab_redis_shared_state do + include DesignManagementTestHelpers + + let_it_be(:user) { create(:user) } + let_it_be(:issue) { create(:issue) } + let_it_be(:target_issue, refind: true) { create(:issue) } + let_it_be(:design) { create(:design, issue: issue, project: issue.project) } + + subject { described_class.new(user, issue, target_issue).execute } + + before do + enable_design_management + end + + it 'returns an error if user does not have permission' do + expect(subject).to be_kind_of(ServiceResponse) + expect(subject).to be_error + expect(subject.message).to eq('User cannot copy designs to issue') + end + + context 'when user has permission' do + before_all do + issue.project.add_reporter(user) + target_issue.project.add_reporter(user) + end + + it 'returns an error if design collection copy_state is not queuable' do + target_issue.design_collection.start_copy! + + expect(subject).to be_kind_of(ServiceResponse) + expect(subject).to be_error + expect(subject.message).to eq('Target design collection copy state must be `ready`') + end + + it 'sets the design collection copy state' do + expect { subject }.to change { target_issue.design_collection.copy_state }.from('ready').to('in_progress') + end + + it 'queues a DesignManagement::CopyDesignCollectionWorker' do + expect { subject }.to change(DesignManagement::CopyDesignCollectionWorker.jobs, :size).by(1) + end + + it 'returns success' do + expect(subject).to be_kind_of(ServiceResponse) + expect(subject).to be_success + end + end +end diff --git a/spec/services/design_management/delete_designs_service_spec.rb b/spec/services/design_management/delete_designs_service_spec.rb index ace63b6e59c..9b2866cab11 100644 --- a/spec/services/design_management/delete_designs_service_spec.rb +++ b/spec/services/design_management/delete_designs_service_spec.rb @@ -105,7 +105,7 @@ RSpec.describe DesignManagement::DeleteDesignsService do end it 'informs the new-version-worker' do - expect(::DesignManagement::NewVersionWorker).to receive(:perform_async).with(Integer) + expect(::DesignManagement::NewVersionWorker).to receive(:perform_async).with(Integer, false) run_service end diff --git a/spec/services/design_management/generate_image_versions_service_spec.rb b/spec/services/design_management/generate_image_versions_service_spec.rb index 631eec97e5a..749030af97d 100644 --- a/spec/services/design_management/generate_image_versions_service_spec.rb +++ b/spec/services/design_management/generate_image_versions_service_spec.rb @@ -52,25 +52,50 @@ RSpec.describe DesignManagement::GenerateImageVersionsService do end context 'when an error is encountered when generating the image versions' do - before do - expect_next_instance_of(DesignManagement::DesignV432x230Uploader) do |uploader| - expect(uploader).to receive(:cache!).and_raise(CarrierWave::DownloadError, 'foo') + context "CarrierWave::IntegrityError" do + before do + expect_next_instance_of(DesignManagement::DesignV432x230Uploader) do |uploader| + expect(uploader).to receive(:cache!).and_raise(CarrierWave::IntegrityError, 'foo') + end + end + + it 'logs the exception' do + expect(Gitlab::ErrorTracking).to receive(:log_exception).with( + instance_of(CarrierWave::IntegrityError), + project_id: project.id, version_id: version.id, design_id: version.designs.first.id + ) + + described_class.new(version).execute end - end - it 'logs the error' do - expect(Gitlab::AppLogger).to receive(:error).with('foo') + it 'logs the error' do + expect(Gitlab::AppLogger).to receive(:error).with('foo') - described_class.new(version).execute + described_class.new(version).execute + end end - it 'tracks the error' do - expect(Gitlab::ErrorTracking).to receive(:track_exception).with( - instance_of(CarrierWave::DownloadError), - project_id: project.id, version_id: version.id, design_id: version.designs.first.id - ) + context "CarrierWave::UploadError" do + before do + expect_next_instance_of(DesignManagement::DesignV432x230Uploader) do |uploader| + expect(uploader).to receive(:cache!).and_raise(CarrierWave::UploadError, 'foo') + end + end - described_class.new(version).execute + it 'logs the error' do + expect(Gitlab::AppLogger).to receive(:error).with('foo') + + described_class.new(version).execute + end + + it 'tracks the error' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with( + instance_of(CarrierWave::UploadError), + project_id: project.id, version_id: version.id, design_id: version.designs.first.id + ) + + described_class.new(version).execute + end end end end diff --git a/spec/services/design_management/save_designs_service_spec.rb b/spec/services/design_management/save_designs_service_spec.rb index abba5de2c27..ec241daf3cd 100644 --- a/spec/services/design_management/save_designs_service_spec.rb +++ b/spec/services/design_management/save_designs_service_spec.rb @@ -32,7 +32,7 @@ RSpec.describe DesignManagement::SaveDesignsService do end allow(::DesignManagement::NewVersionWorker) - .to receive(:perform_async).with(Integer).and_return(nil) + .to receive(:perform_async).with(Integer, false).and_return(nil) end def run_service(files_to_upload = nil) @@ -128,6 +128,25 @@ RSpec.describe DesignManagement::SaveDesignsService do expect { run_parallel(blocks) }.to change(DesignManagement::Version, :count).by(parellism) end + context 'when the design collection is in the process of being copied', :clean_gitlab_redis_shared_state do + before do + issue.design_collection.start_copy! + end + + it_behaves_like 'a service error' + end + + context 'when the design collection has a copy error', :clean_gitlab_redis_shared_state do + before do + issue.design_collection.copy_state = 'error' + issue.design_collection.send(:set_stored_copy_state!) + end + + it 'resets the copy state' do + expect { run_service }.to change { issue.design_collection.copy_state }.from('error').to('ready') + end + end + describe 'the response' do it 'includes designs with the expected properties' do updated_designs = response[:designs] @@ -220,7 +239,7 @@ RSpec.describe DesignManagement::SaveDesignsService do counter = Gitlab::UsageDataCounters::DesignsCounter expect(::DesignManagement::NewVersionWorker) - .to receive(:perform_async).once.with(Integer).and_return(nil) + .to receive(:perform_async).once.with(Integer, false).and_return(nil) expect { run_service } .to change { Event.count }.by(2) @@ -254,7 +273,7 @@ RSpec.describe DesignManagement::SaveDesignsService do design_repository.has_visible_content? expect(::DesignManagement::NewVersionWorker) - .to receive(:perform_async).once.with(Integer).and_return(nil) + .to receive(:perform_async).once.with(Integer, false).and_return(nil) expect { service.execute } .to change { issue.designs.count }.from(0).to(2) @@ -271,6 +290,14 @@ RSpec.describe DesignManagement::SaveDesignsService do expect(response[:message]).to match(/only \d+ files are allowed simultaneously/i) end end + + context 'when uploading duplicate files' do + let(:files) { [rails_sample, dk_png, rails_sample] } + + it 'returns the correct error' do + expect(response[:message]).to match('Duplicate filenames are not allowed!') + end + end end context 'when the user is not allowed to upload designs' do diff --git a/spec/services/feature_flags/create_service_spec.rb b/spec/services/feature_flags/create_service_spec.rb new file mode 100644 index 00000000000..e80a24f9760 --- /dev/null +++ b/spec/services/feature_flags/create_service_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe FeatureFlags::CreateService do + let(:project) { create(:project) } + let(:developer) { create(:user) } + let(:reporter) { create(:user) } + let(:user) { developer } + + before do + project.add_developer(developer) + project.add_reporter(reporter) + end + + describe '#execute' do + subject do + described_class.new(project, user, params).execute + end + + let(:feature_flag) { subject[:feature_flag] } + + context 'when feature flag can not be created' do + let(:params) { {} } + + it 'returns status error' do + expect(subject[:status]).to eq(:error) + end + + it 'returns validation errors' do + expect(subject[:message]).to include("Name can't be blank") + end + + it 'does not create audit log' do + expect { subject }.not_to change { AuditEvent.count } + end + end + + context 'when feature flag is saved correctly' do + let(:params) do + { + name: 'feature_flag', + description: 'description', + scopes_attributes: [{ environment_scope: '*', active: true }, + { environment_scope: 'production', active: false }] + } + end + + it 'returns status success' do + expect(subject[:status]).to eq(:success) + end + + it 'creates feature flag' do + expect { subject }.to change { Operations::FeatureFlag.count }.by(1) + end + + it 'creates audit event' do + expected_message = 'Created feature flag <strong>feature_flag</strong> '\ + 'with description <strong>"description"</strong>. '\ + 'Created rule <strong>*</strong> and set it as <strong>active</strong> '\ + 'with strategies <strong>[{"name"=>"default", "parameters"=>{}}]</strong>. '\ + 'Created rule <strong>production</strong> and set it as <strong>inactive</strong> '\ + 'with strategies <strong>[{"name"=>"default", "parameters"=>{}}]</strong>.' + + expect { subject }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq(expected_message) + end + + context 'when user is reporter' do + let(:user) { reporter } + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Access Denied') + end + end + end + end +end diff --git a/spec/services/feature_flags/destroy_service_spec.rb b/spec/services/feature_flags/destroy_service_spec.rb new file mode 100644 index 00000000000..df83969e167 --- /dev/null +++ b/spec/services/feature_flags/destroy_service_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe FeatureFlags::DestroyService do + include FeatureFlagHelpers + + let(:project) { create(:project) } + let(:developer) { create(:user) } + let(:reporter) { create(:user) } + let(:user) { developer } + let!(:feature_flag) { create(:operations_feature_flag, project: project) } + + before do + project.add_developer(developer) + project.add_reporter(reporter) + end + + describe '#execute' do + subject { described_class.new(project, user, params).execute(feature_flag) } + + let(:audit_event_message) { AuditEvent.last.details[:custom_message] } + let(:params) { {} } + + it 'returns status success' do + expect(subject[:status]).to eq(:success) + end + + it 'destroys feature flag' do + expect { subject }.to change { Operations::FeatureFlag.count }.by(-1) + end + + it 'creates audit log' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to eq("Deleted feature flag <strong>#{feature_flag.name}</strong>.") + end + + context 'when user is reporter' do + let(:user) { reporter } + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Access Denied') + end + end + + context 'when feature flag can not be destroyed' do + before do + allow(feature_flag).to receive(:destroy).and_return(false) + end + + it 'returns status error' do + expect(subject[:status]).to eq(:error) + end + + it 'does not create audit log' do + expect { subject }.not_to change { AuditEvent.count } + end + end + end +end diff --git a/spec/services/feature_flags/disable_service_spec.rb b/spec/services/feature_flags/disable_service_spec.rb new file mode 100644 index 00000000000..00369d63ccf --- /dev/null +++ b/spec/services/feature_flags/disable_service_spec.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe FeatureFlags::DisableService do + include FeatureFlagHelpers + + let_it_be(:user) { create(:user) } + let(:project) { create(:project) } + let(:service) { described_class.new(project, user, params) } + let(:params) { {} } + + before do + project.add_developer(user) + end + + describe '#execute' do + subject { service.execute } + + context 'with params to disable default strategy on prd scope' do + let(:params) do + { + name: 'awesome', + environment_scope: 'prd', + strategy: { name: 'userWithId', parameters: { 'userIds': 'User:1' } }.deep_stringify_keys + } + end + + context 'when there is a persisted feature flag' do + let!(:feature_flag) { create_flag(project, params[:name]) } + + context 'when there is a persisted scope' do + let!(:scope) do + create_scope(feature_flag, params[:environment_scope], true, strategies) + end + + context 'when there is a persisted strategy' do + let(:strategies) do + [ + { name: 'userWithId', parameters: { 'userIds': 'User:1' } }.deep_stringify_keys, + { name: 'userWithId', parameters: { 'userIds': 'User:2' } }.deep_stringify_keys + ] + end + + it 'deletes the specified strategy' do + subject + + scope.reload + expect(scope.strategies.count).to eq(1) + expect(scope.strategies).not_to include(params[:strategy]) + end + + context 'when strategies will be empty' do + let(:strategies) { [params[:strategy]] } + + it 'deletes the persisted scope' do + subject + + expect(feature_flag.scopes.exists?(environment_scope: params[:environment_scope])) + .to eq(false) + end + end + end + + context 'when there is no persisted strategy' do + let(:strategies) { [{ name: 'default', parameters: {} }] } + + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include('Strategy not found') + end + end + end + + context 'when there is no persisted scope' do + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include('Feature Flag Scope not found') + end + end + end + + context 'when there is no persisted feature flag' do + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include('Feature Flag not found') + end + end + end + end +end diff --git a/spec/services/feature_flags/enable_service_spec.rb b/spec/services/feature_flags/enable_service_spec.rb new file mode 100644 index 00000000000..26dffcae0c7 --- /dev/null +++ b/spec/services/feature_flags/enable_service_spec.rb @@ -0,0 +1,153 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe FeatureFlags::EnableService do + include FeatureFlagHelpers + + let_it_be(:user) { create(:user) } + let(:project) { create(:project) } + let(:service) { described_class.new(project, user, params) } + let(:params) { {} } + + before do + project.add_developer(user) + end + + describe '#execute' do + subject { service.execute } + + context 'with params to enable default strategy on prd scope' do + let(:params) do + { + name: 'awesome', + environment_scope: 'prd', + strategy: { name: 'default', parameters: {} }.stringify_keys + } + end + + context 'when there is no persisted feature flag' do + it 'creates a new feature flag with scope' do + feature_flag = subject[:feature_flag] + scope = feature_flag.scopes.find_by_environment_scope(params[:environment_scope]) + expect(subject[:status]).to eq(:success) + expect(feature_flag.name).to eq(params[:name]) + expect(feature_flag.default_scope).not_to be_active + expect(scope).to be_active + expect(scope.strategies).to include(params[:strategy]) + end + + context 'when params include default scope' do + let(:params) do + { + name: 'awesome', + environment_scope: '*', + strategy: { name: 'userWithId', parameters: { 'userIds': 'abc' } }.deep_stringify_keys + } + end + + it 'create a new feature flag with an active default scope with the specified strategy' do + feature_flag = subject[:feature_flag] + expect(subject[:status]).to eq(:success) + expect(feature_flag.default_scope).to be_active + expect(feature_flag.default_scope.strategies).to include(params[:strategy]) + end + end + end + + context 'when there is a persisted feature flag' do + let!(:feature_flag) { create_flag(project, params[:name]) } + + context 'when there is no persisted scope' do + it 'creates a new scope for the persisted feature flag' do + feature_flag = subject[:feature_flag] + scope = feature_flag.scopes.find_by_environment_scope(params[:environment_scope]) + expect(subject[:status]).to eq(:success) + expect(feature_flag.name).to eq(params[:name]) + expect(scope).to be_active + expect(scope.strategies).to include(params[:strategy]) + end + end + + context 'when there is a persisted scope' do + let!(:feature_flag_scope) do + create_scope(feature_flag, params[:environment_scope], active, strategies) + end + + let(:active) { true } + + context 'when the persisted scope does not have the specified strategy yet' do + let(:strategies) { [{ name: 'userWithId', parameters: { 'userIds': 'abc' } }] } + + it 'adds the specified strategy to the scope' do + subject + + feature_flag_scope.reload + expect(feature_flag_scope.strategies).to include(params[:strategy]) + end + + context 'when the persisted scope is inactive' do + let(:active) { false } + + it 'reactivates the scope' do + expect { subject } + .to change { feature_flag_scope.reload.active }.from(false).to(true) + end + end + end + + context 'when the persisted scope has the specified strategy already' do + let(:strategies) { [params[:strategy]] } + + it 'does not add a duplicated strategy to the scope' do + expect { subject } + .not_to change { feature_flag_scope.reload.strategies.count } + end + end + end + end + end + + context 'when strategy is not specified in params' do + let(:params) do + { + name: 'awesome', + environment_scope: 'prd' + } + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include('Scopes strategies must be an array of strategy hashes') + end + end + + context 'when environment scope is not specified in params' do + let(:params) do + { + name: 'awesome', + strategy: { name: 'default', parameters: {} }.stringify_keys + } + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include("Scopes environment scope can't be blank") + end + end + + context 'when name is not specified in params' do + let(:params) do + { + environment_scope: 'prd', + strategy: { name: 'default', parameters: {} }.stringify_keys + } + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include("Name can't be blank") + end + end + end +end diff --git a/spec/services/feature_flags/update_service_spec.rb b/spec/services/feature_flags/update_service_spec.rb new file mode 100644 index 00000000000..16c3ff23443 --- /dev/null +++ b/spec/services/feature_flags/update_service_spec.rb @@ -0,0 +1,250 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe FeatureFlags::UpdateService do + let(:project) { create(:project) } + let(:developer) { create(:user) } + let(:reporter) { create(:user) } + let(:user) { developer } + let(:feature_flag) { create(:operations_feature_flag, project: project, active: true) } + + before do + project.add_developer(developer) + project.add_reporter(reporter) + end + + describe '#execute' do + subject { described_class.new(project, user, params).execute(feature_flag) } + + let(:params) { { name: 'new_name' } } + let(:audit_event_message) do + AuditEvent.last.details[:custom_message] + end + + it 'returns success status' do + expect(subject[:status]).to eq(:success) + end + + it 'creates audit event with correct message' do + name_was = feature_flag.name + + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to( + eq("Updated feature flag <strong>new_name</strong>. "\ + "Updated name from <strong>\"#{name_was}\"</strong> "\ + "to <strong>\"new_name\"</strong>.") + ) + end + + context 'with invalid params' do + let(:params) { { name: nil } } + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + expect(subject[:http_status]).to eq(:bad_request) + end + + it 'returns error messages' do + expect(subject[:message]).to include("Name can't be blank") + end + + it 'does not create audit event' do + expect { subject }.not_to change { AuditEvent.count } + end + end + + context 'when user is reporter' do + let(:user) { reporter } + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Access Denied') + end + end + + context 'when nothing is changed' do + let(:params) { {} } + + it 'returns success status' do + expect(subject[:status]).to eq(:success) + end + + it 'does not create audit event' do + expect { subject }.not_to change { AuditEvent.count } + end + end + + context 'description is being changed' do + let(:params) { { description: 'new description' } } + + it 'creates audit event with changed description' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to( + include("Updated description from <strong>\"\"</strong>"\ + " to <strong>\"new description\"</strong>.") + ) + end + end + + context 'when flag active state is changed' do + let(:params) do + { + active: false + } + end + + it 'creates audit event about changing active state' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to( + include('Updated active from <strong>"true"</strong> to <strong>"false"</strong>.') + ) + end + end + + context 'when scope active state is changed' do + let(:params) do + { + scopes_attributes: [{ id: feature_flag.scopes.first.id, active: false }] + } + end + + it 'creates audit event about changing active state' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to( + include("Updated rule <strong>*</strong> active state "\ + "from <strong>true</strong> to <strong>false</strong>.") + ) + end + end + + context 'when scope is renamed' do + let(:changed_scope) { feature_flag.scopes.create!(environment_scope: 'review', active: true) } + let(:params) do + { + scopes_attributes: [{ id: changed_scope.id, environment_scope: 'staging' }] + } + end + + it 'creates audit event with changed name' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to( + include("Updated rule <strong>staging</strong> environment scope "\ + "from <strong>review</strong> to <strong>staging</strong>.") + ) + end + + context 'when scope can not be updated' do + let(:params) do + { + scopes_attributes: [{ id: changed_scope.id, environment_scope: '' }] + } + end + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + end + + it 'returns error messages' do + expect(subject[:message]).to include("Scopes environment scope can't be blank") + end + + it 'does not create audit event' do + expect { subject }.not_to change { AuditEvent.count } + end + end + end + + context 'when scope is deleted' do + let(:deleted_scope) { feature_flag.scopes.create!(environment_scope: 'review', active: true) } + let(:params) do + { + scopes_attributes: [{ id: deleted_scope.id, '_destroy': true }] + } + end + + it 'creates audit event with deleted scope' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to include("Deleted rule <strong>review</strong>.") + end + + context 'when scope can not be deleted' do + before do + allow(deleted_scope).to receive(:destroy).and_return(false) + end + + it 'does not create audit event' do + expect do + subject + end.to not_change { AuditEvent.count }.and raise_error(ActiveRecord::RecordNotDestroyed) + end + end + end + + context 'when new scope is being added' do + let(:new_environment_scope) { 'review' } + let(:params) do + { + scopes_attributes: [{ environment_scope: new_environment_scope, active: true }] + } + end + + it 'creates audit event with new scope' do + expected = 'Created rule <strong>review</strong> and set it as <strong>active</strong> '\ + 'with strategies <strong>[{"name"=>"default", "parameters"=>{}}]</strong>.' + + subject + + expect(audit_event_message).to include(expected) + end + + context 'when scope can not be created' do + let(:new_environment_scope) { '' } + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + end + + it 'returns error messages' do + expect(subject[:message]).to include("Scopes environment scope can't be blank") + end + + it 'does not create audit event' do + expect { subject }.not_to change { AuditEvent.count } + end + end + end + + context 'when the strategy is changed' do + let(:scope) do + create(:operations_feature_flag_scope, + feature_flag: feature_flag, + environment_scope: 'sandbox', + strategies: [{ name: "default", parameters: {} }]) + end + + let(:params) do + { + scopes_attributes: [{ + id: scope.id, + environment_scope: 'sandbox', + strategies: [{ + name: 'gradualRolloutUserId', + parameters: { + groupId: 'mygroup', + percentage: "40" + } + }] + }] + } + end + + it 'creates an audit event' do + expected = %r{Updated rule <strong>sandbox</strong> strategies from <strong>.*</strong> to <strong>.*</strong>.} + + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to match(expected) + end + end + end +end diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb index db25bb766c9..a5290f0be68 100644 --- a/spec/services/git/branch_hooks_service_spec.rb +++ b/spec/services/git/branch_hooks_service_spec.rb @@ -429,4 +429,26 @@ RSpec.describe Git::BranchHooksService do end end end + + describe 'Metrics dashboard sync' do + context 'with feature flag enabled' do + before do + Feature.enable(:metrics_dashboards_sync) + end + + it 'imports metrics to database' do + expect(Metrics::Dashboard::SyncDashboardsWorker).to receive(:perform_async) + + service.execute + end + end + + context 'with feature flag disabled' do + it 'imports metrics to database' do + expect(Metrics::Dashboard::SyncDashboardsWorker).to receive(:perform_async) + + service.execute + end + end + end end diff --git a/spec/services/git/wiki_push_service_spec.rb b/spec/services/git/wiki_push_service_spec.rb index 816f20f0bc3..cd38f2e97fb 100644 --- a/spec/services/git/wiki_push_service_spec.rb +++ b/spec/services/git/wiki_push_service_spec.rb @@ -254,24 +254,6 @@ RSpec.describe Git::WikiPushService, services: true do service.execute end end - - context 'the wiki_events_on_git_push feature is disabled' do - before do - stub_feature_flags(wiki_events_on_git_push: false) - end - - it_behaves_like 'a no-op push' - - context 'but is enabled for a given container' do - before do - stub_feature_flags(wiki_events_on_git_push: wiki.container) - end - - it 'creates events' do - expect { process_changes { write_new_page } }.to change(Event, :count).by(1) - end - end - end end end diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index fc877f45a39..d5479fa2a06 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -138,4 +138,91 @@ RSpec.describe Groups::CreateService, '#execute' do expect(group.namespace_settings).to be_persisted end end + + describe 'create service for the group' do + let(:service) { described_class.new(user, group_params) } + let(:created_group) { service.execute } + + context 'with an active instance-level integration' do + let!(:instance_integration) { create(:prometheus_service, :instance, api_url: 'https://prometheus.instance.com/') } + + it 'creates a service from the instance-level integration' do + expect(created_group.services.count).to eq(1) + expect(created_group.services.first.api_url).to eq(instance_integration.api_url) + expect(created_group.services.first.inherit_from_id).to eq(instance_integration.id) + end + + context 'with an active group-level integration' do + let(:service) { described_class.new(user, group_params.merge(parent_id: group.id)) } + let!(:group_integration) { create(:prometheus_service, group: group, project: nil, api_url: 'https://prometheus.group.com/') } + let(:group) do + create(:group).tap do |group| + group.add_owner(user) + end + end + + it 'creates a service from the group-level integration' do + expect(created_group.services.count).to eq(1) + expect(created_group.services.first.api_url).to eq(group_integration.api_url) + expect(created_group.services.first.inherit_from_id).to eq(group_integration.id) + end + + context 'with an active subgroup' do + let(:service) { described_class.new(user, group_params.merge(parent_id: subgroup.id)) } + let!(:subgroup_integration) { create(:prometheus_service, group: subgroup, project: nil, api_url: 'https://prometheus.subgroup.com/') } + let(:subgroup) do + create(:group, parent: group).tap do |subgroup| + subgroup.add_owner(user) + end + end + + it 'creates a service from the subgroup-level integration' do + expect(created_group.services.count).to eq(1) + expect(created_group.services.first.api_url).to eq(subgroup_integration.api_url) + expect(created_group.services.first.inherit_from_id).to eq(subgroup_integration.id) + end + end + end + end + end + + context 'shared runners configuration' do + context 'parent group present' do + using RSpec::Parameterized::TableSyntax + + where(:shared_runners_config, :descendants_override_disabled_shared_runners_config) do + true | false + false | false + # true | true # invalid at the group level, leaving as comment to make explicit + false | true + end + + with_them do + let!(:group) { create(:group, shared_runners_enabled: shared_runners_config, allow_descendants_override_disabled_shared_runners: descendants_override_disabled_shared_runners_config) } + let!(:service) { described_class.new(user, group_params.merge(parent_id: group.id)) } + + before do + group.add_owner(user) + end + + it 'creates group following the parent config' do + new_group = service.execute + + expect(new_group.shared_runners_enabled).to eq(shared_runners_config) + expect(new_group.allow_descendants_override_disabled_shared_runners).to eq(descendants_override_disabled_shared_runners_config) + end + end + end + + context 'root group' do + let!(:service) { described_class.new(user) } + + it 'follows default config' do + new_group = service.execute + + expect(new_group.shared_runners_enabled).to eq(true) + expect(new_group.allow_descendants_override_disabled_shared_runners).to eq(false) + end + end + end end diff --git a/spec/services/groups/import_export/import_service_spec.rb b/spec/services/groups/import_export/import_service_spec.rb index 4aac602a6da..f284225e23a 100644 --- a/spec/services/groups/import_export/import_service_spec.rb +++ b/spec/services/groups/import_export/import_service_spec.rb @@ -10,6 +10,15 @@ RSpec.describe Groups::ImportExport::ImportService do context 'when the job can be successfully scheduled' do subject(:import_service) { described_class.new(group: group, user: user) } + it 'creates group import state' do + import_service.async_execute + + import_state = group.import_state + + expect(import_state.user).to eq(user) + expect(import_state.group).to eq(group) + end + it 'enqueues an import job' do expect(GroupImportWorker).to receive(:perform_async).with(user.id, group.id) diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index 89e4d091ff7..6144b86a316 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -285,6 +285,44 @@ RSpec.describe Groups::TransferService do end end + context 'shared runners configuration' do + before do + create(:group_member, :owner, group: new_parent_group, user: user) + end + + context 'if parent group has disabled shared runners but allows overrides' do + let(:new_parent_group) { create(:group, shared_runners_enabled: false, allow_descendants_override_disabled_shared_runners: true) } + + it 'calls update service' do + expect(Groups::UpdateSharedRunnersService).to receive(:new).with(group, user, { shared_runners_setting: 'disabled_with_override' }).and_call_original + + transfer_service.execute(new_parent_group) + end + end + + context 'if parent group does not allow shared runners' do + let(:new_parent_group) { create(:group, shared_runners_enabled: false, allow_descendants_override_disabled_shared_runners: false) } + + it 'calls update service' do + expect(Groups::UpdateSharedRunnersService).to receive(:new).with(group, user, { shared_runners_setting: 'disabled_and_unoverridable' }).and_call_original + + transfer_service.execute(new_parent_group) + end + end + + context 'if parent group allows shared runners' do + let(:group) { create(:group, :public, :nested, shared_runners_enabled: false) } + let(:new_parent_group) { create(:group, shared_runners_enabled: true) } + + it 'does not call update service and keeps them disabled on the group' do + expect(Groups::UpdateSharedRunnersService).not_to receive(:new) + + transfer_service.execute(new_parent_group) + expect(group.reload.shared_runners_enabled).to be_falsy + end + end + end + context 'when a group is transferred to its subgroup' do let(:new_parent_group) { create(:group, parent: group) } diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 1e6a8d53354..a79cda86a86 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -283,6 +283,31 @@ RSpec.describe Groups::UpdateService do end end + context 'change shared Runners config' do + let(:group) { create(:group) } + let(:project) { create(:project, shared_runners_enabled: true, group: group) } + + subject { described_class.new(group, user, shared_runners_setting: 'disabled_and_unoverridable').execute } + + before do + group.add_owner(user) + end + + it 'calls the shared runners update service' do + expect_any_instance_of(::Groups::UpdateSharedRunnersService).to receive(:execute).and_return({ status: :success }) + + expect(subject).to be_truthy + end + + it 'handles errors in the shared runners update service' do + expect_any_instance_of(::Groups::UpdateSharedRunnersService).to receive(:execute).and_return({ status: :error, message: 'something happened' }) + + expect(subject).to be_falsy + + expect(group.errors[:update_shared_runners].first).to eq('something happened') + end + end + def update_group(group, user, opts) Groups::UpdateService.new(group, user, opts).execute end diff --git a/spec/services/groups/update_shared_runners_service_spec.rb b/spec/services/groups/update_shared_runners_service_spec.rb index 9fd8477a455..e2838c4ce0b 100644 --- a/spec/services/groups/update_shared_runners_service_spec.rb +++ b/spec/services/groups/update_shared_runners_service_spec.rb @@ -13,17 +13,14 @@ RSpec.describe Groups::UpdateSharedRunnersService do context 'when current_user is not the group owner' do let_it_be(:group) { create(:group) } - let(:params) { { shared_runners_enabled: '0' } } + let(:params) { { shared_runners_setting: 'enabled' } } before do group.add_maintainer(user) end it 'results error and does not call any method' do - expect(group).not_to receive(:enable_shared_runners!) - expect(group).not_to receive(:disable_shared_runners!) - expect(group).not_to receive(:allow_descendants_override_disabled_shared_runners!) - expect(group).not_to receive(:disallow_descendants_override_disabled_shared_runners!) + expect(group).not_to receive(:update_shared_runners_setting!) expect(subject[:status]).to eq(:error) expect(subject[:message]).to eq('Operation not allowed') @@ -37,191 +34,60 @@ RSpec.describe Groups::UpdateSharedRunnersService do end context 'enable shared Runners' do - where(:desired_params) do - ['1', true] - end - - with_them do - let(:params) { { shared_runners_enabled: desired_params } } - - context 'group that its ancestors have shared runners disabled' do - let_it_be(:parent) { create(:group, :shared_runners_disabled) } - let_it_be(:group) { create(:group, :shared_runners_disabled, parent: parent) } - - it 'results error' do - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Shared Runners disabled for the parent group') - end - end + let(:params) { { shared_runners_setting: 'enabled' } } - context 'root group with shared runners disabled' do - let_it_be(:group) { create(:group, :shared_runners_disabled) } + context 'group that its ancestors have shared runners disabled' do + let_it_be(:parent) { create(:group, :shared_runners_disabled) } + let_it_be(:group) { create(:group, :shared_runners_disabled, parent: parent) } - it 'receives correct method and succeeds' do - expect(group).to receive(:enable_shared_runners!) - expect(group).not_to receive(:disable_shared_runners!) - expect(group).not_to receive(:allow_descendants_override_disabled_shared_runners!) - expect(group).not_to receive(:disallow_descendants_override_disabled_shared_runners!) - - expect(subject[:status]).to eq(:success) - end + it 'results error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Validation failed: Shared runners enabled cannot be enabled because parent group has shared Runners disabled') end end - end - - context 'disable shared Runners' do - let_it_be(:group) { create(:group) } - - where(:desired_params) do - ['0', false] - end - with_them do - let(:params) { { shared_runners_enabled: desired_params } } + context 'root group with shared runners disabled' do + let_it_be(:group) { create(:group, :shared_runners_disabled) } it 'receives correct method and succeeds' do - expect(group).to receive(:disable_shared_runners!) - expect(group).not_to receive(:enable_shared_runners!) - expect(group).not_to receive(:allow_descendants_override_disabled_shared_runners!) - expect(group).not_to receive(:disallow_descendants_override_disabled_shared_runners!) + expect(group).to receive(:update_shared_runners_setting!).with('enabled') expect(subject[:status]).to eq(:success) end end end - context 'allow descendants to override' do - where(:desired_params) do - ['1', true] - end - - with_them do - let(:params) { { allow_descendants_override_disabled_shared_runners: desired_params } } - - context 'top level group' do - let_it_be(:group) { create(:group, :shared_runners_disabled) } - - it 'receives correct method and succeeds' do - expect(group).to receive(:allow_descendants_override_disabled_shared_runners!) - expect(group).not_to receive(:disallow_descendants_override_disabled_shared_runners!) - expect(group).not_to receive(:enable_shared_runners!) - expect(group).not_to receive(:disable_shared_runners!) - - expect(subject[:status]).to eq(:success) - end - end + context 'disable shared Runners' do + let_it_be(:group) { create(:group) } + let(:params) { { shared_runners_setting: 'disabled_and_unoverridable' } } - context 'when parent does not allow' do - let_it_be(:parent) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false ) } - let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false, parent: parent) } + it 'receives correct method and succeeds' do + expect(group).to receive(:update_shared_runners_setting!).with('disabled_and_unoverridable') - it 'results error' do - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Group level shared Runners not allowed') - end - end + expect(subject[:status]).to eq(:success) end end - context 'disallow descendants to override' do - where(:desired_params) do - ['0', false] - end - - with_them do - let(:params) { { allow_descendants_override_disabled_shared_runners: desired_params } } - - context 'top level group' do - let_it_be(:group) { create(:group, :shared_runners_disabled, :allow_descendants_override_disabled_shared_runners ) } - - it 'receives correct method and succeeds' do - expect(group).to receive(:disallow_descendants_override_disabled_shared_runners!) - expect(group).not_to receive(:allow_descendants_override_disabled_shared_runners!) - expect(group).not_to receive(:enable_shared_runners!) - expect(group).not_to receive(:disable_shared_runners!) - - expect(subject[:status]).to eq(:success) - end - end - - context 'top level group that has shared Runners enabled' do - let_it_be(:group) { create(:group, shared_runners_enabled: true) } - - it 'results error' do - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Shared Runners enabled') - end - end - end - end + context 'allow descendants to override' do + let(:params) { { shared_runners_setting: 'disabled_with_override' } } - context 'both params are present' do - context 'shared_runners_enabled: 1 and allow_descendants_override_disabled_shared_runners' do + context 'top level group' do let_it_be(:group) { create(:group, :shared_runners_disabled) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, parent: group) } - let_it_be(:project) { create(:project, shared_runners_enabled: false, group: sub_group) } - where(:allow_descendants_override) do - ['1', true, '0', false] - end + it 'receives correct method and succeeds' do + expect(group).to receive(:update_shared_runners_setting!).with('disabled_with_override') - with_them do - let(:params) { { shared_runners_enabled: '1', allow_descendants_override_disabled_shared_runners: allow_descendants_override } } - - it 'results in an error because shared Runners are enabled' do - expect { subject } - .to not_change { group.reload.shared_runners_enabled } - .and not_change { sub_group.reload.shared_runners_enabled } - .and not_change { project.reload.shared_runners_enabled } - .and not_change { group.reload.allow_descendants_override_disabled_shared_runners } - .and not_change { sub_group.reload.allow_descendants_override_disabled_shared_runners } - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Cannot set shared_runners_enabled to true and allow_descendants_override_disabled_shared_runners') - end + expect(subject[:status]).to eq(:success) end end - context 'shared_runners_enabled: 0 and allow_descendants_override_disabled_shared_runners: 0' do - let_it_be(:group) { create(:group, :allow_descendants_override_disabled_shared_runners) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, :allow_descendants_override_disabled_shared_runners, parent: group) } - let_it_be(:sub_group_2) { create(:group, parent: group) } - let_it_be(:project) { create(:project, group: group, shared_runners_enabled: true) } - let_it_be(:project_2) { create(:project, group: sub_group_2, shared_runners_enabled: true) } - - let(:params) { { shared_runners_enabled: '0', allow_descendants_override_disabled_shared_runners: '0' } } - - it 'disables shared Runners and disable allow_descendants_override_disabled_shared_runners' do - expect { subject } - .to change { group.reload.shared_runners_enabled }.from(true).to(false) - .and change { group.reload.allow_descendants_override_disabled_shared_runners }.from(true).to(false) - .and not_change { sub_group.reload.shared_runners_enabled } - .and change { sub_group.reload.allow_descendants_override_disabled_shared_runners }.from(true).to(false) - .and change { sub_group_2.reload.shared_runners_enabled }.from(true).to(false) - .and not_change { sub_group_2.reload.allow_descendants_override_disabled_shared_runners } - .and change { project.reload.shared_runners_enabled }.from(true).to(false) - .and change { project_2.reload.shared_runners_enabled }.from(true).to(false) - end - end + context 'when parent does not allow' do + let_it_be(:parent) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false ) } + let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false, parent: parent) } - context 'shared_runners_enabled: 0 and allow_descendants_override_disabled_shared_runners: 1' do - let_it_be(:group) { create(:group) } - let_it_be(:sub_group) { create(:group, :shared_runners_disabled, parent: group) } - let_it_be(:sub_group_2) { create(:group, parent: group) } - let_it_be(:project) { create(:project, group: group, shared_runners_enabled: true) } - let_it_be(:project_2) { create(:project, group: sub_group_2, shared_runners_enabled: true) } - - let(:params) { { shared_runners_enabled: '0', allow_descendants_override_disabled_shared_runners: '1' } } - - it 'disables shared Runners and enable allow_descendants_override_disabled_shared_runners only for itself' do - expect { subject } - .to change { group.reload.shared_runners_enabled }.from(true).to(false) - .and change { group.reload.allow_descendants_override_disabled_shared_runners }.from(false).to(true) - .and not_change { sub_group.reload.shared_runners_enabled } - .and not_change { sub_group.reload.allow_descendants_override_disabled_shared_runners } - .and change { sub_group_2.reload.shared_runners_enabled }.from(true).to(false) - .and not_change { sub_group_2.reload.allow_descendants_override_disabled_shared_runners } - .and change { project.reload.shared_runners_enabled }.from(true).to(false) - .and change { project_2.reload.shared_runners_enabled }.from(true).to(false) + it 'results error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Validation failed: Allow descendants override disabled shared runners cannot be enabled because parent group does not allow it') end end end diff --git a/spec/services/incident_management/incidents/update_severity_service_spec.rb b/spec/services/incident_management/incidents/update_severity_service_spec.rb new file mode 100644 index 00000000000..bc1abf82cf2 --- /dev/null +++ b/spec/services/incident_management/incidents/update_severity_service_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IncidentManagement::Incidents::UpdateSeverityService do + let_it_be(:user) { create(:user) } + + describe '#execute' do + let(:severity) { 'low' } + let(:system_note_worker) { ::IncidentManagement::AddSeveritySystemNoteWorker } + + subject(:update_severity) { described_class.new(issuable, user, severity).execute } + + before do + allow(system_note_worker).to receive(:perform_async) + end + + shared_examples 'adds a system note' do + it 'calls AddSeveritySystemNoteWorker' do + update_severity + + expect(system_note_worker).to have_received(:perform_async).with(issuable.id, user.id) + end + end + + context 'when issuable not an incident' do + %i(issue merge_request).each do |issuable_type| + let(:issuable) { build_stubbed(issuable_type) } + + it { is_expected.to be_nil } + + it 'does not set severity' do + expect { update_severity }.not_to change(IssuableSeverity, :count) + end + + it 'does not add a system note' do + update_severity + + expect(system_note_worker).not_to have_received(:perform_async) + end + end + end + + context 'when issuable is an incident' do + let!(:issuable) { create(:incident) } + + context 'when issuable does not have issuable severity yet' do + it 'creates new record' do + expect { update_severity }.to change { IssuableSeverity.where(issue: issuable).count }.to(1) + end + + it 'sets severity to specified value' do + expect { update_severity }.to change { issuable.severity }.to('low') + end + + it_behaves_like 'adds a system note' + end + + context 'when issuable has an issuable severity' do + let!(:issuable_severity) { create(:issuable_severity, issue: issuable, severity: 'medium') } + + it 'does not create new record' do + expect { update_severity }.not_to change(IssuableSeverity, :count) + end + + it 'updates existing issuable severity' do + expect { update_severity }.to change { issuable_severity.severity }.to(severity) + end + + it_behaves_like 'adds a system note' + end + + context 'when severity value is unsupported' do + let(:severity) { 'unsupported-severity' } + + it 'sets the severity to default value' do + update_severity + + expect(issuable.issuable_severity.severity).to eq(IssuableSeverity::DEFAULT) + end + + it_behaves_like 'adds a system note' + end + end + end +end diff --git a/spec/services/issuable/bulk_update_service_spec.rb b/spec/services/issuable/bulk_update_service_spec.rb index 168a80a97c0..f2bc4f717af 100644 --- a/spec/services/issuable/bulk_update_service_spec.rb +++ b/spec/services/issuable/bulk_update_service_spec.rb @@ -254,7 +254,7 @@ RSpec.describe Issuable::BulkUpdateService do describe 'unsubscribe from issues' do let(:issues) do create_list(:closed_issue, 2, project: project) do |issue| - issue.subscriptions.create(user: user, project: project, subscribed: true) + issue.subscriptions.create!(user: user, project: project, subscribed: true) end end diff --git a/spec/services/issuable/clone/attributes_rewriter_spec.rb b/spec/services/issuable/clone/attributes_rewriter_spec.rb index 372e6d480e3..7f434b8b246 100644 --- a/spec/services/issuable/clone/attributes_rewriter_spec.rb +++ b/spec/services/issuable/clone/attributes_rewriter_spec.rb @@ -20,7 +20,7 @@ RSpec.describe Issuable::Clone::AttributesRewriter do group_label = create(:group_label, title: 'group_label', group: group) create(:label, title: 'label3', project: project2) - original_issue.update(labels: [project1_label_1, project1_label_2, group_label]) + original_issue.update!(labels: [project1_label_1, project1_label_2, group_label]) subject.execute @@ -48,7 +48,7 @@ RSpec.describe Issuable::Clone::AttributesRewriter do it 'sets milestone to nil when old issue milestone is not in the new project' do milestone = create(:milestone, title: 'milestone', project: project1) - original_issue.update(milestone: milestone) + original_issue.update!(milestone: milestone) subject.execute @@ -59,7 +59,7 @@ RSpec.describe Issuable::Clone::AttributesRewriter do milestone_project1 = create(:milestone, title: 'milestone', project: project1) milestone_project2 = create(:milestone, title: 'milestone', project: project2) - original_issue.update(milestone: milestone_project1) + original_issue.update!(milestone: milestone_project1) subject.execute @@ -69,7 +69,7 @@ RSpec.describe Issuable::Clone::AttributesRewriter do it 'copies the milestone when old issue milestone is a group milestone' do milestone = create(:milestone, title: 'milestone', group: group) - original_issue.update(milestone: milestone) + original_issue.update!(milestone: milestone) subject.execute @@ -85,7 +85,7 @@ RSpec.describe Issuable::Clone::AttributesRewriter do let!(:milestone2_project2) { create(:milestone, title: 'milestone2', project: project2) } before do - original_issue.update(milestone: milestone2_project1) + original_issue.update!(milestone: milestone2_project1) create_event(milestone1_project1) create_event(milestone2_project1) diff --git a/spec/services/issuable/common_system_notes_service_spec.rb b/spec/services/issuable/common_system_notes_service_spec.rb index 217550542bb..fc01ee8f672 100644 --- a/spec/services/issuable/common_system_notes_service_spec.rb +++ b/spec/services/issuable/common_system_notes_service_spec.rb @@ -19,7 +19,7 @@ RSpec.describe Issuable::CommonSystemNotesService do before do issuable.labels << label - issuable.save + issuable.save! end it 'creates a resource label event' do @@ -69,7 +69,7 @@ RSpec.describe Issuable::CommonSystemNotesService do subject { described_class.new(project, user).execute(issuable, old_labels: [], is_update: false) } it 'does not create system note for title and description' do - issuable.save + issuable.save! expect { subject }.not_to change { issuable.notes.count } end @@ -78,7 +78,7 @@ RSpec.describe Issuable::CommonSystemNotesService do label = create(:label, project: project) issuable.labels << label - issuable.save + issuable.save! expect { subject }.to change { issuable.resource_label_events.count }.from(0).to(1) @@ -104,7 +104,7 @@ RSpec.describe Issuable::CommonSystemNotesService do it 'creates a system note for due_date set' do issuable.due_date = Date.today - issuable.save + issuable.save! expect { subject }.to change { issuable.notes.count }.from(0).to(1) expect(issuable.notes.last.note).to match('changed due date') diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 4db6e5cac12..9076fb11c9b 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -233,26 +233,11 @@ RSpec.describe Issues::CloseService do expect(email.subject).to include(issue.title) end - context 'when resource state events are disabled' do - before do - stub_feature_flags(track_resource_state_change_events: false) - end - - it 'creates system note about the issue being closed' do - close_issue - - note = issue.notes.last - expect(note.note).to include "closed" - end - end - - context 'when resource state events are enabled' do - it 'creates resource state event about the issue being closed' do - close_issue + it 'creates resource state event about the issue being closed' do + close_issue - event = issue.resource_state_events.last - expect(event.state).to eq('closed') - end + event = issue.resource_state_events.last + expect(event.state).to eq('closed') end it 'marks todos as done' do diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index c2989dc86cf..7997b8de3fd 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Issues::MoveService do + include DesignManagementTestHelpers + let_it_be(:user) { create(:user) } let_it_be(:author) { create(:user) } let_it_be(:title) { 'Some issue' } @@ -201,6 +203,54 @@ RSpec.describe Issues::MoveService do expect(copied_notes.order('id ASC').pluck(:note)).to eq(notes.map(&:note)) end end + + context 'issue with a design', :clean_gitlab_redis_shared_state do + let!(:design) { create(:design, :with_lfs_file, issue: old_issue) } + let!(:note) { create(:diff_note_on_design, noteable: design, issue: old_issue, project: old_issue.project) } + let(:subject) { move_service.execute(old_issue, new_project) } + + before do + enable_design_management + end + + it 'calls CopyDesignCollection::QueueService' do + expect(DesignManagement::CopyDesignCollection::QueueService).to receive(:new) + .with(user, old_issue, kind_of(Issue)) + .and_call_original + + subject + end + + it 'logs if QueueService returns an error', :aggregate_failures do + error_message = 'error' + + expect_next_instance_of(DesignManagement::CopyDesignCollection::QueueService) do |service| + expect(service).to receive(:execute).and_return( + ServiceResponse.error(message: error_message) + ) + end + expect(Gitlab::AppLogger).to receive(:error).with(error_message) + + subject + end + + it 'does not call QueueService when the feature flag is disabled' do + stub_feature_flags(design_management_copy_designs: false) + + expect(DesignManagement::CopyDesignCollection::QueueService).not_to receive(:new) + + subject + end + + # Perform a small integration test to ensure the services and worker + # can correctly create designs. + it 'copies the design and its notes', :sidekiq_inline, :aggregate_failures do + new_issue = subject + + expect(new_issue.designs.size).to eq(1) + expect(new_issue.designs.first.notes.size).to eq(1) + end + end end describe 'move permissions' do diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index b3e8fba4e9a..cfda27795c7 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -650,7 +650,7 @@ RSpec.describe Issues::UpdateService, :mailer do context 'when the labels change' do before do - Timecop.freeze(1.minute.from_now) do + travel_to(1.minute.from_now) do update_issue(label_ids: [label.id]) end end diff --git a/spec/services/keys/last_used_service_spec.rb b/spec/services/keys/last_used_service_spec.rb index 82b6b05975b..a2cd5ffdd38 100644 --- a/spec/services/keys/last_used_service_spec.rb +++ b/spec/services/keys/last_used_service_spec.rb @@ -8,7 +8,7 @@ RSpec.describe Keys::LastUsedService do key = create(:key, last_used_at: 1.year.ago) time = Time.zone.now - Timecop.freeze(time) { described_class.new(key).execute } + travel_to(time) { described_class.new(key).execute } expect(key.reload.last_used_at).to be_like_time(time) end diff --git a/spec/services/lfs/push_service_spec.rb b/spec/services/lfs/push_service_spec.rb index 8e5b98fdc9c..f67284ff48d 100644 --- a/spec/services/lfs/push_service_spec.rb +++ b/spec/services/lfs/push_service_spec.rb @@ -19,7 +19,7 @@ RSpec.describe Lfs::PushService do stub_lfs_batch(lfs_object) expect(lfs_client) - .to receive(:upload) + .to receive(:upload!) .with(lfs_object, upload_action_spec(lfs_object), authenticated: true) expect(service.execute).to eq(status: :success) @@ -28,7 +28,7 @@ RSpec.describe Lfs::PushService do it 'does nothing if there are no LFS objects' do lfs_object.destroy! - expect(lfs_client).not_to receive(:upload) + expect(lfs_client).not_to receive(:upload!) expect(service.execute).to eq(status: :success) end @@ -36,20 +36,39 @@ RSpec.describe Lfs::PushService do it 'does not upload the object when upload is not requested' do stub_lfs_batch(lfs_object, upload: false) - expect(lfs_client).not_to receive(:upload) + expect(lfs_client).not_to receive(:upload!) expect(service.execute).to eq(status: :success) end + it 'verifies the upload if requested' do + stub_lfs_batch(lfs_object, verify: true) + + expect(lfs_client).to receive(:upload!) + expect(lfs_client) + .to receive(:verify!) + .with(lfs_object, verify_action_spec(lfs_object), authenticated: true) + + expect(service.execute).to eq(status: :success) + end + + it 'skips verification if requested but upload fails' do + stub_lfs_batch(lfs_object, verify: true) + + expect(lfs_client).to receive(:upload!) { raise 'failed' } + expect(lfs_client).not_to receive(:verify!) + expect(service.execute).to eq(status: :error, message: 'failed') + end + it 'returns a failure when submitting a batch fails' do - expect(lfs_client).to receive(:batch) { raise 'failed' } + expect(lfs_client).to receive(:batch!) { raise 'failed' } expect(service.execute).to eq(status: :error, message: 'failed') end it 'returns a failure when submitting an upload fails' do stub_lfs_batch(lfs_object) - expect(lfs_client).to receive(:upload) { raise 'failed' } + expect(lfs_client).to receive(:upload!) { raise 'failed' } expect(service.execute).to eq(status: :error, message: 'failed') end @@ -71,23 +90,28 @@ RSpec.describe Lfs::PushService do create(:lfs_objects_project, project: project, repository_type: type).lfs_object end - def stub_lfs_batch(*objects, upload: true) + def stub_lfs_batch(*objects, upload: true, verify: false) expect(lfs_client) - .to receive(:batch).with('upload', containing_exactly(*objects)) - .and_return('transfer' => 'basic', 'objects' => objects.map { |o| object_spec(o, upload: upload) }) + .to receive(:batch!).with('upload', containing_exactly(*objects)) + .and_return('transfer' => 'basic', 'objects' => objects.map { |o| object_spec(o, upload: upload, verify: verify) }) end - def batch_spec(*objects, upload: true) + def batch_spec(*objects, upload: true, verify: false) { 'transfer' => 'basic', 'objects' => objects.map {|o| object_spec(o, upload: upload) } } end - def object_spec(object, upload: true) - { 'oid' => object.oid, 'size' => object.size, 'authenticated' => true }.tap do |spec| - spec['actions'] = { 'upload' => upload_action_spec(object) } if upload + def object_spec(object, upload: true, verify: false) + { 'oid' => object.oid, 'size' => object.size, 'authenticated' => true, 'actions' => {} }.tap do |spec| + spec['actions']['upload'] = upload_action_spec(object) if upload + spec['actions']['verify'] = verify_action_spec(object) if verify end end def upload_action_spec(object) { 'href' => "https://example.com/#{object.oid}/#{object.size}", 'header' => { 'Key' => 'value' } } end + + def verify_action_spec(object) + { 'href' => "https://example.com/#{object.oid}/#{object.size}/verify", 'header' => { 'Key' => 'value' } } + end end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 3b3f2f3b95a..4f731ad5852 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -29,15 +29,15 @@ RSpec.describe Members::DestroyService do end it 'destroys the member' do - expect { described_class.new(current_user).execute(member, opts) }.to change { member.source.members_and_requesters.count }.by(-1) + expect { described_class.new(current_user).execute(member, **opts) }.to change { member.source.members_and_requesters.count }.by(-1) end it 'destroys member notification_settings' do if member_user.notification_settings.any? - expect { described_class.new(current_user).execute(member, opts) } + expect { described_class.new(current_user).execute(member, **opts) } .to change { member_user.notification_settings.count }.by(-1) else - expect { described_class.new(current_user).execute(member, opts) } + expect { described_class.new(current_user).execute(member, **opts) } .not_to change { member_user.notification_settings.count } end end @@ -63,7 +63,7 @@ RSpec.describe Members::DestroyService do expect(service).to receive(:enqueue_unassign_issuables).with(member) end - service.execute(member, opts) + service.execute(member, **opts) expect(member_user.assigned_open_merge_requests_count).to be(0) expect(member_user.assigned_open_issues_count).to be(0) @@ -83,14 +83,14 @@ RSpec.describe Members::DestroyService do it 'calls Member#after_decline_request' do expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(member) - described_class.new(current_user).execute(member, opts) + described_class.new(current_user).execute(member, **opts) end context 'when current user is the member' do it 'does not call Member#after_decline_request' do expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member) - described_class.new(member_user).execute(member, opts) + described_class.new(member_user).execute(member, **opts) end end end @@ -280,7 +280,6 @@ RSpec.describe Members::DestroyService do context 'subresources' do let(:user) { create(:user) } let(:member_user) { create(:user) } - let(:opts) { {} } let(:group) { create(:group, :public) } let(:subgroup) { create(:group, parent: group) } @@ -303,7 +302,7 @@ RSpec.describe Members::DestroyService do group_member = create(:group_member, :developer, group: group, user: member_user) - described_class.new(user).execute(group_member, opts) + described_class.new(user).execute(group_member) end it 'removes the project membership' do @@ -350,7 +349,6 @@ RSpec.describe Members::DestroyService do context 'deletion of invitations created by deleted project member' do let(:user) { project.owner } let(:member_user) { create(:user) } - let(:opts) { {} } let(:project) { create(:project) } @@ -359,7 +357,7 @@ RSpec.describe Members::DestroyService do project_member = create(:project_member, :maintainer, user: member_user, project: project) - described_class.new(user).execute(project_member, opts) + described_class.new(user).execute(project_member) end it 'removes project members invited by deleted user' do diff --git a/spec/services/members/invitation_reminder_email_service_spec.rb b/spec/services/members/invitation_reminder_email_service_spec.rb new file mode 100644 index 00000000000..88280869476 --- /dev/null +++ b/spec/services/members/invitation_reminder_email_service_spec.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Members::InvitationReminderEmailService do + describe 'sending invitation reminders' do + subject { described_class.new(invitation).execute } + + let_it_be(:frozen_time) { Date.today.beginning_of_day } + let_it_be(:invitation) { build(:group_member, :invited, created_at: frozen_time) } + + context 'when the experiment is disabled' do + before do + allow(Gitlab::Experimentation).to receive(:enabled_for_attribute?).and_return(false) + invitation.expires_at = frozen_time + 2.days + end + + it 'does not send an invitation' do + travel_to(frozen_time + 1.day) do + expect(invitation).not_to receive(:send_invitation_reminder) + + subject + end + end + end + + context 'when the experiment is enabled' do + before do + allow(Gitlab::Experimentation).to receive(:enabled_for_attribute?).and_return(true) + invitation.expires_at = frozen_time + expires_at_days.days if expires_at_days + end + + using RSpec::Parameterized::TableSyntax + + where(:expires_at_days, :send_reminder_at_days) do + 0 | [] + 1 | [] + 2 | [1] + 3 | [1, 2] + 4 | [1, 2, 3] + 5 | [1, 2, 4] + 6 | [1, 3, 5] + 7 | [1, 3, 5] + 8 | [2, 3, 6] + 9 | [2, 4, 7] + 10 | [2, 4, 8] + 11 | [2, 4, 8] + 12 | [2, 5, 9] + 13 | [2, 5, 10] + 14 | [2, 5, 10] + 15 | [2, 5, 10] + nil | [2, 5, 10] + end + + with_them do + # Create an invitation today with an expiration date from 0 to 10 days in the future or without an expiration date + # We chose 10 days here, because we fetch invitations that were created at most 10 days ago. + (0..10).each do |day| + it 'sends an invitation reminder only on the expected days' do + next if day > (expires_at_days || 10) # We don't need to test after the invitation has already expired + + # We are traveling in a loop from today to 10 days from now + travel_to(frozen_time + day.days) do + # Given an expiration date and the number of days after the creation of the invitation based on the current day in the loop, a reminder may be sent + if (reminder_index = send_reminder_at_days.index(day)) + expect(invitation).to receive(:send_invitation_reminder).with(reminder_index) + else + expect(invitation).not_to receive(:send_invitation_reminder) + end + + subject + end + end + end + end + end + end +end diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index e7ac286f48b..67fb4eaade5 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -19,54 +19,45 @@ RSpec.describe MergeRequests::CloseService do describe '#execute' do it_behaves_like 'cache counters invalidator' - [true, false].each do |state_tracking_enabled| - context "valid params with state_tracking #{state_tracking_enabled ? 'enabled' : 'disabled'}" do - let(:service) { described_class.new(project, user, {}) } + context 'valid params' do + let(:service) { described_class.new(project, user, {}) } - before do - stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - - allow(service).to receive(:execute_hooks) + before do + allow(service).to receive(:execute_hooks) - perform_enqueued_jobs do - @merge_request = service.execute(merge_request) - end + perform_enqueued_jobs do + @merge_request = service.execute(merge_request) end + end - it { expect(@merge_request).to be_valid } - it { expect(@merge_request).to be_closed } + it { expect(@merge_request).to be_valid } + it { expect(@merge_request).to be_closed } - it 'executes hooks with close action' do - expect(service).to have_received(:execute_hooks) - .with(@merge_request, 'close') - end + it 'executes hooks with close action' do + expect(service).to have_received(:execute_hooks) + .with(@merge_request, 'close') + end - it 'sends email to user2 about assign of new merge_request', :sidekiq_might_not_need_inline do - email = ActionMailer::Base.deliveries.last - expect(email.to.first).to eq(user2.email) - expect(email.subject).to include(merge_request.title) - end + it 'sends email to user2 about assign of new merge_request', :sidekiq_might_not_need_inline do + email = ActionMailer::Base.deliveries.last + expect(email.to.first).to eq(user2.email) + expect(email.subject).to include(merge_request.title) + end - it 'creates system note about merge_request reassign' do - if state_tracking_enabled - event = @merge_request.resource_state_events.last - expect(event.state).to eq('closed') - else - note = @merge_request.notes.last - expect(note.note).to include 'closed' - end - end + it 'creates a resource event' do + event = @merge_request.resource_state_events.last + expect(event.state).to eq('closed') + end - it 'marks todos as done' do - expect(todo.reload).to be_done - end + it 'marks todos as done' do + expect(todo.reload).to be_done + end - context 'when auto merge is enabled' do - let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } + context 'when auto merge is enabled' do + let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } - it 'cancels the auto merge' do - expect(@merge_request).not_to be_auto_merge_enabled - end + it 'cancels the auto merge' do + expect(@merge_request).not_to be_auto_merge_enabled end end end diff --git a/spec/services/merge_requests/export_csv_service_spec.rb b/spec/services/merge_requests/export_csv_service_spec.rb new file mode 100644 index 00000000000..8161a444231 --- /dev/null +++ b/spec/services/merge_requests/export_csv_service_spec.rb @@ -0,0 +1,115 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::ExportCsvService do + let_it_be(:merge_request) { create(:merge_request) } + let(:csv) { CSV.parse(subject.csv_data, headers: true).first } + + subject { described_class.new(MergeRequest.where(id: merge_request.id)) } + + describe 'csv_data' do + it 'contains the correct information', :aggregate_failures do + expect(csv['MR IID']).to eq(merge_request.iid.to_s) + expect(csv['Title']).to eq(merge_request.title) + expect(csv['State']).to eq(merge_request.state) + expect(csv['Description']).to eq(merge_request.description) + expect(csv['Source Branch']).to eq(merge_request.source_branch) + expect(csv['Target Branch']).to eq(merge_request.target_branch) + expect(csv['Source Project ID']).to eq(merge_request.source_project_id.to_s) + expect(csv['Target Project ID']).to eq(merge_request.target_project_id.to_s) + expect(csv['Author']).to eq(merge_request.author.name) + expect(csv['Author Username']).to eq(merge_request.author.username) + end + + describe 'assignees' do + context 'when assigned' do + let_it_be(:merge_request) { create(:merge_request, assignees: create_list(:user, 2)) } + + it 'contains the names of assignees' do + expect(csv['Assignees']).to eq(merge_request.assignees.map(&:name).join(', ')) + end + + it 'contains the usernames of assignees' do + expect(csv['Assignee Usernames']).to eq(merge_request.assignees.map(&:username).join(', ')) + end + end + + context 'when not assigned' do + it 'returns empty strings' do + expect(csv['Assignees']).to eq('') + expect(csv['Assignee Usernames']).to eq('') + end + end + end + + describe 'approvers' do + context 'when approved' do + let_it_be(:merge_request) { create(:merge_request) } + let(:approvers) { create_list(:user, 2) } + + before do + merge_request.approved_by_users = approvers + end + + it 'contains the names of approvers separated by a comma' do + expect(csv['Approvers'].split(', ')).to contain_exactly(approvers[0].name, approvers[1].name) + end + + it 'contains the usernames of approvers separated by a comma' do + expect(csv['Approver Usernames'].split(', ')).to contain_exactly(approvers[0].username, approvers[1].username) + end + end + + context 'when not approved' do + it 'returns empty strings' do + expect(csv['Approvers']).to eq('') + expect(csv['Approver Usernames']).to eq('') + end + end + end + + describe 'merged user' do + context 'MR is merged' do + let_it_be(:merge_request) { create(:merge_request, :merged, :with_merged_metrics) } + + it 'is merged' do + expect(csv['State']).to eq('merged') + end + + it 'has a merged user' do + expect(csv['Merged User']).to eq(merge_request.metrics.merged_by.name) + expect(csv['Merged Username']).to eq(merge_request.metrics.merged_by.username) + end + end + + context 'MR is not merged' do + it 'returns empty strings' do + expect(csv['Merged User']).to eq('') + expect(csv['Merged Username']).to eq('') + end + end + end + + describe 'milestone' do + context 'milestone is assigned' do + let_it_be(:merge_request) { create(:merge_request) } + let_it_be(:milestone) { create(:milestone, :active, project: merge_request.project) } + + before do + merge_request.update!(milestone_id: milestone.id) + end + + it 'contains the milestone ID' do + expect(csv['Milestone ID']).to eq(merge_request.milestone.id.to_s) + end + end + + context 'no milestone is assigned' do + it 'returns an empty string' do + expect(csv['Milestone ID']).to eq('') + end + end + end + end +end diff --git a/spec/services/merge_requests/ff_merge_service_spec.rb b/spec/services/merge_requests/ff_merge_service_spec.rb index 55856deeaca..64c473d947f 100644 --- a/spec/services/merge_requests/ff_merge_service_spec.rb +++ b/spec/services/merge_requests/ff_merge_service_spec.rb @@ -22,74 +22,72 @@ RSpec.describe MergeRequests::FfMergeService do end describe '#execute' do - [true, false].each do |state_tracking_enabled| - context "valid params with state_tracking #{state_tracking_enabled ? 'enabled' : 'disabled'}" do - let(:service) { described_class.new(project, user, valid_merge_params) } - - def execute_ff_merge - perform_enqueued_jobs do - service.execute(merge_request) - end + context 'valid params' do + let(:service) { described_class.new(project, user, valid_merge_params) } + + def execute_ff_merge + perform_enqueued_jobs do + service.execute(merge_request) end + end - before do - stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) + before do + allow(service).to receive(:execute_hooks) + end - allow(service).to receive(:execute_hooks) - end + it "does not create merge commit" do + execute_ff_merge - it "does not create merge commit" do - execute_ff_merge + source_branch_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha + target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha - source_branch_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha - target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha + expect(source_branch_sha).to eq(target_branch_sha) + end - expect(source_branch_sha).to eq(target_branch_sha) - end + it 'keeps the merge request valid' do + expect { execute_ff_merge } + .not_to change { merge_request.valid? } + end - it 'keeps the merge request valid' do - expect { execute_ff_merge } - .not_to change { merge_request.valid? } - end + it 'updates the merge request to merged' do + expect { execute_ff_merge } + .to change { merge_request.merged? } + .from(false) + .to(true) + end - it 'updates the merge request to merged' do - expect { execute_ff_merge } - .to change { merge_request.merged? } - .from(false) - .to(true) - end + it 'sends email to user2 about merge of new merge_request' do + execute_ff_merge - it 'sends email to user2 about merge of new merge_request' do - execute_ff_merge + email = ActionMailer::Base.deliveries.last + expect(email.to.first).to eq(user2.email) + expect(email.subject).to include(merge_request.title) + end - email = ActionMailer::Base.deliveries.last - expect(email.to.first).to eq(user2.email) - expect(email.subject).to include(merge_request.title) - end + it 'creates resource event about merge_request merge' do + execute_ff_merge - it 'creates system note about merge_request merge' do - execute_ff_merge + event = merge_request.resource_state_events.last + expect(event.state).to eq('merged') + end - if state_tracking_enabled - event = merge_request.resource_state_events.last - expect(event.state).to eq('merged') - else - note = merge_request.notes.last - expect(note.note).to include 'merged' - end - end + it 'does not update squash_commit_sha if it is not a squash' do + expect(merge_request).to receive(:update_and_mark_in_progress_merge_commit_sha).twice.and_call_original - it 'does not update squash_commit_sha if it is not a squash' do - expect { execute_ff_merge }.not_to change { merge_request.squash_commit_sha } - end + expect { execute_ff_merge }.not_to change { merge_request.squash_commit_sha } + expect(merge_request.in_progress_merge_commit_sha).to be_nil + end - it 'updates squash_commit_sha if it is a squash' do - merge_request.update!(squash: true) + it 'updates squash_commit_sha if it is a squash' do + expect(merge_request).to receive(:update_and_mark_in_progress_merge_commit_sha).twice.and_call_original - expect { execute_ff_merge } - .to change { merge_request.squash_commit_sha } - .from(nil) - end + merge_request.update!(squash: true) + + expect { execute_ff_merge } + .to change { merge_request.squash_commit_sha } + .from(nil) + + expect(merge_request.in_progress_merge_commit_sha).to be_nil end end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 8328f461029..d0e3102f157 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -20,12 +20,9 @@ RSpec.describe MergeRequests::MergeService do end context 'valid params' do - let(:state_tracking) { true } - before do - stub_feature_flags(track_resource_state_change_events: state_tracking) - allow(service).to receive(:execute_hooks) + expect(merge_request).to receive(:update_and_mark_in_progress_merge_commit_sha).twice.and_call_original perform_enqueued_jobs do service.execute(merge_request) @@ -47,20 +44,9 @@ RSpec.describe MergeRequests::MergeService do end context 'note creation' do - context 'when resource state event tracking is disabled' do - let(:state_tracking) { false } - - it 'creates system note about merge_request merge' do - note = merge_request.notes.last - expect(note.note).to include 'merged' - end - end - - context 'when resource state event tracking is enabled' do - it 'creates resource state event about merge_request merge' do - event = merge_request.resource_state_events.last - expect(event.state).to eq('merged') - end + it 'creates resource state event about merge_request merge' do + event = merge_request.resource_state_events.last + expect(event.state).to eq('merged') end end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index cace1e0bf09..ca0c4b29ebe 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -367,76 +367,58 @@ RSpec.describe MergeRequests::RefreshService do end end - [true, false].each do |state_tracking_enabled| - context "push to origin repo target branch with state tracking #{state_tracking_enabled ? 'enabled' : 'disabled'}", :sidekiq_might_not_need_inline do + context 'push to origin repo target branch', :sidekiq_might_not_need_inline do + context 'when all MRs to the target branch had diffs' do before do - stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) + service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + reload_mrs end - context 'when all MRs to the target branch had diffs' do - before do - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') - reload_mrs - end + it 'updates the merge state' do + expect(@merge_request).to be_merged + expect(@fork_merge_request).to be_merged + expect(@build_failed_todo).to be_done + expect(@fork_build_failed_todo).to be_done - it 'updates the merge state' do - expect(@merge_request).to be_merged - expect(@fork_merge_request).to be_merged - expect(@build_failed_todo).to be_done - expect(@fork_build_failed_todo).to be_done - - if state_tracking_enabled - expect(@merge_request.resource_state_events.last.state).to eq('merged') - expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') - else - expect(@merge_request.notes.last.note).to include('merged') - expect(@fork_merge_request.notes.last.note).to include('merged') - end - end + expect(@merge_request.resource_state_events.last.state).to eq('merged') + expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') end + end - context 'when an MR to be closed was empty already' do - let!(:empty_fork_merge_request) do - create(:merge_request, - source_project: @fork_project, - source_branch: 'master', - target_branch: 'master', - target_project: @project) - end + context 'when an MR to be closed was empty already' do + let!(:empty_fork_merge_request) do + create(:merge_request, + source_project: @fork_project, + source_branch: 'master', + target_branch: 'master', + target_project: @project) + end - before do - # This spec already has a fake push, so pretend that we were targeting - # feature all along. - empty_fork_merge_request.update_columns(target_branch: 'feature') + before do + # This spec already has a fake push, so pretend that we were targeting + # feature all along. + empty_fork_merge_request.update_columns(target_branch: 'feature') - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') - reload_mrs - empty_fork_merge_request.reload - end + service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + reload_mrs + empty_fork_merge_request.reload + end - it 'only updates the non-empty MRs' do - expect(@merge_request).to be_merged - expect(@fork_merge_request).to be_merged - - expect(empty_fork_merge_request).to be_open - expect(empty_fork_merge_request.merge_request_diff.state).to eq('empty') - expect(empty_fork_merge_request.notes).to be_empty - - if state_tracking_enabled - expect(@merge_request.resource_state_events.last.state).to eq('merged') - expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') - else - expect(@merge_request.notes.last.note).to include('merged') - expect(@fork_merge_request.notes.last.note).to include('merged') - end - end + it 'only updates the non-empty MRs' do + expect(@merge_request).to be_merged + expect(@fork_merge_request).to be_merged + + expect(empty_fork_merge_request).to be_open + expect(empty_fork_merge_request.merge_request_diff.state).to eq('empty') + expect(empty_fork_merge_request.notes).to be_empty + + expect(@merge_request.resource_state_events.last.state).to eq('merged') + expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') end end - context "manual merge of source branch #{state_tracking_enabled ? 'enabled' : 'disabled'}", :sidekiq_might_not_need_inline do + context 'manual merge of source branch', :sidekiq_might_not_need_inline do before do - stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - # Merge master -> feature branch @project.repository.merge(@user, @merge_request.diff_head_sha, @merge_request, 'Test message') commit = @project.repository.commit('feature') @@ -445,13 +427,8 @@ RSpec.describe MergeRequests::RefreshService do end it 'updates the merge state' do - if state_tracking_enabled - expect(@merge_request.resource_state_events.last.state).to eq('merged') - expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') - else - expect(@merge_request.notes.last.note).to include('merged') - expect(@fork_merge_request.notes.last.note).to include('merged') - end + expect(@merge_request.resource_state_events.last.state).to eq('merged') + expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') expect(@merge_request).to be_merged expect(@merge_request.diffs.size).to be > 0 @@ -616,29 +593,21 @@ RSpec.describe MergeRequests::RefreshService do end end - [true, false].each do |state_tracking_enabled| - context "push to origin repo target branch after fork project was removed #{state_tracking_enabled ? 'enabled' : 'disabled'}" do - before do - stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) + context 'push to origin repo target branch after fork project was removed' do + before do + @fork_project.destroy! + service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + reload_mrs + end - @fork_project.destroy! - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') - reload_mrs - end + it 'updates the merge request state' do + expect(@merge_request.resource_state_events.last.state).to eq('merged') - it 'updates the merge request state' do - if state_tracking_enabled - expect(@merge_request.resource_state_events.last.state).to eq('merged') - else - expect(@merge_request.notes.last.note).to include('merged') - end - - expect(@merge_request).to be_merged - expect(@fork_merge_request).to be_open - expect(@fork_merge_request.notes).to be_empty - expect(@build_failed_todo).to be_done - expect(@fork_build_failed_todo).to be_done - end + expect(@merge_request).to be_merged + expect(@fork_merge_request).to be_open + expect(@fork_merge_request.notes).to be_empty + expect(@build_failed_todo).to be_done + expect(@fork_build_failed_todo).to be_done end end diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb index 0066834180e..ffc2ebb344c 100644 --- a/spec/services/merge_requests/reopen_service_spec.rb +++ b/spec/services/merge_requests/reopen_service_spec.rb @@ -20,11 +20,8 @@ RSpec.describe MergeRequests::ReopenService do context 'valid params' do let(:service) { described_class.new(project, user, {}) } - let(:state_tracking) { true } before do - stub_feature_flags(track_resource_state_change_events: state_tracking) - allow(service).to receive(:execute_hooks) perform_enqueued_jobs do @@ -47,20 +44,9 @@ RSpec.describe MergeRequests::ReopenService do end context 'note creation' do - context 'when state event tracking is disabled' do - let(:state_tracking) { false } - - it 'creates system note about merge_request reopen' do - note = merge_request.notes.last - expect(note.note).to include 'reopened' - end - end - - context 'when state event tracking is enabled' do - it 'creates resource state event about merge_request reopen' do - event = merge_request.resource_state_events.last - expect(event.state).to eq('reopened') - end + it 'creates resource state event about merge_request reopen' do + event = merge_request.resource_state_events.last + expect(event.state).to eq('reopened') end end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 3c3e10495d3..ed8872b71f7 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -53,7 +53,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do title: 'New title', description: 'Also please fix', assignee_ids: [user.id], - reviewer_ids: [user.id], + reviewer_ids: [], state_event: 'close', label_ids: [label.id], target_branch: 'target', @@ -78,7 +78,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(@merge_request).to be_valid expect(@merge_request.title).to eq('New title') expect(@merge_request.assignees).to match_array([user]) - expect(@merge_request.reviewers).to match_array([user]) + expect(@merge_request.reviewers).to match_array([]) expect(@merge_request).to be_closed expect(@merge_request.labels.count).to eq(1) expect(@merge_request.labels.first.title).to eq(label.name) @@ -116,6 +116,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do labels: [], mentioned_users: [user2], assignees: [user3], + reviewers: [], milestone: nil, total_time_spent: 0, description: "FYI #{user2.to_reference}" @@ -138,6 +139,35 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(note.note).to include "assigned to #{user.to_reference} and unassigned #{user3.to_reference}" end + context 'with reviewers' do + let(:opts) { { reviewer_ids: [user2.id] } } + + context 'when merge_request_reviewers feature is disabled' do + before(:context) do + stub_feature_flags(merge_request_reviewers: false) + end + + it 'does not create a system note about merge_request review request' do + note = find_note('review requested from') + + expect(note).to be_nil + end + end + + context 'when merge_request_reviewers feature is enabled' do + before(:context) do + stub_feature_flags(merge_request_reviewers: true) + end + + it 'creates system note about merge_request review request' do + note = find_note('requested review from') + + expect(note).not_to be_nil + expect(note.note).to include "requested review from #{user2.to_reference}" + end + end + end + it 'creates a resource label event' do event = merge_request.resource_label_events.last @@ -467,15 +497,15 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end context 'when reviewers gets changed' do - before do + it 'marks pending todo as done' do update_merge_request({ reviewer_ids: [user2.id] }) - end - it 'marks pending todo as done' do expect(pending_todo.reload).to be_done end it 'creates a pending todo for new review request' do + update_merge_request({ reviewer_ids: [user2.id] }) + attributes = { project: project, author: user, @@ -488,6 +518,17 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(Todo.where(attributes).count).to eq 1 end + + it 'sends email reviewer change notifications to old and new reviewers', :sidekiq_might_not_need_inline do + merge_request.reviewers = [user2] + + perform_enqueued_jobs do + update_merge_request({ reviewer_ids: [user3.id] }) + end + + should_email(user2) + should_email(user3) + end end context 'when the milestone is removed' do @@ -542,7 +583,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do context 'when the labels change' do before do - Timecop.freeze(1.minute.from_now) do + travel_to(1.minute.from_now) do update_merge_request({ label_ids: [label.id] }) 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 aea9c25d104..5dc30c156ac 100644 --- a/spec/services/metrics/dashboard/custom_dashboard_service_spec.rb +++ b/spec/services/metrics/dashboard/custom_dashboard_service_spec.rb @@ -67,6 +67,23 @@ RSpec.describe Metrics::Dashboard::CustomDashboardService, :use_clean_rails_memo .at_least(:once) end + context 'with metric in database' do + let!(:prometheus_metric) do + create(:prometheus_metric, project: project, identifier: 'metric_a1', group: 'custom') + end + + it 'includes metric_id' do + dashboard = described_class.new(*service_params).get_dashboard + + metric_id = dashboard[:dashboard][:panel_groups].find { |panel_group| panel_group[:group] == 'Group A' } + .fetch(:panels).find { |panel| panel[:title] == 'Super Chart A1' } + .fetch(:metrics).find { |metric| metric[:id] == 'metric_a1' } + .fetch(:metric_id) + + expect(metric_id).to eq(prometheus_metric.id) + end + end + context 'and the dashboard is then deleted' do it 'does not return the previously cached dashboard' do described_class.new(*service_params).get_dashboard diff --git a/spec/services/milestones/destroy_service_spec.rb b/spec/services/milestones/destroy_service_spec.rb index 66c5c504c64..dd68471d927 100644 --- a/spec/services/milestones/destroy_service_spec.rb +++ b/spec/services/milestones/destroy_service_spec.rb @@ -45,7 +45,7 @@ RSpec.describe Milestones::DestroyService do let(:group_milestone) { create(:milestone, group: group) } before do - project.update(namespace: group) + project.update!(namespace: group) group.add_developer(user) end diff --git a/spec/services/milestones/promote_service_spec.rb b/spec/services/milestones/promote_service_spec.rb index f0a34241c74..8f4201d8d94 100644 --- a/spec/services/milestones/promote_service_spec.rb +++ b/spec/services/milestones/promote_service_spec.rb @@ -23,7 +23,7 @@ RSpec.describe Milestones::PromoteService do end it 'raises error if project does not belong to a group' do - project.update(namespace: user.namespace) + project.update!(namespace: user.namespace) expect { service.execute(milestone) }.to raise_error(described_class::PromoteMilestoneError) end diff --git a/spec/services/milestones/transfer_service_spec.rb b/spec/services/milestones/transfer_service_spec.rb index 4a626fe688a..6f4f55b2bd0 100644 --- a/spec/services/milestones/transfer_service_spec.rb +++ b/spec/services/milestones/transfer_service_spec.rb @@ -23,7 +23,7 @@ RSpec.describe Milestones::TransferService do new_group.add_maintainer(user) project.add_maintainer(user) # simulate project transfer - project.update(group: new_group) + project.update!(group: new_group) end context 'without existing milestone at the new group level' do diff --git a/spec/services/namespace_settings/update_service_spec.rb b/spec/services/namespace_settings/update_service_spec.rb new file mode 100644 index 00000000000..2319bdcd4de --- /dev/null +++ b/spec/services/namespace_settings/update_service_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe NamespaceSettings::UpdateService do + let(:user) { create(:user) } + let(:group) { create(:group) } + let(:settings) { {} } + + subject(:service) { described_class.new(user, group, settings) } + + describe "#execute" do + context "group has no namespace_settings" do + it "builds out a new namespace_settings record" do + expect do + service.execute + end.to change { NamespaceSetting.count }.by(1) + end + end + + context "group has a namespace_settings" do + before do + create(:namespace_settings, namespace: group) + + service.execute + end + + it "doesn't create a new namespace_setting record" do + expect do + service.execute + end.not_to change { NamespaceSetting.count } + end + end + end +end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 7c0d4b756bd..4da9f4115a1 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -437,7 +437,7 @@ RSpec.describe Notes::CreateService do expect do existing_note - Timecop.freeze(Time.current + 1.minute) { subject } + travel_to(Time.current + 1.minute) { subject } existing_note.reload end.to change { existing_note.type }.from(nil).to('DiscussionNote') diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index 47b8ba0cd72..66efdf8abe7 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -42,7 +42,7 @@ RSpec.describe Notes::UpdateService do end it 'does not update the note when params is blank' do - Timecop.freeze(1.day.from_now) do + travel_to(1.day.from_now) do expect { update_note({}) }.not_to change { note.reload.updated_at } end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 03e24524f9f..473a06c4c8c 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -150,6 +150,16 @@ RSpec.describe NotificationService, :mailer do end end + shared_examples 'participating by reviewer notification' do + it 'emails the participant' do + issuable.reviewers << participant + + notification_trigger + + should_email(participant) + end + end + shared_examples_for 'participating notifications' do it_behaves_like 'participating by note notification' it_behaves_like 'participating by author notification' @@ -1778,6 +1788,60 @@ RSpec.describe NotificationService, :mailer do end end + describe '#changed_reviewer_of_merge_request' do + let(:merge_request) { create(:merge_request, author: author, source_project: project, reviewers: [reviewer], description: 'cc @participant') } + + let_it_be(:current_user) { create(:user) } + let_it_be(:reviewer) { create(:user) } + + before do + update_custom_notification(:change_reviewer_merge_request, @u_guest_custom, resource: project) + update_custom_notification(:change_reviewer_merge_request, @u_custom_global) + end + + it 'sends emails to relevant users only', :aggregate_failures do + notification.changed_reviewer_of_merge_request(merge_request, current_user, [reviewer]) + + merge_request.reviewers.each { |reviewer| should_email(reviewer) } + should_email(merge_request.author) + should_email(@u_watcher) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_email(@watcher_and_subscriber) + should_email(@u_guest_watcher) + should_email(@u_guest_custom) + should_email(@u_custom_global) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + should_not_email(@u_lazy_participant) + end + + it 'adds "review requested" reason for new reviewer' do + notification.changed_reviewer_of_merge_request(merge_request, current_user, [reviewer]) + + merge_request.reviewers.each do |assignee| + email = find_email_for(assignee) + + expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::REVIEW_REQUESTED) + end + end + + context 'participating notifications with reviewers' do + let(:participant) { create(:user, username: 'user-participant') } + let(:issuable) { merge_request } + let(:notification_trigger) { notification.changed_reviewer_of_merge_request(merge_request, current_user, [reviewer]) } + + it_behaves_like 'participating notifications' + it_behaves_like 'participating by reviewer notification' + end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.changed_reviewer_of_merge_request(merge_request, current_user, [reviewer]) } + end + end + describe '#push_to_merge_request' do before do update_custom_notification(:push_to_merge_request, @u_guest_custom, resource: project) @@ -3018,32 +3082,25 @@ RSpec.describe NotificationService, :mailer do describe '#prometheus_alerts_fired' do let!(:project) { create(:project) } - let!(:prometheus_alert) { create(:prometheus_alert, project: project) } let!(:master) { create(:user) } let!(:developer) { create(:user) } + let(:alert_attributes) { build(:alert_management_alert, project: project).attributes } before do project.add_maintainer(master) end it 'sends the email to owners and masters' do - expect(Notify).to receive(:prometheus_alert_fired_email).with(project.id, master.id, prometheus_alert).and_call_original - expect(Notify).to receive(:prometheus_alert_fired_email).with(project.id, project.owner.id, prometheus_alert).and_call_original - expect(Notify).not_to receive(:prometheus_alert_fired_email).with(project.id, developer.id, prometheus_alert) + expect(Notify).to receive(:prometheus_alert_fired_email).with(project.id, master.id, alert_attributes).and_call_original + expect(Notify).to receive(:prometheus_alert_fired_email).with(project.id, project.owner.id, alert_attributes).and_call_original + expect(Notify).not_to receive(:prometheus_alert_fired_email).with(project.id, developer.id, alert_attributes) - subject.prometheus_alerts_fired(prometheus_alert.project, [prometheus_alert]) + subject.prometheus_alerts_fired(project, [alert_attributes]) end it_behaves_like 'project emails are disabled' do - before do - allow_next_instance_of(::Gitlab::Alerting::Alert) do |instance| - allow(instance).to receive(:valid?).and_return(true) - end - end - - let(:alert_params) { { 'labels' => { 'gitlab_alert_id' => 'unknown' } } } - let(:notification_target) { prometheus_alert.project } - let(:notification_trigger) { subject.prometheus_alerts_fired(prometheus_alert.project, [alert_params]) } + let(:notification_target) { project } + let(:notification_trigger) { subject.prometheus_alerts_fired(project, [alert_attributes]) } around do |example| perform_enqueued_jobs { example.run } diff --git a/spec/services/packages/create_event_service_spec.rb b/spec/services/packages/create_event_service_spec.rb new file mode 100644 index 00000000000..7e66b430a8c --- /dev/null +++ b/spec/services/packages/create_event_service_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Packages::CreateEventService do + let(:scope) { 'container' } + let(:event_name) { 'push_package' } + + let(:params) do + { + scope: scope, + event_name: event_name + } + end + + subject { described_class.new(nil, user, params).execute } + + describe '#execute' do + shared_examples 'package event creation' do |originator_type, expected_scope| + it 'creates the event' do + expect { subject }.to change { Packages::Event.count }.by(1) + + expect(subject.originator_type).to eq(originator_type) + expect(subject.originator).to eq(user&.id) + expect(subject.event_scope).to eq(expected_scope) + expect(subject.event_type).to eq(event_name) + end + end + + context 'with a user' do + let(:user) { create(:user) } + + it_behaves_like 'package event creation', 'user', 'container' + end + + context 'with a deploy token' do + let(:user) { create(:deploy_token) } + + it_behaves_like 'package event creation', 'deploy_token', 'container' + end + + context 'with no user' do + let(:user) { nil } + + it_behaves_like 'package event creation', 'guest', 'container' + end + + context 'with a package as scope' do + let(:user) { nil } + let(:scope) { create(:npm_package) } + + it_behaves_like 'package event creation', 'guest', 'npm' + end + end +end diff --git a/spec/services/packages/generic/create_package_file_service_spec.rb b/spec/services/packages/generic/create_package_file_service_spec.rb new file mode 100644 index 00000000000..0ae109ef996 --- /dev/null +++ b/spec/services/packages/generic/create_package_file_service_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Generic::CreatePackageFileService do + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + + describe '#execute' do + let(:sha256) { '440e5e148a25331bbd7991575f7d54933c0ebf6cc735a18ee5066ac1381bb590' } + let(:temp_file) { Tempfile.new("test") } + let(:file) { UploadedFile.new(temp_file.path, sha256: sha256) } + let(:package) { create(:generic_package, project: project) } + let(:params) do + { + package_name: 'mypackage', + package_version: '0.0.1', + file: file, + file_name: 'myfile.tar.gz.1' + } + end + + before do + FileUtils.touch(temp_file) + end + + after do + FileUtils.rm_f(temp_file) + end + + it 'creates package file' do + package_service = double + package_params = { + name: params[:package_name], + version: params[:package_version], + build: params[:build] + } + expect(::Packages::Generic::FindOrCreatePackageService).to receive(:new).with(project, user, package_params).and_return(package_service) + expect(package_service).to receive(:execute).and_return(package) + + service = described_class.new(project, user, params) + + expect { service.execute }.to change { package.package_files.count }.by(1) + + package_file = package.package_files.last + aggregate_failures do + expect(package_file.package).to eq(package) + expect(package_file.file_name).to eq('myfile.tar.gz.1') + expect(package_file.size).to eq(file.size) + expect(package_file.file_sha256).to eq(sha256) + end + end + end +end diff --git a/spec/services/packages/generic/find_or_create_package_service_spec.rb b/spec/services/packages/generic/find_or_create_package_service_spec.rb new file mode 100644 index 00000000000..5a9b8b03279 --- /dev/null +++ b/spec/services/packages/generic/find_or_create_package_service_spec.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Generic::FindOrCreatePackageService do + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be(:ci_build) { create(:ci_build, :running, user: user) } + + let(:params) do + { + name: 'mypackage', + version: '0.0.1' + } + end + + describe '#execute' do + context 'when packages does not exist yet' do + it 'creates package' do + service = described_class.new(project, user, params) + + expect { service.execute }.to change { project.packages.generic.count }.by(1) + + package = project.packages.generic.last + + aggregate_failures do + expect(package.creator).to eq(user) + expect(package.name).to eq('mypackage') + expect(package.version).to eq('0.0.1') + expect(package.build_info).to be_nil + end + end + + it 'creates package and package build info when build is provided' do + service = described_class.new(project, user, params.merge(build: ci_build)) + + expect { service.execute }.to change { project.packages.generic.count }.by(1) + + package = project.packages.generic.last + + aggregate_failures do + expect(package.creator).to eq(user) + expect(package.name).to eq('mypackage') + expect(package.version).to eq('0.0.1') + expect(package.build_info.pipeline).to eq(ci_build.pipeline) + end + end + end + + context 'when packages already exists' do + let!(:package) { project.packages.generic.create!(params) } + + context 'when package was created manually' do + it 'finds the package and does not create package build info even if build is provided' do + service = described_class.new(project, user, params.merge(build: ci_build)) + + expect do + found_package = service.execute + + expect(found_package).to eq(package) + end.not_to change { project.packages.generic.count } + + expect(package.reload.build_info).to be_nil + end + end + + context 'when package was created by pipeline' do + let(:pipeline) { create(:ci_pipeline, project: project) } + + before do + package.create_build_info!(pipeline: pipeline) + end + + it 'finds the package and does not change package build info even if build is provided' do + service = described_class.new(project, user, params.merge(build: ci_build)) + + expect do + found_package = service.execute + + expect(found_package).to eq(package) + end.not_to change { project.packages.generic.count } + + expect(package.reload.build_info.pipeline).to eq(pipeline) + end + end + end + end +end diff --git a/spec/services/projects/alerting/notify_service_spec.rb b/spec/services/projects/alerting/notify_service_spec.rb index 77a0e330109..d8e94a0885b 100644 --- a/spec/services/projects/alerting/notify_service_spec.rb +++ b/spec/services/projects/alerting/notify_service_spec.rb @@ -89,6 +89,7 @@ RSpec.describe Projects::Alerting::NotifyService do it 'creates a system note corresponding to alert creation' do expect { subject }.to change(Note, :count).by(1) + expect(Note.last.note).to include(payload_raw.fetch(:monitoring_tool)) end context 'existing alert with same fingerprint' do @@ -127,23 +128,8 @@ RSpec.describe Projects::Alerting::NotifyService do let(:alert) { create(:alert_management_alert, :with_issue, project: project, fingerprint: fingerprint_sha) } let(:issue) { alert.issue } - context 'state_tracking is enabled' do - before do - stub_feature_flags(track_resource_state_change_events: true) - end - - it { expect { subject }.to change { issue.reload.state }.from('opened').to('closed') } - it { expect { subject }.to change(ResourceStateEvent, :count).by(1) } - end - - context 'state_tracking is disabled' do - before do - stub_feature_flags(track_resource_state_change_events: false) - end - - it { expect { subject }.to change { issue.reload.state }.from('opened').to('closed') } - it { expect { subject }.to change(Note, :count).by(1) } - end + it { expect { subject }.to change { issue.reload.state }.from('opened').to('closed') } + it { expect { subject }.to change(ResourceStateEvent, :count).by(1) } end end end @@ -208,15 +194,19 @@ RSpec.describe Projects::Alerting::NotifyService do environment_id: nil ) end + + it 'creates a system note corresponding to alert creation' do + expect { subject }.to change(Note, :count).by(1) + expect(Note.last.note).to include('Generic Alert Endpoint') + end end end context 'with overlong payload' do - let(:payload_raw) do - { - title: 'a' * Gitlab::Utils::DeepSize::DEFAULT_MAX_SIZE, - start_time: starts_at.rfc3339 - } + let(:deep_size_object) { instance_double(Gitlab::Utils::DeepSize, valid?: false) } + + before do + allow(Gitlab::Utils::DeepSize).to receive(:new).and_return(deep_size_object) end it_behaves_like 'does not process incident issues due to error', http_status: :bad_request @@ -230,17 +220,6 @@ RSpec.describe Projects::Alerting::NotifyService do it_behaves_like 'processes incident issues' - context 'with an invalid payload' do - before do - allow(Gitlab::Alerting::NotificationPayloadParser) - .to receive(:call) - .and_raise(Gitlab::Alerting::NotificationPayloadParser::BadPayloadError) - end - - it_behaves_like 'does not process incident issues due to error', http_status: :bad_request - it_behaves_like 'does not an create alert management alert' - end - context 'when alert already exists' do let(:fingerprint_sha) { Digest::SHA1.hexdigest(fingerprint) } let!(:alert) { create(:alert_management_alert, project: project, fingerprint: fingerprint_sha) } 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 2c708e75a25..2f2474f2681 100644 --- a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb @@ -245,7 +245,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do end it 'succeeds without a user' do - expect_delete(%w(Bb Ba C)) + expect_delete(%w(Bb Ba C), container_expiration_policy: true) is_expected.to include(status: :success, deleted: %w(Bb Ba C)) end @@ -287,10 +287,10 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do end end - def expect_delete(tags) + def expect_delete(tags, container_expiration_policy: nil) expect(Projects::ContainerRepository::DeleteTagsService) .to receive(:new) - .with(repository.project, user, tags: tags) + .with(repository.project, user, tags: tags, container_expiration_policy: container_expiration_policy) .and_call_original expect_any_instance_of(Projects::ContainerRepository::DeleteTagsService) diff --git a/spec/services/projects/container_repository/delete_tags_service_spec.rb b/spec/services/projects/container_repository/delete_tags_service_spec.rb index 5116427dad2..c3ae26b1f05 100644 --- a/spec/services/projects/container_repository/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/delete_tags_service_spec.rb @@ -85,6 +85,41 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do end end + RSpec.shared_examples 'supporting fast delete' do + context 'when the registry supports fast delete' do + before do + allow(repository.client).to receive(:supports_tag_delete?).and_return(true) + end + + it_behaves_like 'calling the correct delete tags service', ::Projects::ContainerRepository::Gitlab::DeleteTagsService + + it_behaves_like 'handling invalid params' + + context 'with the real service' do + before do + stub_delete_reference_requests(tags) + expect_delete_tag_by_names(tags) + end + + it { is_expected.to include(status: :success) } + + it_behaves_like 'logging a success response' + end + + context 'with a timeout error' do + before do + expect_next_instance_of(::Projects::ContainerRepository::Gitlab::DeleteTagsService) do |delete_service| + expect(delete_service).to receive(:delete_tags).and_raise(::Projects::ContainerRepository::Gitlab::DeleteTagsService::TimeoutError) + end + end + + it { is_expected.to include(status: :error, message: 'timeout while deleting tags') } + + it_behaves_like 'logging an error response', message: 'timeout while deleting tags' + end + end + end + describe '#execute' do let(:tags) { %w[A Ba] } @@ -103,62 +138,7 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do project.add_developer(user) end - context 'when the registry supports fast delete' do - context 'and the feature is enabled' do - before do - allow(repository.client).to receive(:supports_tag_delete?).and_return(true) - end - - it_behaves_like 'calling the correct delete tags service', ::Projects::ContainerRepository::Gitlab::DeleteTagsService - - it_behaves_like 'handling invalid params' - - context 'with the real service' do - before do - stub_delete_reference_requests(tags) - expect_delete_tag_by_names(tags) - end - - it { is_expected.to include(status: :success) } - - it_behaves_like 'logging a success response' - end - - context 'with a timeout error' do - before do - expect_next_instance_of(::Projects::ContainerRepository::Gitlab::DeleteTagsService) do |delete_service| - expect(delete_service).to receive(:delete_tags).and_raise(::Projects::ContainerRepository::Gitlab::DeleteTagsService::TimeoutError) - end - end - - it { is_expected.to include(status: :error, message: 'timeout while deleting tags') } - - it_behaves_like 'logging an error response', message: 'timeout while deleting tags' - end - end - - context 'and the feature is disabled' do - before do - stub_feature_flags(container_registry_fast_tag_delete: false) - end - - it_behaves_like 'calling the correct delete tags service', ::Projects::ContainerRepository::ThirdParty::DeleteTagsService - - it_behaves_like 'handling invalid params' - - context 'with the real service' do - before do - stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') - tags.each { |tag| stub_put_manifest_request(tag) } - expect_delete_tag_by_digest('sha256:dummy') - end - - it { is_expected.to include(status: :success) } - - it_behaves_like 'logging a success response' - end - end - end + it_behaves_like 'supporting fast delete' context 'when the registry does not support fast delete' do before do @@ -170,5 +150,19 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do it_behaves_like 'handling invalid params' end end + + context 'without user' do + let_it_be(:user) { nil } + + context 'when not run by a cleanup policy' do + it { is_expected.to include(status: :error) } + end + + context 'when run by a cleanup policy' do + let(:params) { { tags: tags, container_expiration_policy: true } } + + it_behaves_like 'supporting fast delete' + end + end end end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index e1df8700795..b81b3e095cf 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -487,18 +487,7 @@ RSpec.describe Projects::CreateService, '#execute' do describe 'create service for the project' do subject(:project) { create_project(user, opts) } - context 'when there is an active instance-level and an active template integration' do - let!(:template_integration) { create(:prometheus_service, :template, api_url: 'https://prometheus.template.com/') } - let!(:instance_integration) { create(:prometheus_service, :instance, api_url: 'https://prometheus.instance.com/') } - - it 'creates a service from the instance-level integration' do - expect(project.services.count).to eq(1) - expect(project.services.first.api_url).to eq(instance_integration.api_url) - expect(project.services.first.inherit_from_id).to eq(instance_integration.id) - end - end - - context 'when there is an active service template' do + context 'with an active service template' do let!(:template_integration) { create(:prometheus_service, :template, api_url: 'https://prometheus.template.com/') } it 'creates a service from the template' do @@ -506,6 +495,60 @@ RSpec.describe Projects::CreateService, '#execute' do expect(project.services.first.api_url).to eq(template_integration.api_url) expect(project.services.first.inherit_from_id).to be_nil end + + context 'with an active instance-level integration' do + let!(:instance_integration) { create(:prometheus_service, :instance, api_url: 'https://prometheus.instance.com/') } + + it 'creates a service from the instance-level integration' do + expect(project.services.count).to eq(1) + expect(project.services.first.api_url).to eq(instance_integration.api_url) + expect(project.services.first.inherit_from_id).to eq(instance_integration.id) + end + + context 'with an active group-level integration' do + let!(:group_integration) { create(:prometheus_service, group: group, project: nil, api_url: 'https://prometheus.group.com/') } + let!(:group) do + create(:group).tap do |group| + group.add_owner(user) + end + end + + let(:opts) do + { + name: 'GitLab', + namespace_id: group.id + } + end + + it 'creates a service from the group-level integration' do + expect(project.services.count).to eq(1) + expect(project.services.first.api_url).to eq(group_integration.api_url) + expect(project.services.first.inherit_from_id).to eq(group_integration.id) + end + + context 'with an active subgroup' do + let!(:subgroup_integration) { create(:prometheus_service, group: subgroup, project: nil, api_url: 'https://prometheus.subgroup.com/') } + let!(:subgroup) do + create(:group, parent: group).tap do |subgroup| + subgroup.add_owner(user) + end + end + + let(:opts) do + { + name: 'GitLab', + namespace_id: subgroup.id + } + end + + it 'creates a service from the subgroup-level integration' do + expect(project.services.count).to eq(1) + expect(project.services.first.api_url).to eq(subgroup_integration.api_url) + expect(project.services.first.inherit_from_id).to eq(subgroup_integration.id) + end + end + end + end end context 'when there is an invalid integration' do @@ -739,4 +782,100 @@ RSpec.describe Projects::CreateService, '#execute' do def create_project(user, opts) Projects::CreateService.new(user, opts).execute end + + context 'shared Runners config' do + using RSpec::Parameterized::TableSyntax + + let_it_be(:user) { create :user } + + context 'when parent group is present' do + let_it_be(:group) do + create(:group) do |group| + group.add_owner(user) + end + end + + before do + allow_next_found_instance_of(Group) do |group| + allow(group).to receive(:shared_runners_setting).and_return(shared_runners_setting) + end + + user.refresh_authorized_projects # Ensure cache is warm + end + + context 'default value based on parent group setting' do + where(:shared_runners_setting, :desired_config_for_new_project, :expected_result_for_project) do + 'enabled' | nil | true + 'disabled_with_override' | nil | false + 'disabled_and_unoverridable' | nil | false + end + + with_them do + it 'creates project following the parent config' do + params = opts.merge(namespace_id: group.id) + params = params.merge(shared_runners_enabled: desired_config_for_new_project) unless desired_config_for_new_project.nil? + project = create_project(user, params) + + expect(project).to be_valid + expect(project.shared_runners_enabled).to eq(expected_result_for_project) + end + end + end + + context 'parent group is present and allows desired config' do + where(:shared_runners_setting, :desired_config_for_new_project, :expected_result_for_project) do + 'enabled' | true | true + 'enabled' | false | false + 'disabled_with_override' | false | false + 'disabled_with_override' | true | true + 'disabled_and_unoverridable' | false | false + end + + with_them do + it 'creates project following the parent config' do + params = opts.merge(namespace_id: group.id, shared_runners_enabled: desired_config_for_new_project) + project = create_project(user, params) + + expect(project).to be_valid + expect(project.shared_runners_enabled).to eq(expected_result_for_project) + end + end + end + + context 'parent group is present and disallows desired config' do + where(:shared_runners_setting, :desired_config_for_new_project) do + 'disabled_and_unoverridable' | true + end + + with_them do + it 'does not create project' do + params = opts.merge(namespace_id: group.id, shared_runners_enabled: desired_config_for_new_project) + project = create_project(user, params) + + expect(project.persisted?).to eq(false) + expect(project).to be_invalid + expect(project.errors[:shared_runners_enabled]).to include('cannot be enabled because parent group does not allow it') + end + end + end + end + + context 'parent group is not present' do + where(:desired_config, :expected_result) do + true | true + false | false + nil | true + end + + with_them do + it 'follows desired config' do + opts[:shared_runners_enabled] = desired_config unless desired_config.nil? + project = create_project(user, opts) + + expect(project).to be_valid + expect(project.shared_runners_enabled).to eq(expected_result) + end + end + end + end end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index a0e83fb4a21..3ae96d7a5ab 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -314,6 +314,37 @@ RSpec.describe Projects::TransferService do end end + context 'shared Runners group level configurations' do + using RSpec::Parameterized::TableSyntax + + where(:project_shared_runners_enabled, :shared_runners_setting, :expected_shared_runners_enabled) do + true | 'disabled_and_unoverridable' | false + false | 'disabled_and_unoverridable' | false + true | 'disabled_with_override' | true + false | 'disabled_with_override' | false + true | 'enabled' | true + false | 'enabled' | false + end + + with_them do + let(:project) { create(:project, :public, :repository, namespace: user.namespace, shared_runners_enabled: project_shared_runners_enabled) } + let(:group) { create(:group) } + + before do + group.add_owner(user) + expect_next_found_instance_of(Group) do |group| + expect(group).to receive(:shared_runners_setting).and_return(shared_runners_setting) + end + + execute_transfer + end + + it 'updates shared runners based on the parent group' do + expect(project.shared_runners_enabled).to eq(expected_shared_runners_enabled) + end + end + end + context 'missing group labels applied to issues or merge requests' do it 'delegates transfer to Labels::TransferService' do group.add_owner(user) diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index bfb3cbb0131..b6fd72cd4f5 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -16,8 +16,6 @@ RSpec.describe Projects::UpdatePagesService do subject { described_class.new(project, build) } before do - stub_feature_flags(safezip_use_rubyzip: true) - project.remove_pages end @@ -104,10 +102,6 @@ RSpec.describe Projects::UpdatePagesService do let(:file) { fixture_file_upload("spec/fixtures/pages_non_writeable.zip") } context 'when using RubyZip' do - before do - stub_feature_flags(safezip_use_rubyzip: true) - end - it 'succeeds to extract' do expect(execute).to eq(:success) expect(project.pages_metadatum).to be_deployed diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb index 1de04888e0a..f0e76bb81b3 100644 --- a/spec/services/projects/update_remote_mirror_service_spec.rb +++ b/spec/services/projects/update_remote_mirror_service_spec.rb @@ -68,25 +68,12 @@ RSpec.describe Projects::UpdateRemoteMirrorService do end context "when given URLs containing escaped elements" do - using RSpec::Parameterized::TableSyntax + it_behaves_like "URLs containing escaped elements return expected status" do + let(:result) { execute! } - where(:url, :result_status) do - "https://user:0a%23@test.example.com/project.git" | :success - "https://git.example.com:1%2F%2F@source.developers.google.com/project.git" | :success - CGI.escape("git://localhost:1234/some-path?some-query=some-val\#@example.com/") | :error - CGI.escape(CGI.escape("https://user:0a%23@test.example.com/project.git")) | :error - end - - with_them do before do allow(remote_mirror).to receive(:url).and_return(url) end - - it "returns expected status" do - result = execute! - - expect(result[:status]).to eq(result_status) - end end end diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 7832d727220..3375d9762c8 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -151,6 +151,32 @@ RSpec.describe Projects::UpdateService do expect(project.reload).to be_internal end end + + context 'when updating shared runners' do + context 'can enable shared runners' do + let(:group) { create(:group, shared_runners_enabled: true) } + let(:project) { create(:project, namespace: group, shared_runners_enabled: false) } + + it 'enables shared runners' do + result = update_project(project, user, shared_runners_enabled: true) + + expect(result).to eq({ status: :success }) + expect(project.reload.shared_runners_enabled).to be_truthy + end + end + + context 'cannot enable shared runners' do + let(:group) { create(:group, :shared_runners_disabled) } + let(:project) { create(:project, namespace: group, shared_runners_enabled: false) } + + it 'does not enable shared runners' do + result = update_project(project, user, shared_runners_enabled: true) + + expect(result).to eq({ status: :error, message: 'Shared runners enabled cannot be enabled because parent group does not allow it' }) + expect(project.reload.shared_runners_enabled).to be_falsey + end + end + end end describe 'when updating project that has forks' do diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index b970a48051f..8f0c60d5d65 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -3,23 +3,27 @@ require 'spec_helper' RSpec.describe QuickActions::InterpretService do - let(:project) { create(:project, :public) } - let(:developer) { create(:user) } - let(:developer2) { create(:user) } - let(:issue) { create(:issue, project: project) } + let_it_be(:public_project) { create(:project, :public) } + let_it_be(:repository_project) { create(:project, :repository) } + let_it_be(:project) { public_project } + let_it_be(:developer) { create(:user) } + let_it_be(:developer2) { create(:user) } + let_it_be_with_reload(:issue) { create(:issue, project: project) } let(:milestone) { create(:milestone, project: project, title: '9.10') } let(:commit) { create(:commit, project: project) } - let(:inprogress) { create(:label, project: project, title: 'In Progress') } - let(:helmchart) { create(:label, project: project, title: 'Helm Chart Registry') } - let(:bug) { create(:label, project: project, title: 'Bug') } - let(:note) { build(:note, commit_id: merge_request.diff_head_sha) } + let_it_be(:inprogress) { create(:label, project: project, title: 'In Progress') } + let_it_be(:helmchart) { create(:label, project: project, title: 'Helm Chart Registry') } + let_it_be(:bug) { create(:label, project: project, title: 'Bug') } let(:service) { described_class.new(project, developer) } + before_all do + public_project.add_developer(developer) + repository_project.add_developer(developer) + end + before do stub_licensed_features(multiple_issue_assignees: false, multiple_merge_request_assignees: false) - - project.add_developer(developer) end describe '#execute' do @@ -146,7 +150,6 @@ RSpec.describe QuickActions::InterpretService do shared_examples 'multiword label name starting without ~' do it 'fetches label ids and populates add_label_ids if content contains /label' do - helmchart # populate the label _, updates = service.execute(content, issuable) expect(updates).to eq(add_label_ids: [helmchart.id]) @@ -155,7 +158,6 @@ RSpec.describe QuickActions::InterpretService do shared_examples 'label name is included in the middle of another label name' do it 'ignores the sublabel when the content contains the includer label name' do - helmchart # populate the label create(:label, project: project, title: 'Chart') _, updates = service.execute(content, issuable) @@ -493,7 +495,7 @@ RSpec.describe QuickActions::InterpretService do end shared_examples 'merge immediately command' do - let(:project) { create(:project, :repository) } + let(:project) { repository_project } it 'runs merge command if content contains /merge' do _, updates, _ = service.execute(content, issuable) @@ -509,7 +511,7 @@ RSpec.describe QuickActions::InterpretService do end shared_examples 'merge automatically command' do - let(:project) { create(:project, :repository) } + let(:project) { repository_project } it 'runs merge command if content contains /merge and returns merge message' do _, updates, message = service.execute(content, issuable) @@ -600,7 +602,7 @@ RSpec.describe QuickActions::InterpretService do context 'when issuable is already confidential' do before do - issuable.update(confidential: true) + issuable.update!(confidential: true) end it 'does not return the success message' do @@ -722,7 +724,7 @@ RSpec.describe QuickActions::InterpretService do end context 'when sha is missing' do - let(:project) { create(:project, :repository) } + let(:project) { repository_project } let(:service) { described_class.new(project, developer, {}) } it 'precheck passes and returns merge command' do @@ -844,7 +846,7 @@ RSpec.describe QuickActions::InterpretService do end it 'returns the unassign message for all the assignee if content contains /unassign' do - issue.update(assignee_ids: [developer.id, developer2.id]) + issue.update!(assignee_ids: [developer.id, developer2.id]) _, _, message = service.execute(content, issue) expect(message).to eq("Removed assignees #{developer.to_reference} and #{developer2.to_reference}.") @@ -860,7 +862,7 @@ RSpec.describe QuickActions::InterpretService do end it 'returns the unassign message for all the assignee if content contains /unassign' do - merge_request.update(assignee_ids: [developer.id, developer2.id]) + merge_request.update!(assignee_ids: [developer.id, developer2.id]) _, _, message = service.execute(content, merge_request) expect(message).to eq("Removed assignees #{developer.to_reference} and #{developer2.to_reference}.") @@ -879,10 +881,14 @@ RSpec.describe QuickActions::InterpretService do end context 'only group milestones available' do - let(:ancestor_group) { create(:group) } - let(:group) { create(:group, parent: ancestor_group) } - let(:project) { create(:project, :public, namespace: group) } - let(:milestone) { create(:milestone, group: ancestor_group, title: '10.0') } + let_it_be(:ancestor_group) { create(:group) } + let_it_be(:group) { create(:group, parent: ancestor_group) } + let_it_be(:project) { create(:project, :public, namespace: group) } + let_it_be(:milestone) { create(:milestone, group: ancestor_group, title: '10.0') } + + before_all do + project.add_developer(developer) + end it_behaves_like 'milestone command' do let(:content) { "/milestone %#{milestone.title}" } @@ -1457,14 +1463,14 @@ RSpec.describe QuickActions::InterpretService do end context '/board_move command' do - let(:todo) { create(:label, project: project, title: 'To Do') } - let(:inreview) { create(:label, project: project, title: 'In Review') } + let_it_be(:todo) { create(:label, project: project, title: 'To Do') } + let_it_be(:inreview) { create(:label, project: project, title: 'In Review') } let(:content) { %{/board_move ~"#{inreview.title}"} } - let!(:board) { create(:board, project: project) } - let!(:todo_list) { create(:list, board: board, label: todo) } - let!(:inreview_list) { create(:list, board: board, label: inreview) } - let!(:inprogress_list) { create(:list, board: board, label: inprogress) } + let_it_be(:board) { create(:board, project: project) } + let_it_be(:todo_list) { create(:list, board: board, label: todo) } + let_it_be(:inreview_list) { create(:list, board: board, label: inreview) } + let_it_be(:inprogress_list) { create(:list, board: board, label: inprogress) } it 'populates remove_label_ids for all current board columns' do issue.update!(label_ids: [todo.id, inprogress.id]) @@ -1599,6 +1605,10 @@ RSpec.describe QuickActions::InterpretService do context "when logged user cannot create_merge_requests in the project" do let(:project) { create(:project, :archived) } + before do + project.add_developer(developer) + end + it_behaves_like 'empty command' end @@ -1844,8 +1854,7 @@ RSpec.describe QuickActions::InterpretService do end describe 'relabel command' do - let(:content) { '/relabel Bug' } - let!(:bug) { create(:label, project: project, title: 'Bug') } + let(:content) { "/relabel #{bug.title}" } let(:feature) { create(:label, project: project, title: 'Feature') } it 'includes label name' do @@ -1938,8 +1947,7 @@ RSpec.describe QuickActions::InterpretService do end describe 'board move command' do - let(:content) { '/board_move ~bug' } - let!(:bug) { create(:label, project: project, title: 'bug') } + let(:content) { "/board_move ~#{bug.title}" } let!(:board) { create(:board, project: project) } it 'includes the label name' do diff --git a/spec/services/resource_access_tokens/create_service_spec.rb b/spec/services/resource_access_tokens/create_service_spec.rb index 7dbd55a6909..4a77e5fed9b 100644 --- a/spec/services/resource_access_tokens/create_service_spec.rb +++ b/spec/services/resource_access_tokens/create_service_spec.rb @@ -24,16 +24,6 @@ RSpec.describe ResourceAccessTokens::CreateService do end end - shared_examples 'fails when flag is disabled' do - before do - stub_feature_flags(resource_access_token: false) - end - - it 'returns nil' do - expect(subject).to be nil - end - end - shared_examples 'fails on gitlab.com' do before do allow(Gitlab).to receive(:com?) { true } @@ -53,6 +43,7 @@ RSpec.describe ResourceAccessTokens::CreateService do access_token = response.payload[:access_token] expect(access_token.user.reload.user_type).to eq("#{resource_type}_bot") + expect(access_token.user.created_by_id).to eq(user.id) end context 'email confirmation status' do @@ -180,7 +171,6 @@ RSpec.describe ResourceAccessTokens::CreateService do let_it_be(:resource) { project } it_behaves_like 'fails when user does not have the permission to create a Resource Bot' - it_behaves_like 'fails when flag is disabled' it_behaves_like 'fails on gitlab.com' context 'user with valid permission' do diff --git a/spec/services/search_service_spec.rb b/spec/services/search_service_spec.rb index f6bb7acee57..fc613a6224a 100644 --- a/spec/services/search_service_spec.rb +++ b/spec/services/search_service_spec.rb @@ -444,7 +444,7 @@ RSpec.describe SearchService do context 'with :with_api_entity_associations' do let(:unredacted_results) { ar_relation(MergeRequest.with_api_entity_associations, readable, unreadable) } - it_behaves_like "redaction limits N+1 queries", limit: 7 + it_behaves_like "redaction limits N+1 queries", limit: 8 end end @@ -481,7 +481,7 @@ RSpec.describe SearchService do end context 'with :with_api_entity_associations' do - it_behaves_like "redaction limits N+1 queries", limit: 12 + it_behaves_like "redaction limits N+1 queries", limit: 13 end end @@ -496,7 +496,7 @@ RSpec.describe SearchService do end context 'with :with_api_entity_associations' do - it_behaves_like "redaction limits N+1 queries", limit: 3 + it_behaves_like "redaction limits N+1 queries", limit: 4 end end diff --git a/spec/services/snippets/update_service_spec.rb b/spec/services/snippets/update_service_spec.rb index 641fc56294a..406ece30bd7 100644 --- a/spec/services/snippets/update_service_spec.rb +++ b/spec/services/snippets/update_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Snippets::UpdateService do - describe '#execute' do + describe '#execute', :aggregate_failures do let_it_be(:user) { create(:user) } let_it_be(:admin) { create :user, admin: true } let(:visibility_level) { Gitlab::VisibilityLevel::PRIVATE } @@ -97,40 +97,81 @@ RSpec.describe Snippets::UpdateService do end shared_examples 'creates repository and creates file' do - it 'creates repository' do - expect(snippet.repository).not_to exist + context 'when file_name and content params are used' do + it 'creates repository' do + expect(snippet.repository).not_to exist - subject + subject - expect(snippet.repository).to exist - end + expect(snippet.repository).to exist + end - it 'commits the files to the repository' do - subject + it 'commits the files to the repository' do + subject - expect(snippet.blobs.count).to eq 1 + expect(snippet.blobs.count).to eq 1 - blob = snippet.repository.blob_at('master', options[:file_name]) + blob = snippet.repository.blob_at('master', options[:file_name]) - expect(blob.data).to eq options[:content] + expect(blob.data).to eq options[:content] + end + + context 'when the repository creation fails' do + before do + allow(snippet).to receive(:repository_exists?).and_return(false) + end + + it 'raise an error' do + expect(subject).to be_error + expect(subject.payload[:snippet].errors[:repository].to_sentence).to eq 'Error updating the snippet - Repository could not be created' + end + + it 'does not try to commit file' do + expect(service).not_to receive(:create_commit) + + subject + end + end end - context 'when the repository creation fails' do - before do - allow(snippet).to receive(:repository_exists?).and_return(false) + context 'when snippet_actions param is used' do + let(:file_path) { 'CHANGELOG' } + let(:created_file_path) { 'New file'} + let(:content) { 'foobar' } + let(:snippet_actions) { [{ action: :move, previous_path: snippet.file_name, file_path: file_path }, { action: :create, file_path: created_file_path, content: content }] } + let(:base_opts) do + { + snippet_actions: snippet_actions + } end - it 'raise an error' do - response = subject + it 'performs operation without raising errors' do + db_content = snippet.content - expect(response).to be_error - expect(response.payload[:snippet].errors[:repository].to_sentence).to eq 'Error updating the snippet - Repository could not be created' + expect(subject).to be_success + + new_blob = snippet.repository.blob_at('master', file_path) + created_file = snippet.repository.blob_at('master', created_file_path) + + expect(new_blob.data).to eq db_content + expect(created_file.data).to eq content end - it 'does not try to commit file' do - expect(service).not_to receive(:create_commit) + context 'when the repository is not created' do + it 'keeps snippet database data' do + old_file_name = snippet.file_name + old_file_content = snippet.content - subject + expect_next_instance_of(described_class) do |instance| + expect(instance).to receive(:create_repository_for).and_raise(StandardError) + end + + snippet = subject.payload[:snippet] + + expect(subject).to be_error + expect(snippet.file_name).to eq(old_file_name) + expect(snippet.content).to eq(old_file_content) + end end end end @@ -366,10 +407,9 @@ RSpec.describe Snippets::UpdateService do let(:snippet_actions) { [{ action: 'invalid_action' }] } it 'raises a validation error' do - response = subject - snippet = response.payload[:snippet] + snippet = subject.payload[:snippet] - expect(response).to be_error + expect(subject).to be_error expect(snippet.errors.full_messages_for(:snippet_actions)).to eq ['Snippet actions have invalid data'] end end @@ -377,13 +417,12 @@ RSpec.describe Snippets::UpdateService do context 'when an error is raised committing the file' do it 'keeps any snippet modifications' do expect_next_instance_of(described_class) do |instance| - expect(instance).to receive(:create_repository_for).and_raise(StandardError) + expect(instance).to receive(:create_commit).and_raise(StandardError) end - response = subject - snippet = response.payload[:snippet] + snippet = subject.payload[:snippet] - expect(response).to be_error + expect(subject).to be_error expect(snippet.title).to eq(new_title) expect(snippet.file_name).to eq(file_path) expect(snippet.content).to eq(content) diff --git a/spec/services/static_site_editor/config_service_spec.rb b/spec/services/static_site_editor/config_service_spec.rb index 5fff4e0af53..fed373828a1 100644 --- a/spec/services/static_site_editor/config_service_spec.rb +++ b/spec/services/static_site_editor/config_service_spec.rb @@ -7,8 +7,8 @@ RSpec.describe StaticSiteEditor::ConfigService do let_it_be(:user) { create(:user) } # params - let(:ref) { double(:ref) } - let(:path) { double(:path) } + let(:ref) { 'master' } + let(:path) { 'README.md' } let(:return_url) { double(:return_url) } # stub data @@ -42,22 +42,84 @@ RSpec.describe StaticSiteEditor::ConfigService do allow_next_instance_of(Gitlab::StaticSiteEditor::Config::GeneratedConfig) do |config| allow(config).to receive(:data) { generated_data } end + end + + context 'when reading file from repo fails with an unexpected error' do + let(:unexpected_error) { RuntimeError.new('some unexpected error') } - allow_next_instance_of(Gitlab::StaticSiteEditor::Config::FileConfig) do |config| - allow(config).to receive(:data) { file_data } + before do + allow(project.repository).to receive(:blob_data_at).and_raise(unexpected_error) + end + + it 'returns an error response' do + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_exception).with(unexpected_error).and_call_original + expect { execute }.to raise_error(unexpected_error) end end - it 'returns merged generated data and config file data' do - expect(execute).to be_success - expect(execute.payload).to eq(generated: true, file: true) + context 'when file is missing' do + before do + allow(project.repository).to receive(:blob_data_at).and_raise(GRPC::NotFound) + expect_next_instance_of(Gitlab::StaticSiteEditor::Config::FileConfig, '{}') do |config| + allow(config).to receive(:valid?) { true } + allow(config).to receive(:to_hash_with_defaults) { file_data } + end + end + + it 'returns default config' do + expect(execute).to be_success + expect(execute.payload).to eq(generated: true, file: true) + end end - it 'returns an error if any keys would be overwritten by the merge' do - generated_data[:duplicate_key] = true - file_data[:duplicate_key] = true - expect(execute).to be_error - expect(execute.message).to match(/duplicate key.*duplicate_key.*found/i) + context 'when file is present' do + before do + allow(project.repository).to receive(:blob_data_at).with(ref, anything) do + config_content + end + end + + context 'and configuration is not valid' do + let(:config_content) { 'invalid content' } + + before do + expect_next_instance_of(Gitlab::StaticSiteEditor::Config::FileConfig, config_content) do |config| + error = 'error' + allow(config).to receive_message_chain('errors.first') { error } + allow(config).to receive(:valid?) { false } + end + end + + it 'returns an error' do + expect(execute).to be_error + expect(execute.message).to eq('Invalid configuration format') + end + end + + context 'and configuration is valid' do + # NOTE: This has to be a valid config, even though it is mocked, because + # `expect_next_instance_of` executes the constructor logic. + let(:config_content) { 'static_site_generator: middleman' } + + before do + expect_next_instance_of(Gitlab::StaticSiteEditor::Config::FileConfig, config_content) do |config| + allow(config).to receive(:valid?) { true } + allow(config).to receive(:to_hash_with_defaults) { file_data } + end + end + + it 'returns merged generated data and config file data' do + expect(execute).to be_success + expect(execute.payload).to eq(generated: true, file: true) + end + + it 'returns an error if any keys would be overwritten by the merge' do + generated_data[:duplicate_key] = true + file_data[:duplicate_key] = true + expect(execute).to be_error + expect(execute.message).to match(/duplicate key.*duplicate_key.*found/i) + end + end end end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 47b8621b5c9..42e48b9ad81 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -64,6 +64,18 @@ RSpec.describe SystemNoteService do end end + describe '.change_issuable_reviewers' do + let(:reviewers) { [double, double] } + + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_issuable_reviewers).with(reviewers) + end + + described_class.change_issuable_reviewers(noteable, project, author, reviewers) + end + end + describe '.close_after_error_tracking_resolve' do it 'calls IssuableService' do expect_next_instance_of(::SystemNotes::IssuablesService) do |service| @@ -741,4 +753,16 @@ RSpec.describe SystemNoteService do described_class.create_new_alert(alert, monitoring_tool) end end + + describe '.change_incident_severity' do + let(:incident) { build(:incident) } + + it 'calls IncidentService' do + expect_next_instance_of(SystemNotes::IncidentService) do |service| + expect(service).to receive(:change_incident_severity) + end + + described_class.change_incident_severity(incident, author) + end + end end diff --git a/spec/services/system_notes/incident_service_spec.rb b/spec/services/system_notes/incident_service_spec.rb new file mode 100644 index 00000000000..ab9b9eb2bd4 --- /dev/null +++ b/spec/services/system_notes/incident_service_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::SystemNotes::IncidentService do + let_it_be(:author) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:noteable) { create(:incident, project: project) } + let_it_be(:issuable_severity) { create(:issuable_severity, issue: noteable, severity: :medium) } + + describe '#change_incident_severity' do + subject(:change_severity) { described_class.new(noteable: noteable, project: project, author: author).change_incident_severity } + + before do + allow(Gitlab::AppLogger).to receive(:error).and_call_original + end + + it_behaves_like 'a system note' do + let(:action) { 'severity' } + end + + IssuableSeverity.severities.keys.each do |severity| + context "with #{severity} severity" do + before do + issuable_severity.update!(severity: severity) + end + + it 'has the appropriate message' do + severity_label = IssuableSeverity::SEVERITY_LABELS.fetch(severity.to_sym) + + expect(change_severity.note).to eq("changed the severity to **#{severity_label}**") + end + end + end + + context 'when severity is invalid' do + let(:invalid_severity) { 'invalid-severity' } + + before do + allow(noteable).to receive(:severity).and_return(invalid_severity) + end + + it 'does not create system note' do + expect { change_severity }.not_to change { noteable.notes.count } + end + + it 'writes error to logs' do + change_severity + + expect(Gitlab::AppLogger).to have_received(:error).with( + message: 'Cannot create a system note for severity change', + noteable_class: noteable.class.to_s, + noteable_id: noteable.id, + severity: invalid_severity + ) + end + end + end +end diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index fec2a711dc2..e78b00fb67a 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -128,49 +128,76 @@ RSpec.describe ::SystemNotes::IssuablesService do end end - describe '#change_status' do - subject { service.change_status(status, source) } + describe '#change_issuable_reviewers' do + subject { service.change_issuable_reviewers([reviewer]) } - context 'when resource state event tracking is enabled' do - let(:status) { 'reopened' } - let(:source) { nil } + let_it_be(:noteable) { create(:merge_request, :simple, source_project: project) } + let_it_be(:reviewer) { create(:user) } + let_it_be(:reviewer1) { create(:user) } + let_it_be(:reviewer2) { create(:user) } + let_it_be(:reviewer3) { create(:user) } - it 'does not change note count' do - expect { subject }.not_to change { Note.count } - end + it_behaves_like 'a system note' do + let(:action) { 'reviewer' } end - context 'with status reopened' do - before do - stub_feature_flags(track_resource_state_change_events: false) - end + def build_note(old_reviewers, new_reviewers) + noteable.reviewers = new_reviewers + service.change_issuable_reviewers(old_reviewers).note + end - let(:status) { 'reopened' } - let(:source) { nil } + it 'builds a correct phrase when a reviewer is added to a non-assigned merge request' do + expect(build_note([], [reviewer1])).to eq "requested review from @#{reviewer1.username}" + end - it_behaves_like 'a note with overridable created_at' + it 'builds a correct phrase when reviewer is removed' do + expect(build_note([reviewer], [])).to eq "removed review request for @#{reviewer.username}" + end - it_behaves_like 'a system note' do - let(:action) { 'opened' } - end + it 'builds a correct phrase when reviewers changed' do + expect(build_note([reviewer1], [reviewer2])).to( + eq("requested review from @#{reviewer2.username} and removed review request for @#{reviewer1.username}") + ) end - context 'with a source' do - before do - stub_feature_flags(track_resource_state_change_events: false) - end + it 'builds a correct phrase when three reviewers removed and one added' do + expect(build_note([reviewer, reviewer1, reviewer2], [reviewer3])).to( + eq("requested review from @#{reviewer3.username} and removed review request for @#{reviewer.username}, @#{reviewer1.username}, and @#{reviewer2.username}") + ) + end - let(:status) { 'opened' } - let(:source) { double('commit', gfm_reference: 'commit 123456') } + it 'builds a correct phrase when one reviewer is changed from a set' do + expect(build_note([reviewer, reviewer1], [reviewer, reviewer2])).to( + eq("requested review from @#{reviewer2.username} and removed review request for @#{reviewer1.username}") + ) + end - it_behaves_like 'a note with overridable created_at' + it 'builds a correct phrase when one reviewer removed from a set' do + expect(build_note([reviewer, reviewer1, reviewer2], [reviewer, reviewer1])).to( + eq( "removed review request for @#{reviewer2.username}") + ) + end - it 'sets the note text' do - expect(subject.note).to eq "#{status} via commit 123456" + it 'builds a correct phrase when the locale is different' do + Gitlab::I18n.with_locale('pt-BR') do + expect(build_note([reviewer, reviewer1, reviewer2], [reviewer3])).to( + eq("requested review from @#{reviewer3.username} and removed review request for @#{reviewer.username}, @#{reviewer1.username}, and @#{reviewer2.username}") + ) end end end + describe '#change_status' do + subject { service.change_status(status, source) } + + let(:status) { 'reopened' } + let(:source) { nil } + + it 'creates a resource state event' do + expect { subject }.to change { ResourceStateEvent.count }.by(1) + end + end + describe '#change_title' do let(:noteable) { create(:issue, project: project, title: 'Lorem ipsum') } @@ -636,67 +663,26 @@ RSpec.describe ::SystemNotes::IssuablesService do describe '#close_after_error_tracking_resolve' do subject { service.close_after_error_tracking_resolve } - context 'when state tracking is enabled' do - before do - stub_feature_flags(track_resource_state_change_events: true) - end - - it 'creates the expected state event' do - subject - - event = ResourceStateEvent.last - - expect(event.close_after_error_tracking_resolve).to eq(true) - expect(event.state).to eq('closed') - end - end + it 'creates the expected state event' do + subject - context 'when state tracking is disabled' do - before do - stub_feature_flags(track_resource_state_change_events: false) - end + event = ResourceStateEvent.last - it_behaves_like 'a system note' do - let(:action) { 'closed' } - end - - it 'creates the expected system note' do - expect(subject.note) - .to eq('resolved the corresponding error and closed the issue.') - end + expect(event.close_after_error_tracking_resolve).to eq(true) + expect(event.state).to eq('closed') end end describe '#auto_resolve_prometheus_alert' do subject { service.auto_resolve_prometheus_alert } - context 'when state tracking is enabled' do - before do - stub_feature_flags(track_resource_state_change_events: true) - end + it 'creates the expected state event' do + subject - it 'creates the expected state event' do - subject + event = ResourceStateEvent.last - event = ResourceStateEvent.last - - expect(event.close_auto_resolve_prometheus_alert).to eq(true) - expect(event.state).to eq('closed') - end - end - - context 'when state tracking is disabled' do - before do - stub_feature_flags(track_resource_state_change_events: false) - end - - it_behaves_like 'a system note' do - let(:action) { 'closed' } - end - - it 'creates the expected system note' do - expect(subject.note).to eq('automatically closed this issue because the alert resolved.') - end + expect(event.close_auto_resolve_prometheus_alert).to eq(true) + expect(event.state).to eq('closed') end end end diff --git a/spec/services/users/build_service_spec.rb b/spec/services/users/build_service_spec.rb index c14fdb35bfa..b445b5b81b1 100644 --- a/spec/services/users/build_service_spec.rb +++ b/spec/services/users/build_service_spec.rb @@ -16,6 +16,10 @@ RSpec.describe Users::BuildService do expect(service.execute).to be_valid end + it 'sets the created_by_id' do + expect(service.execute.created_by_id).to eq(admin_user.id) + end + context 'calls the UpdateCanonicalEmailService' do specify do expect(Users::UpdateCanonicalEmailService).to receive(:new).and_call_original @@ -128,6 +132,16 @@ RSpec.describe Users::BuildService do it 'raises AccessDeniedError exception' do expect { service.execute }.to raise_error Gitlab::Access::AccessDeniedError end + + context 'when authorization is skipped' do + subject(:built_user) { service.execute(skip_authorization: true) } + + it { is_expected.to be_valid } + + it 'sets the created_by_id' do + expect(built_user.created_by_id).to eq(user.id) + end + end end context 'with nil user' do |