diff options
Diffstat (limited to 'spec/services')
125 files changed, 4601 insertions, 1613 deletions
diff --git a/spec/services/admin/propagate_integration_service_spec.rb b/spec/services/admin/propagate_integration_service_spec.rb index 49d974b7154..5df4d9db8b1 100644 --- a/spec/services/admin/propagate_integration_service_spec.rb +++ b/spec/services/admin/propagate_integration_service_spec.rb @@ -10,129 +10,73 @@ 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!(:instance_integration) do - JiraService.create!( - instance: true, - active: true, - push_events: true, - url: 'http://update-jira.instance.com', - username: 'user', - password: 'secret' - ) - end + let(:group) { create(:group) } - let!(:inherited_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' - ) + let_it_be(:project) { create(:project) } + let_it_be(:instance_integration) { create(:jira_service, :instance) } + let_it_be(:not_inherited_integration) { create(:jira_service, project: project) } + let_it_be(:inherited_integration) do + create(:jira_service, project: create(:project), inherit_from_id: instance_integration.id) end - - let!(:not_inherited_integration) do - JiraService.create!( - project: create(:project), - inherit_from_id: nil, - instance: false, - active: true, - push_events: false, - url: 'http://jira.instance.com', - username: 'user', - password: 'secret' - ) + let_it_be(:different_type_inherited_integration) do + create(:redmine_service, project: project, inherit_from_id: instance_integration.id) end - let!(:different_type_inherited_integration) do - BambooService.create!( - project: create(:project), - inherit_from_id: instance_integration.id, - instance: false, - active: true, - push_events: false, - bamboo_url: 'http://gitlab.com', - username: 'mic', - password: 'password', - build_key: 'build' - ) - end + context 'with inherited integration' do + let(:integration) { inherited_integration } - shared_examples 'inherits settings from integration' do - it 'updates the inherited integrations' do - described_class.propagate(instance_integration) + it 'calls to PropagateIntegrationProjectWorker' do + expect(PropagateIntegrationInheritWorker).to receive(:perform_async) + .with(instance_integration.id, inherited_integration.id, inherited_integration.id) - 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)) + described_class.propagate(instance_integration) end + end - context 'integration with data fields' do - let(:excluded_attributes) { %w[id service_id created_at updated_at] } + context 'with a project without integration' do + let(:another_project) { create(:project) } - it 'updates the data fields from inherited integrations' do - described_class.propagate(instance_integration) + it 'calls to PropagateIntegrationProjectWorker' do + expect(PropagateIntegrationProjectWorker).to receive(:perform_async) + .with(instance_integration.id, another_project.id, another_project.id) - expect(integration.reload.data_fields.attributes.except(*excluded_attributes)) - .to eq(instance_integration.data_fields.attributes.except(*excluded_attributes)) - end + described_class.propagate(instance_integration) 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) + context 'with a group without integration' do + it 'calls to PropagateIntegrationProjectWorker' do + expect(PropagateIntegrationGroupWorker).to receive(:perform_async) + .with(instance_integration.id, group.id, group.id) - expect(integration.reload.attributes.except(*excluded_attributes)) - .not_to eq(instance_integration.attributes.except(*excluded_attributes)) + described_class.propagate(instance_integration) 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 + context 'for a group-level integration' do + let(:group_integration) { create(:jira_service, group: group, project: nil) } - 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, group: group) } - 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(group_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(group_integration) + end 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(:subgroup) { create(:group, parent: group) } - expect(project.reload.has_external_issue_tracker).to eq(true) - end - - 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) + it 'calls to PropagateIntegrationGroupWorker' do + expect(PropagateIntegrationGroupWorker).to receive(:perform_async) + .with(group_integration.id, subgroup.id, subgroup.id) - expect(project.reload.has_external_wiki).to eq(true) + described_class.propagate(group_integration) + end + 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/alerts/update_service_spec.rb b/spec/services/alert_management/alerts/update_service_spec.rb index ee04fc55984..4b47efca9ed 100644 --- a/spec/services/alert_management/alerts/update_service_spec.rb +++ b/spec/services/alert_management/alerts/update_service_spec.rb @@ -160,7 +160,7 @@ RSpec.describe AlertManagement::Alerts::UpdateService do context 'when a status is included' do let(:params) { { status: new_status } } - let(:new_status) { AlertManagement::Alert::STATUSES[:acknowledged] } + let(:new_status) { :acknowledged } it 'successfully changes the status' do expect { response }.to change { alert.acknowledged? }.to(true) @@ -171,13 +171,13 @@ RSpec.describe AlertManagement::Alerts::UpdateService do it_behaves_like 'adds a system note' context 'with unknown status' do - let(:new_status) { -1 } + let(:new_status) { :unknown_status } it_behaves_like 'error response', 'Invalid status' end context 'with resolving status' do - let(:new_status) { AlertManagement::Alert::STATUSES[:resolved] } + let(:new_status) { :resolved } it 'changes the status' do expect { response }.to change { alert.resolved? }.to(true) 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..ae0b8d6d7ac 100644 --- a/spec/services/alert_management/process_prometheus_alert_service_spec.rb +++ b/spec/services/alert_management/process_prometheus_alert_service_spec.rb @@ -117,15 +117,19 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do end context 'when alert cannot be created' do + let(:errors) { double(messages: { hosts: ['hosts array is over 255 chars'] })} + before do - payload['annotations']['title'] = 'description' * 50 + allow(service).to receive(:alert).and_call_original + allow(service).to receive_message_chain(:alert, :save).and_return(false) + allow(service).to receive_message_chain(:alert, :errors).and_return(errors) end it 'writes a warning to the log' do expect(Gitlab::AppLogger).to receive(:warn).with( message: 'Unable to create AlertManagement::Alert', project_id: project.id, - alert_errors: { title: ["is too long (maximum is 200 characters)"] } + alert_errors: { hosts: ['hosts array is over 255 chars'] } ) execute @@ -148,28 +152,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) } + context 'existing issue' do + let!(:alert) { create(:alert_management_alert, :with_issue, project: project, fingerprint: fingerprint) } - it 'closes the issue' do - issue = alert.issue + it 'closes the issue' do + issue = alert.issue - expect { execute } - .to change { issue.reload.state } - .from('opened') - .to('closed') - end + 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 + 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/build_report_result_service_spec.rb b/spec/services/ci/build_report_result_service_spec.rb index 70bcf74ba43..134b662a72a 100644 --- a/spec/services/ci/build_report_result_service_spec.rb +++ b/spec/services/ci/build_report_result_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Ci::BuildReportResultService do - describe "#execute" do + describe '#execute', :clean_gitlab_redis_shared_state do subject(:build_report_result) { described_class.new.execute(build) } context 'when build is finished' do @@ -17,6 +17,25 @@ RSpec.describe Ci::BuildReportResultService do expect(build_report_result.tests_skipped).to eq(0) expect(build_report_result.tests_duration).to eq(0.010284) expect(Ci::BuildReportResult.count).to eq(1) + + unique_test_cases_parsed = Gitlab::UsageDataCounters::HLLRedisCounter.unique_events( + event_names: described_class::EVENT_NAME, + start_date: 2.weeks.ago, + end_date: 2.weeks.from_now + ) + expect(unique_test_cases_parsed).to eq(4) + end + + context 'when feature flag for tracking is disabled' do + before do + stub_feature_flags(track_unique_test_cases_parsed: false) + end + + it 'creates the report but does not track the event' do + expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event) + expect(build_report_result.tests_name).to eq("test") + expect(Ci::BuildReportResult.count).to eq(1) + end end context 'when data has already been persisted' do diff --git a/spec/services/ci/create_downstream_pipeline_service_spec.rb b/spec/services/ci/create_downstream_pipeline_service_spec.rb index a6ea30e4703..0cc380439a7 100644 --- a/spec/services/ci/create_downstream_pipeline_service_spec.rb +++ b/spec/services/ci/create_downstream_pipeline_service_spec.rb @@ -325,20 +325,6 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do expect(bridge.reload).to be_success end - - context 'when FF ci_child_of_child_pipeline is disabled' do - before do - stub_feature_flags(ci_child_of_child_pipeline: false) - end - - it 'does not create a further child pipeline' do - expect { service.execute(bridge) } - .not_to change { Ci::Pipeline.count } - - expect(bridge.reload).to be_failed - expect(bridge.failure_reason).to eq 'bridge_pipeline_is_child_pipeline' - end - end end context 'when upstream pipeline has a parent pipeline, which has a parent pipeline' do diff --git a/spec/services/ci/create_pipeline_service/cache_spec.rb b/spec/services/ci/create_pipeline_service/cache_spec.rb index 614e46f1b1a..1438c2e4aa0 100644 --- a/spec/services/ci/create_pipeline_service/cache_spec.rb +++ b/spec/services/ci/create_pipeline_service/cache_spec.rb @@ -36,7 +36,8 @@ RSpec.describe Ci::CreatePipelineService do 'key' => 'a-key', 'paths' => ['logs/', 'binaries/'], 'policy' => 'pull-push', - 'untracked' => true + 'untracked' => true, + 'when' => 'on_success' } expect(pipeline).to be_persisted @@ -67,7 +68,8 @@ RSpec.describe Ci::CreatePipelineService do expected = { 'key' => /[a-f0-9]{40}/, 'paths' => ['logs/'], - 'policy' => 'pull-push' + 'policy' => 'pull-push', + 'when' => 'on_success' } expect(pipeline).to be_persisted @@ -82,7 +84,8 @@ RSpec.describe Ci::CreatePipelineService do expected = { 'key' => /default/, 'paths' => ['logs/'], - 'policy' => 'pull-push' + 'policy' => 'pull-push', + 'when' => 'on_success' } expect(pipeline).to be_persisted @@ -114,7 +117,8 @@ RSpec.describe Ci::CreatePipelineService do expected = { 'key' => /\$ENV_VAR-[a-f0-9]{40}/, 'paths' => ['logs/'], - 'policy' => 'pull-push' + 'policy' => 'pull-push', + 'when' => 'on_success' } expect(pipeline).to be_persisted @@ -129,7 +133,8 @@ RSpec.describe Ci::CreatePipelineService do expected = { 'key' => /\$ENV_VAR-default/, 'paths' => ['logs/'], - 'policy' => 'pull-push' + 'policy' => 'pull-push', + 'when' => 'on_success' } expect(pipeline).to be_persisted diff --git a/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb b/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb index 016a5dfd18b..fb6cdf55be3 100644 --- a/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb +++ b/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb @@ -22,7 +22,7 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do end shared_examples 'successful creation' do - it 'creates bridge jobs correctly' do + it 'creates bridge jobs correctly', :aggregate_failures do pipeline = create_pipeline! test = pipeline.statuses.find_by(name: 'test') @@ -221,6 +221,65 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do end end end + + context 'when including configs from a project' do + context 'when specifying all attributes' do + let(:config) do + <<~YAML + test: + script: rspec + deploy: + variables: + CROSS: downstream + stage: deploy + trigger: + include: + - project: my-namespace/my-project + file: 'path/to/child.yml' + ref: 'master' + YAML + end + + it_behaves_like 'successful creation' do + let(:expected_bridge_options) do + { + 'trigger' => { + 'include' => [ + { + 'file' => 'path/to/child.yml', + 'project' => 'my-namespace/my-project', + 'ref' => 'master' + } + ] + } + } + end + end + end + + context 'without specifying file' do + let(:config) do + <<~YAML + test: + script: rspec + deploy: + variables: + CROSS: downstream + stage: deploy + trigger: + include: + - project: my-namespace/my-project + ref: 'master' + YAML + end + + it_behaves_like 'creation failure' do + let(:expected_error) do + /include config must specify the file where to fetch the config from/ + end + end + end + end end def create_pipeline! 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/delete_objects_service_spec.rb b/spec/services/ci/delete_objects_service_spec.rb new file mode 100644 index 00000000000..448f8979681 --- /dev/null +++ b/spec/services/ci/delete_objects_service_spec.rb @@ -0,0 +1,133 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::DeleteObjectsService, :aggregate_failure do + let(:service) { described_class.new } + let(:artifact) { create(:ci_job_artifact, :archive) } + let(:data) { [artifact] } + + describe '#execute' do + before do + Ci::DeletedObject.bulk_import(data) + # We disable the check because the specs are wrapped in a transaction + allow(service).to receive(:transaction_open?).and_return(false) + end + + subject(:execute) { service.execute } + + it 'deletes records' do + expect { execute }.to change { Ci::DeletedObject.count }.by(-1) + end + + it 'deletes files' do + expect { execute }.to change { artifact.file.exists? } + end + + context 'when trying to execute without records' do + let(:data) { [] } + + it 'does not change the number of objects' do + expect { execute }.not_to change { Ci::DeletedObject.count } + end + end + + context 'when trying to remove the same file multiple times' do + let(:objects) { Ci::DeletedObject.all.to_a } + + before do + expect(service).to receive(:load_next_batch).twice.and_return(objects) + end + + it 'executes successfully' do + 2.times { expect(service.execute).to be_truthy } + end + end + + context 'with artifacts both ready and not ready for deletion' do + let(:data) { [] } + + let_it_be(:past_ready) { create(:ci_deleted_object, pick_up_at: 2.days.ago) } + let_it_be(:ready) { create(:ci_deleted_object, pick_up_at: 1.day.ago) } + + it 'skips records with pick_up_at in the future' do + not_ready = create(:ci_deleted_object, pick_up_at: 1.day.from_now) + + expect { execute }.to change { Ci::DeletedObject.count }.from(3).to(1) + expect(not_ready.reload.present?).to be_truthy + end + + it 'limits the number of records removed' do + stub_const("#{described_class}::BATCH_SIZE", 1) + + expect { execute }.to change { Ci::DeletedObject.count }.by(-1) + end + + it 'removes records in order' do + stub_const("#{described_class}::BATCH_SIZE", 1) + + execute + + expect { past_ready.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect(ready.reload.present?).to be_truthy + end + + it 'updates pick_up_at timestamp' do + allow(service).to receive(:destroy_everything) + + execute + + expect(past_ready.reload.pick_up_at).to be_like_time(10.minutes.from_now) + end + + it 'does not delete objects for which file deletion has failed' do + expect(past_ready) + .to receive(:delete_file_from_storage) + .and_return(false) + + expect(service) + .to receive(:load_next_batch) + .and_return([past_ready, ready]) + + expect { execute }.to change { Ci::DeletedObject.count }.from(2).to(1) + expect(past_ready.reload.present?).to be_truthy + end + end + + context 'with an open database transaction' do + it 'raises an exception and does not remove records' do + expect(service).to receive(:transaction_open?).and_return(true) + + expect { execute } + .to raise_error(Ci::DeleteObjectsService::TransactionInProgressError) + .and change { Ci::DeletedObject.count }.by(0) + end + end + end + + describe '#remaining_batches_count' do + subject { service.remaining_batches_count(max_batch_count: 3) } + + context 'when there is less than one batch size' do + before do + Ci::DeletedObject.bulk_import(data) + end + + it { is_expected.to eq(1) } + end + + context 'when there is more than one batch size' do + before do + objects_scope = double + + expect(Ci::DeletedObject) + .to receive(:ready_for_destruction) + .and_return(objects_scope) + + expect(objects_scope).to receive(:size).and_return(110) + end + + it { is_expected.to eq(2) } + end + end +end diff --git a/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb b/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb index 1c96be42a2f..3d5329811ad 100644 --- a/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb +++ b/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb @@ -9,9 +9,10 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared subject { service.execute } let(:service) { described_class.new } - let!(:artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) } - before do + let_it_be(:artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) } + + before(:all) do artifact.job.pipeline.unlocked! end @@ -38,7 +39,9 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared end context 'when artifact is not expired' do - let!(:artifact) { create(:ci_job_artifact, expire_at: 1.day.since) } + before do + artifact.update_column(:expire_at, 1.day.since) + end it 'does not destroy expired job artifacts' do expect { subject }.not_to change { Ci::JobArtifact.count } @@ -46,7 +49,9 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared end context 'when artifact is permanent' do - let!(:artifact) { create(:ci_job_artifact, expire_at: nil) } + before do + artifact.update_column(:expire_at, nil) + end it 'does not destroy expired job artifacts' do expect { subject }.not_to change { Ci::JobArtifact.count } 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/list_config_variables_service_spec.rb b/spec/services/ci/list_config_variables_service_spec.rb new file mode 100644 index 00000000000..5cc0481768b --- /dev/null +++ b/spec/services/ci/list_config_variables_service_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::ListConfigVariablesService do + let_it_be(:project) { create(:project, :repository) } + let(:service) { described_class.new(project) } + let(:result) { YAML.dump(ci_config) } + + subject { service.execute(sha) } + + before do + stub_gitlab_ci_yml_for_sha(sha, result) + end + + context 'when sending a valid sha' do + let(:sha) { 'master' } + let(:ci_config) do + { + variables: { + KEY1: { value: 'val 1', description: 'description 1' }, + KEY2: { value: 'val 2', description: '' }, + KEY3: { value: 'val 3' }, + KEY4: 'val 4' + }, + test: { + stage: 'test', + script: 'echo' + } + } + end + + it 'returns variable list' do + expect(subject['KEY1']).to eq({ value: 'val 1', description: 'description 1' }) + expect(subject['KEY2']).to eq({ value: 'val 2', description: '' }) + expect(subject['KEY3']).to eq({ value: 'val 3', description: nil }) + expect(subject['KEY4']).to eq({ value: 'val 4', description: nil }) + end + end + + context 'when sending an invalid sha' do + let(:sha) { 'invalid-sha' } + let(:ci_config) { nil } + + it 'returns empty json' do + expect(subject).to eq({}) + end + end + + context 'when sending an invalid config' do + let(:sha) { 'master' } + let(:ci_config) do + { + variables: { + KEY1: { value: 'val 1', description: 'description 1' } + }, + test: { + stage: 'invalid', + script: 'echo' + } + } + end + + it 'returns empty result' do + expect(subject).to eq({}) + end + end + + private + + def stub_gitlab_ci_yml_for_sha(sha, result) + allow_any_instance_of(Repository) + .to receive(:gitlab_ci_yml_for) + .with(sha, '.gitlab-ci.yml') + .and_return(result) + end +end diff --git a/spec/services/ci/pipeline_processing/shared_processing_service.rb b/spec/services/ci/pipeline_processing/shared_processing_service.rb index 7de22b6a4cc..bbd7422b435 100644 --- a/spec/services/ci/pipeline_processing/shared_processing_service.rb +++ b/spec/services/ci/pipeline_processing/shared_processing_service.rb @@ -259,14 +259,14 @@ RSpec.shared_examples 'Pipeline Processing Service' do expect(builds_names_and_statuses).to eq({ 'build': 'success', 'rollout10%': 'scheduled' }) - Timecop.travel 2.minutes.from_now do + travel_to 2.minutes.from_now do enqueue_scheduled('rollout10%') end succeed_pending expect(builds_names_and_statuses).to eq({ 'build': 'success', 'rollout10%': 'success', 'rollout100%': 'scheduled' }) - Timecop.travel 2.minutes.from_now do + travel_to 2.minutes.from_now do enqueue_scheduled('rollout100%') end succeed_pending @@ -330,7 +330,7 @@ RSpec.shared_examples 'Pipeline Processing Service' do expect(builds_names_and_statuses).to eq({ 'build': 'success', 'rollout10%': 'scheduled' }) - Timecop.travel 2.minutes.from_now do + travel_to 2.minutes.from_now do enqueue_scheduled('rollout10%') end fail_running_or_pending @@ -398,7 +398,7 @@ RSpec.shared_examples 'Pipeline Processing Service' do expect(process_pipeline).to be_truthy expect(builds_names_and_statuses).to eq({ 'delayed1': 'scheduled', 'delayed2': 'scheduled' }) - Timecop.travel 2.minutes.from_now do + travel_to 2.minutes.from_now do enqueue_scheduled('delayed1') end @@ -419,7 +419,7 @@ RSpec.shared_examples 'Pipeline Processing Service' do expect(process_pipeline).to be_truthy expect(builds_names_and_statuses).to eq({ 'delayed': 'scheduled' }) - Timecop.travel 2.minutes.from_now do + travel_to 2.minutes.from_now do enqueue_scheduled('delayed') end fail_running_or_pending 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/play_bridge_service_spec.rb b/spec/services/ci/play_bridge_service_spec.rb new file mode 100644 index 00000000000..0482ad4d76f --- /dev/null +++ b/spec/services/ci/play_bridge_service_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::PlayBridgeService, '#execute' do + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:downstream_project) { create(:project) } + let(:bridge) { create(:ci_bridge, :playable, pipeline: pipeline, downstream: downstream_project) } + let(:instance) { described_class.new(project, user) } + + subject(:execute_service) { instance.execute(bridge) } + + context 'when user can run the bridge' do + before do + allow(instance).to receive(:can?).with(user, :play_job, bridge).and_return(true) + end + + it 'marks the bridge pending' do + execute_service + + expect(bridge.reload).to be_pending + end + + it 'enqueues Ci::CreateCrossProjectPipelineWorker' do + expect(::Ci::CreateCrossProjectPipelineWorker).to receive(:perform_async).with(bridge.id) + + execute_service + end + + it "updates bridge's user" do + execute_service + + expect(bridge.reload.user).to eq(user) + end + + context 'when bridge is not playable' do + let(:bridge) { create(:ci_bridge, :failed, pipeline: pipeline, downstream: downstream_project) } + + it 'raises StateMachines::InvalidTransition' do + expect { execute_service }.to raise_error StateMachines::InvalidTransition + end + end + end + + context 'when user can not run the bridge' do + before do + allow(instance).to receive(:can?).with(user, :play_job, bridge).and_return(false) + end + + it 'allows user with developer role to play a bridge' do + expect { execute_service }.to raise_error Gitlab::Access::AccessDeniedError + end + end +end diff --git a/spec/services/ci/play_manual_stage_service_spec.rb b/spec/services/ci/play_manual_stage_service_spec.rb index e30ec8bfda5..3e2a95ee975 100644 --- a/spec/services/ci/play_manual_stage_service_spec.rb +++ b/spec/services/ci/play_manual_stage_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Ci::PlayManualStageService, '#execute' do let(:current_user) { create(:user) } let(:pipeline) { create(:ci_pipeline, user: current_user) } let(:project) { pipeline.project } + let(:downstream_project) { create(:project) } let(:service) { described_class.new(project, current_user, pipeline: pipeline) } let(:stage_status) { 'manual' } @@ -18,40 +19,42 @@ RSpec.describe Ci::PlayManualStageService, '#execute' do before do project.add_maintainer(current_user) + downstream_project.add_maintainer(current_user) create_builds_for_stage(status: stage_status) + create_bridge_for_stage(status: stage_status) end - context 'when pipeline has manual builds' do + context 'when pipeline has manual processables' do before do service.execute(stage) end - it 'starts manual builds from pipeline' do - expect(pipeline.builds.manual.count).to eq(0) + it 'starts manual processables from pipeline' do + expect(pipeline.processables.manual.count).to eq(0) end - it 'updates manual builds' do - pipeline.builds.each do |build| - expect(build.user).to eq(current_user) + it 'updates manual processables' do + pipeline.processables.each do |processable| + expect(processable.user).to eq(current_user) end end end - context 'when pipeline has no manual builds' do + context 'when pipeline has no manual processables' do let(:stage_status) { 'failed' } before do service.execute(stage) end - it 'does not update the builds' do - expect(pipeline.builds.failed.count).to eq(3) + it 'does not update the processables' do + expect(pipeline.processables.failed.count).to eq(4) end end - context 'when user does not have permission on a specific build' do + context 'when user does not have permission on a specific processable' do before do - allow_next_instance_of(Ci::Build) do |instance| + allow_next_instance_of(Ci::Processable) do |instance| allow(instance).to receive(:play).and_raise(Gitlab::Access::AccessDeniedError) end @@ -60,12 +63,14 @@ RSpec.describe Ci::PlayManualStageService, '#execute' do it 'logs the error' do expect(Gitlab::AppLogger).to receive(:error) - .exactly(stage.builds.manual.count) + .exactly(stage.processables.manual.count) service.execute(stage) end end + private + def create_builds_for_stage(options) options.merge!({ when: 'manual', @@ -77,4 +82,17 @@ RSpec.describe Ci::PlayManualStageService, '#execute' do create_list(:ci_build, 3, options) end + + def create_bridge_for_stage(options) + options.merge!({ + when: 'manual', + pipeline: pipeline, + stage: stage.name, + stage_id: stage.id, + user: pipeline.user, + downstream: downstream_project + }) + + create(:ci_bridge, options) + end end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 51741440075..81d56a0e42a 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -3,25 +3,32 @@ require 'spec_helper' RSpec.describe Ci::RetryBuildService do - let_it_be(:user) { create(:user) } + let_it_be(:reporter) { create(:user) } + let_it_be(:developer) { create(:user) } let_it_be(:project) { create(:project, :repository) } let_it_be(:pipeline) do create(:ci_pipeline, project: project, sha: 'b83d6e391c22777fca1ed3012fce84f633d7fed0') end - let(:stage) do + let_it_be(:stage) do create(:ci_stage_entity, project: project, pipeline: pipeline, name: 'test') end - let(:build) { create(:ci_build, pipeline: pipeline, stage_id: stage.id) } + let_it_be_with_refind(:build) { create(:ci_build, pipeline: pipeline, stage_id: stage.id) } + let(:user) { developer } let(:service) do described_class.new(project, user) end + before_all do + project.add_developer(developer) + project.add_reporter(reporter) + end + clone_accessors = described_class.clone_accessors reject_accessors = @@ -39,7 +46,8 @@ RSpec.describe Ci::RetryBuildService do job_variables waiting_for_resource_at job_artifacts_metrics_referee job_artifacts_network_referee job_artifacts_dotenv job_artifacts_cobertura needs job_artifacts_accessibility - job_artifacts_requirements job_artifacts_coverage_fuzzing].freeze + job_artifacts_requirements job_artifacts_coverage_fuzzing + job_artifacts_api_fuzzing].freeze ignore_accessors = %i[type lock_version target_url base_tags trace_sections @@ -53,9 +61,9 @@ RSpec.describe Ci::RetryBuildService do pipeline_id report_results pending_state pages_deployments].freeze shared_examples 'build duplication' do - let(:another_pipeline) { create(:ci_empty_pipeline, project: project) } + let_it_be(:another_pipeline) { create(:ci_empty_pipeline, project: project) } - let(:build) do + let_it_be(:build) do create(:ci_build, :failed, :expired, :erased, :queued, :coverage, :tags, :allowed_to_fail, :on_tag, :triggered, :teardown_environment, :resource_group, description: 'my-job', stage: 'test', stage_id: stage.id, @@ -63,7 +71,7 @@ RSpec.describe Ci::RetryBuildService do scheduled_at: 10.seconds.since) end - before do + before_all do # Test correctly behaviour of deprecated artifact because it can be still in use stub_feature_flags(drop_license_management_artifact: false) @@ -81,8 +89,6 @@ RSpec.describe Ci::RetryBuildService do create(:ci_job_variable, job: build) create(:ci_build_need, build: build) - - build.reload end describe 'clone accessors' do @@ -154,7 +160,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 @@ -162,8 +168,6 @@ RSpec.describe Ci::RetryBuildService do context 'when user has ability to execute build' do before do stub_not_protect_default_branch - - project.add_developer(user) end it_behaves_like 'build duplication' @@ -235,7 +239,6 @@ RSpec.describe Ci::RetryBuildService do context 'when the pipeline is a child pipeline and the bridge is depended' do let!(:parent_pipeline) { create(:ci_pipeline, project: project) } - let!(:pipeline) { create(:ci_pipeline, project: project) } let!(:bridge) { create(:ci_bridge, :strategy_depend, pipeline: parent_pipeline, status: 'success') } let!(:source_pipeline) { create(:ci_sources_pipeline, pipeline: pipeline, source_job: bridge) } @@ -248,6 +251,8 @@ RSpec.describe Ci::RetryBuildService do end context 'when user does not have ability to execute build' do + let(:user) { reporter } + it 'raises an error' do expect { service.execute(build) } .to raise_error Gitlab::Access::AccessDeniedError @@ -257,7 +262,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 @@ -265,8 +270,6 @@ RSpec.describe Ci::RetryBuildService do context 'when user has ability to execute build' do before do stub_not_protect_default_branch - - project.add_developer(user) end it_behaves_like 'build duplication' @@ -316,6 +319,8 @@ RSpec.describe Ci::RetryBuildService do end context 'when user does not have ability to execute build' do + let(:user) { reporter } + it 'raises an error' do expect { service.reprocess!(build) } .to raise_error Gitlab::Access::AccessDeniedError 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..2545909bf56 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,60 @@ RSpec.describe Ci::UpdateBuildStateService do .to have_received(:increment_trace_operation) .with(operation: :finalized) end + + it 'records migration duration in a histogram' do + freeze_time do + create(:ci_build_pending_state, build: build, created_at: 0.5.seconds.ago) + + execute_with_stubbed_metrics! + end + + expect(metrics) + .to have_received(:observe_migration_duration) + .with(0.5) + 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 +203,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 +217,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 +228,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..d8c95a70bd0 100644 --- a/spec/services/clusters/gcp/finalize_creation_service_spec.rb +++ b/spec/services/clusters/gcp/finalize_creation_service_spec.rb @@ -83,12 +83,7 @@ RSpec.describe Clusters::Gcp::FinalizeCreationService, '#execute' do shared_context 'kubernetes information successfully fetched' do before do stub_cloud_platform_get_zone_cluster( - provider.gcp_project_id, provider.zone, cluster.name, - { - endpoint: endpoint, - username: username, - password: password - } + provider.gcp_project_id, provider.zone, cluster.name, { endpoint: endpoint, username: username, password: password } ) stub_kubeclient_discover(api_url) @@ -101,11 +96,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/create_or_update_service_account_service_spec.rb b/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb index f3fa6c2c0bb..257e2e53733 100644 --- a/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb +++ b/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb @@ -160,26 +160,60 @@ RSpec.describe Clusters::Kubernetes::CreateOrUpdateServiceAccountService do it_behaves_like 'creates service account and token' - it 'creates a namespaced role binding with edit access' do - subject + context 'kubernetes_cluster_namespace_role_admin FF is enabled' do + before do + stub_feature_flags(kubernetes_cluster_namespace_role_admin: true) + end + + it 'creates a namespaced role binding with admin access' do + subject + + expect(WebMock).to have_requested(:put, api_url + "/apis/rbac.authorization.k8s.io/v1/namespaces/#{namespace}/rolebindings/#{role_binding_name}").with( + body: hash_including( + metadata: { name: "gitlab-#{namespace}", namespace: "#{namespace}" }, + roleRef: { + apiGroup: 'rbac.authorization.k8s.io', + kind: 'ClusterRole', + name: 'admin' + }, + subjects: [ + { + kind: 'ServiceAccount', + name: service_account_name, + namespace: namespace + } + ] + ) + ) + end + end - expect(WebMock).to have_requested(:put, api_url + "/apis/rbac.authorization.k8s.io/v1/namespaces/#{namespace}/rolebindings/#{role_binding_name}").with( - body: hash_including( - metadata: { name: "gitlab-#{namespace}", namespace: "#{namespace}" }, - roleRef: { - apiGroup: 'rbac.authorization.k8s.io', - kind: 'ClusterRole', - name: 'edit' - }, - subjects: [ - { - kind: 'ServiceAccount', - name: service_account_name, - namespace: namespace - } - ] + context 'kubernetes_cluster_namespace_role_admin FF is disabled' do + before do + stub_feature_flags(kubernetes_cluster_namespace_role_admin: false) + end + + it 'creates a namespaced role binding with edit access' do + subject + + expect(WebMock).to have_requested(:put, api_url + "/apis/rbac.authorization.k8s.io/v1/namespaces/#{namespace}/rolebindings/#{role_binding_name}").with( + body: hash_including( + metadata: { name: "gitlab-#{namespace}", namespace: "#{namespace}" }, + roleRef: { + apiGroup: 'rbac.authorization.k8s.io', + kind: 'ClusterRole', + name: 'edit' + }, + subjects: [ + { + kind: 'ServiceAccount', + name: service_account_name, + namespace: namespace + } + ] + ) ) - ) + end end it 'creates a role binding granting crossplane database permissions to the service account' do 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..03c402fb066 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 @@ -79,11 +75,9 @@ RSpec.describe Clusters::Kubernetes::FetchKubernetesTokenService do before do stub_kubeclient_get_secret_missing_token_then_with_token( 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 +90,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/create_service_spec.rb b/spec/services/deployments/create_service_spec.rb index d1f977c28d3..2d157c9d114 100644 --- a/spec/services/deployments/create_service_spec.rb +++ b/spec/services/deployments/create_service_spec.rb @@ -19,8 +19,9 @@ RSpec.describe Deployments::CreateService do status: 'success' ) - expect(Deployments::SuccessWorker).to receive(:perform_async) - expect(Deployments::FinishedWorker).to receive(:perform_async) + expect(Deployments::UpdateEnvironmentWorker).to receive(:perform_async) + expect(Deployments::LinkMergeRequestWorker).to receive(:perform_async) + expect(Deployments::ExecuteHooksWorker).to receive(:perform_async) expect(service.execute).to be_persisted end @@ -34,8 +35,9 @@ RSpec.describe Deployments::CreateService do tag: false ) - expect(Deployments::SuccessWorker).not_to receive(:perform_async) - expect(Deployments::FinishedWorker).not_to receive(:perform_async) + expect(Deployments::UpdateEnvironmentWorker).not_to receive(:perform_async) + expect(Deployments::LinkMergeRequestWorker).not_to receive(:perform_async) + expect(Deployments::ExecuteHooksWorker).not_to receive(:perform_async) expect(service.execute).to be_persisted end diff --git a/spec/services/deployments/after_create_service_spec.rb b/spec/services/deployments/update_environment_service_spec.rb index 6cdb4c88191..92488c62315 100644 --- a/spec/services/deployments/after_create_service_spec.rb +++ b/spec/services/deployments/update_environment_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Deployments::AfterCreateService do +RSpec.describe Deployments::UpdateEnvironmentService do let(:user) { create(:user) } let(:project) { create(:project, :repository) } let(:options) { { name: 'production' } } @@ -31,7 +31,8 @@ RSpec.describe Deployments::AfterCreateService do subject(:service) { described_class.new(deployment) } before do - allow(Deployments::FinishedWorker).to receive(:perform_async) + allow(Deployments::LinkMergeRequestWorker).to receive(:perform_async) + allow(Deployments::ExecuteHooksWorker).to receive(:perform_async) job.success! # Create/Succeed deployment end @@ -100,8 +101,8 @@ RSpec.describe Deployments::AfterCreateService do end before do - environment.update(name: 'review-apps/master') - job.update(environment: 'review-apps/$CI_COMMIT_REF_NAME') + environment.update!(name: 'review-apps/master') + job.update!(environment: 'review-apps/$CI_COMMIT_REF_NAME') end it 'does not create a new environment' do @@ -241,7 +242,7 @@ RSpec.describe Deployments::AfterCreateService do end it 'does not raise errors if the merge request does not have a metrics record' do - merge_request.metrics.destroy + merge_request.metrics.destroy! expect(merge_request.reload.metrics).to be_nil expect { service.execute }.not_to raise_error @@ -257,7 +258,7 @@ RSpec.describe Deployments::AfterCreateService do expect(merge_request.reload.metrics.first_deployed_to_production_at).to be_like_time(deployment.finished_at) # Current deploy - Timecop.travel(12.hours.from_now) do + travel_to(12.hours.from_now) do service.execute expect(merge_request.reload.metrics.first_deployed_to_production_at).to be_like_time(deployment.finished_at) @@ -269,14 +270,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..ed161b4c8ff 100644 --- a/spec/services/design_management/delete_designs_service_spec.rb +++ b/spec/services/design_management/delete_designs_service_spec.rb @@ -80,6 +80,16 @@ RSpec.describe DesignManagement::DeleteDesignsService do expect { run_service rescue nil } .not_to change { [counter.totals, Event.count] } end + + it 'does not log any UsageData metrics' do + redis_hll = ::Gitlab::UsageDataCounters::HLLRedisCounter + event = Gitlab::UsageDataCounters::IssueActivityUniqueCounter::ISSUE_DESIGNS_REMOVED + + expect { run_service rescue nil } + .not_to change { redis_hll.unique_events(event_names: event, start_date: 1.day.ago, end_date: 1.day.from_now) } + + run_service rescue nil + end end context 'one design is passed' do @@ -98,6 +108,12 @@ RSpec.describe DesignManagement::DeleteDesignsService do expect { run_service }.to change { counter.read(:delete) }.by(1) end + it 'updates UsageData for removed designs' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_designs_removed_action).with(author: user) + + run_service + end + it 'creates an event in the activity stream' do expect { run_service } .to change { Event.count }.by(1) @@ -105,7 +121,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..f36e68c8dbd 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) @@ -102,9 +102,11 @@ RSpec.describe DesignManagement::SaveDesignsService do end end - it 'creates a commit, an event in the activity stream and updates the creation count' do + it 'creates a commit, an event in the activity stream and updates the creation count', :aggregate_failures do counter = Gitlab::UsageDataCounters::DesignsCounter + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_designs_added_action).with(author: user) + expect { run_service } .to change { Event.count }.by(1) .and change { Event.for_design.created_action.count }.by(1) @@ -128,6 +130,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] @@ -171,6 +192,12 @@ RSpec.describe DesignManagement::SaveDesignsService do expect(updated_designs.first.versions.size).to eq(2) end + it 'updates UsageData for changed designs' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_designs_modified_action).with(author: user) + + run_service + end + it 'records the correct events' do counter = Gitlab::UsageDataCounters::DesignsCounter expect { run_service } @@ -220,7 +247,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 +281,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 +298,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..2cd19000f99 --- /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_it_be(:project) { create(:project) } + let_it_be(:developer) { create(:user) } + let_it_be(:reporter) { create(:user) } + let(:user) { developer } + + before_all 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..b35de02c628 --- /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_it_be(:project) { create(:project) } + let_it_be(:developer) { create(:user) } + let_it_be(:reporter) { create(:user) } + let(:user) { developer } + let!(:feature_flag) { create(:operations_feature_flag, project: project) } + + before_all 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..de0f70bf552 --- /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(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let(:params) { {} } + let(:service) { described_class.new(project, user, params) } + + before_all 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..88c8028f6c5 --- /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(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let(:params) { {} } + let(:service) { described_class.new(project, user, params) } + + before_all 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..a982dd5166b --- /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_it_be(:project) { create(:project) } + let_it_be(:developer) { create(:user) } + let_it_be(:reporter) { create(:user) } + let(:user) { developer } + let(:feature_flag) { create(:operations_feature_flag, project: project, active: true) } + + before_all 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..4f5bc3a3d5a 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -45,6 +45,15 @@ RSpec.describe Groups::CreateService, '#execute' do end end + context 'creating a group with `allow_mfa_for_subgroups` attribute' do + let(:params) { group_params.merge(allow_mfa_for_subgroups: false) } + let(:service) { described_class.new(user, params) } + + it 'creates group without error' do + expect(service.execute).to be_persisted + end + end + describe 'creating a top level group' do let(:service) { described_class.new(user, group_params) } @@ -138,4 +147,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..ae04eca3a9f 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) } @@ -529,6 +567,39 @@ RSpec.describe Groups::TransferService do end end + context 'when transferring a group with two factor authentication switched on' do + before do + TestEnv.clean_test_path + create(:group_member, :owner, group: new_parent_group, user: user) + create(:group, :private, parent: group, require_two_factor_authentication: true) + group.update!(require_two_factor_authentication: true) + end + + it 'does not update group two factor authentication setting' do + transfer_service.execute(new_parent_group) + + expect(group.require_two_factor_authentication).to eq(true) + end + + context 'when new parent disallows two factor authentication switched on for descendants' do + before do + new_parent_group.namespace_settings.update!(allow_mfa_for_subgroups: false) + end + + it 'updates group two factor authentication setting' do + transfer_service.execute(new_parent_group) + + expect(group.require_two_factor_authentication).to eq(false) + end + + it 'schedules update of group two factor authentication setting for descendants' do + expect(DisallowTwoFactorForSubgroupsWorker).to receive(:perform_async).with(group.id) + + transfer_service.execute(new_parent_group) + end + end + end + context 'when updating the group goes wrong' do let!(:subgroup1) { create(:group, :public, parent: group) } let!(:subgroup2) { create(:group, :public, parent: group) } diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 1e6a8d53354..bc7c066fa04 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -283,6 +283,50 @@ 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 + + context 'changes allowing subgroups to establish own 2FA' do + let(:group) { create(:group) } + let(:params) { { allow_mfa_for_subgroups: false } } + + subject { described_class.new(group, user, params).execute } + + it 'changes settings' do + subject + + expect(group.namespace_settings.reload.allow_mfa_for_subgroups).to eq(false) + end + + it 'enqueues update subgroups and its members' do + expect(DisallowTwoFactorForSubgroupsWorker).to receive(:perform_async).with(group.id) + + subject + 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/create_incident_label_service_spec.rb b/spec/services/incident_management/create_incident_label_service_spec.rb index 4771dfc9e64..441cddf1d2e 100644 --- a/spec/services/incident_management/create_incident_label_service_spec.rb +++ b/spec/services/incident_management/create_incident_label_service_spec.rb @@ -3,65 +3,5 @@ require 'spec_helper' RSpec.describe IncidentManagement::CreateIncidentLabelService do - let_it_be(:project) { create(:project, :private) } - let_it_be(:user) { User.alert_bot } - let(:service) { described_class.new(project, user) } - - subject(:execute) { service.execute } - - describe 'execute' do - let(:incident_label_attributes) { attributes_for(:label, :incident) } - let(:title) { incident_label_attributes[:title] } - let(:color) { incident_label_attributes[:color] } - let(:description) { incident_label_attributes[:description] } - - shared_examples 'existing label' do - it 'returns the existing label' do - expect { execute }.not_to change(Label, :count) - - expect(execute).to be_success - expect(execute.payload).to eq(label: label) - end - end - - shared_examples 'new label' do - it 'creates a new label' do - expect { execute }.to change(Label, :count).by(1) - - label = project.reload.labels.last - expect(execute).to be_success - expect(execute.payload).to eq(label: label) - expect(label.title).to eq(title) - expect(label.color).to eq(color) - expect(label.description).to eq(description) - end - end - - context 'with predefined project label' do - it_behaves_like 'existing label' do - let!(:label) { create(:label, project: project, title: title) } - end - end - - context 'with predefined group label' do - let(:project) { create(:project, group: group) } - let(:group) { create(:group) } - - it_behaves_like 'existing label' do - let!(:label) { create(:group_label, group: group, title: title) } - end - end - - context 'without label' do - context 'when user has permissions to create labels' do - it_behaves_like 'new label' - end - - context 'when user has no permissions to create labels' do - let_it_be(:user) { create(:user) } - - it_behaves_like 'new label' - end - end - end + it_behaves_like 'incident management label service' 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/build_service_spec.rb b/spec/services/issues/build_service_spec.rb index 93eef8a2732..16433d49ca1 100644 --- a/spec/services/issues/build_service_spec.rb +++ b/spec/services/issues/build_service_spec.rb @@ -3,11 +3,14 @@ require 'spec_helper.rb' RSpec.describe Issues::BuildService do - let(:project) { create(:project, :repository) } - let(:user) { create(:user) } - - before do - project.add_developer(user) + let_it_be(:project) { create(:project, :repository) } + let_it_be(:developer) { create(:user) } + let_it_be(:guest) { create(:user) } + let(:user) { developer } + + before_all do + project.add_developer(developer) + project.add_guest(guest) end def build_issue(issue_params = {}) @@ -134,31 +137,56 @@ RSpec.describe Issues::BuildService do end describe '#execute' do - it 'builds a new issues with given params' do - milestone = create(:milestone, project: project) - issue = build_issue(milestone_id: milestone.id) + context 'as developer' do + it 'builds a new issues with given params' do + milestone = create(:milestone, project: project) + issue = build_issue(milestone_id: milestone.id) - expect(issue.milestone).to eq(milestone) - end + expect(issue.milestone).to eq(milestone) + end - it 'sets milestone to nil if it is not available for the project' do - milestone = create(:milestone, project: create(:project)) - issue = build_issue(milestone_id: milestone.id) + it 'sets milestone to nil if it is not available for the project' do + milestone = create(:milestone, project: create(:project)) + issue = build_issue(milestone_id: milestone.id) - expect(issue.milestone).to be_nil + expect(issue.milestone).to be_nil + end end - context 'setting issue type' do - it 'sets the issue_type on the issue' do - issue = build_issue(issue_type: 'incident') + context 'as guest' do + let(:user) { guest } - expect(issue.issue_type).to eq('incident') + it 'cannot set milestone' do + milestone = create(:milestone, project: project) + issue = build_issue(milestone_id: milestone.id) + + expect(issue.milestone).to be_nil end - it 'defaults to issue if issue_type not given' do - issue = build_issue + context 'setting issue type' do + it 'defaults to issue if issue_type not given' do + issue = build_issue + + expect(issue).to be_issue + end + + it 'sets issue' do + issue = build_issue(issue_type: 'issue') + + expect(issue).to be_issue + end + + it 'sets incident' do + issue = build_issue(issue_type: 'incident') - expect(issue.issue_type).to eq('issue') + expect(issue).to be_incident + end + + it 'cannot set invalid type' do + expect do + build_issue(issue_type: 'invalid type') + end.to raise_error(ArgumentError, "'invalid type' is not a valid issue_type") + end end end end 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..ae1454ce9bb 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,47 @@ 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_it_be(:new_project) { create(:project) } + 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 + + # 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/jira/requests/projects/list_service_spec.rb b/spec/services/jira/requests/projects/list_service_spec.rb index 415dd42c795..f7bcfa997df 100644 --- a/spec/services/jira/requests/projects/list_service_spec.rb +++ b/spec/services/jira/requests/projects/list_service_spec.rb @@ -45,6 +45,10 @@ RSpec.describe Jira::Requests::Projects::ListService do end it 'returns an error response' do + expect(Gitlab::ProjectServiceLogger).to receive(:error).with( + hash_including( + error: hash_including(:exception_class, :exception_message, :exception_backtrace))) + .and_call_original expect(subject.error?).to be_truthy expect(subject.message).to eq('Jira request error: Timeout::Error') 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/cleanup_refs_service_spec.rb b/spec/services/merge_requests/cleanup_refs_service_spec.rb index b38ccee4aa0..a051b3c9355 100644 --- a/spec/services/merge_requests/cleanup_refs_service_spec.rb +++ b/spec/services/merge_requests/cleanup_refs_service_spec.rb @@ -35,6 +35,17 @@ RSpec.describe MergeRequests::CleanupRefsService do end end + context 'when merge request has no head ref' do + before do + # Simulate a merge request with no head ref + merge_request.project.repository.delete_refs(merge_request.ref_path) + end + + it 'does not fail' do + expect(result[:status]).to eq(:success) + end + end + context 'when merge request has merge ref' do before do MergeRequests::MergeToRefService 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/create_from_issue_service_spec.rb b/spec/services/merge_requests/create_from_issue_service_spec.rb index fa70ad8c559..86e49fe601c 100644 --- a/spec/services/merge_requests/create_from_issue_service_spec.rb +++ b/spec/services/merge_requests/create_from_issue_service_spec.rb @@ -154,7 +154,7 @@ RSpec.describe MergeRequests::CreateFromIssueService do result = service.execute - expect(result[:merge_request].label_ids).to eq(label_ids) + expect(result[:merge_request].label_ids).to match_array(label_ids) end it "inherits milestones" do 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..ecb17b3fe77 --- /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), merge_request.project) } + + 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 5c44af87470..aec5a3b3fa3 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/merge_to_ref_service_spec.rb b/spec/services/merge_requests/merge_to_ref_service_spec.rb index b482e8d6724..14ef5b0b772 100644 --- a/spec/services/merge_requests/merge_to_ref_service_spec.rb +++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb @@ -252,5 +252,16 @@ RSpec.describe MergeRequests::MergeToRefService do end end end + + context 'allow conflicts to be merged in diff' do + let(:params) { { allow_conflicts: true } } + + it 'calls merge_to_ref with allow_conflicts param' do + expect(project.repository).to receive(:merge_to_ref) + .with(anything, anything, anything, anything, anything, anything, true) + + service.execute(merge_request) + end + end end end diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb index 543da46f883..725fc16fa7c 100644 --- a/spec/services/merge_requests/mergeability_check_service_spec.rb +++ b/spec/services/merge_requests/mergeability_check_service_spec.rb @@ -41,16 +41,6 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar subject end - context 'when merge_ref_head_comments is disabled' do - it 'does not update diff discussion positions' do - stub_feature_flags(merge_ref_head_comments: false) - - expect(Discussions::CaptureDiffNotePositionsService).not_to receive(:new) - - subject - end - end - it 'updates the merge ref' do expect { subject }.to change(merge_request, :merge_ref_head).from(nil) end @@ -221,11 +211,18 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar target_branch: 'conflict-start') end - it_behaves_like 'unmergeable merge request' + it 'does not change the merge ref HEAD' do + expect(merge_request.merge_ref_head).to be_nil - it 'returns ServiceResponse.error' do + subject + + expect(merge_request.reload.merge_ref_head).not_to be_nil + end + + it 'returns ServiceResponse.error and keeps merge status as cannot_be_merged' do result = subject + expect(merge_request.merge_status).to eq('cannot_be_merged') expect(result).to be_a(ServiceResponse) expect(result.error?).to be(true) expect(result.message).to eq('Merge request is not mergeable') @@ -383,5 +380,27 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar end end end + + context 'merge with conflicts' do + it 'calls MergeToRefService with true allow_conflicts param' do + expect(MergeRequests::MergeToRefService).to receive(:new) + .with(project, merge_request.author, { allow_conflicts: true }).and_call_original + + subject + end + + context 'when display_merge_conflicts_in_diff is disabled' do + before do + stub_feature_flags(display_merge_conflicts_in_diff: false) + end + + it 'calls MergeToRefService with false allow_conflicts param' do + expect(MergeRequests::MergeToRefService).to receive(:new) + .with(project, merge_request.author, { allow_conflicts: false }).and_call_original + + subject + end + end + end end end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index cace1e0bf09..d603cbb16aa 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) - - @fork_project.destroy! - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') - reload_mrs - end + 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 - 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 + it 'updates the merge request state' do + expect(@merge_request.resource_state_events.last.state).to eq('merged') - 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 @@ -827,10 +796,6 @@ RSpec.describe MergeRequests::RefreshService do subject { service.execute(oldrev, newrev, 'refs/heads/merge-commit-analyze-before') } context 'feature enabled' do - before do - stub_feature_flags(branch_push_merge_commit_analyze: true) - end - it "updates merge requests' merge_commits" do expect(Gitlab::BranchPushMergeCommitAnalyzer).to receive(:new).and_wrap_original do |original_method, commits| expect(commits.map(&:id)).to eq(%w{646ece5cfed840eca0a4feb21bcd6a81bb19bda3 29284d9bcc350bcae005872d0be6edd016e2efb5 5f82584f0a907f3b30cfce5bb8df371454a90051 8a994512e8c8f0dfcf22bb16df6e876be7a61036 689600b91aabec706e657e38ea706ece1ee8268f db46a1c5a5e474aa169b6cdb7a522d891bc4c5f9}) @@ -847,24 +812,6 @@ RSpec.describe MergeRequests::RefreshService do expect(merge_request_side_branch.merge_commit.id).to eq('29284d9bcc350bcae005872d0be6edd016e2efb5') end end - - context 'when feature is disabled' do - before do - stub_feature_flags(branch_push_merge_commit_analyze: false) - end - - it "does not trigger analysis" do - expect(Gitlab::BranchPushMergeCommitAnalyzer).not_to receive(:new) - - subject - - merge_request.reload - merge_request_side_branch.reload - - expect(merge_request.merge_commit).to eq(nil) - expect(merge_request_side_branch.merge_commit).to eq(nil) - end - end end describe '#abort_ff_merge_requests_with_when_pipeline_succeeds' do 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..b588bf2034d --- /dev/null +++ b/spec/services/namespace_settings/update_service_spec.rb @@ -0,0 +1,48 @@ +# 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 + before do + group.namespace_settings.destroy! + end + + 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 + 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 + + context "updating :default_branch_name" do + let(:example_branch_name) { "example_branch_name" } + let(:settings) { { default_branch_name: example_branch_name } } + + it "changes settings" do + expect { service.execute } + .to change { group.namespace_settings.default_branch_name } + .from(nil).to(example_branch_name) + end + end + end +end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 7c0d4b756bd..1e5536a2d0b 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -163,14 +163,6 @@ RSpec.describe Notes::CreateService do expect(note.note_diff_file).to be_present expect(note.diff_note_positions).to be_present end - - it 'does not create diff positions merge_ref_head_comments is disabled' do - stub_feature_flags(merge_ref_head_comments: false) - - expect(Discussions::CaptureDiffNotePositionService).not_to receive(:new) - - described_class.new(project_with_repo, user, new_opts).execute - end end context 'when DiffNote is a reply' do @@ -437,7 +429,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..caa9961424e 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) @@ -2229,6 +2293,25 @@ RSpec.describe NotificationService, :mailer do end end + describe '#invite_member_reminder' do + let_it_be(:group_member) { create(:group_member) } + + subject { notification.invite_member_reminder(group_member, 'token', 0) } + + it 'calls the Notify.invite_member_reminder method with the right params' do + expect(Notify).to receive(:member_invited_reminder_email).with('Group', group_member.id, 'token', 0).at_least(:once).and_call_original + + subject + end + + it 'sends exactly one email' do + subject + + expect_delivery_jobs_count(1) + expect_enqueud_email('Group', group_member.id, 'token', 0, mail: 'member_invited_reminder_email') + end + end + describe 'GroupMember', :deliver_mails_inline do let(:added_user) { create(:user) } @@ -3018,32 +3101,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/composer/composer_json_service_spec.rb b/spec/services/packages/composer/composer_json_service_spec.rb index 3996fcea679..378016a6ffb 100644 --- a/spec/services/packages/composer/composer_json_service_spec.rb +++ b/spec/services/packages/composer/composer_json_service_spec.rb @@ -23,7 +23,7 @@ RSpec.describe Packages::Composer::ComposerJsonService do let(:json) { '{ name": "package-name"}' } it 'raises an error' do - expect { subject }.to raise_error(/Invalid/) + expect { subject }.to raise_error(described_class::InvalidJson, /Invalid/) end end end @@ -32,7 +32,7 @@ RSpec.describe Packages::Composer::ComposerJsonService do let(:project) { create(:project, :repository) } it 'raises an error' do - expect { subject }.to raise_error(/not found/) + expect { subject }.to raise_error(described_class::InvalidJson, /not found/) end end end 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/after_rename_service_spec.rb b/spec/services/projects/after_rename_service_spec.rb index f03e1ed0e22..a8db87e48d0 100644 --- a/spec/services/projects/after_rename_service_spec.rb +++ b/spec/services/projects/after_rename_service_spec.rb @@ -243,7 +243,7 @@ RSpec.describe Projects::AfterRenameService do def service_execute # AfterRenameService is called by UpdateService after a successful model.update # the initialization will include before and after paths values - project.update(path: path_after_rename) + project.update!(path: path_after_rename) described_class.new(project, path_before: path_before_rename, full_path_before: full_path_before_rename).execute end diff --git a/spec/services/projects/alerting/notify_service_spec.rb b/spec/services/projects/alerting/notify_service_spec.rb index 77a0e330109..809b12910a1 100644 --- a/spec/services/projects/alerting/notify_service_spec.rb +++ b/spec/services/projects/alerting/notify_service_spec.rb @@ -62,7 +62,7 @@ RSpec.describe Projects::Alerting::NotifyService do title: payload_raw.fetch(:title), started_at: Time.zone.parse(payload_raw.fetch(:start_time)), severity: payload_raw.fetch(:severity), - status: AlertManagement::Alert::STATUSES[:triggered], + status: AlertManagement::Alert.status_value(:triggered), events: 1, hosts: payload_raw.fetch(:hosts), payload: payload_raw.with_indifferent_access, @@ -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 @@ -194,7 +180,7 @@ RSpec.describe Projects::Alerting::NotifyService do title: payload_raw.fetch(:title), started_at: Time.zone.parse(payload_raw.fetch(:start_time)), severity: 'critical', - status: AlertManagement::Alert::STATUSES[:triggered], + status: AlertManagement::Alert.status_value(:triggered), events: 1, hosts: [], payload: payload_raw.with_indifferent_access, @@ -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/autocomplete_service_spec.rb b/spec/services/projects/autocomplete_service_spec.rb index 336aa37096a..aff1aa41091 100644 --- a/spec/services/projects/autocomplete_service_spec.rb +++ b/spec/services/projects/autocomplete_service_spec.rb @@ -123,7 +123,7 @@ RSpec.describe Projects::AutocompleteService do let!(:subgroup_milestone) { create(:milestone, group: subgroup) } before do - project.update(namespace: subgroup) + project.update!(namespace: subgroup) end it 'includes project milestones and all acestors milestones' do @@ -138,7 +138,7 @@ RSpec.describe Projects::AutocompleteService do def expect_labels_to_equal(labels, expected_labels) expect(labels.size).to eq(expected_labels.size) extract_title = lambda { |label| label['title'] } - expect(labels.map(&extract_title)).to eq(expected_labels.map(&extract_title)) + expect(labels.map(&extract_title)).to match_array(expected_labels.map(&extract_title)) end let(:user) { create(:user) } 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 2f2474f2681..8ddcb8ce660 100644 --- a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb @@ -12,8 +12,6 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do before do project.add_maintainer(user) - stub_feature_flags(container_registry_cleanup: true) - stub_container_registry_config(enabled: true) stub_container_registry_tags( 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 54375193067..c3ae26b1f05 100644 --- a/spec/services/projects/container_repository/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/delete_tags_service_spec.rb @@ -87,59 +87,35 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do RSpec.shared_examples 'supporting fast delete' do 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' + before do + allow(repository.client).to receive(:supports_tag_delete?).and_return(true) + end - context 'with the real service' do - before do - stub_delete_reference_requests(tags) - expect_delete_tag_by_names(tags) - end + it_behaves_like 'calling the correct delete tags service', ::Projects::ContainerRepository::Gitlab::DeleteTagsService - it { is_expected.to include(status: :success) } + it_behaves_like 'handling invalid params' - it_behaves_like 'logging a success response' + context 'with the real service' do + before do + stub_delete_reference_requests(tags) + expect_delete_tag_by_names(tags) 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 { is_expected.to include(status: :success) } - it_behaves_like 'logging an error response', message: 'timeout while deleting tags' - end + it_behaves_like 'logging a success response' end - context 'and the feature is disabled' do + context 'with a timeout error' 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') + 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: :success) } + it { is_expected.to include(status: :error, message: 'timeout while deleting tags') } - it_behaves_like 'logging a success response' - end + it_behaves_like 'logging an error response', message: 'timeout while deleting tags' end end end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index e1df8700795..d959cc87901 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -14,15 +14,30 @@ RSpec.describe Projects::CreateService, '#execute' do } end - it 'creates labels on Project creation if there are templates' do - Label.create(title: "bug", template: true) - project = create_project(user, opts) + context 'with labels' do + subject(:project) { create_project(user, opts) } + + before_all do + Label.create!(title: 'bug', template: true) + end + + it 'creates labels on project creation' do + created_label = project.labels.last - created_label = project.reload.labels.last + expect(created_label.type).to eq('ProjectLabel') + expect(created_label.project_id).to eq(project.id) + expect(created_label.title).to eq('bug') + end + + context 'using gitlab project import' do + before do + opts[:import_type] = 'gitlab_project' + end - expect(created_label.type).to eq('ProjectLabel') - expect(created_label.project_id).to eq(project.id) - expect(created_label.title).to eq('bug') + it 'does not creates labels on project creation' do + expect(project.labels.size).to eq(0) + end + end end context 'user namespace' do @@ -59,10 +74,6 @@ RSpec.describe Projects::CreateService, '#execute' do context "admin creates project with other user's namespace_id" do it 'sets the correct permissions' do admin = create(:admin) - opts = { - name: 'GitLab', - namespace_id: user.namespace.id - } project = create_project(admin, opts) expect(project).to be_persisted @@ -487,18 +498,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 +506,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 +793,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/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index a3711c9e17f..f0f09218b06 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -72,7 +72,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do context 'when project has remote mirrors' do let!(:project) do create(:project, :repository, namespace: user.namespace).tap do |project| - project.remote_mirrors.create(url: 'http://test.com') + project.remote_mirrors.create!(url: 'http://test.com') end end diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index 166a2dae55b..555f2f5a5e5 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -179,7 +179,7 @@ RSpec.describe Projects::ForkService do context "when origin has git depth specified" do before do - @from_project.update(ci_default_git_depth: 42) + @from_project.update!(ci_default_git_depth: 42) end it "inherits default_git_depth from the origin project" do @@ -201,7 +201,7 @@ RSpec.describe Projects::ForkService do context "when project has restricted visibility level" do context "and only one visibility level is restricted" do before do - @from_project.update(visibility_level: Gitlab::VisibilityLevel::INTERNAL) + @from_project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::INTERNAL]) end diff --git a/spec/services/projects/hashed_storage/base_attachment_service_spec.rb b/spec/services/projects/hashed_storage/base_attachment_service_spec.rb index 969381b8748..86e3fb3820c 100644 --- a/spec/services/projects/hashed_storage/base_attachment_service_spec.rb +++ b/spec/services/projects/hashed_storage/base_attachment_service_spec.rb @@ -45,7 +45,7 @@ RSpec.describe Projects::HashedStorage::BaseAttachmentService do describe '#move_folder!' do context 'when old_path is not a directory' do it 'adds information to the logger and returns true' do - Tempfile.create do |old_path| + Tempfile.create do |old_path| # rubocop:disable Rails/SaveBang new_path = "#{old_path}-new" expect(subject.send(:move_folder!, old_path, new_path)).to be_truthy diff --git a/spec/services/projects/move_access_service_spec.rb b/spec/services/projects/move_access_service_spec.rb index de3871414af..02f80988dd1 100644 --- a/spec/services/projects/move_access_service_spec.rb +++ b/spec/services/projects/move_access_service_spec.rb @@ -17,9 +17,9 @@ RSpec.describe Projects::MoveAccessService do project_with_access.add_maintainer(maintainer_user) project_with_access.add_developer(developer_user) project_with_access.add_reporter(reporter_user) - project_with_access.project_group_links.create(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER) - project_with_access.project_group_links.create(group: developer_group, group_access: Gitlab::Access::DEVELOPER) - project_with_access.project_group_links.create(group: reporter_group, group_access: Gitlab::Access::REPORTER) + project_with_access.project_group_links.create!(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER) + project_with_access.project_group_links.create!(group: developer_group, group_access: Gitlab::Access::DEVELOPER) + project_with_access.project_group_links.create!(group: reporter_group, group_access: Gitlab::Access::REPORTER) end subject { described_class.new(target_project, user) } @@ -97,7 +97,7 @@ RSpec.describe Projects::MoveAccessService do end it 'does not remove remaining group links' do - target_project.project_group_links.create(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER) + target_project.project_group_links.create!(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER) subject.execute(project_with_access, options) diff --git a/spec/services/projects/move_project_group_links_service_spec.rb b/spec/services/projects/move_project_group_links_service_spec.rb index 196a8f2b339..6304eded8d3 100644 --- a/spec/services/projects/move_project_group_links_service_spec.rb +++ b/spec/services/projects/move_project_group_links_service_spec.rb @@ -14,9 +14,9 @@ RSpec.describe Projects::MoveProjectGroupLinksService do describe '#execute' do before do - project_with_groups.project_group_links.create(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER) - project_with_groups.project_group_links.create(group: developer_group, group_access: Gitlab::Access::DEVELOPER) - project_with_groups.project_group_links.create(group: reporter_group, group_access: Gitlab::Access::REPORTER) + project_with_groups.project_group_links.create!(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER) + project_with_groups.project_group_links.create!(group: developer_group, group_access: Gitlab::Access::DEVELOPER) + project_with_groups.project_group_links.create!(group: reporter_group, group_access: Gitlab::Access::REPORTER) end it 'moves the group links from one project to another' do @@ -30,8 +30,8 @@ RSpec.describe Projects::MoveProjectGroupLinksService do end it 'does not move existent group links in the current project' do - target_project.project_group_links.create(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER) - target_project.project_group_links.create(group: developer_group, group_access: Gitlab::Access::DEVELOPER) + target_project.project_group_links.create!(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER) + target_project.project_group_links.create!(group: developer_group, group_access: Gitlab::Access::DEVELOPER) expect(project_with_groups.project_group_links.count).to eq 3 expect(target_project.project_group_links.count).to eq 2 @@ -55,8 +55,8 @@ RSpec.describe Projects::MoveProjectGroupLinksService do let(:options) { { remove_remaining_elements: false } } it 'does not remove remaining project group links' do - target_project.project_group_links.create(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER) - target_project.project_group_links.create(group: developer_group, group_access: Gitlab::Access::DEVELOPER) + target_project.project_group_links.create!(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER) + target_project.project_group_links.create!(group: developer_group, group_access: Gitlab::Access::DEVELOPER) subject.execute(project_with_groups, options) diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb index 8a538bc67ed..018bfa8ef61 100644 --- a/spec/services/projects/operations/update_service_spec.rb +++ b/spec/services/projects/operations/update_service_spec.rb @@ -3,8 +3,8 @@ require 'spec_helper' RSpec.describe Projects::Operations::UpdateService do + let_it_be_with_refind(:project) { create(:project) } let_it_be(:user) { create(:user) } - let_it_be(:project, refind: true) { create(:project) } let(:result) { subject.execute } @@ -12,7 +12,7 @@ RSpec.describe Projects::Operations::UpdateService do describe '#execute' do context 'alerting setting' do - before do + before_all do project.add_maintainer(user) end @@ -430,5 +430,93 @@ RSpec.describe Projects::Operations::UpdateService do end end end + + context 'tracing setting' do + context 'with valid params' do + let(:params) do + { + tracing_setting_attributes: { + external_url: 'http://some-url.com' + } + } + end + + context 'with an existing setting' do + before do + create(:project_tracing_setting, project: project) + end + + shared_examples 'setting deletion' do + let!(:original_params) { params.deep_dup } + + it 'deletes the setting' do + expect(result[:status]).to eq(:success) + expect(project.reload.tracing_setting).to be_nil + end + + it 'does not modify original params' do + subject.execute + + expect(params).to eq(original_params) + end + end + + it 'updates the setting' do + expect(project.tracing_setting).not_to be_nil + + expect(result[:status]).to eq(:success) + expect(project.reload.tracing_setting.external_url) + .to eq('http://some-url.com') + end + + context 'with missing external_url' do + before do + params[:tracing_setting_attributes].delete(:external_url) + end + + it_behaves_like 'setting deletion' + end + + context 'with empty external_url' do + before do + params[:tracing_setting_attributes][:external_url] = '' + end + + it_behaves_like 'setting deletion' + end + + context 'with blank external_url' do + before do + params[:tracing_setting_attributes][:external_url] = ' ' + end + + it_behaves_like 'setting deletion' + end + end + + context 'without an existing setting' do + it 'creates a setting' do + expect(project.tracing_setting).to be_nil + + expect(result[:status]).to eq(:success) + expect(project.reload.tracing_setting.external_url) + .to eq('http://some-url.com') + end + end + end + + context 'with empty params' do + let(:params) { {} } + + let!(:tracing_setting) do + create(:project_tracing_setting, project: project) + end + + it 'does nothing' do + expect(result[:status]).to eq(:success) + expect(project.reload.tracing_setting).to eq(tracing_setting) + end + end + end end end diff --git a/spec/services/projects/overwrite_project_service_spec.rb b/spec/services/projects/overwrite_project_service_spec.rb index a03746d0271..cc6a863a11d 100644 --- a/spec/services/projects/overwrite_project_service_spec.rb +++ b/spec/services/projects/overwrite_project_service_spec.rb @@ -111,9 +111,9 @@ RSpec.describe Projects::OverwriteProjectService do create_list(:deploy_keys_project, 2, project: project_from) create_list(:notification_setting, 2, source: project_from) create_list(:users_star_project, 2, project: project_from) - project_from.project_group_links.create(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER) - project_from.project_group_links.create(group: developer_group, group_access: Gitlab::Access::DEVELOPER) - project_from.project_group_links.create(group: reporter_group, group_access: Gitlab::Access::REPORTER) + project_from.project_group_links.create!(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER) + project_from.project_group_links.create!(group: developer_group, group_access: Gitlab::Access::DEVELOPER) + project_from.project_group_links.create!(group: reporter_group, group_access: Gitlab::Access::REPORTER) project_from.add_maintainer(maintainer_user) project_from.add_developer(developer_user) project_from.add_reporter(reporter_user) 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/unlink_fork_service_spec.rb b/spec/services/projects/unlink_fork_service_spec.rb index 073e2e09397..2a8965e62ce 100644 --- a/spec/services/projects/unlink_fork_service_spec.rb +++ b/spec/services/projects/unlink_fork_service_spec.rb @@ -61,7 +61,7 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin context 'when the original project was deleted' do it 'does not fail when the original project is deleted' do source = forked_project.forked_from_project - source.destroy + source.destroy! forked_project.reload expect { subject.execute }.not_to raise_error diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index bfb3cbb0131..d3eb84a3137 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 @@ -59,6 +57,28 @@ RSpec.describe Projects::UpdatePagesService do end end + it 'creates pages_deployment and saves it in the metadata' do + expect do + expect(execute).to eq(:success) + end.to change { project.pages_deployments.count }.by(1) + + deployment = project.pages_deployments.last + + expect(deployment.size).to eq(file.size) + expect(deployment.file).to be + expect(project.pages_metadatum.reload.pages_deployment_id).to eq(deployment.id) + end + + it 'does not create deployment when zip_pages_deployments feature flag is disabled' do + stub_feature_flags(zip_pages_deployments: false) + + expect do + expect(execute).to eq(:success) + end.not_to change { project.pages_deployments.count } + + expect(project.pages_metadatum.reload.pages_deployment_id).to be_nil + end + it 'limits pages size' do stub_application_setting(max_pages_size: 1) expect(execute).not_to eq(:success) @@ -75,14 +95,14 @@ RSpec.describe Projects::UpdatePagesService do expect(project.pages_deployed?).to be_truthy expect(Dir.exist?(File.join(project.pages_path))).to be_truthy - project.destroy + project.destroy! expect(Dir.exist?(File.join(project.pages_path))).to be_falsey expect(ProjectPagesMetadatum.find_by_project_id(project)).to be_nil end it 'fails if sha on branch is not latest' do - build.update(ref: 'feature') + build.update!(ref: 'feature') expect(execute).not_to eq(:success) expect(project.pages_metadatum).not_to be_deployed @@ -104,10 +124,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 @@ -175,7 +191,7 @@ RSpec.describe Projects::UpdatePagesService do it 'fails to remove project pages when no pages is deployed' do expect(PagesWorker).not_to receive(:perform_in) expect(project.pages_deployed?).to be_falsey - project.destroy + project.destroy! end it 'fails if no artifacts' do diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb index 1de04888e0a..30530da8013 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 @@ -136,54 +123,36 @@ RSpec.describe Projects::UpdateRemoteMirrorService do stub_lfs_setting(enabled: true) end - context 'feature flag enabled' do - before do - stub_feature_flags(push_mirror_syncs_lfs: true) - end - - it 'pushes LFS objects to a HTTP repository' do - expect_next_instance_of(Lfs::PushService) do |service| - expect(service).to receive(:execute) - end - - execute! + it 'pushes LFS objects to a HTTP repository' do + expect_next_instance_of(Lfs::PushService) do |service| + expect(service).to receive(:execute) end - it 'does nothing to an SSH repository' do - remote_mirror.update!(url: 'ssh://example.com') - - expect_any_instance_of(Lfs::PushService).not_to receive(:execute) - - execute! - end + execute! + end - it 'does nothing if LFS is disabled' do - expect(project).to receive(:lfs_enabled?) { false } + it 'does nothing to an SSH repository' do + remote_mirror.update!(url: 'ssh://example.com') - expect_any_instance_of(Lfs::PushService).not_to receive(:execute) + expect_any_instance_of(Lfs::PushService).not_to receive(:execute) - execute! - end + execute! + end - it 'does nothing if non-password auth is specified' do - remote_mirror.update!(auth_method: 'ssh_public_key') + it 'does nothing if LFS is disabled' do + expect(project).to receive(:lfs_enabled?) { false } - expect_any_instance_of(Lfs::PushService).not_to receive(:execute) + expect_any_instance_of(Lfs::PushService).not_to receive(:execute) - execute! - end + execute! end - context 'feature flag disabled' do - before do - stub_feature_flags(push_mirror_syncs_lfs: false) - end + it 'does nothing if non-password auth is specified' do + remote_mirror.update!(auth_method: 'ssh_public_key') - it 'does nothing' do - expect_any_instance_of(Lfs::PushService).not_to receive(:execute) + expect_any_instance_of(Lfs::PushService).not_to receive(:execute) - execute! - end + execute! end end end diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 7832d727220..989426fde8b 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -141,7 +141,7 @@ RSpec.describe Projects::UpdateService do let(:group) { create(:group, visibility_level: Gitlab::VisibilityLevel::INTERNAL) } before do - project.update(namespace: group, visibility_level: group.visibility_level) + project.update!(namespace: group, visibility_level: group.visibility_level) end it 'does not update project visibility level' do @@ -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 @@ -230,7 +256,7 @@ RSpec.describe Projects::UpdateService do end it 'handles empty project feature attributes' do - project.project_feature.update(wiki_access_level: ProjectFeature::DISABLED) + project.project_feature.update!(wiki_access_level: ProjectFeature::DISABLED) result = update_project(project, user, { name: 'test1' }) @@ -241,7 +267,7 @@ RSpec.describe Projects::UpdateService do context 'when enabling a wiki' do it 'creates a wiki' do - project.project_feature.update(wiki_access_level: ProjectFeature::DISABLED) + project.project_feature.update!(wiki_access_level: ProjectFeature::DISABLED) TestEnv.rm_storage_dir(project.repository_storage, project.wiki.path) result = update_project(project, user, project_feature_attributes: { wiki_access_level: ProjectFeature::ENABLED }) @@ -252,7 +278,7 @@ RSpec.describe Projects::UpdateService do end it 'logs an error and creates a metric when wiki can not be created' do - project.project_feature.update(wiki_access_level: ProjectFeature::DISABLED) + project.project_feature.update!(wiki_access_level: ProjectFeature::DISABLED) expect_any_instance_of(ProjectWiki).to receive(:wiki).and_raise(Wiki::CouldNotCreateWikiError) expect_any_instance_of(described_class).to receive(:log_error).with("Could not create wiki for #{project.full_name}") diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index b970a48051f..6f3814095f9 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) @@ -226,7 +228,7 @@ RSpec.describe QuickActions::InterpretService do it 'returns the todo message' do _, _, message = service.execute(content, issuable) - expect(message).to eq('Added a To Do.') + expect(message).to eq('Added a to do.') end end @@ -242,7 +244,7 @@ RSpec.describe QuickActions::InterpretService do TodoService.new.mark_todo(issuable, developer) _, _, message = service.execute(content, issuable) - expect(message).to eq('Marked To Do as done.') + expect(message).to eq('Marked to do as done.') end end @@ -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/repositories/destroy_service_spec.rb b/spec/services/repositories/destroy_service_spec.rb index 30ec84b44e7..81bda2130a6 100644 --- a/spec/services/repositories/destroy_service_spec.rb +++ b/spec/services/repositories/destroy_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Repositories::DestroyService do let(:path) { repository.disk_path } let(:remove_path) { "#{path}+#{project.id}#{described_class::DELETED_FLAG}" } - subject { described_class.new(project.repository).execute } + subject { described_class.new(repository).execute } it 'moves the repository to a +deleted folder' do expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_truthy @@ -92,4 +92,22 @@ RSpec.describe Repositories::DestroyService do service.execute end end + + context 'with a project wiki repository' do + let(:project) { create(:project, :wiki_repo) } + let(:repository) { project.wiki.repository } + + it 'schedules the repository deletion' do + subject + + expect(Repositories::ShellDestroyService).to receive(:new).with(repository).and_call_original + + expect(GitlabShellWorker).to receive(:perform_in) + .with(Repositories::ShellDestroyService::REPO_REMOVAL_DELAY, :remove_repository, project.repository_storage, remove_path) + + # Because GitlabShellWorker is inside a run_after_commit callback we need to + # trigger the callback + project.touch + end + end end diff --git a/spec/services/repository_archive_clean_up_service_spec.rb b/spec/services/repository_archive_clean_up_service_spec.rb index c6d673fb1b5..8db1a6858fa 100644 --- a/spec/services/repository_archive_clean_up_service_spec.rb +++ b/spec/services/repository_archive_clean_up_service_spec.rb @@ -18,6 +18,16 @@ RSpec.describe RepositoryArchiveCleanUpService do end end + it 'removes outdated archives and directories in a versioned path' do + in_directory_with_files("project-#{non_existing_record_id}/#{sha}/@v2", %w[tar tar.bz2 tar.gz zip], 3.hours) do |dirname, files| + service.execute + + files.each { |filename| expect(File.exist?(filename)).to be_falsy } + expect(File.directory?(dirname)).to be_falsy + expect(File.directory?(File.dirname(dirname))).to be_falsy + end + end + it 'does not remove directories when they contain outdated non-archives' do in_directory_with_files("project-#{non_existing_record_id}/#{sha}", %w[tar conf rb], 2.hours) do |dirname, files| service.execute @@ -64,7 +74,9 @@ RSpec.describe RepositoryArchiveCleanUpService do end it 'removes files older than 2 hours that matches valid archive extensions' do - in_directory_with_files('sample.git', %w[tar tar.bz2 tar.gz zip], 2.hours) do |dir, files| + # In macOS, the the `mmin` parameter for `find` rounds up, so add a full + # minute to ensure these files are deemed old. + in_directory_with_files('sample.git', %w[tar tar.bz2 tar.gz zip], 121.minutes) do |dir, files| service.execute files.each { |file| expect(File.exist?(file)).to eq false } @@ -73,11 +85,11 @@ RSpec.describe RepositoryArchiveCleanUpService do end context 'with files older than 2 hours that does not matches valid archive extensions' do - it_behaves_like 'invalid archive files', 'sample.git', %w[conf rb], 2.hours + it_behaves_like 'invalid archive files', 'sample.git', %w[conf rb], 121.minutes end context 'with files older than 2 hours inside invalid directories' do - it_behaves_like 'invalid archive files', 'john/doe/sample.git', %w[conf rb tar tar.gz], 2.hours + it_behaves_like 'invalid archive files', 'john/t/doe/sample.git', %w[conf rb tar tar.gz], 121.minutes end context 'with files newer than 2 hours that matches valid archive extensions' do @@ -110,8 +122,6 @@ RSpec.describe RepositoryArchiveCleanUpService do def create_temporary_files(dir, extensions, mtime) FileUtils.mkdir_p(dir) - # rubocop: disable Rails/TimeZone - FileUtils.touch(extensions.map { |ext| File.join(dir, "sample.#{ext}") }, mtime: Time.now - mtime) - # rubocop: enable Rails/TimeZone + FileUtils.touch(extensions.map { |ext| File.join(dir, "sample.#{ext}") }, mtime: Time.now.utc - mtime) end end diff --git a/spec/services/resource_access_tokens/create_service_spec.rb b/spec/services/resource_access_tokens/create_service_spec.rb index 7dbd55a6909..d8b12cda632 100644 --- a/spec/services/resource_access_tokens/create_service_spec.rb +++ b/spec/services/resource_access_tokens/create_service_spec.rb @@ -24,26 +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 } - end - - it 'returns nil' do - expect(subject).to be nil - end - end - shared_examples 'allows creation of bot with valid params' do it { expect { subject }.to change { User.count }.by(1) } @@ -53,6 +33,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 @@ -77,8 +58,8 @@ RSpec.describe ResourceAccessTokens::CreateService do end context 'bot name' do - context 'when no value is passed' do - it 'uses default value' do + context 'when no name is passed' do + it 'uses default name' do response = subject access_token = response.payload[:access_token] @@ -86,10 +67,10 @@ RSpec.describe ResourceAccessTokens::CreateService do end end - context 'when user provides value' do + context 'when user provides name' do let_it_be(:params) { { name: 'Random bot' } } - it 'overrides the default value' do + it 'overrides the default name value' do response = subject access_token = response.payload[:access_token] @@ -121,7 +102,7 @@ RSpec.describe ResourceAccessTokens::CreateService do context 'when user provides scope explicitly' do let_it_be(:params) { { scopes: Gitlab::Auth::REPOSITORY_SCOPES } } - it 'overrides the default value' do + it 'overrides the default scope value' do response = subject access_token = response.payload[:access_token] @@ -130,24 +111,44 @@ RSpec.describe ResourceAccessTokens::CreateService do end context 'expires_at' do - context 'when no value is passed' do - it 'uses default value' do + context 'when no expiration value is passed' do + it 'uses nil expiration value' do response = subject access_token = response.payload[:access_token] expect(access_token.expires_at).to eq(nil) end + + context 'expiry of the project bot member' do + it 'project bot membership does not expire' do + response = subject + access_token = response.payload[:access_token] + project_bot = access_token.user + + expect(project.members.find_by(user_id: project_bot.id).expires_at).to eq(nil) + end + end end - context 'when user provides value' do + context 'when user provides expiration value' do let_it_be(:params) { { expires_at: Date.today + 1.month } } - it 'overrides the default value' do + it 'overrides the default expiration value' do response = subject access_token = response.payload[:access_token] expect(access_token.expires_at).to eq(params[:expires_at]) end + + context 'expiry of the project bot member' do + it 'sets the project bot to expire on the same day as the token' do + response = subject + access_token = response.payload[:access_token] + project_bot = access_token.user + + expect(project.members.find_by(user_id: project_bot.id).expires_at).to eq(params[:expires_at]) + end + end end context 'when invalid scope is passed' do @@ -164,7 +165,7 @@ RSpec.describe ResourceAccessTokens::CreateService do context 'when access provisioning fails' do before do - allow(resource).to receive(:add_maintainer).and_return(nil) + allow(resource).to receive(:add_user).and_return(nil) end it 'returns error' do @@ -180,8 +181,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 before_all do diff --git a/spec/services/resource_access_tokens/revoke_service_spec.rb b/spec/services/resource_access_tokens/revoke_service_spec.rb index ffc06d770f8..af29ee2a721 100644 --- a/spec/services/resource_access_tokens/revoke_service_spec.rb +++ b/spec/services/resource_access_tokens/revoke_service_spec.rb @@ -8,17 +8,17 @@ RSpec.describe ResourceAccessTokens::RevokeService do let_it_be(:user) { create(:user) } let(:access_token) { create(:personal_access_token, user: resource_bot) } - describe '#execute' do + describe '#execute', :sidekiq_inline do # Created shared_examples as it will easy to include specs for group bots in https://gitlab.com/gitlab-org/gitlab/-/issues/214046 shared_examples 'revokes access token' do it { expect(subject.success?).to be true } - it { expect(subject.message).to eq("Revoked access token: #{access_token.name}") } + it { expect(subject.message).to eq("Access token #{access_token.name} has been revoked and the bot user has been scheduled for deletion.") } - it 'revokes token access' do - subject + it 'calls delete user worker' do + expect(DeleteUserWorker).to receive(:perform_async).with(user.id, resource_bot.id, skip_authorization: true) - expect(access_token.reload.revoked?).to be true + subject end it 'removes membership of bot user' do @@ -34,6 +34,12 @@ RSpec.describe ResourceAccessTokens::RevokeService do expect(issue.reload.author.ghost?).to be true end + + it 'deletes project bot user' do + subject + + expect(User.exists?(resource_bot.id)).to be_falsy + end end shared_examples 'rollback revoke steps' do @@ -56,49 +62,71 @@ RSpec.describe ResourceAccessTokens::RevokeService do expect(issue.reload.author.ghost?).to be false end + + it 'does not destroy project bot user' do + subject + + expect(User.exists?(resource_bot.id)).to be_truthy + end end context 'when resource is a project' do let_it_be(:resource) { create(:project, :private) } - let_it_be(:resource_bot) { create(:user, :project_bot) } + let(:resource_bot) { create(:user, :project_bot) } - before_all do + before do resource.add_maintainer(user) resource.add_maintainer(resource_bot) end it_behaves_like 'revokes access token' - context 'when revoke fails' do - context 'invalid resource type' do - subject { described_class.new(user, resource, access_token).execute } + context 'revoke fails' do + let_it_be(:other_user) { create(:user) } - let_it_be(:resource) { double } - let_it_be(:resource_bot) { create(:user, :project_bot) } + context 'when access token does not belong to this project' do + it 'does not find the bot' do + other_access_token = create(:personal_access_token, user: other_user) - it 'returns error response' do - response = subject + response = described_class.new(user, resource, other_access_token).execute expect(response.success?).to be false expect(response.message).to eq("Failed to find bot user") + expect(access_token.reload.revoked?).to be false end - - it { expect { subject }.not_to change(access_token.reload, :revoked) } end - context 'when migration to ghost user fails' do - before do - allow_next_instance_of(::Members::DestroyService) do |service| - allow(service).to receive(:execute).and_return(false) + context 'when user does not have permission to destroy bot' do + context 'when non-project member tries to delete project bot' do + it 'does not allow other user to delete bot' do + response = described_class.new(other_user, resource, access_token).execute + + expect(response.success?).to be false + expect(response.message).to eq("#{other_user.name} cannot delete #{access_token.user.name}") + expect(access_token.reload.revoked?).to be false end end - it_behaves_like 'rollback revoke steps' + context 'when non-maintainer project member tries to delete project bot' do + let(:developer) { create(:user) } + + before do + resource.add_developer(developer) + end + + it 'does not allow developer to delete bot' do + response = described_class.new(developer, resource, access_token).execute + + expect(response.success?).to be false + expect(response.message).to eq("#{developer.name} cannot delete #{access_token.user.name}") + expect(access_token.reload.revoked?).to be false + end + end end - context 'when migration to ghost user fails' do + context 'when deletion of bot user fails' do before do - allow_next_instance_of(::Users::MigrateToGhostUserService) do |service| + allow_next_instance_of(::ResourceAccessTokens::RevokeService) do |service| allow(service).to receive(:execute).and_return(false) end end diff --git a/spec/services/search/global_service_spec.rb b/spec/services/search/global_service_spec.rb index 90ad18e5571..7b914a4d3d6 100644 --- a/spec/services/search/global_service_spec.rb +++ b/spec/services/search/global_service_spec.rb @@ -52,4 +52,34 @@ RSpec.describe Search::GlobalService do end end end + + context 'issues' do + let(:scope) { 'issues' } + + context 'sort by created_at' do + let!(:project) { create(:project, :public) } + let!(:old_result) { create(:issue, project: project, title: 'sorted old', created_at: 1.month.ago) } + let!(:new_result) { create(:issue, project: project, title: 'sorted recent', created_at: 1.day.ago) } + let!(:very_old_result) { create(:issue, project: project, title: 'sorted very old', created_at: 1.year.ago) } + + include_examples 'search results sorted' do + let(:results) { described_class.new(nil, search: 'sorted', sort: sort).execute } + end + end + end + + context 'merge_request' do + let(:scope) { 'merge_requests' } + + context 'sort by created_at' do + let!(:project) { create(:project, :public) } + let!(:old_result) { create(:merge_request, :opened, source_project: project, source_branch: 'old-1', title: 'sorted old', created_at: 1.month.ago) } + let!(:new_result) { create(:merge_request, :opened, source_project: project, source_branch: 'new-1', title: 'sorted recent', created_at: 1.day.ago) } + let!(:very_old_result) { create(:merge_request, :opened, source_project: project, source_branch: 'very-old-1', title: 'sorted very old', created_at: 1.year.ago) } + + include_examples 'search results sorted' do + let(:results) { described_class.new(nil, search: 'sorted', sort: sort).execute } + end + end + end end diff --git a/spec/services/search/group_service_spec.rb b/spec/services/search/group_service_spec.rb index d3026d158d4..2bfe714f393 100644 --- a/spec/services/search/group_service_spec.rb +++ b/spec/services/search/group_service_spec.rb @@ -40,4 +40,36 @@ RSpec.describe Search::GroupService do describe 'basic search' do include_examples 'group search' end + + context 'issues' do + let(:scope) { 'issues' } + + context 'sort by created_at' do + let!(:group) { create(:group) } + let!(:project) { create(:project, :public, group: group) } + let!(:old_result) { create(:issue, project: project, title: 'sorted old', created_at: 1.month.ago) } + let!(:new_result) { create(:issue, project: project, title: 'sorted recent', created_at: 1.day.ago) } + let!(:very_old_result) { create(:issue, project: project, title: 'sorted very old', created_at: 1.year.ago) } + + include_examples 'search results sorted' do + let(:results) { described_class.new(nil, group, search: 'sorted', sort: sort).execute } + end + end + end + + context 'merge requests' do + let(:scope) { 'merge_requests' } + + context 'sort by created_at' do + let!(:group) { create(:group) } + let!(:project) { create(:project, :public, group: group) } + let!(:old_result) { create(:merge_request, :opened, source_project: project, source_branch: 'old-1', title: 'sorted old', created_at: 1.month.ago) } + let!(:new_result) { create(:merge_request, :opened, source_project: project, source_branch: 'new-1', title: 'sorted recent', created_at: 1.day.ago) } + let!(:very_old_result) { create(:merge_request, :opened, source_project: project, source_branch: 'very-old-1', title: 'sorted very old', created_at: 1.year.ago) } + + include_examples 'search results sorted' do + let(:results) { described_class.new(nil, group, search: 'sorted', sort: sort).execute } + end + end + end end 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/repository_validation_service_spec.rb b/spec/services/snippets/repository_validation_service_spec.rb index e2a0d0faa18..8166ce144e1 100644 --- a/spec/services/snippets/repository_validation_service_spec.rb +++ b/spec/services/snippets/repository_validation_service_spec.rb @@ -40,7 +40,7 @@ RSpec.describe Snippets::RepositoryValidationService do end it 'returns error when the repository has more file than the limit' do - limit = Snippet.max_file_limit(user) + 1 + limit = Snippet.max_file_limit + 1 files = Array.new(limit) { FFaker::Filesystem.file_name } allow(repository).to receive(:ls_files).and_return(files) @@ -56,7 +56,9 @@ RSpec.describe Snippets::RepositoryValidationService do end it 'returns error when the repository size is over the limit' do - expect_any_instance_of(Gitlab::RepositorySizeChecker).to receive(:above_size_limit?).and_return(true) + expect_next_instance_of(Gitlab::RepositorySizeChecker) do |checker| + expect(checker).to receive(:above_size_limit?).and_return(true) + end expect(subject).to be_error expect(subject.message).to match /Repository size is above the limit/ 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/system_notes/time_tracking_service_spec.rb b/spec/services/system_notes/time_tracking_service_spec.rb index f671e66cdcd..ec126cb5447 100644 --- a/spec/services/system_notes/time_tracking_service_spec.rb +++ b/spec/services/system_notes/time_tracking_service_spec.rb @@ -6,124 +6,181 @@ RSpec.describe ::SystemNotes::TimeTrackingService do let_it_be(:author) { create(:user) } let_it_be(:project) { create(:project, :repository) } - let(:noteable) { create(:issue, project: project) } - describe '#change_due_date' do subject { described_class.new(noteable: noteable, project: project, author: author).change_due_date(due_date) } let(:due_date) { Date.today } - it_behaves_like 'a note with overridable created_at' + context 'when noteable is an issue' do + let_it_be(:noteable) { create(:issue, project: project) } - it_behaves_like 'a system note' do - let(:action) { 'due_date' } - end + it_behaves_like 'a note with overridable created_at' + + it_behaves_like 'a system note' do + let(:action) { 'due_date' } + end - context 'when due date added' do - it 'sets the note text' do - expect(subject.note).to eq "changed due date to #{due_date.to_s(:long)}" + context 'when due date added' do + it 'sets the note text' do + expect(subject.note).to eq "changed due date to #{due_date.to_s(:long)}" + end + end + + context 'when due date removed' do + let(:due_date) { nil } + + it 'sets the note text' do + expect(subject.note).to eq 'removed due date' + end + end + + it 'tracks the issue event in usage ping' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_due_date_changed_action).with(author: author) + + subject end end - context 'when due date removed' do - let(:due_date) { nil } + context 'when noteable is a merge request' do + let_it_be(:noteable) { create(:merge_request, source_project: project) } + + it 'does not track the issue event in usage ping' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_due_date_changed_action).with(author: author) - it 'sets the note text' do - expect(subject.note).to eq 'removed due date' + subject end end end - describe '.change_time_estimate' do + describe '#change_time_estimate' do subject { described_class.new(noteable: noteable, project: project, author: author).change_time_estimate } - it_behaves_like 'a system note' do - let(:action) { 'time_tracking' } - end - - context 'with a time estimate' do - it 'sets the note text' do - noteable.update_attribute(:time_estimate, 277200) + context 'when noteable is an issue' do + let_it_be(:noteable, reload: true) { create(:issue, project: project) } - expect(subject.note).to eq "changed time estimate to 1w 4d 5h" + it_behaves_like 'a system note' do + let(:action) { 'time_tracking' } end - context 'when time_tracking_limit_to_hours setting is true' do - before do - stub_application_setting(time_tracking_limit_to_hours: true) - end - + context 'with a time estimate' do it 'sets the note text' do noteable.update_attribute(:time_estimate, 277200) - expect(subject.note).to eq "changed time estimate to 77h" + expect(subject.note).to eq "changed time estimate to 1w 4d 5h" + end + + context 'when time_tracking_limit_to_hours setting is true' do + before do + stub_application_setting(time_tracking_limit_to_hours: true) + end + + it 'sets the note text' do + noteable.update_attribute(:time_estimate, 277200) + + expect(subject.note).to eq "changed time estimate to 77h" + end end end - end - context 'without a time estimate' do - it 'sets the note text' do - expect(subject.note).to eq "removed time estimate" + context 'without a time estimate' do + it 'sets the note text' do + expect(subject.note).to eq "removed time estimate" + end end - end - end - describe '.change_time_spent' do - # We need a custom noteable in order to the shared examples to be green. - let(:noteable) do - mr = create(:merge_request, source_project: project) - mr.spend_time(duration: 360000, user_id: author.id) - mr.save! - mr - end + it 'tracks the issue event in usage ping' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_time_estimate_changed_action).with(author: author) - subject do - described_class.new(noteable: noteable, project: project, author: author).change_time_spent + subject + end end - it_behaves_like 'a system note' do - let(:action) { 'time_tracking' } - end + context 'when noteable is a merge request' do + let_it_be(:noteable) { create(:merge_request, source_project: project) } - context 'when time was added' do - it 'sets the note text' do - spend_time!(277200) + it 'does not track the issue event in usage ping' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_time_estimate_changed_action).with(author: author) - expect(subject.note).to eq "added 1w 4d 5h of time spent" + subject end end + end - context 'when time was subtracted' do - it 'sets the note text' do - spend_time!(-277200) + describe '#change_time_spent' do + subject { described_class.new(noteable: noteable, project: project, author: author).change_time_spent } - expect(subject.note).to eq "subtracted 1w 4d 5h of time spent" - end - end + context 'when noteable is an issue' do + let_it_be(:noteable, reload: true) { create(:issue, project: project) } - context 'when time was removed' do - it 'sets the note text' do - spend_time!(:reset) + it_behaves_like 'a system note' do + let(:action) { 'time_tracking' } - expect(subject.note).to eq "removed time spent" + before do + spend_time!(277200) + end end - end - context 'when time_tracking_limit_to_hours setting is true' do - before do - stub_application_setting(time_tracking_limit_to_hours: true) + context 'when time was added' do + it 'sets the note text' do + spend_time!(277200) + + expect(subject.note).to eq "added 1w 4d 5h of time spent" + end + + context 'when time was subtracted' do + it 'sets the note text' do + spend_time!(360000) + spend_time!(-277200) + + expect(subject.note).to eq "subtracted 1w 4d 5h of time spent" + end + end + + context 'when time was removed' do + it 'sets the note text' do + spend_time!(:reset) + + expect(subject.note).to eq "removed time spent" + end + end + + context 'when time_tracking_limit_to_hours setting is true' do + before do + stub_application_setting(time_tracking_limit_to_hours: true) + end + + it 'sets the note text' do + spend_time!(277200) + + expect(subject.note).to eq "added 77h of time spent" + end + end + + it 'tracks the issue event in usage ping' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_time_spent_changed_action).with(author: author) + + spend_time!(277200) + + subject + end end - it 'sets the note text' do - spend_time!(277200) + context 'when noteable is a merge request' do + let_it_be(:noteable) { create(:merge_request, source_project: project) } + + it 'does not track the issue event in usage ping' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_time_estimate_changed_action).with(author: author) + + spend_time!(277200) - expect(subject.note).to eq "added 77h of time spent" + subject + end end - end - def spend_time!(seconds) - noteable.spend_time(duration: seconds, user_id: author.id) - noteable.save! + def spend_time!(seconds) + noteable.spend_time(duration: seconds, user_id: author.id) + noteable.save! + end end end end diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb index 921037bd5db..4126eb88b0b 100644 --- a/spec/services/todos/destroy/entity_leave_service_spec.rb +++ b/spec/services/todos/destroy/entity_leave_service_spec.rb @@ -19,20 +19,14 @@ RSpec.describe Todos::Destroy::EntityLeaveService do let!(:todo_issue_c_user) { create(:todo, user: user, target: issue_c, project: project) } let!(:todo_issue_c_user2) { create(:todo, user: user2, target: issue_c, project: project) } - shared_examples 'using different access permissions' do |access_table| - using RSpec::Parameterized::TableSyntax - - where(:group_access, :project_access, :c_todos, :mr_todos, :method, &access_table) - - with_them do - before do - set_access(project, user, project_access) if project_access - set_access(group, user, group_access) if group_access - end + shared_examples 'using different access permissions' do + before do + set_access(project, user, project_access) if project_access + set_access(group, user, group_access) if group_access + end - it "#{params[:method].to_s.humanize(capitalize: false)}" do - send(method) - end + it "#{params[:method].to_s.humanize(capitalize: false)}" do + send(method_name) end end @@ -84,22 +78,20 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end context 'access permissions' do - # rubocop:disable RSpec/LeakyConstantDeclaration - PRIVATE_PROJECT_PRIVATE_GROUP_ACCESS_TABLE = - lambda do |_| - [ - # :group_access, :project_access, :c_todos, :mr_todos, :method - [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], - [nil, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], - [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], - [:guest, nil, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], - [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], - [:guest, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos] - ] - end - # rubocop:enable RSpec/LeakyConstantDeclaration + where(:group_access, :project_access, :method_name) do + [ + [nil, :reporter, :does_not_remove_any_todos], + [nil, :guest, :removes_confidential_issues_and_merge_request_todos], + [:reporter, nil, :does_not_remove_any_todos], + [:guest, nil, :removes_confidential_issues_and_merge_request_todos], + [:guest, :reporter, :does_not_remove_any_todos], + [:guest, :guest, :removes_confidential_issues_and_merge_request_todos] + ] + end - it_behaves_like 'using different access permissions', PRIVATE_PROJECT_PRIVATE_GROUP_ACCESS_TABLE + with_them do + it_behaves_like 'using different access permissions' + end end end @@ -117,22 +109,20 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end context 'access permissions' do - # rubocop:disable RSpec/LeakyConstantDeclaration - PRIVATE_PROJECT_INTERNAL_GROUP_ACCESS_TABLE = - lambda do |_| - [ - # :group_access, :project_access, :c_todos, :mr_todos, :method - [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], - [nil, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], - [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], - [:guest, nil, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], - [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], - [:guest, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos] - ] - end - # rubocop:enable RSpec/LeakyConstantDeclaration + where(:group_access, :project_access, :method_name) do + [ + [nil, :reporter, :does_not_remove_any_todos], + [nil, :guest, :removes_confidential_issues_and_merge_request_todos], + [:reporter, nil, :does_not_remove_any_todos], + [:guest, nil, :removes_confidential_issues_and_merge_request_todos], + [:guest, :reporter, :does_not_remove_any_todos], + [:guest, :guest, :removes_confidential_issues_and_merge_request_todos] + ] + end - it_behaves_like 'using different access permissions', PRIVATE_PROJECT_INTERNAL_GROUP_ACCESS_TABLE + with_them do + it_behaves_like 'using different access permissions' + end end end @@ -172,22 +162,20 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end context 'access permissions' do - # rubocop:disable RSpec/LeakyConstantDeclaration - INTERNAL_PROJECT_INTERNAL_GROUP_ACCESS_TABLE = - lambda do |_| - [ - # :group_access, :project_access, :c_todos, :mr_todos, :method - [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], - [nil, :guest, :delete, :keep, :removes_only_confidential_issues_todos], - [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], - [:guest, nil, :delete, :keep, :removes_only_confidential_issues_todos], - [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], - [:guest, :guest, :delete, :keep, :removes_only_confidential_issues_todos] - ] - end - # rubocop:enable RSpec/LeakyConstantDeclaration - - it_behaves_like 'using different access permissions', INTERNAL_PROJECT_INTERNAL_GROUP_ACCESS_TABLE + where(:group_access, :project_access, :method_name) do + [ + [nil, :reporter, :does_not_remove_any_todos], + [nil, :guest, :removes_only_confidential_issues_todos], + [:reporter, nil, :does_not_remove_any_todos], + [:guest, nil, :removes_only_confidential_issues_todos], + [:guest, :reporter, :does_not_remove_any_todos], + [:guest, :guest, :removes_only_confidential_issues_todos] + ] + end + + with_them do + it_behaves_like 'using different access permissions' + end end end @@ -219,22 +207,20 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end context 'access permissions' do - # rubocop:disable RSpec/LeakyConstantDeclaration - PRIVATE_GROUP_PRIVATE_PROJECT_ACCESS_TABLE = - lambda do |_| - [ - # :group_access, :project_access, :c_todos, :mr_todos, :method - [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], - [nil, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], - [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], - [:guest, nil, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], - [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], - [:guest, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos] - ] - end - # rubocop:enable RSpec/LeakyConstantDeclaration + where(:group_access, :project_access, :method_name) do + [ + [nil, :reporter, :does_not_remove_any_todos], + [nil, :guest, :removes_confidential_issues_and_merge_request_todos], + [:reporter, nil, :does_not_remove_any_todos], + [:guest, nil, :removes_confidential_issues_and_merge_request_todos], + [:guest, :reporter, :does_not_remove_any_todos], + [:guest, :guest, :removes_confidential_issues_and_merge_request_todos] + ] + end - it_behaves_like 'using different access permissions', PRIVATE_GROUP_PRIVATE_PROJECT_ACCESS_TABLE + with_them do + it_behaves_like 'using different access permissions' + end end context 'with nested groups' do @@ -320,23 +306,21 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end context 'access permissions' do - # rubocop:disable RSpec/LeakyConstantDeclaration - INTERNAL_GROUP_INTERNAL_PROJECT_ACCESS_TABLE = - lambda do |_| - [ - # :group_access, :project_access, :c_todos, :mr_todos, :method - [nil, nil, :delete, :keep, :removes_only_confidential_issues_todos], - [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], - [nil, :guest, :delete, :keep, :removes_only_confidential_issues_todos], - [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], - [:guest, nil, :delete, :keep, :removes_only_confidential_issues_todos], - [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], - [:guest, :guest, :delete, :keep, :removes_only_confidential_issues_todos] - ] - end - # rubocop:enable RSpec/LeakyConstantDeclaration + where(:group_access, :project_access, :method_name) do + [ + [nil, nil, :removes_only_confidential_issues_todos], + [nil, :reporter, :does_not_remove_any_todos], + [nil, :guest, :removes_only_confidential_issues_todos], + [:reporter, nil, :does_not_remove_any_todos], + [:guest, nil, :removes_only_confidential_issues_todos], + [:guest, :reporter, :does_not_remove_any_todos], + [:guest, :guest, :removes_only_confidential_issues_todos] + ] + end - it_behaves_like 'using different access permissions', INTERNAL_GROUP_INTERNAL_PROJECT_ACCESS_TABLE + with_them do + it_behaves_like 'using different access permissions' + end end end end diff --git a/spec/services/users/approve_service_spec.rb b/spec/services/users/approve_service_spec.rb new file mode 100644 index 00000000000..50f2b6b0827 --- /dev/null +++ b/spec/services/users/approve_service_spec.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::ApproveService do + let_it_be(:current_user) { create(:admin) } + let(:user) { create(:user, :blocked_pending_approval) } + + subject(:execute) { described_class.new(current_user).execute(user) } + + describe '#execute' do + context 'failures' do + context 'when the executor user is not allowed to approve users' do + let(:current_user) { create(:user) } + + it 'returns error result' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to match(/You are not allowed to approve a user/) + end + end + + context 'when user is not in pending approval state' do + let(:user) { create(:user, state: 'active') } + + it 'returns error result' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]) + .to match(/The user you are trying to approve is not pending an approval/) + end + end + + context 'when user cannot be activated' do + let(:user) do + build(:user, state: 'blocked_pending_approval', email: 'invalid email') + end + + it 'returns error result' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to match(/Email is invalid/) + end + + it 'does not change the state of the user' do + expect { subject }.not_to change { user.state } + end + end + end + + context 'success' do + it 'activates the user' do + expect(subject[:status]).to eq(:success) + expect(user.reload).to be_active + end + + context 'email confirmation status' do + context 'user is unconfirmed' do + let(:user) { create(:user, :blocked_pending_approval, :unconfirmed) } + + it 'sends confirmation instructions' do + expect { subject } + .to have_enqueued_mail(DeviseMailer, :confirmation_instructions) + end + end + + context 'user is confirmed' do + it 'does not send a confirmation email' do + expect { subject } + .not_to have_enqueued_mail(DeviseMailer, :confirmation_instructions) + end + end + end + + context 'pending invitiations' do + let!(:project_member_invite) { create(:project_member, :invited, invite_email: user.email) } + let!(:group_member_invite) { create(:group_member, :invited, invite_email: user.email) } + + context 'user is unconfirmed' do + let(:user) { create(:user, :blocked_pending_approval, :unconfirmed) } + + it 'does not accept pending invites of the user' do + expect(subject[:status]).to eq(:success) + + group_member_invite.reload + project_member_invite.reload + + expect(group_member_invite).to be_invite + expect(project_member_invite).to be_invite + end + end + + context 'user is confirmed' do + it 'accepts pending invites of the user' do + expect(subject[:status]).to eq(:success) + + group_member_invite.reload + project_member_invite.reload + + expect(group_member_invite).not_to be_invite + expect(project_member_invite).not_to be_invite + expect(group_member_invite.user).to eq(user) + expect(project_member_invite.user).to eq(user) + end + end + end + end + end +end diff --git a/spec/services/users/block_service_spec.rb b/spec/services/users/block_service_spec.rb index e170a5494aa..45a5b1e5100 100644 --- a/spec/services/users/block_service_spec.rb +++ b/spec/services/users/block_service_spec.rb @@ -34,5 +34,15 @@ RSpec.describe Users::BlockService do expect { operation }.not_to change { user.state } end end + + context 'when internal user' do + let(:user) { create(:user, :bot) } + + it 'returns error result' do + expect(operation[:status]).to eq(:error) + expect(operation[:message]).to eq('An internal user cannot be blocked') + expect(operation[:http_status]).to eq(403) + end + end end end diff --git a/spec/services/users/build_service_spec.rb b/spec/services/users/build_service_spec.rb index c14fdb35bfa..446741221b3 100644 --- a/spec/services/users/build_service_spec.rb +++ b/spec/services/users/build_service_spec.rb @@ -4,11 +4,11 @@ require 'spec_helper' RSpec.describe Users::BuildService do describe '#execute' do - let(:params) do - { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass' } - end + let(:params) { build_stubbed(:user).slice(:first_name, :last_name, :username, :email, :password) } context 'with an admin user' do + let(:params) { build_stubbed(:user).slice(:name, :username, :email, :password) } + let(:admin_user) { create(:admin) } let(:service) { described_class.new(admin_user, ActionController::Parameters.new(params).permit!) } @@ -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 diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index ff919257b3c..6de685dd89a 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -234,6 +234,14 @@ RSpec.describe Users::DestroyService do expect(User.exists?(user.id)).to be(false) end + + it 'allows user to be deleted if skip_authorization: true' do + other_user = create(:user) + + described_class.new(user).execute(other_user, skip_authorization: true) + + expect(User.exists?(other_user.id)).to be(false) + end end context "migrating associated records" do diff --git a/spec/services/users/validate_otp_service_spec.rb b/spec/services/users/validate_otp_service_spec.rb new file mode 100644 index 00000000000..826755d6145 --- /dev/null +++ b/spec/services/users/validate_otp_service_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::ValidateOtpService do + let_it_be(:user) { create(:user) } + let(:otp_code) { 42 } + + subject(:validate) { described_class.new(user).execute(otp_code) } + + context 'Devise' do + it 'calls Devise strategy' do + expect_next_instance_of(::Gitlab::Auth::Otp::Strategies::Devise) do |strategy| + expect(strategy).to receive(:validate).with(otp_code).once + end + + validate + end + end + + context 'FortiAuthenticator' do + before do + stub_feature_flags(forti_authenticator: true) + end + + it 'calls FortiAuthenticator strategy' do + expect_next_instance_of(::Gitlab::Auth::Otp::Strategies::FortiAuthenticator) do |strategy| + expect(strategy).to receive(:validate).with(otp_code).once + end + + validate + end + end +end diff --git a/spec/services/web_hooks/destroy_service_spec.rb b/spec/services/web_hooks/destroy_service_spec.rb new file mode 100644 index 00000000000..fda40eb01e2 --- /dev/null +++ b/spec/services/web_hooks/destroy_service_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WebHooks::DestroyService do + let_it_be(:user) { create(:user) } + + subject { described_class.new(user) } + + shared_examples 'batched destroys' do + it 'destroys all hooks in batches' do + stub_const("#{described_class}::BATCH_SIZE", 1) + expect(subject).to receive(:delete_web_hook_logs_in_batches).exactly(4).times.and_call_original + + expect do + status = subject.execute(hook) + expect(status[:async]).to be false + end + .to change { WebHook.count }.from(1).to(0) + .and change { WebHookLog.count }.from(3).to(0) + end + + it 'returns an error if sync destroy fails' do + expect(hook).to receive(:destroy).and_return(false) + + result = subject.sync_destroy(hook) + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq("Unable to destroy #{hook.model_name.human}") + end + + it 'schedules an async delete' do + stub_const('WebHooks::DestroyService::LOG_COUNT_THRESHOLD', 1) + + expect(WebHooks::DestroyWorker).to receive(:perform_async).with(user.id, hook.id).and_call_original + + status = subject.execute(hook) + + expect(status[:async]).to be true + end + end + + context 'with system hook' do + let_it_be(:hook) { create(:system_hook, url: "http://example.com") } + let_it_be(:log) { create_list(:web_hook_log, 3, web_hook: hook) } + + it_behaves_like 'batched destroys' + end + + context 'with project hook' do + let_it_be(:hook) { create(:project_hook) } + let_it_be(:log) { create_list(:web_hook_log, 3, web_hook: hook) } + + it_behaves_like 'batched destroys' + end +end |