diff options
Diffstat (limited to 'spec/services')
105 files changed, 2746 insertions, 871 deletions
diff --git a/spec/services/activity_pub/projects/releases_follow_service_spec.rb b/spec/services/activity_pub/projects/releases_follow_service_spec.rb new file mode 100644 index 00000000000..6d0d400b9c6 --- /dev/null +++ b/spec/services/activity_pub/projects/releases_follow_service_spec.rb @@ -0,0 +1,145 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ActivityPub::Projects::ReleasesFollowService, feature_category: :release_orchestration do + let_it_be(:project) { create(:project, :public) } + let_it_be_with_reload(:existing_subscription) { create(:activity_pub_releases_subscription, project: project) } + + describe '#execute' do + let(:service) { described_class.new(project, payload) } + let(:payload) { nil } + + before do + allow(ActivityPub::Projects::ReleasesSubscriptionWorker).to receive(:perform_async) + end + + context 'with a valid payload' do + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://example.com/new-actor#follow-1', + type: 'Follow', + actor: actor, + object: 'https://localhost/our/project/-/releases' + }.with_indifferent_access + end + + let(:actor) { 'https://example.com/new-actor' } + + context 'when there is no subscription for that actor' do + before do + allow(ActivityPub::ReleasesSubscription).to receive(:find_by_project_and_subscriber).and_return(nil) + end + + it 'sets the subscriber url' do + service.execute + expect(ActivityPub::ReleasesSubscription.last.subscriber_url).to eq 'https://example.com/new-actor' + end + + it 'sets the payload' do + service.execute + expect(ActivityPub::ReleasesSubscription.last.payload).to eq payload + end + + it 'sets the project' do + service.execute + expect(ActivityPub::ReleasesSubscription.last.project_id).to eq project.id + end + + it 'saves the subscription' do + expect { service.execute }.to change { ActivityPub::ReleasesSubscription.count }.by(1) + end + + it 'queues the subscription job' do + service.execute + expect(ActivityPub::Projects::ReleasesSubscriptionWorker).to have_received(:perform_async) + end + + it 'returns true' do + expect(service.execute).to be_truthy + end + end + + context 'when there is already a subscription for that actor' do + before do + allow(ActivityPub::ReleasesSubscription).to receive(:find_by_project_and_subscriber) { existing_subscription } + end + + it 'does not save the subscription' do + expect { service.execute }.not_to change { ActivityPub::ReleasesSubscription.count } + end + + it 'does not queue the subscription job' do + service.execute + expect(ActivityPub::Projects::ReleasesSubscriptionWorker).not_to have_received(:perform_async) + end + + it 'returns true' do + expect(service.execute).to be_truthy + end + end + end + + shared_examples 'invalid follow request' do + it 'does not save the subscription' do + expect { service.execute }.not_to change { ActivityPub::ReleasesSubscription.count } + end + + it 'does not queue the subscription job' do + service.execute + expect(ActivityPub::Projects::ReleasesSubscriptionWorker).not_to have_received(:perform_async) + end + + it 'sets an error' do + service.execute + expect(service.errors).not_to be_empty + end + + it 'returns false' do + expect(service.execute).to be_falsey + end + end + + context 'when actor is missing' do + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://example.com/new-actor', + type: 'Follow', + object: 'https://localhost/our/project/-/releases' + }.with_indifferent_access + end + + it_behaves_like 'invalid follow request' + end + + context 'when actor is an object with no id attribute' do + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://example.com/new-actor', + type: 'Follow', + actor: { type: 'Person' }, + object: 'https://localhost/our/project/-/releases' + }.with_indifferent_access + end + + it_behaves_like 'invalid follow request' + end + + context 'when actor is neither a string nor an object' do + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://example.com/new-actor', + type: 'Follow', + actor: 27.13, + object: 'https://localhost/our/project/-/releases' + }.with_indifferent_access + end + + it_behaves_like 'invalid follow request' + end + end +end diff --git a/spec/services/activity_pub/projects/releases_unfollow_service_spec.rb b/spec/services/activity_pub/projects/releases_unfollow_service_spec.rb new file mode 100644 index 00000000000..c732d82a2ad --- /dev/null +++ b/spec/services/activity_pub/projects/releases_unfollow_service_spec.rb @@ -0,0 +1,152 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ActivityPub::Projects::ReleasesUnfollowService, feature_category: :release_orchestration do + let_it_be(:project) { create(:project, :public) } + let_it_be_with_reload(:existing_subscription) { create(:activity_pub_releases_subscription, project: project) } + + describe '#execute' do + let(:service) { described_class.new(project, payload) } + let(:payload) { nil } + + context 'with a valid payload' do + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://example.com/new-actor#unfollow-1', + type: 'Undo', + actor: actor, + object: { + id: 'https://example.com/new-actor#follow-1', + type: 'Follow', + actor: actor, + object: 'https://localhost/our/project/-/releases' + } + }.with_indifferent_access + end + + let(:actor) { existing_subscription.subscriber_url } + + context 'when there is a subscription for this actor' do + it 'deletes the subscription' do + service.execute + expect(ActivityPub::ReleasesSubscription.where(id: existing_subscription.id).first).to be_nil + end + + it 'returns true' do + expect(service.execute).to be_truthy + end + end + + context 'when there is no subscription for this actor' do + before do + allow(ActivityPub::ReleasesSubscription).to receive(:find_by_project_and_subscriber).and_return(nil) + end + + it 'does not delete anything' do + expect { service.execute }.not_to change { ActivityPub::ReleasesSubscription.count } + end + + it 'returns true' do + expect(service.execute).to be_truthy + end + end + end + + shared_examples 'invalid unfollow request' do + it 'does not delete anything' do + expect { service.execute }.not_to change { ActivityPub::ReleasesSubscription.count } + end + + it 'sets an error' do + service.execute + expect(service.errors).not_to be_empty + end + + it 'returns false' do + expect(service.execute).to be_falsey + end + end + + context 'when actor is missing' do + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://example.com/new-actor#unfollow-1', + type: 'Undo', + object: { + id: 'https://example.com/new-actor#follow-1', + type: 'Follow', + object: 'https://localhost/our/project/-/releases' + } + }.with_indifferent_access + end + + it_behaves_like 'invalid unfollow request' + end + + context 'when actor is an object with no id attribute' do + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://example.com/new-actor#unfollow-1', + actor: { type: 'Person' }, + type: 'Undo', + object: { + id: 'https://example.com/new-actor#follow-1', + type: 'Follow', + actor: { type: 'Person' }, + object: 'https://localhost/our/project/-/releases' + } + }.with_indifferent_access + end + + it_behaves_like 'invalid unfollow request' + end + + context 'when actor is neither a string nor an object' do + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://example.com/new-actor#unfollow-1', + actor: 27.13, + type: 'Undo', + object: { + id: 'https://example.com/new-actor#follow-1', + type: 'Follow', + actor: 27.13, + object: 'https://localhost/our/project/-/releases' + } + }.with_indifferent_access + end + + it_behaves_like 'invalid unfollow request' + end + + context "when actor tries to delete someone else's subscription" do + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://example.com/actor#unfollow-1', + type: 'Undo', + actor: 'https://example.com/nasty-actor', + object: { + id: 'https://example.com/actor#follow-1', + type: 'Follow', + actor: existing_subscription.subscriber_url, + object: 'https://localhost/our/project/-/releases' + } + }.with_indifferent_access + end + + it 'does not delete anything' do + expect { service.execute }.not_to change { ActivityPub::ReleasesSubscription.count } + end + + it 'returns true' do + expect(service.execute).to be_truthy + end + end + end +end diff --git a/spec/services/auth/dependency_proxy_authentication_service_spec.rb b/spec/services/auth/dependency_proxy_authentication_service_spec.rb index 3ef9c8fc96e..e81f59cff39 100644 --- a/spec/services/auth/dependency_proxy_authentication_service_spec.rb +++ b/spec/services/auth/dependency_proxy_authentication_service_spec.rb @@ -4,15 +4,17 @@ require 'spec_helper' RSpec.describe Auth::DependencyProxyAuthenticationService, feature_category: :dependency_proxy do let_it_be(:user) { create(:user) } + let_it_be(:params) { {} } - let(:service) { described_class.new(nil, user) } + let(:authentication_abilities) { nil } + let(:service) { described_class.new(nil, user, params) } before do - stub_config(dependency_proxy: { enabled: true }) + stub_config(dependency_proxy: { enabled: true }, registry: { enabled: true }) end describe '#execute' do - subject { service.execute(authentication_abilities: nil) } + subject { service.execute(authentication_abilities: authentication_abilities) } shared_examples 'returning' do |status:, message:| it "returns #{message}", :aggregate_failures do @@ -21,9 +23,13 @@ RSpec.describe Auth::DependencyProxyAuthenticationService, feature_category: :de end end - shared_examples 'returning a token' do - it 'returns a token' do - expect(subject[:token]).not_to be_nil + shared_examples 'returning a token with an encoded field' do |field| + it 'returns a token with encoded field' do + token = subject[:token] + expect(token).not_to be_nil + + decoded_token = decode(token) + expect(decoded_token[field]).not_to be_nil end end @@ -41,14 +47,73 @@ RSpec.describe Auth::DependencyProxyAuthenticationService, feature_category: :de it_behaves_like 'returning', status: 403, message: 'access forbidden' end - context 'with a deploy token as user' do - let_it_be(:user) { create(:deploy_token, :group, :dependency_proxy_scopes) } + context 'with a deploy token' do + let_it_be(:deploy_token) { create(:deploy_token, :group, :dependency_proxy_scopes) } + let_it_be(:params) { { deploy_token: deploy_token } } + + it_behaves_like 'returning a token with an encoded field', 'deploy_token' + end + + context 'with a human user' do + it_behaves_like 'returning a token with an encoded field', 'user_id' + end + + context 'all other user types' do + User::USER_TYPES.except(:human, :project_bot).each_value do |user_type| + context "with user_type #{user_type}" do + before do + user.update!(user_type: user_type) + end + + it_behaves_like 'returning a token with an encoded field', 'user_id' + end + end + end + + context 'with a group access token' do + let_it_be(:user) { create(:user, :project_bot) } + let_it_be_with_reload(:token) { create(:personal_access_token, user: user) } + + context 'with insufficient authentication abilities' do + it_behaves_like 'returning', status: 403, message: 'access forbidden' - it_behaves_like 'returning a token' + context 'packages_dependency_proxy_containers_scope_check disabled' do + before do + stub_feature_flags(packages_dependency_proxy_containers_scope_check: false) + end + + it_behaves_like 'returning a token with an encoded field', 'user_id' + end + end + + context 'with sufficient authentication abilities' do + let_it_be(:authentication_abilities) { Auth::DependencyProxyAuthenticationService::REQUIRED_ABILITIES } + let_it_be(:params) { { raw_token: token.token } } + + subject { service.execute(authentication_abilities: authentication_abilities) } + + it_behaves_like 'returning a token with an encoded field', 'user_id' + + context 'revoked' do + before do + token.revoke! + end + + it_behaves_like 'returning', status: 403, message: 'access forbidden' + end + + context 'expired' do + before do + token.update_column(:expires_at, 1.day.ago) + end + + it_behaves_like 'returning', status: 403, message: 'access forbidden' + end + end end - context 'with a user' do - it_behaves_like 'returning a token' + def decode(token) + DependencyProxy::AuthTokenService.new(token).execute end end end diff --git a/spec/services/bulk_imports/batched_relation_export_service_spec.rb b/spec/services/bulk_imports/batched_relation_export_service_spec.rb index dd85961befd..cb356b90c61 100644 --- a/spec/services/bulk_imports/batched_relation_export_service_spec.rb +++ b/spec/services/bulk_imports/batched_relation_export_service_spec.rb @@ -57,6 +57,16 @@ RSpec.describe BulkImports::BatchedRelationExportService, feature_category: :imp expect(export.batches.count).to eq(11) end end + + context 'when an error occurs during batches creation' do + it 'does not enqueue FinishBatchedRelationExportWorker' do + allow(service).to receive(:enqueue_batch_exports).and_raise(StandardError) + + expect(BulkImports::FinishBatchedRelationExportWorker).not_to receive(:perform_async) + + expect { service.execute }.to raise_error(StandardError) + end + end end context 'when there are no batches to export' do diff --git a/spec/services/bulk_imports/create_service_spec.rb b/spec/services/bulk_imports/create_service_spec.rb index 20872623802..024e7a0aa44 100644 --- a/spec/services/bulk_imports/create_service_spec.rb +++ b/spec/services/bulk_imports/create_service_spec.rb @@ -123,7 +123,8 @@ RSpec.describe BulkImports::CreateService, feature_category: :importers do ) allow_next_instance_of(BulkImports::Clients::HTTP) do |client| - allow(client).to receive(:validate_import_scopes!).and_raise(BulkImports::Error.scope_validation_failure) + allow(client).to receive(:validate_import_scopes!) + .and_raise(BulkImports::Error.scope_or_url_validation_failure) end result = subject.execute @@ -132,8 +133,7 @@ RSpec.describe BulkImports::CreateService, feature_category: :importers do expect(result).to be_error expect(result.message) .to eq( - "Personal access token does not " \ - "have the required 'api' scope or is no longer valid." + "Check that the source instance base URL and the personal access token meet the necessary requirements." ) end end @@ -546,7 +546,8 @@ RSpec.describe BulkImports::CreateService, feature_category: :importers do expect(result).to be_a(ServiceResponse) expect(result).to be_error expect(result.message) - .to eq("Import failed. Destination 'destination-namespace' is invalid, or you don't have permission.") + .to eq("Import failed. Destination 'destination-namespace' is invalid, " \ + "or you don't have permission.") end end @@ -571,7 +572,8 @@ RSpec.describe BulkImports::CreateService, feature_category: :importers do expect(result).to be_a(ServiceResponse) expect(result).to be_error expect(result.message) - .to eq("Import failed. Destination '#{parent_group.path}' is invalid, or you don't have permission.") + .to eq("Import failed. Destination '#{parent_group.path}' is invalid, " \ + "or you don't have permission.") end end @@ -596,7 +598,8 @@ RSpec.describe BulkImports::CreateService, feature_category: :importers do expect(result).to be_a(ServiceResponse) expect(result).to be_error expect(result.message) - .to eq("Import failed. Destination '#{parent_group.path}' is invalid, or you don't have permission.") + .to eq("Import failed. Destination '#{parent_group.path}' is invalid, " \ + "or you don't have permission.") end end end diff --git a/spec/services/bulk_imports/file_download_service_spec.rb b/spec/services/bulk_imports/file_download_service_spec.rb index b2971c75bce..0c3eef69fa5 100644 --- a/spec/services/bulk_imports/file_download_service_spec.rb +++ b/spec/services/bulk_imports/file_download_service_spec.rb @@ -22,9 +22,10 @@ RSpec.describe BulkImports::FileDownloadService, feature_category: :importers do } end - let(:chunk_size) { 100 } let(:chunk_code) { 200 } - let(:chunk_double) { double('chunk', size: chunk_size, code: chunk_code, http_response: double(to_hash: headers)) } + let(:chunk_double) do + double('chunk', size: 100, code: chunk_code, http_response: double(to_hash: headers), to_s: 'some chunk context') + end subject(:service) do described_class.new( @@ -92,7 +93,9 @@ RSpec.describe BulkImports::FileDownloadService, feature_category: :importers do it 'logs and raises an error' do expect(import_logger).to receive(:warn).once.with( message: 'Invalid content type', - response_headers: headers + response_code: chunk_code, + response_headers: headers, + last_chunk_context: 'some chunk context' ) expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid content type') @@ -145,15 +148,26 @@ RSpec.describe BulkImports::FileDownloadService, feature_category: :importers do end context 'when chunk code is not 200' do - let(:chunk_code) { 500 } + let(:chunk_code) { 404 } it 'raises an error' do expect { subject.execute }.to raise_error( described_class::ServiceError, - 'File download error 500' + 'File download error 404' ) end + context 'when chunk code is retriable' do + let(:chunk_code) { 502 } + + it 'raises a retriable error' do + expect { subject.execute }.to raise_error( + BulkImports::NetworkError, + 'Error downloading file from /test. Error code: 502' + ) + end + end + context 'when chunk code is redirection' do let(:chunk_code) { 303 } diff --git a/spec/services/bulk_imports/process_service_spec.rb b/spec/services/bulk_imports/process_service_spec.rb index f5566819039..a295b170c2f 100644 --- a/spec/services/bulk_imports/process_service_spec.rb +++ b/spec/services/bulk_imports/process_service_spec.rb @@ -205,28 +205,20 @@ RSpec.describe BulkImports::ProcessService, feature_category: :importers do it 'logs an info message for the skipped pipelines' do expect_next_instance_of(BulkImports::Logger) do |logger| + expect(logger).to receive(:with_entity).with(entity).and_call_original.twice + expect(logger).to receive(:info).with( message: 'Pipeline skipped as source instance version not compatible with pipeline', - bulk_import_entity_id: entity.id, - bulk_import_id: entity.bulk_import_id, - bulk_import_entity_type: entity.source_type, - source_full_path: entity.source_full_path, pipeline_class: 'PipelineClass4', minimum_source_version: '15.1.0', - maximum_source_version: nil, - source_version: '15.0.0' + maximum_source_version: nil ) expect(logger).to receive(:info).with( message: 'Pipeline skipped as source instance version not compatible with pipeline', - bulk_import_entity_id: entity.id, - bulk_import_id: entity.bulk_import_id, - bulk_import_entity_type: entity.source_type, - source_full_path: entity.source_full_path, pipeline_class: 'PipelineClass5', minimum_source_version: '16.0.0', - maximum_source_version: nil, - source_version: '15.0.0' + maximum_source_version: nil ) end diff --git a/spec/services/ci/catalog/resources/destroy_service_spec.rb b/spec/services/ci/catalog/resources/destroy_service_spec.rb new file mode 100644 index 00000000000..da5ba7ad0bc --- /dev/null +++ b/spec/services/ci/catalog/resources/destroy_service_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::Catalog::Resources::DestroyService, feature_category: :pipeline_composition do + let_it_be(:project) { create(:project, :catalog_resource_with_components) } + let_it_be(:catalog_resource) { create(:ci_catalog_resource, project: project) } + let_it_be(:user) { create(:user) } + + let(:service) { described_class.new(project, user) } + + before do + stub_licensed_features(ci_namespace_catalog: true) + end + + describe '#execute' do + context 'with an unauthorized user' do + it 'raises an AccessDeniedError' do + expect { service.execute(catalog_resource) }.to raise_error(Gitlab::Access::AccessDeniedError) + end + end + + context 'with an authorized user' do + before_all do + project.add_owner(user) + end + + it 'destroys a catalog resource' do + expect(project.catalog_resource).to eq(catalog_resource) + + response = service.execute(catalog_resource) + + expect(project.reload.catalog_resource).to be_nil + expect(response.status).to be(:success) + end + end + end +end diff --git a/spec/services/ci/components/fetch_service_spec.rb b/spec/services/ci/components/fetch_service_spec.rb index 21b7df19f4a..82be795e997 100644 --- a/spec/services/ci/components/fetch_service_spec.rb +++ b/spec/services/ci/components/fetch_service_spec.rb @@ -21,14 +21,15 @@ RSpec.describe Ci::Components::FetchService, feature_category: :pipeline_composi project = create( :project, :custom_repo, files: { - 'template.yml' => content, - 'my-component/template.yml' => content, - 'my-dir/my-component/template.yml' => content + 'templates/first-component.yml' => content, + 'templates/second-component/template.yml' => content } ) project.repository.add_tag(project.creator, 'v0.1', project.repository.commit.sha) + create(:release, project: project, tag: 'v0.1', sha: project.repository.commit.sha) + project end @@ -119,32 +120,27 @@ RSpec.describe Ci::Components::FetchService, feature_category: :pipeline_composi context 'when address points to an external component' do let(:address) { "#{current_host}/#{component_path}@#{version}" } - context 'when component path is the full path to a project' do - let(:component_path) { project.full_path } - let(:component_yaml_path) { 'template.yml' } + context 'when component path points to a template file in a project' do + let(:component_path) { "#{project.full_path}/first-component" } it_behaves_like 'an external component' end - context 'when component path points to a directory in a project' do - let(:component_path) { "#{project.full_path}/my-component" } - let(:component_yaml_path) { 'my-component/template.yml' } + context 'when component path points to a template directory in a project' do + let(:component_path) { "#{project.full_path}/second-component" } it_behaves_like 'an external component' end - context 'when component path points to a nested directory in a project' do - let(:component_path) { "#{project.full_path}/my-dir/my-component" } - let(:component_yaml_path) { 'my-dir/my-component/template.yml' } + context 'when the project exists but the component does not' do + let(:component_path) { "#{project.full_path}/unknown-component" } + let(:version) { '~latest' } - it_behaves_like 'an external component' + it 'returns a content not found error' do + expect(result).to be_error + expect(result.reason).to eq(:content_not_found) + end end end end - - def stub_project_blob(ref, path, content) - allow_next_instance_of(Repository) do |instance| - allow(instance).to receive(:blob_data_at).with(ref, path).and_return(content) - end - end end diff --git a/spec/services/ci/create_pipeline_service/workflow_auto_cancel_spec.rb b/spec/services/ci/create_pipeline_service/workflow_auto_cancel_spec.rb new file mode 100644 index 00000000000..851c6f8fbea --- /dev/null +++ b/spec/services/ci/create_pipeline_service/workflow_auto_cancel_spec.rb @@ -0,0 +1,169 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectness, + feature_category: :pipeline_composition do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { project.first_owner } + + let(:service) { described_class.new(project, user, { ref: 'master' }) } + let(:pipeline) { service.execute(:push).payload } + + before do + stub_ci_pipeline_yaml_file(config) + end + + describe 'on_new_commit' do + context 'when is set to interruptible' do + let(:config) do + <<~YAML + workflow: + auto_cancel: + on_new_commit: interruptible + + test1: + script: exit 0 + YAML + end + + before do + stub_ci_pipeline_yaml_file(config) + end + + it 'creates a pipeline with on_new_commit' do + expect(pipeline).to be_persisted + expect(pipeline.errors).to be_empty + expect(pipeline.pipeline_metadata.auto_cancel_on_new_commit).to eq('interruptible') + end + end + + context 'when is set to invalid' do + let(:config) do + <<~YAML + workflow: + auto_cancel: + on_new_commit: invalid + + test1: + script: exit 0 + YAML + end + + before do + stub_ci_pipeline_yaml_file(config) + end + + it 'creates a pipeline with errors' do + expect(pipeline).to be_persisted + expect(pipeline.errors.full_messages).to include( + 'workflow:auto_cancel on new commit must be one of: conservative, interruptible, disabled') + end + end + end + + describe 'on_job_failure' do + context 'when is set to none' do + let(:config) do + <<~YAML + workflow: + auto_cancel: + on_job_failure: none + + test1: + script: exit 0 + YAML + end + + before do + stub_ci_pipeline_yaml_file(config) + end + + it 'creates a pipeline with on_job_failure' do + expect(pipeline).to be_persisted + expect(pipeline.errors).to be_empty + expect(pipeline.pipeline_metadata.auto_cancel_on_job_failure).to eq('none') + end + end + + context 'when is set to all' do + let(:config) do + <<~YAML + workflow: + auto_cancel: + on_job_failure: all + + test1: + script: exit 0 + YAML + end + + before do + stub_ci_pipeline_yaml_file(config) + end + + it 'creates a pipeline with on_job_failure' do + expect(pipeline).to be_persisted + expect(pipeline.errors).to be_empty + expect(pipeline.pipeline_metadata.auto_cancel_on_job_failure).to eq('all') + end + + context 'when auto_cancel_pipeline_on_job_failure feature flag is disabled' do + before do + stub_feature_flags(auto_cancel_pipeline_on_job_failure: false) + end + + context 'when there are no other metadata settings present' do + it 'creates a pipeline without metadata' do + expect(pipeline).to be_persisted + expect(pipeline.errors).to be_empty + expect(pipeline.pipeline_metadata).to be_nil + end + end + + context 'when other metadata settings are present' do + let(:config) do + <<~YAML + workflow: + name: pipeline_name + auto_cancel: + on_job_failure: all + + test1: + script: exit 0 + YAML + end + + it 'creates a pipeline with on_job_failure' do + expect(pipeline).to be_persisted + expect(pipeline.errors).to be_empty + expect(pipeline.pipeline_metadata.auto_cancel_on_job_failure).to eq('none') + end + end + end + end + + context 'when on_job_failure is set to invalid' do + let(:config) do + <<~YAML + workflow: + auto_cancel: + on_job_failure: invalid + + test1: + script: exit 0 + YAML + end + + before do + stub_ci_pipeline_yaml_file(config) + end + + it 'creates a pipeline with errors' do + expect(pipeline).to be_persisted + expect(pipeline.errors.full_messages).to include( + 'workflow:auto_cancel on job failure must be one of: none, all') + end + end + end +end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 19e55c22df8..7dea50ba270 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -1757,7 +1757,7 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes let(:sha) do components_project.repository.create_file( user, - 'my-component/template.yml', + 'templates/my-component/template.yml', template, message: 'Add my first CI component', branch_name: 'master' @@ -1894,7 +1894,7 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes let(:sha) do components_project.repository.create_file( user, - 'my-component/template.yml', + 'templates/my-component/template.yml', template, message: 'Add my first CI component', branch_name: 'master' diff --git a/spec/services/ci/generate_coverage_reports_service_spec.rb b/spec/services/ci/generate_coverage_reports_service_spec.rb index 811431bf9d6..dac8dc57261 100644 --- a/spec/services/ci/generate_coverage_reports_service_spec.rb +++ b/spec/services/ci/generate_coverage_reports_service_spec.rb @@ -25,6 +25,21 @@ RSpec.describe Ci::GenerateCoverageReportsService, feature_category: :code_testi expect(subject[:status]).to eq(:parsed) expect(subject[:data]).to eq(files: {}) end + + context 'when there is a parsing error' do + before do + allow_next_found_instance_of(MergeRequest) do |merge_request| + allow(merge_request).to receive(:new_paths).and_raise(StandardError) + end + end + + it 'returns status with error message and tracks the error' do + expect(service).to receive(:track_exception).and_call_original + + expect(subject[:status]).to eq(:error) + expect(subject[:status_reason]).to include('An error occurred while fetching coverage reports.') + end + end end context 'when head pipeline does not have a coverage report artifact' do @@ -38,6 +53,8 @@ RSpec.describe Ci::GenerateCoverageReportsService, feature_category: :code_testi end it 'returns status and error message' do + expect(service).not_to receive(:track_exception) + expect(subject[:status]).to eq(:error) expect(subject[:status_reason]).to include('An error occurred while fetching coverage reports.') end @@ -48,6 +65,8 @@ RSpec.describe Ci::GenerateCoverageReportsService, feature_category: :code_testi let!(:base_pipeline) { nil } it 'returns status and error message' do + expect(service).not_to receive(:track_exception) + expect(subject[:status]).to eq(:error) expect(subject[:status_reason]).to include('An error occurred while fetching coverage reports.') end diff --git a/spec/services/ci/job_artifacts/create_service_spec.rb b/spec/services/ci/job_artifacts/create_service_spec.rb index a23ba250daf..0d6a15b0ea3 100644 --- a/spec/services/ci/job_artifacts/create_service_spec.rb +++ b/spec/services/ci/job_artifacts/create_service_spec.rb @@ -139,59 +139,19 @@ RSpec.describe Ci::JobArtifacts::CreateService, :clean_gitlab_redis_shared_state shared_examples_for 'handling accessibility' do shared_examples 'public accessibility' do it 'sets accessibility to public level' do + subject + + expect(job.job_artifacts).not_to be_empty expect(job.job_artifacts).to all be_public_accessibility end end shared_examples 'private accessibility' do it 'sets accessibility to private level' do - expect(job.job_artifacts).to all be_private_accessibility - end - end - - context 'when non_public_artifacts flag is disabled' do - before do - stub_feature_flags(non_public_artifacts: false) - end - - it_behaves_like 'public accessibility' - end - - context 'when non_public_artifacts flag is enabled' do - context 'and accessibility is defined in the params' do - context 'and is passed as private' do - before do - params.merge!('accessibility' => 'private') - end - - it_behaves_like 'private accessibility' - end - - context 'and is passed as public' do - before do - params.merge!('accessibility' => 'public') - end - - it_behaves_like 'public accessibility' - end - end - - context 'and accessibility is not defined in the params' do - context 'and job has no public artifacts defined in its CI config' do - it_behaves_like 'public accessibility' - end - - context 'and job artifacts defined as private in the CI config' do - let(:job) { create(:ci_build, :with_private_artifacts_config, project: project) } - - it_behaves_like 'private accessibility' - end - - context 'and job artifacts defined as public in the CI config' do - let(:job) { create(:ci_build, :with_public_artifacts_config, project: project) } + subject - it_behaves_like 'public accessibility' - end + expect(job.job_artifacts).not_to be_empty + expect(job.job_artifacts).to all be_private_accessibility end end diff --git a/spec/services/ci/pipeline_creation/cancel_redundant_pipelines_service_spec.rb b/spec/services/ci/pipeline_creation/cancel_redundant_pipelines_service_spec.rb index a5dda1d13aa..0d83187f9e4 100644 --- a/spec/services/ci/pipeline_creation/cancel_redundant_pipelines_service_spec.rb +++ b/spec/services/ci/pipeline_creation/cancel_redundant_pipelines_service_spec.rb @@ -207,12 +207,12 @@ RSpec.describe Ci::PipelineCreation::CancelRedundantPipelinesService, feature_ca it 'does not cancel any builds' do expect(build_statuses(prev_pipeline)).to contain_exactly('running', 'success', 'created') - expect(build_statuses(parent_pipeline)).to contain_exactly('running', 'running') + expect(build_statuses(parent_pipeline)).to contain_exactly('created', 'running', 'running') execute expect(build_statuses(prev_pipeline)).to contain_exactly('running', 'success', 'created') - expect(build_statuses(parent_pipeline)).to contain_exactly('running', 'running') + expect(build_statuses(parent_pipeline)).to contain_exactly('created', 'running', 'running') end end @@ -227,6 +227,25 @@ RSpec.describe Ci::PipelineCreation::CancelRedundantPipelinesService, feature_ca end end + context 'when there are trigger jobs' do + before do + create(:ci_bridge, :created, pipeline: prev_pipeline) + create(:ci_bridge, :running, pipeline: prev_pipeline) + create(:ci_bridge, :success, pipeline: prev_pipeline) + create(:ci_bridge, :interruptible, :created, pipeline: prev_pipeline) + create(:ci_bridge, :interruptible, :running, pipeline: prev_pipeline) + create(:ci_bridge, :interruptible, :success, pipeline: prev_pipeline) + end + + it 'still cancels the pipeline because auto-cancel is not affected by non-interruptible started triggers' do + execute + + expect(job_statuses(prev_pipeline)).to contain_exactly( + 'canceled', 'success', 'canceled', 'canceled', 'canceled', 'success', 'canceled', 'canceled', 'success') + expect(job_statuses(pipeline)).to contain_exactly('pending') + end + end + it 'does not cancel future pipelines' do expect(prev_pipeline.id).to be < pipeline.id expect(build_statuses(pipeline)).to contain_exactly('pending') @@ -269,7 +288,8 @@ RSpec.describe Ci::PipelineCreation::CancelRedundantPipelinesService, feature_ca private - def build_statuses(pipeline) - pipeline.builds.pluck(:status) + def job_statuses(pipeline) + pipeline.statuses.pluck(:status) end + alias_method :build_statuses, :job_statuses end diff --git a/spec/services/ci/process_sync_events_service_spec.rb b/spec/services/ci/process_sync_events_service_spec.rb index c58d73815b0..cea5eec294e 100644 --- a/spec/services/ci/process_sync_events_service_spec.rb +++ b/spec/services/ci/process_sync_events_service_spec.rb @@ -163,5 +163,70 @@ RSpec.describe Ci::ProcessSyncEventsService, feature_category: :continuous_integ execute end end + + context 'for Ci::Catalog::Resources::SyncEvent' do + let(:sync_event_class) { Ci::Catalog::Resources::SyncEvent } + let(:hierarchy_class) { Ci::Catalog::Resource } + + let_it_be(:project1) { create(:project) } + let_it_be(:project2) { create(:project) } + let_it_be_with_refind(:resource1) { create(:ci_catalog_resource, project: project1) } + let_it_be(:resource2) { create(:ci_catalog_resource, project: project2) } + + before_all do + create(:ci_catalog_resource_sync_event, catalog_resource: resource1, status: :processed) + # PG trigger adds an event for each update + project1.update!(name: 'Name 1', description: 'Test 1') + project1.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) + project2.update!(name: 'Name 2', description: 'Test 2') + project2.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + end + + it 'processes the events', :aggregate_failures do + # 2 pending events from resource1 + 2 pending events from resource2 + expect { execute }.to change(Ci::Catalog::Resources::SyncEvent.status_pending, :count).from(4).to(0) + + expect(resource1.reload.name).to eq(project1.name) + expect(resource2.reload.name).to eq(project2.name) + expect(resource1.reload.description).to eq(project1.description) + expect(resource2.reload.description).to eq(project2.description) + expect(resource1.reload.visibility_level).to eq(project1.visibility_level) + expect(resource2.reload.visibility_level).to eq(project2.visibility_level) + end + + context 'when there are no remaining unprocessed events' do + it 'does not enqueue Ci::Catalog::Resources::ProcessSyncEventsWorker' do + stub_const("#{described_class}::BATCH_SIZE", 4) + + expect(Ci::Catalog::Resources::ProcessSyncEventsWorker).not_to receive(:perform_async) + + execute + end + end + + context 'when there are remaining unprocessed events' do + it 'enqueues Ci::Catalog::Resources::ProcessSyncEventsWorker' do + stub_const("#{described_class}::BATCH_SIZE", 1) + + expect(Ci::Catalog::Resources::ProcessSyncEventsWorker).to receive(:perform_async) + + execute + end + end + + # The `p_catalog_resource_sync_events` table does not enforce an FK on catalog_resource_id + context 'when there are orphaned sync events' do + it 'processes the events', :aggregate_failures do + resource1.destroy! + + # 2 pending events from resource1 + 2 pending events from resource2 + expect { execute }.to change(Ci::Catalog::Resources::SyncEvent.status_pending, :count).from(4).to(0) + + expect(resource2.reload.name).to eq(project2.name) + expect(resource2.reload.description).to eq(project2.description) + expect(resource2.reload.visibility_level).to eq(project2.visibility_level) + end + end + end end end diff --git a/spec/services/ci/runners/assign_runner_service_spec.rb b/spec/services/ci/runners/assign_runner_service_spec.rb index 00fbb5e2d26..eb0b7478ad3 100644 --- a/spec/services/ci/runners/assign_runner_service_spec.rb +++ b/spec/services/ci/runners/assign_runner_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ::Ci::Runners::AssignRunnerService, '#execute', feature_category: :runner_fleet do +RSpec.describe ::Ci::Runners::AssignRunnerService, '#execute', feature_category: :fleet_visibility do subject(:execute) { described_class.new(runner, new_project, user).execute } let_it_be(:owner_group) { create(:group) } diff --git a/spec/services/ci/runners/bulk_delete_runners_service_spec.rb b/spec/services/ci/runners/bulk_delete_runners_service_spec.rb index 5e697565972..b57cae00867 100644 --- a/spec/services/ci/runners/bulk_delete_runners_service_spec.rb +++ b/spec/services/ci/runners/bulk_delete_runners_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ::Ci::Runners::BulkDeleteRunnersService, '#execute', feature_category: :runner_fleet do +RSpec.describe ::Ci::Runners::BulkDeleteRunnersService, '#execute', feature_category: :fleet_visibility do subject(:execute) { described_class.new(**service_args).execute } let_it_be(:admin_user) { create(:user, :admin) } diff --git a/spec/services/ci/runners/create_runner_service_spec.rb b/spec/services/ci/runners/create_runner_service_spec.rb index db337b0b005..eaba7b9e4db 100644 --- a/spec/services/ci/runners/create_runner_service_spec.rb +++ b/spec/services/ci/runners/create_runner_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ::Ci::Runners::CreateRunnerService, "#execute", feature_category: :runner_fleet do +RSpec.describe ::Ci::Runners::CreateRunnerService, "#execute", feature_category: :fleet_visibility do subject(:execute) { described_class.new(user: current_user, params: params).execute } let(:runner) { execute.payload[:runner] } diff --git a/spec/services/ci/runners/process_runner_version_update_service_spec.rb b/spec/services/ci/runners/process_runner_version_update_service_spec.rb index f8b7aa281af..cc8df6579d4 100644 --- a/spec/services/ci/runners/process_runner_version_update_service_spec.rb +++ b/spec/services/ci/runners/process_runner_version_update_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::Runners::ProcessRunnerVersionUpdateService, feature_category: :runner_fleet do +RSpec.describe Ci::Runners::ProcessRunnerVersionUpdateService, feature_category: :fleet_visibility do subject(:service) { described_class.new(version) } let(:version) { '1.0.0' } diff --git a/spec/services/ci/runners/reconcile_existing_runner_versions_service_spec.rb b/spec/services/ci/runners/reconcile_existing_runner_versions_service_spec.rb index 8d7e97e5ea8..88f0a930599 100644 --- a/spec/services/ci/runners/reconcile_existing_runner_versions_service_spec.rb +++ b/spec/services/ci/runners/reconcile_existing_runner_versions_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ::Ci::Runners::ReconcileExistingRunnerVersionsService, '#execute', feature_category: :runner_fleet do +RSpec.describe ::Ci::Runners::ReconcileExistingRunnerVersionsService, '#execute', feature_category: :fleet_visibility do include RunnerReleasesHelper subject(:execute) { described_class.new.execute } diff --git a/spec/services/ci/runners/register_runner_service_spec.rb b/spec/services/ci/runners/register_runner_service_spec.rb index 4b997855657..aabf30d975a 100644 --- a/spec/services/ci/runners/register_runner_service_spec.rb +++ b/spec/services/ci/runners/register_runner_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ::Ci::Runners::RegisterRunnerService, '#execute', feature_category: :runner_fleet do +RSpec.describe ::Ci::Runners::RegisterRunnerService, '#execute', feature_category: :fleet_visibility do let(:registration_token) { 'abcdefg123456' } let(:token) {} let(:args) { {} } diff --git a/spec/services/ci/runners/reset_registration_token_service_spec.rb b/spec/services/ci/runners/reset_registration_token_service_spec.rb index c8115236034..68faa9fa387 100644 --- a/spec/services/ci/runners/reset_registration_token_service_spec.rb +++ b/spec/services/ci/runners/reset_registration_token_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ::Ci::Runners::ResetRegistrationTokenService, '#execute', feature_category: :runner_fleet do +RSpec.describe ::Ci::Runners::ResetRegistrationTokenService, '#execute', feature_category: :fleet_visibility do subject(:execute) { described_class.new(scope, current_user).execute } let_it_be(:user) { build(:user) } diff --git a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb index 8d612174a0b..b617cb0a006 100644 --- a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb +++ b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute', feature_category: :runner_fleet do +RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute', feature_category: :fleet_visibility do subject(:execute) do described_class.new(runner: runner, current_user: user, project_ids: new_projects.map(&:id)).execute end diff --git a/spec/services/ci/runners/stale_managers_cleanup_service_spec.rb b/spec/services/ci/runners/stale_managers_cleanup_service_spec.rb index 0a20c12bc15..4931f24d5d8 100644 --- a/spec/services/ci/runners/stale_managers_cleanup_service_spec.rb +++ b/spec/services/ci/runners/stale_managers_cleanup_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::Runners::StaleManagersCleanupService, feature_category: :runner_fleet do +RSpec.describe Ci::Runners::StaleManagersCleanupService, feature_category: :fleet_visibility do let(:service) { described_class.new } let!(:runner_manager3) { create(:ci_runner_machine, created_at: 6.months.ago, contacted_at: Time.current) } diff --git a/spec/services/ci/runners/unassign_runner_service_spec.rb b/spec/services/ci/runners/unassign_runner_service_spec.rb index e91d4249473..99cf087cf78 100644 --- a/spec/services/ci/runners/unassign_runner_service_spec.rb +++ b/spec/services/ci/runners/unassign_runner_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ::Ci::Runners::UnassignRunnerService, '#execute', feature_category: :runner_fleet do +RSpec.describe ::Ci::Runners::UnassignRunnerService, '#execute', feature_category: :fleet_visibility do let_it_be(:project) { create(:project) } let_it_be(:runner) { create(:ci_runner, :project, projects: [project]) } diff --git a/spec/services/ci/runners/unregister_runner_manager_service_spec.rb b/spec/services/ci/runners/unregister_runner_manager_service_spec.rb index 8bfda8e2083..590df18469d 100644 --- a/spec/services/ci/runners/unregister_runner_manager_service_spec.rb +++ b/spec/services/ci/runners/unregister_runner_manager_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ::Ci::Runners::UnregisterRunnerManagerService, '#execute', feature_category: :runner_fleet do +RSpec.describe ::Ci::Runners::UnregisterRunnerManagerService, '#execute', feature_category: :fleet_visibility do subject(:execute) { described_class.new(runner, 'some_token', system_id: system_id).execute } context 'with runner registered with registration token' do diff --git a/spec/services/ci/runners/unregister_runner_service_spec.rb b/spec/services/ci/runners/unregister_runner_service_spec.rb index fb779e1a673..e73dcb2511e 100644 --- a/spec/services/ci/runners/unregister_runner_service_spec.rb +++ b/spec/services/ci/runners/unregister_runner_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ::Ci::Runners::UnregisterRunnerService, '#execute', feature_category: :runner_fleet do +RSpec.describe ::Ci::Runners::UnregisterRunnerService, '#execute', feature_category: :fleet_visibility do subject(:execute) { described_class.new(runner, 'some_token').execute } let(:runner) { create(:ci_runner) } diff --git a/spec/services/ci/runners/update_runner_service_spec.rb b/spec/services/ci/runners/update_runner_service_spec.rb index 86875df70a2..9483d122c35 100644 --- a/spec/services/ci/runners/update_runner_service_spec.rb +++ b/spec/services/ci/runners/update_runner_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::Runners::UpdateRunnerService, '#execute', feature_category: :runner_fleet do +RSpec.describe Ci::Runners::UpdateRunnerService, '#execute', feature_category: :fleet_visibility do subject(:execute) { described_class.new(runner).execute(params) } let(:runner) { create(:ci_runner) } diff --git a/spec/services/ci/stuck_builds/drop_pending_service_spec.rb b/spec/services/ci/stuck_builds/drop_pending_service_spec.rb index 9da63930057..b3045d838a1 100644 --- a/spec/services/ci/stuck_builds/drop_pending_service_spec.rb +++ b/spec/services/ci/stuck_builds/drop_pending_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::StuckBuilds::DropPendingService, feature_category: :runner_fleet do +RSpec.describe Ci::StuckBuilds::DropPendingService, feature_category: :fleet_visibility do let_it_be(:runner) { create(:ci_runner) } let_it_be(:pipeline) { create(:ci_empty_pipeline) } let_it_be_with_reload(:job) do diff --git a/spec/services/ci/stuck_builds/drop_running_service_spec.rb b/spec/services/ci/stuck_builds/drop_running_service_spec.rb index c2f8a643f24..74b02240ea5 100644 --- a/spec/services/ci/stuck_builds/drop_running_service_spec.rb +++ b/spec/services/ci/stuck_builds/drop_running_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::StuckBuilds::DropRunningService, feature_category: :runner_fleet do +RSpec.describe Ci::StuckBuilds::DropRunningService, feature_category: :fleet_visibility do let!(:runner) { create :ci_runner } let!(:job) { create(:ci_build, runner: runner, created_at: created_at, updated_at: updated_at, status: status) } diff --git a/spec/services/ci/stuck_builds/drop_scheduled_service_spec.rb b/spec/services/ci/stuck_builds/drop_scheduled_service_spec.rb index 5560eaf9b40..5a95b55054f 100644 --- a/spec/services/ci/stuck_builds/drop_scheduled_service_spec.rb +++ b/spec/services/ci/stuck_builds/drop_scheduled_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::StuckBuilds::DropScheduledService, feature_category: :runner_fleet do +RSpec.describe Ci::StuckBuilds::DropScheduledService, feature_category: :fleet_visibility do let_it_be(:runner) { create :ci_runner } let!(:job) { create :ci_build, :scheduled, scheduled_at: scheduled_at, runner: runner } diff --git a/spec/services/container_registry/protection/create_rule_service_spec.rb b/spec/services/container_registry/protection/create_rule_service_spec.rb index 3c319caf25c..4559a8fb131 100644 --- a/spec/services/container_registry/protection/create_rule_service_spec.rb +++ b/spec/services/container_registry/protection/create_rule_service_spec.rb @@ -22,7 +22,7 @@ RSpec.describe ContainerRegistry::Protection::CreateRuleService, '#execute', fea container_registry_protection_rule: be_a(ContainerRegistry::Protection::Rule) .and(have_attributes( - container_path_pattern: params[:container_path_pattern], + repository_path_pattern: params[:repository_path_pattern], push_protected_up_to_access_level: params[:push_protected_up_to_access_level].to_s, delete_protected_up_to_access_level: params[:delete_protected_up_to_access_level].to_s )) @@ -36,7 +36,7 @@ RSpec.describe ContainerRegistry::Protection::CreateRuleService, '#execute', fea expect( ContainerRegistry::Protection::Rule.where( project: project, - container_path_pattern: params[:container_path_pattern], + repository_path_pattern: params[:repository_path_pattern], push_protected_up_to_access_level: params[:push_protected_up_to_access_level] ) ).to exist @@ -57,7 +57,7 @@ RSpec.describe ContainerRegistry::Protection::CreateRuleService, '#execute', fea expect( ContainerRegistry::Protection::Rule.where( project: project, - container_path_pattern: params[:container_path_pattern], + repository_path_pattern: params[:repository_path_pattern], push_protected_up_to_access_level: params[:push_protected_up_to_access_level] ) ).not_to exist @@ -67,12 +67,12 @@ RSpec.describe ContainerRegistry::Protection::CreateRuleService, '#execute', fea it_behaves_like 'a successful service response' context 'when fields are invalid' do - context 'when container_path_pattern is invalid' do - let(:params) { super().merge(container_path_pattern: '') } + context 'when repository_path_pattern is invalid' do + let(:params) { super().merge(repository_path_pattern: '') } it_behaves_like 'an erroneous service response' - it { is_expected.to have_attributes(message: match(/Container path pattern can't be blank/)) } + it { is_expected.to have_attributes(message: match(/Repository path pattern can't be blank/)) } end context 'when delete_protected_up_to_access_level is invalid' do @@ -100,8 +100,8 @@ RSpec.describe ContainerRegistry::Protection::CreateRuleService, '#execute', fea context 'when container registry name pattern is slightly different' do let(:params) do super().merge( - # The field `container_path_pattern` is unique; this is why we change the value in a minimum way - container_path_pattern: "#{existing_container_registry_protection_rule.container_path_pattern}-unique", + # The field `repository_path_pattern` is unique; this is why we change the value in a minimum way + repository_path_pattern: "#{existing_container_registry_protection_rule.repository_path_pattern}-unique", push_protected_up_to_access_level: existing_container_registry_protection_rule.push_protected_up_to_access_level ) @@ -110,17 +110,17 @@ RSpec.describe ContainerRegistry::Protection::CreateRuleService, '#execute', fea it_behaves_like 'a successful service response' end - context 'when field `container_path_pattern` is taken' do + context 'when field `repository_path_pattern` is taken' do let(:params) do super().merge( - container_path_pattern: existing_container_registry_protection_rule.container_path_pattern, + repository_path_pattern: existing_container_registry_protection_rule.repository_path_pattern, push_protected_up_to_access_level: :maintainer ) end it_behaves_like 'an erroneous service response' - it { is_expected.to have_attributes(errors: ['Container path pattern has already been taken']) } + it { is_expected.to have_attributes(errors: ['Repository path pattern has already been taken']) } it { expect { subject }.not_to change { existing_container_registry_protection_rule.updated_at } } end diff --git a/spec/services/container_registry/protection/delete_rule_service_spec.rb b/spec/services/container_registry/protection/delete_rule_service_spec.rb new file mode 100644 index 00000000000..acefe6a55d0 --- /dev/null +++ b/spec/services/container_registry/protection/delete_rule_service_spec.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ContainerRegistry::Protection::DeleteRuleService, '#execute', feature_category: :container_registry do + let_it_be(:project) { create(:project) } + let_it_be(:current_user) { create(:user, maintainer_projects: [project]) } + let_it_be_with_refind(:container_registry_protection_rule) do + create(:container_registry_protection_rule, project: project) + end + + subject(:service_execute) do + described_class.new(container_registry_protection_rule, current_user: current_user).execute + end + + shared_examples 'a successful service response' do + it { is_expected.to be_success } + + it do + is_expected.to have_attributes( + errors: be_blank, + message: be_blank, + payload: { container_registry_protection_rule: container_registry_protection_rule } + ) + end + + it do + service_execute + + expect { container_registry_protection_rule.reload }.to raise_error ActiveRecord::RecordNotFound + end + end + + shared_examples 'an erroneous service response' do + it { is_expected.to be_error } + + it do + is_expected.to have_attributes(message: be_present, payload: { container_registry_protection_rule: be_blank }) + end + + it do + expect { service_execute }.not_to change { ContainerRegistry::Protection::Rule.count } + + expect { container_registry_protection_rule.reload }.not_to raise_error + end + end + + it_behaves_like 'a successful service response' + + it 'deletes the container registry protection rule in the database' do + expect { service_execute } + .to change { + project.reload.container_registry_protection_rules + }.from([container_registry_protection_rule]).to([]) + .and change { ::ContainerRegistry::Protection::Rule.count }.from(1).to(0) + end + + context 'with deleted container registry protection rule' do + let!(:container_registry_protection_rule) do + create(:container_registry_protection_rule, project: project, + repository_path_pattern: 'protection_rule_deleted').destroy! + end + + it_behaves_like 'a successful service response' + end + + context 'when error occurs during delete operation' do + before do + allow(container_registry_protection_rule).to receive(:destroy!).and_raise(StandardError.new('Some error')) + end + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes message: /Some error/ } + end + + context 'when current_user does not have permission' do + let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } + let_it_be(:reporter) { create(:user).tap { |u| project.add_reporter(u) } } + let_it_be(:guest) { create(:user).tap { |u| project.add_guest(u) } } + let_it_be(:anonymous) { create(:user) } + + where(:current_user) do + [ref(:developer), ref(:reporter), ref(:guest), ref(:anonymous)] + end + + with_them do + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes message: /Unauthorized to delete a container registry protection rule/ } + end + end + + context 'without container registry protection rule' do + let(:container_registry_protection_rule) { nil } + + it { expect { service_execute }.to raise_error(ArgumentError) } + end + + context 'without current_user' do + let(:current_user) { nil } + let(:container_registry_protection_rule) { build_stubbed(:container_registry_protection_rule, project: project) } + + it { expect { service_execute }.to raise_error(ArgumentError) } + end +end diff --git a/spec/services/container_registry/protection/update_rule_service_spec.rb b/spec/services/container_registry/protection/update_rule_service_spec.rb new file mode 100644 index 00000000000..28933b5764a --- /dev/null +++ b/spec/services/container_registry/protection/update_rule_service_spec.rb @@ -0,0 +1,167 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ContainerRegistry::Protection::UpdateRuleService, '#execute', feature_category: :container_registry do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:current_user) { create(:user, maintainer_projects: [project]) } + let_it_be_with_reload(:container_registry_protection_rule) do + create(:container_registry_protection_rule, project: project) + end + + let(:service) { described_class.new(container_registry_protection_rule, current_user: current_user, params: params) } + + let(:params) do + attributes_for( + :container_registry_protection_rule, + repository_path_pattern: "#{container_registry_protection_rule.repository_path_pattern}-updated", + delete_protected_up_to_access_level: 'owner', + push_protected_up_to_access_level: 'owner' + ) + end + + subject(:service_execute) { service.execute } + + shared_examples 'a successful service response' do + let(:expected_attributes) { params } + + it { is_expected.to be_success } + + it do + is_expected.to have_attributes( + errors: be_blank, + message: be_blank, + payload: { + container_registry_protection_rule: + be_a(ContainerRegistry::Protection::Rule) + .and(have_attributes(expected_attributes)) + } + ) + end + + it { expect { subject }.not_to change { ContainerRegistry::Protection::Rule.count } } + + it { subject.tap { expect(container_registry_protection_rule.reload).to have_attributes expected_attributes } } + end + + shared_examples 'an erroneous service response' do + it { is_expected.to be_error } + + it do + is_expected.to have_attributes( + errors: be_present, + message: be_present, + payload: { container_registry_protection_rule: nil } + ) + end + + it { expect { subject }.not_to change { ContainerRegistry::Protection::Rule.count } } + it { expect { subject }.not_to change { container_registry_protection_rule.reload.updated_at } } + end + + it_behaves_like 'a successful service response' + + context 'with disallowed params' do + let(:params) { super().merge!(project_id: 1, unsupported_param: 'unsupported_param_value') } + + it_behaves_like 'a successful service response' do + let(:expected_attributes) { params.except(:project_id, :unsupported_param) } + end + end + + context 'with invalid params' do + using RSpec::Parameterized::TableSyntax + + where(:params_invalid, :message_expected) do + { repository_path_pattern: '' } | [/Repository path pattern can't be blank/] + { delete_protected_up_to_access_level: 1000 } | /not a valid delete_protected_up_to_access_level/ + { push_protected_up_to_access_level: 1000 } | /not a valid push_protected_up_to_access_level/ + end + + with_them do + let(:params) do + super().merge(params_invalid) + end + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes message: message_expected } + end + end + + context 'with empty params' do + let(:params) { {} } + + it_behaves_like 'a successful service response' do + let(:expected_attributes) { container_registry_protection_rule.attributes } + end + + it { expect { service_execute }.not_to change { container_registry_protection_rule.reload.updated_at } } + end + + context 'with nil params' do + let(:params) { nil } + + it_behaves_like 'a successful service response' do + let(:expected_attributes) { container_registry_protection_rule.attributes } + end + + it { expect { service_execute }.not_to change { container_registry_protection_rule.reload.updated_at } } + end + + context 'when updated field `repository_path_pattern` is already taken' do + let_it_be_with_reload(:other_existing_container_registry_protection_rule) do + create(:container_registry_protection_rule, project: project, + repository_path_pattern: "#{container_registry_protection_rule.repository_path_pattern}-other") + end + + let(:params) do + { repository_path_pattern: other_existing_container_registry_protection_rule.repository_path_pattern } + end + + it_behaves_like 'an erroneous service response' + + it do + expect { service_execute }.not_to( + change { other_existing_container_registry_protection_rule.reload.repository_path_pattern } + ) + end + + it do + is_expected.to have_attributes( + errors: match_array([/Repository path pattern has already been taken/]), + message: match_array([/Repository path pattern has already been taken/]) + ) + end + end + + context 'when current_user does not have permission' do + let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } + let_it_be(:reporter) { create(:user).tap { |u| project.add_reporter(u) } } + let_it_be(:guest) { create(:user).tap { |u| project.add_guest(u) } } + let_it_be(:anonymous) { create(:user) } + + where(:current_user) do + [ref(:developer), ref(:reporter), ref(:guest), ref(:anonymous)] + end + + with_them do + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes errors: match_array(/Unauthorized/), message: /Unauthorized/ } + end + end + + context 'without container registry protection rule' do + let(:container_registry_protection_rule) { nil } + let(:params) { {} } + + it { expect { service_execute }.to raise_error(ArgumentError) } + end + + context 'without current_user' do + let(:current_user) { nil } + + it { expect { service_execute }.to raise_error(ArgumentError) } + 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 5534dea85b2..79274599b99 100644 --- a/spec/services/design_management/delete_designs_service_spec.rb +++ b/spec/services/design_management/delete_designs_service_spec.rb @@ -139,7 +139,7 @@ RSpec.describe DesignManagement::DeleteDesignsService, feature_category: :design end it 'informs the new-version-worker' do - expect(::DesignManagement::NewVersionWorker).to receive(:perform_async).with(Integer, false) + expect(DesignManagement::NewVersionWorker).to receive(:perform_async).with(Integer, false) run_service end diff --git a/spec/services/design_management/save_designs_service_spec.rb b/spec/services/design_management/save_designs_service_spec.rb index 8a4dd8b5fc2..4ab0080d8a2 100644 --- a/spec/services/design_management/save_designs_service_spec.rb +++ b/spec/services/design_management/save_designs_service_spec.rb @@ -33,7 +33,7 @@ RSpec.describe DesignManagement::SaveDesignsService, feature_category: :design_m issue.design_collection.repository.raw.delete_all_refs_except([Gitlab::Git::BLANK_SHA]) end - allow(::DesignManagement::NewVersionWorker) + allow(DesignManagement::NewVersionWorker) .to receive(:perform_async).with(Integer, false).and_return(nil) end @@ -293,7 +293,7 @@ RSpec.describe DesignManagement::SaveDesignsService, feature_category: :design_m it 'has the correct side-effects' do counter = Gitlab::UsageDataCounters::DesignsCounter - expect(::DesignManagement::NewVersionWorker) + expect(DesignManagement::NewVersionWorker) .to receive(:perform_async).once.with(Integer, false).and_return(nil) expect { run_service } @@ -327,7 +327,7 @@ RSpec.describe DesignManagement::SaveDesignsService, feature_category: :design_m design_repository.create_if_not_exists design_repository.has_visible_content? - expect(::DesignManagement::NewVersionWorker) + expect(DesignManagement::NewVersionWorker) .to receive(:perform_async).once.with(Integer, false).and_return(nil) expect { service.execute } diff --git a/spec/services/groups/participants_service_spec.rb b/spec/services/groups/participants_service_spec.rb index 8359bf1670f..e934921317d 100644 --- a/spec/services/groups/participants_service_spec.rb +++ b/spec/services/groups/participants_service_spec.rb @@ -22,6 +22,12 @@ RSpec.describe Groups::ParticipantsService, feature_category: :groups_and_projec stub_feature_flags(disable_all_mention: false) end + it 'returns results in correct order' do + expect(service_result.pluck(:username)).to eq([ + 'all', developer.username, parent_group.full_path, subgroup.full_path + ]) + end + it 'includes `All Group Members`' do group.add_developer(create(:user)) diff --git a/spec/services/import/github_service_spec.rb b/spec/services/import/github_service_spec.rb index 39832ee4b13..fc649b61426 100644 --- a/spec/services/import/github_service_spec.rb +++ b/spec/services/import/github_service_spec.rb @@ -5,12 +5,7 @@ require 'spec_helper' RSpec.describe Import::GithubService, feature_category: :importers do let_it_be(:user) { create(:user) } let_it_be(:token) { 'complex-token' } - let_it_be(:access_params) do - { - github_access_token: 'github-complex-token', - additional_access_tokens: %w[foo bar] - } - end + let_it_be(:access_params) { { github_access_token: 'github-complex-token' } } let(:settings) { instance_double(Gitlab::GithubImport::Settings) } let(:user_namespace_path) { user.namespace_path } @@ -37,7 +32,6 @@ RSpec.describe Import::GithubService, feature_category: :importers do .to receive(:write) .with( optional_stages: optional_stages, - additional_access_tokens: access_params[:additional_access_tokens], timeout_strategy: timeout_strategy ) end @@ -98,7 +92,6 @@ RSpec.describe Import::GithubService, feature_category: :importers do expect(settings) .to have_received(:write) .with(optional_stages: nil, - additional_access_tokens: access_params[:additional_access_tokens], timeout_strategy: timeout_strategy ) expect_snowplow_event( @@ -124,7 +117,6 @@ RSpec.describe Import::GithubService, feature_category: :importers do .to have_received(:write) .with( optional_stages: nil, - additional_access_tokens: access_params[:additional_access_tokens], timeout_strategy: timeout_strategy ) expect_snowplow_event( @@ -157,7 +149,6 @@ RSpec.describe Import::GithubService, feature_category: :importers do .to have_received(:write) .with( optional_stages: nil, - additional_access_tokens: access_params[:additional_access_tokens], timeout_strategy: timeout_strategy ) expect_snowplow_event( @@ -194,7 +185,6 @@ RSpec.describe Import::GithubService, feature_category: :importers do .to have_received(:write) .with( optional_stages: optional_stages, - additional_access_tokens: access_params[:additional_access_tokens], timeout_strategy: timeout_strategy ) end @@ -210,7 +200,6 @@ RSpec.describe Import::GithubService, feature_category: :importers do .to have_received(:write) .with( optional_stages: optional_stages, - additional_access_tokens: access_params[:additional_access_tokens], timeout_strategy: timeout_strategy ) end @@ -224,7 +213,6 @@ RSpec.describe Import::GithubService, feature_category: :importers do .to have_received(:write) .with( optional_stages: optional_stages, - additional_access_tokens: %w[foo bar], timeout_strategy: timeout_strategy ) end diff --git a/spec/services/integrations/google_cloud_platform/artifact_registry/list_docker_images_service_spec.rb b/spec/services/integrations/google_cloud_platform/artifact_registry/list_docker_images_service_spec.rb new file mode 100644 index 00000000000..3f57add10e3 --- /dev/null +++ b/spec/services/integrations/google_cloud_platform/artifact_registry/list_docker_images_service_spec.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Integrations::GoogleCloudPlatform::ArtifactRegistry::ListDockerImagesService, feature_category: :container_registry do + let_it_be(:project) { create(:project, :private) } + + let(:user) { project.owner } + let(:gcp_project_id) { 'gcp_project_id' } + let(:gcp_location) { 'gcp_location' } + let(:gcp_repository) { 'gcp_repository' } + let(:gcp_wlif) { 'https://wlif.test' } + let(:service) do + described_class.new( + project: project, + current_user: user, + params: { + gcp_project_id: gcp_project_id, + gcp_location: gcp_location, + gcp_repository: gcp_repository, + gcp_wlif: gcp_wlif + } + ) + end + + describe '#execute' do + let(:page_token) { nil } + let(:list_docker_images_response) { dummy_list_response } + let(:client_double) { instance_double('::Integrations::GoogleCloudPlatform::ArtifactRegistry::Client') } + + before do + allow(::Integrations::GoogleCloudPlatform::ArtifactRegistry::Client).to receive(:new) + .with( + project: project, + user: user, + gcp_project_id: gcp_project_id, + gcp_location: gcp_location, + gcp_repository: gcp_repository, + gcp_wlif: gcp_wlif + ).and_return(client_double) + allow(client_double).to receive(:list_docker_images) + .with(page_token: page_token) + .and_return(list_docker_images_response) + end + + subject(:list) { service.execute(page_token: page_token) } + + it 'returns the docker images' do + expect(list).to be_success + expect(list.payload).to include(images: an_instance_of(Array), next_page_token: an_instance_of(String)) + end + + context 'with the client returning an empty hash' do + let(:list_docker_images_response) { {} } + + it 'returns an empty hash' do + expect(list).to be_success + expect(list.payload).to eq({}) + end + end + + context 'with not enough permissions' do + let_it_be(:user) { create(:user) } + + it 'returns an error response' do + expect(list).to be_error + expect(list.message).to eq('Access denied') + end + end + + private + + def dummy_list_response + { + images: [ + { + built_at: '2023-11-30T23:23:11.980068941Z', + media_type: 'application/vnd.docker.distribution.manifest.v2+json', + name: 'projects/project/locations/location/repositories/repo/dockerImages/image@sha256:6a', + size_bytes: 2827903, + tags: %w[tag1 tag2], + updated_at: '2023-12-07T11:48:50.840751Z', + uploaded_at: '2023-12-07T11:48:47.598511Z', + uri: 'location.pkg.dev/project/repo/image@sha256:6a' + } + ], + next_page_token: 'next_page_token' + } + end + end +end diff --git a/spec/services/issue_email_participants/create_service_spec.rb b/spec/services/issue_email_participants/create_service_spec.rb new file mode 100644 index 00000000000..fcfdeeb08f3 --- /dev/null +++ b/spec/services/issue_email_participants/create_service_spec.rb @@ -0,0 +1,161 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IssueEmailParticipants::CreateService, feature_category: :service_desk do + shared_examples 'a successful service execution' do + it 'creates new participants', :aggregate_failures do + response = service.execute + expect(response).to be_success + + issue.reset + note = issue.notes.last + expect(note.system?).to be true + expect(note.author).to eq(user) + + participants_emails = issue.email_participants_emails_downcase + + expected_emails.each do |email| + expect(participants_emails).to include(email) + expect(response.message).to include(email) + expect(note.note).to include(email) + end + end + end + + shared_examples 'a failed service execution' do + it 'returns error ServiceResponse with message', :aggregate_failures do + response = service.execute + expect(response).to be_error + expect(response.message).to eq(error_message) + end + end + + describe '#execute' do + let_it_be_with_reload(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be_with_reload(:issue) { create(:issue, project: project) } + + let(:emails) { nil } + let(:service) { described_class.new(target: issue, current_user: user, emails: emails) } + let(:expected_emails) { emails } + + let(:error_feature_flag) { "Feature flag issue_email_participants is not enabled for this project." } + let(:error_underprivileged) { _("You don't have permission to add email participants.") } + let(:error_no_participants) do + _("No email participants were added. Either none were provided, or they already exist.") + end + + context 'when the user is not a project member' do + let(:error_message) { error_underprivileged } + + it_behaves_like 'a failed service execution' + end + + context 'when user has reporter role in project' do + before_all do + project.add_reporter(user) + end + + context 'when no emails are provided' do + let(:error_message) { error_no_participants } + + it_behaves_like 'a failed service execution' + end + + context 'when one email is provided' do + let(:emails) { ['user@example.com'] } + + it_behaves_like 'a successful service execution' + + context 'when email is already a participant of the issue' do + let(:error_message) { error_no_participants } + + before do + issue.issue_email_participants.create!(email: emails.first) + end + + it_behaves_like 'a failed service execution' + + context 'when email is formatted in a different case' do + let(:emails) { ['USER@example.com'] } + + it_behaves_like 'a failed service execution' + end + + context 'when participants limit on issue is reached' do + before do + stub_const("#{described_class}::MAX_NUMBER_OF_RECORDS", 1) + end + + let(:emails) { ['over-max@example.com'] } + let(:error_message) { error_no_participants } + + it_behaves_like 'a failed service execution' + + it 'logs count of emails above limit' do + expect(Gitlab::AppLogger).to receive(:info).with({ above_limit_count: 1 }).once + service.execute + end + end + end + end + + context 'when multiple emails are provided' do + let(:emails) { ['user@example.com', 'other-user@example.com'] } + + it_behaves_like 'a successful service execution' + + context 'when duplicate email provided' do + let(:emails) { ['user@example.com', 'user@example.com'] } + let(:expected_emails) { emails[...-1] } + + it_behaves_like 'a successful service execution' + end + + context 'when an email is already a participant of the issue' do + let(:expected_emails) { emails[1...] } + + before do + issue.issue_email_participants.create!(email: emails.first) + end + + it_behaves_like 'a successful service execution' + end + + context 'when only some emails can be added because of participants limit' do + before do + stub_const("#{described_class}::MAX_NUMBER_OF_RECORDS", 1) + end + + let(:expected_emails) { emails[...-1] } + + it_behaves_like 'a successful service execution' + + it 'logs count of emails above limit' do + expect(Gitlab::AppLogger).to receive(:info).with({ above_limit_count: 1 }).once + service.execute + end + end + end + + context 'when more than the allowed number of emails are provided' do + let(:emails) { (1..7).map { |i| "user#{i}@example.com" } } + + let(:expected_emails) { emails[...-1] } + + it_behaves_like 'a successful service execution' + end + end + + context 'when feature flag issue_email_participants is disabled' do + let(:error_message) { error_feature_flag } + + before do + stub_feature_flags(issue_email_participants: false) + end + + it_behaves_like 'a failed service execution' + end + end +end diff --git a/spec/services/issues/import_csv_service_spec.rb b/spec/services/issues/import_csv_service_spec.rb index 660686cf805..d3d7277e3e3 100644 --- a/spec/services/issues/import_csv_service_spec.rb +++ b/spec/services/issues/import_csv_service_spec.rb @@ -14,6 +14,8 @@ RSpec.describe Issues::ImportCsvService, feature_category: :team_planning do described_class.new(user, project, uploader) end + let!(:test_milestone) { create(:milestone, project: project, title: '15.10') } + include_examples 'issuable import csv service', 'issue' do let(:issuables) { project.issues } let(:email_method) { :import_issues_csv_email } @@ -36,7 +38,8 @@ RSpec.describe Issues::ImportCsvService, feature_category: :team_planning do description: 'Description', time_estimate: 3600, assignees: include(assignee), - due_date: Date.new(2022, 6, 28) + due_date: Date.new(2022, 6, 28), + milestone_id: test_milestone.id ) ) end diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb index e7fe5c19fa3..8761aba432f 100644 --- a/spec/services/merge_requests/approval_service_spec.rb +++ b/spec/services/merge_requests/approval_service_spec.rb @@ -13,6 +13,7 @@ RSpec.describe MergeRequests::ApprovalService, feature_category: :code_review_wo before do project.add_developer(user) + stub_feature_flags ff_require_saml_auth_to_approve: false end context 'with invalid approval' do diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 25c75ae7244..199f5e3fd9a 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -42,7 +42,7 @@ RSpec.describe MergeRequests::CloseService, feature_category: :code_review_workf .with(@merge_request, 'close') end - it 'sends email to user2 about assign of new merge_request', :sidekiq_might_not_need_inline do + it 'sends email to user2 about assign of new merge_request', :sidekiq_inline do email = ActionMailer::Base.deliveries.last expect(email.to.first).to eq(user2.email) expect(email.subject).to include(merge_request.title) diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 51b1bed1dd3..bf52800b77e 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -52,7 +52,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state, f end.to change { project.open_merge_requests_count }.from(0).to(1) end - it 'creates exactly 1 create MR event', :sidekiq_might_not_need_inline do + it 'creates exactly 1 create MR event', :sidekiq_inline do attributes = { action: :created, target_id: merge_request.id, diff --git a/spec/services/merge_requests/mergeability/check_base_service_spec.rb b/spec/services/merge_requests/mergeability/check_base_service_spec.rb index 806bde61c23..05f5b4f1315 100644 --- a/spec/services/merge_requests/mergeability/check_base_service_spec.rb +++ b/spec/services/merge_requests/mergeability/check_base_service_spec.rb @@ -8,6 +8,22 @@ RSpec.describe MergeRequests::Mergeability::CheckBaseService, feature_category: let(:merge_request) { double } let(:params) { double } + describe '.identifier' do + it 'sets the identifier' do + described_class.identifier("test") + + expect(described_class.identifier).to eq("test") + end + end + + describe '.description' do + it 'sets the description' do + described_class.description("test") + + expect(described_class.description).to eq("test") + end + end + describe '#merge_request' do it 'returns the merge_request' do expect(check_base_service.merge_request).to eq merge_request diff --git a/spec/services/merge_requests/mergeability/check_broken_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_broken_status_service_spec.rb index b6ee1049bb9..f29289be86b 100644 --- a/spec/services/merge_requests/mergeability/check_broken_status_service_spec.rb +++ b/spec/services/merge_requests/mergeability/check_broken_status_service_spec.rb @@ -7,6 +7,8 @@ RSpec.describe MergeRequests::Mergeability::CheckBrokenStatusService, feature_ca let(:merge_request) { build(:merge_request) } + it_behaves_like 'mergeability check service', :broken_status, 'Checks whether the merge request is broken' + describe '#execute' do let(:result) { check_broken_status.execute } @@ -19,7 +21,7 @@ RSpec.describe MergeRequests::Mergeability::CheckBrokenStatusService, feature_ca it 'returns a check result with status failed' do expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS - expect(result.payload[:reason]).to eq(:broken_status) + expect(result.payload[:identifier]).to eq(:broken_status) end end diff --git a/spec/services/merge_requests/mergeability/check_ci_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_ci_status_service_spec.rb index 067e87859e7..aa7920b9b40 100644 --- a/spec/services/merge_requests/mergeability/check_ci_status_service_spec.rb +++ b/spec/services/merge_requests/mergeability/check_ci_status_service_spec.rb @@ -10,6 +10,8 @@ RSpec.describe MergeRequests::Mergeability::CheckCiStatusService, feature_catego let(:params) { { skip_ci_check: skip_check } } let(:skip_check) { false } + it_behaves_like 'mergeability check service', :ci_must_pass, 'Checks whether CI has passed' + describe '#execute' do let(:result) { check_ci_status.execute } @@ -39,7 +41,7 @@ RSpec.describe MergeRequests::Mergeability::CheckCiStatusService, feature_catego it 'returns a check result with status failed' do expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS - expect(result.payload[:reason]).to eq :ci_must_pass + expect(result.payload[:identifier]).to eq :ci_must_pass end end end diff --git a/spec/services/merge_requests/mergeability/check_conflict_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_conflict_status_service_spec.rb index 14173c19bfb..e35de4d4042 100644 --- a/spec/services/merge_requests/mergeability/check_conflict_status_service_spec.rb +++ b/spec/services/merge_requests/mergeability/check_conflict_status_service_spec.rb @@ -7,6 +7,8 @@ RSpec.describe MergeRequests::Mergeability::CheckConflictStatusService, feature_ let(:merge_request) { build(:merge_request) } + it_behaves_like 'mergeability check service', :conflict, 'Checks whether the merge request has a conflict' + describe '#execute' do let(:result) { check_conflict_status.execute } @@ -27,7 +29,7 @@ RSpec.describe MergeRequests::Mergeability::CheckConflictStatusService, feature_ it 'returns a check result with status failed' do expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS - expect(result.payload[:reason]).to eq(:conflict) + expect(result.payload[:identifier]).to eq(:conflict) end end end diff --git a/spec/services/merge_requests/mergeability/check_discussions_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_discussions_status_service_spec.rb index 4a8b28f603d..3d1fe0c838d 100644 --- a/spec/services/merge_requests/mergeability/check_discussions_status_service_spec.rb +++ b/spec/services/merge_requests/mergeability/check_discussions_status_service_spec.rb @@ -10,6 +10,9 @@ RSpec.describe MergeRequests::Mergeability::CheckDiscussionsStatusService, featu let(:params) { { skip_discussions_check: skip_check } } let(:skip_check) { false } + it_behaves_like 'mergeability check service', :discussions_not_resolved, + 'Checks whether the merge request has open discussions' + describe '#execute' do let(:result) { check_discussions_status.execute } @@ -39,7 +42,7 @@ RSpec.describe MergeRequests::Mergeability::CheckDiscussionsStatusService, featu it 'returns a check result with status failed' do expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS - expect(result.payload[:reason]).to eq(:discussions_not_resolved) + expect(result.payload[:identifier]).to eq(:discussions_not_resolved) end end end diff --git a/spec/services/merge_requests/mergeability/check_draft_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_draft_status_service_spec.rb index 3837022232d..cef8169e725 100644 --- a/spec/services/merge_requests/mergeability/check_draft_status_service_spec.rb +++ b/spec/services/merge_requests/mergeability/check_draft_status_service_spec.rb @@ -10,6 +10,8 @@ RSpec.describe MergeRequests::Mergeability::CheckDraftStatusService, feature_cat let(:params) { { skip_draft_check: skip_check } } let(:skip_check) { false } + it_behaves_like 'mergeability check service', :draft_status, 'Checks whether the merge request is draft' + describe '#execute' do let(:result) { check_draft_status.execute } @@ -22,7 +24,7 @@ RSpec.describe MergeRequests::Mergeability::CheckDraftStatusService, feature_cat it 'returns a check result with status failed' do expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS - expect(result.payload[:reason]).to eq(:draft_status) + expect(result.payload[:identifier]).to eq(:draft_status) end end diff --git a/spec/services/merge_requests/mergeability/check_open_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_open_status_service_spec.rb index 53ad77ea4df..f673e43931d 100644 --- a/spec/services/merge_requests/mergeability/check_open_status_service_spec.rb +++ b/spec/services/merge_requests/mergeability/check_open_status_service_spec.rb @@ -7,6 +7,8 @@ RSpec.describe MergeRequests::Mergeability::CheckOpenStatusService, feature_cate let(:merge_request) { build(:merge_request) } + it_behaves_like 'mergeability check service', :not_open, 'Checks whether the merge request is open' + describe '#execute' do let(:result) { check_open_status.execute } @@ -27,7 +29,7 @@ RSpec.describe MergeRequests::Mergeability::CheckOpenStatusService, feature_cate it 'returns a check result with status failed' do expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS - expect(result.payload[:reason]).to eq(:not_open) + expect(result.payload[:identifier]).to eq(:not_open) end end end diff --git a/spec/services/merge_requests/mergeability/check_rebase_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_rebase_status_service_spec.rb index d6948f72c0a..047cf5c13bf 100644 --- a/spec/services/merge_requests/mergeability/check_rebase_status_service_spec.rb +++ b/spec/services/merge_requests/mergeability/check_rebase_status_service_spec.rb @@ -10,6 +10,8 @@ RSpec.describe MergeRequests::Mergeability::CheckRebaseStatusService, feature_ca let(:params) { { skip_rebase_check: skip_check } } let(:skip_check) { false } + it_behaves_like 'mergeability check service', :need_rebase, 'Checks whether the merge request needs to be rebased' + describe '#execute' do let(:result) { check_rebase_status.execute } @@ -31,7 +33,7 @@ RSpec.describe MergeRequests::Mergeability::CheckRebaseStatusService, feature_ca it 'returns a check result with status failed' do expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS - expect(result.payload[:reason]).to eq(:need_rebase) + expect(result.payload[:identifier]).to eq(:need_rebase) end end diff --git a/spec/services/merge_requests/mergeability/detailed_merge_status_service_spec.rb b/spec/services/merge_requests/mergeability/detailed_merge_status_service_spec.rb index 66bcb948cb6..a3c5427ee82 100644 --- a/spec/services/merge_requests/mergeability/detailed_merge_status_service_spec.rb +++ b/spec/services/merge_requests/mergeability/detailed_merge_status_service_spec.rb @@ -64,7 +64,7 @@ RSpec.describe ::MergeRequests::Mergeability::DetailedMergeStatusService, featur merge_request.close! end - it 'returns the failure reason' do + it 'returns the failed check' do expect(detailed_merge_status).to eq(:not_open) end end @@ -77,7 +77,7 @@ RSpec.describe ::MergeRequests::Mergeability::DetailedMergeStatusService, featur end context 'when pipeline does not exist' do - it 'returns the failure reason' do + it 'returns the failed check' do expect(detailed_merge_status).to eq(:ci_must_pass) end end @@ -97,15 +97,21 @@ RSpec.describe ::MergeRequests::Mergeability::DetailedMergeStatusService, featur context 'when the pipeline is running' do let(:ci_status) { :running } - it 'returns the failure reason' do + it 'returns the failed check' do expect(detailed_merge_status).to eq(:ci_still_running) end end + context 'when the pipeline is pending' do + let(:ci_status) { :pending } + + it { expect(detailed_merge_status).to eq(:ci_still_running) } + end + context 'when the pipeline is not running' do let(:ci_status) { :failed } - it 'returns the failure reason' do + it 'returns the failed check' do expect(detailed_merge_status).to eq(:ci_must_pass) end end diff --git a/spec/services/merge_requests/mergeability/run_checks_service_spec.rb b/spec/services/merge_requests/mergeability/run_checks_service_spec.rb index 06e15356a92..a67b01b8069 100644 --- a/spec/services/merge_requests/mergeability/run_checks_service_spec.rb +++ b/spec/services/merge_requests/mergeability/run_checks_service_spec.rb @@ -77,7 +77,7 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService, :clean_gitlab_redi end context 'when one check fails' do - let(:failed_result) { Gitlab::MergeRequests::Mergeability::CheckResult.failed(payload: { reason: 'failed' }) } + let(:failed_result) { Gitlab::MergeRequests::Mergeability::CheckResult.failed(payload: { identifier: 'failed' }) } before do allow_next_instance_of(MergeRequests::Mergeability::CheckOpenStatusService) do |service| @@ -86,11 +86,11 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService, :clean_gitlab_redi end end - it 'returns the failure reason' do + it 'returns the failed check' do result = execute expect(result.success?).to eq(false) - expect(execute.payload[:failure_reason]).to eq(:failed) + expect(execute.payload[:failed_check]).to eq(:failed) end it_behaves_like 'checks are all executed' do diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index f7526c169bd..61c754e30a9 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -153,5 +153,17 @@ RSpec.describe MergeRequests::PostMergeService, feature_category: :code_review_w expect(deploy_job.reload.canceled?).to be false end end + + context 'when the merge request has a pages deployment' do + it 'performs Pages::DeactivateMrDeploymentWorker asynchronously' do + expect(Pages::DeactivateMrDeploymentsWorker) + .to receive(:perform_async) + .with(merge_request) + + subject + + expect(merge_request.reload).to be_merged + end + end end end diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb index e173cd382f2..f3ac55059bc 100644 --- a/spec/services/merge_requests/reopen_service_spec.rb +++ b/spec/services/merge_requests/reopen_service_spec.rb @@ -44,7 +44,7 @@ RSpec.describe MergeRequests::ReopenService, feature_category: :code_review_work expect(Integrations::GroupMentionWorker).not_to receive(:perform_async) end - it 'sends email to user2 about reopen of merge_request', :sidekiq_might_not_need_inline do + it 'sends email to user2 about reopen of merge_request', :sidekiq_inline do email = ActionMailer::Base.deliveries.last expect(email.to.first).to eq(user2.email) expect(email.subject).to include(merge_request.title) diff --git a/spec/services/ml/create_candidate_service_spec.rb b/spec/services/ml/create_candidate_service_spec.rb index fb3456b0bcc..b1a053711d7 100644 --- a/spec/services/ml/create_candidate_service_spec.rb +++ b/spec/services/ml/create_candidate_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe ::Ml::CreateCandidateService, feature_category: :mlops do describe '#execute' do - let_it_be(:model_version) { create(:ml_model_versions) } + let_it_be(:model_version) { create(:ml_model_versions, candidate: nil) } let_it_be(:experiment) { create(:ml_experiments, project: model_version.project) } let(:params) { {} } diff --git a/spec/services/ml/create_model_service_spec.rb b/spec/services/ml/create_model_service_spec.rb index 212f0940635..74c1dd5fec7 100644 --- a/spec/services/ml/create_model_service_spec.rb +++ b/spec/services/ml/create_model_service_spec.rb @@ -9,6 +9,10 @@ RSpec.describe ::Ml::CreateModelService, feature_category: :mlops do let_it_be(:description) { 'description' } let_it_be(:metadata) { [] } + before do + allow(Gitlab::InternalEvents).to receive(:track_event) + end + subject(:create_model) { described_class.new(project, name, user, description, metadata).execute } describe '#execute' do @@ -18,6 +22,10 @@ RSpec.describe ::Ml::CreateModelService, feature_category: :mlops do it 'creates a model', :aggregate_failures do expect { create_model }.to change { Ml::Model.count }.by(1) + expect(Gitlab::InternalEvents).to have_received(:track_event).with( + 'model_registry_ml_model_created', + { project: project, user: user } + ) expect(create_model.name).to eq(name) end @@ -29,6 +37,10 @@ RSpec.describe ::Ml::CreateModelService, feature_category: :mlops do it 'creates a model', :aggregate_failures do expect { create_model }.to change { Ml::Model.count }.by(1) + expect(Gitlab::InternalEvents).to have_received(:track_event).with( + 'model_registry_ml_model_created', + { project: project, user: user } + ) expect(create_model.name).to eq(name) end @@ -40,6 +52,7 @@ RSpec.describe ::Ml::CreateModelService, feature_category: :mlops do it 'raises an error', :aggregate_failures do expect { create_model }.to raise_error(ActiveRecord::RecordInvalid) + expect(Gitlab::InternalEvents).not_to have_received(:track_event) end end diff --git a/spec/services/ml/create_model_version_service_spec.rb b/spec/services/ml/create_model_version_service_spec.rb new file mode 100644 index 00000000000..b3aead4a92c --- /dev/null +++ b/spec/services/ml/create_model_version_service_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ml::CreateModelVersionService, feature_category: :mlops do + let(:model) { create(:ml_models) } + let(:params) { {} } + + before do + allow(Gitlab::InternalEvents).to receive(:track_event) + end + + subject(:service) { described_class.new(model, params).execute } + + context 'when no versions exist' do + it 'creates a model version', :aggregate_failures do + expect { service }.to change { Ml::ModelVersion.count }.by(1).and change { Ml::Candidate.count }.by(1) + expect(model.reload.latest_version.version).to eq('1.0.0') + + expect(Gitlab::InternalEvents).to have_received(:track_event).with( + 'model_registry_ml_model_version_created', + { project: model.project, user: nil } + ) + end + end + + context 'when a version exist' do + before do + create(:ml_model_versions, model: model, version: '3.0.0') + end + + it 'creates another model version and increments the version number', :aggregate_failures do + expect { service }.to change { Ml::ModelVersion.count }.by(1).and change { Ml::Candidate.count }.by(1) + expect(model.reload.latest_version.version).to eq('4.0.0') + + expect(Gitlab::InternalEvents).to have_received(:track_event).with( + 'model_registry_ml_model_version_created', + { project: model.project, user: nil } + ) + end + end + + context 'when a version is created' do + it 'creates a package' do + expect { service }.to change { Ml::ModelVersion.count }.by(1).and change { + Packages::MlModel::Package.count + }.by(1) + expect(model.reload.latest_version.package.name).to eq(model.name) + expect(model.latest_version.package.version).to eq(model.latest_version.version) + end + end + + context 'when a version is created and the package already exists' do + it 'does not creates a package' do + next_version = Ml::IncrementVersionService.new(model.latest_version.try(:version)).execute + create(:ml_model_package, name: model.name, version: next_version, project: model.project) + + expect { service }.to change { Ml::ModelVersion.count }.by(1).and not_change { + Packages::MlModel::Package.count + } + expect(model.reload.latest_version.package.name).to eq(model.name) + expect(model.latest_version.package.version).to eq(model.latest_version.version) + end + end + + context 'when a version is created and an existing package supplied' do + it 'does not creates a package' do + next_version = Ml::IncrementVersionService.new(model.latest_version.try(:version)).execute + package = create(:ml_model_package, name: model.name, version: next_version, project: model.project) + service = described_class.new(model, { package: package }) + + expect { service.execute }.to change { Ml::ModelVersion.count }.by(1).and not_change { + Packages::MlModel::Package.count + } + expect(model.reload.latest_version.package.name).to eq(model.name) + expect(model.latest_version.package.version).to eq(model.latest_version.version) + end + end +end diff --git a/spec/services/ml/destroy_model_service_spec.rb b/spec/services/ml/destroy_model_service_spec.rb new file mode 100644 index 00000000000..79914ff8b22 --- /dev/null +++ b/spec/services/ml/destroy_model_service_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ml::DestroyModelService, feature_category: :mlops do + let_it_be(:user) { create(:user) } + let_it_be(:model) { create(:ml_models, :with_latest_version_and_package) } + let(:service) { described_class.new(model, user) } + + describe '#execute' do + context 'when model name does not exist in the project' do + it 'returns nil' do + allow(model).to receive(:destroy).and_return(false) + expect(service.execute).to be nil + end + end + + context 'when a model exists' do + it 'destroys the model' do + expect(Packages::MarkPackagesForDestructionService).to receive(:new).with(packages: model.all_packages, + current_user: user).and_return(instance_double('Packages::MarkPackagesForDestructionService').tap do |service| + expect(service).to receive(:execute) + end) + expect { service.execute }.to change { Ml::Model.count }.by(-1).and change { Ml::ModelVersion.count }.by(-1) + end + end + end +end diff --git a/spec/services/ml/find_or_create_model_version_service_spec.rb b/spec/services/ml/find_or_create_model_version_service_spec.rb index e5ca7c3a450..88647f23ad9 100644 --- a/spec/services/ml/find_or_create_model_version_service_spec.rb +++ b/spec/services/ml/find_or_create_model_version_service_spec.rb @@ -35,21 +35,29 @@ RSpec.describe ::Ml::FindOrCreateModelVersionService, feature_category: :mlops d end end - context 'when model version does not exist' do + context 'when model does not exist' do let(:project) { existing_version.project } let(:name) { 'a_new_model' } let(:version) { '2.0.0' } + + it 'does not create a new model version', :aggregate_failures do + expect { model_version }.to change { Ml::ModelVersion.count }.by(0) + end + end + + context 'when model exists and model version does not' do + let(:project) { existing_version.project } + let(:name) { existing_version.name } + let(:version) { '2.0.0' } let(:description) { 'A model version' } let(:package) { create(:ml_model_package, project: project, name: name, version: version) } it 'creates a new model version', :aggregate_failures do - expect { model_version }.to change { Ml::ModelVersion.count }.by(1).and change { Ml::Candidate.count }.by(1) + expect { model_version }.to change { Ml::ModelVersion.count }.by(1) - expect(model_version.name).to eq(name) expect(model_version.version).to eq(version) - expect(model_version.package).to eq(package) - expect(model_version.candidate.model_version_id).to eq(model_version.id) + expect(model_version.model).to eq(existing_version.model) expect(model_version.description).to eq(description) end end diff --git a/spec/services/ml/increment_version_service_spec.rb b/spec/services/ml/increment_version_service_spec.rb new file mode 100644 index 00000000000..7e8bf153e63 --- /dev/null +++ b/spec/services/ml/increment_version_service_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ml::IncrementVersionService, feature_category: :mlops do + describe '#execute' do + let(:increment_type) { nil } + let(:finder) { described_class.new(version, increment_type) } + + context 'when given an invalid version format' do + let(:version) { 'foo' } + + it 'raises an error' do + expect { finder.execute }.to raise_error(RuntimeError, "Version must be in a valid SemVer format") + end + end + + context 'when given a non-semver version format' do + let(:version) { 1 } + + it 'raises an error' do + expect { finder.execute }.to raise_error(RuntimeError, "Version must be in a valid SemVer format") + end + end + + context 'when given an unsupported increment type' do + let(:version) { '1.2.3' } + let(:increment_type) { 'foo' } + + it 'raises an error' do + expect do + finder.execute + end.to raise_error(RuntimeError, "Increment type must be one of :patch, :minor, or :major") + end + end + + context 'when valid inputs are provided' do + using RSpec::Parameterized::TableSyntax + + where(:version, :increment_type, :result) do + nil | nil | '1.0.0' + '0.0.1' | nil | '1.0.1' + '1.0.0' | nil | '2.0.0' + '1.0.0' | :major | '2.0.0' + '1.0.0' | :minor | '1.1.0' + '1.0.0' | :patch | '1.0.1' + end + + with_them do + subject { finder.execute } + + it { is_expected.to eq(result) } + end + end + end +end diff --git a/spec/services/ml/model_versions/delete_service_spec.rb b/spec/services/ml/model_versions/delete_service_spec.rb new file mode 100644 index 00000000000..1cc5a2f85a5 --- /dev/null +++ b/spec/services/ml/model_versions/delete_service_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ml::ModelVersions::DeleteService, feature_category: :mlops do + let_it_be(:valid_model_version) do + create(:ml_model_versions, :with_package) + end + + let(:project) { valid_model_version.project } + let(:user) { valid_model_version.project.owner } + let(:name) { valid_model_version.name } + let(:version) { valid_model_version.version } + + subject(:execute_service) { described_class.new(project, name, version, user).execute } + + describe '#execute' do + context 'when model version exists' do + it 'deletes the model version', :aggregate_failures do + expect(execute_service).to be_success + expect(Ml::ModelVersion.find_by(id: valid_model_version.id)).to be_nil + end + end + + context 'when model version does not exist' do + let(:version) { 'wrong-version' } + + it { is_expected.to be_error.and have_attributes(message: 'Model not found') } + end + + context 'when model version has no package' do + before do + valid_model_version.update!(package: nil) + end + + it 'does not trigger destroy package service', :aggregate_failures do + expect(Packages::MarkPackageForDestructionService).not_to receive(:new) + expect(execute_service).to be_success + end + end + + context 'when package cannot be marked for destruction' do + before do + allow_next_instance_of(Packages::MarkPackageForDestructionService) do |service| + allow(service).to receive(:execute).and_return(ServiceResponse.error(message: 'error')) + end + end + + it 'does not delete the model version', :aggregate_failures do + is_expected.to be_error.and have_attributes(message: 'error') + expect(Ml::ModelVersion.find_by(id: valid_model_version.id)).to eq(valid_model_version) + end + end + end +end diff --git a/spec/services/ml/model_versions/update_model_version_service_spec.rb b/spec/services/ml/model_versions/update_model_version_service_spec.rb new file mode 100644 index 00000000000..99ea8b81df3 --- /dev/null +++ b/spec/services/ml/model_versions/update_model_version_service_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ml::ModelVersions::UpdateModelVersionService, feature_category: :mlops do + let_it_be(:existing_version) { create(:ml_model_versions) } + + let(:project) { existing_version.project } + let(:name) { existing_version.name } + let(:version) { existing_version.version } + let(:description) { 'A model version description' } + + subject(:execute_service) { described_class.new(project, name, version, description).execute } + + describe '#execute' do + context 'when model version exists' do + it { is_expected.to be_success } + + it 'updates the model version description' do + execute_service + + expect(execute_service.payload.description).to eq(description) + end + end + + context 'when description is invalid' do + let(:description) { 'a' * 501 } + + it { is_expected.to be_error } + end + + context 'when model does not exist' do + let(:name) { 'a_new_model' } + + it { is_expected.to be_error } + end + + context 'when model version does not exist' do + let(:name) { '2.0.0' } + + it { is_expected.to be_error } + end + end +end diff --git a/spec/services/namespaces/package_settings/update_service_spec.rb b/spec/services/namespaces/package_settings/update_service_spec.rb index 8a4a51e409c..41f3499a1bb 100644 --- a/spec/services/namespaces/package_settings/update_service_spec.rb +++ b/spec/services/namespaces/package_settings/update_service_spec.rb @@ -45,7 +45,8 @@ RSpec.describe ::Namespaces::PackageSettings::UpdateService, feature_category: : npm_package_requests_forwarding: nil, lock_npm_package_requests_forwarding: false, pypi_package_requests_forwarding: nil, - lock_pypi_package_requests_forwarding: false + lock_pypi_package_requests_forwarding: false, + nuget_symbol_server_enabled: false }, to: { maven_duplicates_allowed: false, maven_duplicate_exception_regex: 'RELEASE', @@ -58,7 +59,8 @@ RSpec.describe ::Namespaces::PackageSettings::UpdateService, feature_category: : npm_package_requests_forwarding: true, lock_npm_package_requests_forwarding: true, pypi_package_requests_forwarding: true, - lock_pypi_package_requests_forwarding: true + lock_pypi_package_requests_forwarding: true, + nuget_symbol_server_enabled: true } it_behaves_like 'returning a success' @@ -109,7 +111,8 @@ RSpec.describe ::Namespaces::PackageSettings::UpdateService, feature_category: : npm_package_requests_forwarding: true, lock_npm_package_requests_forwarding: true, pypi_package_requests_forwarding: true, - lock_pypi_package_requests_forwarding: true + lock_pypi_package_requests_forwarding: true, + nuget_symbol_server_enabled: true } end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index c1b15ec7681..a46a1438d18 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -395,27 +395,12 @@ RSpec.describe Notes::CreateService, feature_category: :team_planning do context 'is ipynb file' do before do allow_any_instance_of(::Gitlab::Diff::File).to receive(:ipynb?).and_return(true) - stub_feature_flags(ipynbdiff_notes_tracker: false) end - context ':ipynbdiff_notes_tracker is off' do - it 'does not track ipynb note usage data' do - expect(::Gitlab::UsageDataCounters::IpynbDiffActivityCounter).not_to receive(:note_created) + it 'tracks ipynb diff note creation' do + expect(::Gitlab::UsageDataCounters::IpynbDiffActivityCounter).to receive(:note_created) - described_class.new(project_with_repo, user, new_opts).execute - end - end - - context ':ipynbdiff_notes_tracker is on' do - before do - stub_feature_flags(ipynbdiff_notes_tracker: true) - end - - it 'tracks ipynb diff note creation' do - expect(::Gitlab::UsageDataCounters::IpynbDiffActivityCounter).to receive(:note_created) - - described_class.new(project_with_repo, user, new_opts).execute - end + described_class.new(project_with_repo, user, new_opts).execute end end end diff --git a/spec/services/organizations/create_service_spec.rb b/spec/services/organizations/create_service_spec.rb index 7d9bf64ddd3..aae89517c15 100644 --- a/spec/services/organizations/create_service_spec.rb +++ b/spec/services/organizations/create_service_spec.rb @@ -7,7 +7,10 @@ RSpec.describe Organizations::CreateService, feature_category: :cell do let_it_be(:user) { create(:user) } let(:current_user) { user } - let(:params) { attributes_for(:organization) } + let(:params) { attributes_for(:organization).merge(extra_params) } + let(:avatar_filename) { nil } + let(:extra_params) { {} } + let(:created_organization) { response.payload[:organization] } subject(:response) { described_class.new(current_user: current_user, params: params).execute } @@ -23,17 +26,41 @@ RSpec.describe Organizations::CreateService, feature_category: :cell do end context 'when user has permission' do - it 'creates an organization' do - expect { response }.to change { Organizations::Organization.count } + shared_examples 'creating an organization' do + it 'creates the organization' do + expect { response }.to change { Organizations::Organization.count } + expect(response).to be_success + expect(created_organization.name).to eq(params[:name]) + expect(created_organization.path).to eq(params[:path]) + expect(created_organization.description).to eq(params[:description]) + expect(created_organization.avatar.filename).to eq(avatar_filename) + end + end + + it_behaves_like 'creating an organization' - expect(response).to be_success + context 'with description' do + let(:description) { 'Organization description' } + let(:extra_params) { { description: description } } + + it_behaves_like 'creating an organization' end - it 'returns an error when the organization is not persisted' do - params[:name] = nil + context 'with avatar' do + let(:avatar_filename) { 'dk.png' } + let(:avatar) { fixture_file_upload("spec/fixtures/#{avatar_filename}") } + let(:extra_params) { { avatar: avatar } } - expect(response).to be_error - expect(response.message).to match_array(["Name can't be blank"]) + it_behaves_like 'creating an organization' + end + + context 'when the organization is not persisted' do + let(:extra_params) { { name: nil } } + + it 'returns an error when the organization is not persisted' do + expect(response).to be_error + expect(response.message).to match_array(["Name can't be blank"]) + end end end end diff --git a/spec/services/organizations/update_service_spec.rb b/spec/services/organizations/update_service_spec.rb new file mode 100644 index 00000000000..148840770db --- /dev/null +++ b/spec/services/organizations/update_service_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Organizations::UpdateService, feature_category: :cell do + describe '#execute' do + let_it_be(:user) { create(:user) } + let_it_be_with_reload(:organization) { create(:organization) } + + let_it_be(:current_user) { user } # due to use in before_all + let(:name) { 'Name' } + let(:path) { 'path' } + let(:description) { nil } + let(:avatar_filename) { nil } + let(:params) { { name: name, path: path }.merge(extra_params) } + let(:extra_params) { {} } + let(:updated_organization) { response.payload[:organization] } + + subject(:response) do + described_class.new(organization, current_user: current_user, params: params).execute + end + + context 'when user does not have permission' do + let(:current_user) { nil } + + it 'returns an error' do + expect(response).to be_error + + expect(response.message).to match_array(['You have insufficient permissions to update the organization']) + end + end + + context 'when user has permission' do + before_all do + create(:organization_user, organization: organization, user: current_user) + end + + shared_examples 'updating an organization' do + it 'updates the organization' do + expect(response).to be_success + expect(updated_organization.name).to eq(name) + expect(updated_organization.path).to eq(path) + expect(updated_organization.description).to eq(description) + expect(updated_organization.avatar.filename).to eq(avatar_filename) + end + end + + context 'with description' do + let(:description) { 'Organization description' } + let(:extra_params) { { description: description } } + + it_behaves_like 'updating an organization' + end + + context 'with avatar' do + let(:avatar_filename) { 'dk.png' } + let(:avatar) { fixture_file_upload("spec/fixtures/#{avatar_filename}") } + let(:extra_params) { { avatar: avatar } } + + it_behaves_like 'updating an organization' + end + + include_examples 'updating an organization' + + context 'when the organization is not updated' do + let(:extra_params) { { name: nil } } + + it 'returns an error' do + expect(response).to be_error + expect(updated_organization).to be_instance_of Organizations::Organization + expect(response.message).to match_array(["Name can't be blank"]) + end + end + end + end +end diff --git a/spec/services/packages/mark_package_for_destruction_service_spec.rb b/spec/services/packages/mark_package_for_destruction_service_spec.rb index d65e62b84a6..bd69f995c77 100644 --- a/spec/services/packages/mark_package_for_destruction_service_spec.rb +++ b/spec/services/packages/mark_package_for_destruction_service_spec.rb @@ -17,6 +17,7 @@ RSpec.describe Packages::MarkPackageForDestructionService, feature_category: :pa context 'when it is successful' do it 'marks the package and package files as pending destruction' do expect(package).to receive(:sync_maven_metadata).and_call_original + expect(package).to receive(:sync_npm_metadata_cache).and_call_original expect(package).to receive(:mark_package_files_for_destruction).and_call_original expect { service.execute }.to change { package.status }.from('default').to('pending_destruction') end @@ -45,6 +46,7 @@ RSpec.describe Packages::MarkPackageForDestructionService, feature_category: :pa response = service.execute expect(package).not_to receive(:sync_maven_metadata) + expect(package).not_to receive(:sync_npm_metadata_cache) expect(response).to be_a(ServiceResponse) expect(response).to be_error expect(response.message).to eq("Failed to mark the package as pending destruction") diff --git a/spec/services/packages/mark_packages_for_destruction_service_spec.rb b/spec/services/packages/mark_packages_for_destruction_service_spec.rb index 22278f9927d..cd6426d39ad 100644 --- a/spec/services/packages/mark_packages_for_destruction_service_spec.rb +++ b/spec/services/packages/mark_packages_for_destruction_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Packages::MarkPackagesForDestructionService, :sidekiq_inline, feature_category: :package_registry do let_it_be(:project) { create(:project) } - let_it_be_with_reload(:packages) { create_list(:npm_package, 3, project: project) } + let_it_be_with_reload(:packages) { create_list(:nuget_package, 3, project: project) } let(:user) { project.owner } @@ -15,6 +15,17 @@ RSpec.describe Packages::MarkPackagesForDestructionService, :sidekiq_inline, fea describe '#execute' do subject { service.execute } + shared_examples 'returning service response' do |status:, message:, reason: nil| + it 'returns service response' do + subject + + expect(subject).to be_a(ServiceResponse) + expect(subject.status).to eq(status) + expect(subject.message).to eq(message) + expect(subject.reason).to eq(reason) if reason + end + end + context 'when the user is authorized' do before do project.add_maintainer(user) @@ -23,16 +34,16 @@ RSpec.describe Packages::MarkPackagesForDestructionService, :sidekiq_inline, fea context 'when it is successful' do it 'marks the packages as pending destruction' do expect(::Packages::Maven::Metadata::SyncService).not_to receive(:new) + expect(::Packages::Npm::CreateMetadataCacheService).not_to receive(:new) expect { subject }.to change { ::Packages::Package.pending_destruction.count }.from(0).to(3) .and change { Packages::PackageFile.pending_destruction.count }.from(0).to(3) packages.each { |pkg| expect(pkg.reload).to be_pending_destruction } - - expect(subject).to be_a(ServiceResponse) - expect(subject).to be_success - expect(subject.message).to eq('Packages were successfully marked as pending destruction') end + it_behaves_like 'returning service response', status: :success, + message: 'Packages were successfully marked as pending destruction' + context 'with maven packages' do let_it_be_with_reload(:packages) { create_list(:maven_package, 3, project: project) } @@ -42,12 +53,11 @@ RSpec.describe Packages::MarkPackagesForDestructionService, :sidekiq_inline, fea expect { subject }.to change { ::Packages::Package.pending_destruction.count }.from(0).to(3) .and change { Packages::PackageFile.pending_destruction.count }.from(0).to(9) packages.each { |pkg| expect(pkg.reload).to be_pending_destruction } - - expect(subject).to be_a(ServiceResponse) - expect(subject).to be_success - expect(subject.message).to eq('Packages were successfully marked as pending destruction') end + it_behaves_like 'returning service response', status: :success, + message: 'Packages were successfully marked as pending destruction' + context 'without version' do before do ::Packages::Package.id_in(package_ids).update_all(version: nil) @@ -59,12 +69,26 @@ RSpec.describe Packages::MarkPackagesForDestructionService, :sidekiq_inline, fea expect { subject }.to change { ::Packages::Package.pending_destruction.count }.from(0).to(3) .and change { Packages::PackageFile.pending_destruction.count }.from(0).to(9) packages.each { |pkg| expect(pkg.reload).to be_pending_destruction } - - expect(subject).to be_a(ServiceResponse) - expect(subject).to be_success - expect(subject.message).to eq('Packages were successfully marked as pending destruction') end + + it_behaves_like 'returning service response', status: :success, + message: 'Packages were successfully marked as pending destruction' + end + end + + context 'with npm packages' do + let_it_be_with_reload(:packages) { create_list(:npm_package, 3, project: project, name: 'test-package') } + + it 'marks the packages as pending destruction' do + expect(::Packages::Npm::CreateMetadataCacheService).to receive(:new).once.and_call_original + + expect { subject }.to change { ::Packages::Package.pending_destruction.count }.from(0).to(3) + .and change { Packages::PackageFile.pending_destruction.count }.from(0).to(3) + packages.each { |package| expect(package.reload).to be_pending_destruction } end + + it_behaves_like 'returning service response', status: :success, + message: 'Packages were successfully marked as pending destruction' end end @@ -73,7 +97,7 @@ RSpec.describe Packages::MarkPackagesForDestructionService, :sidekiq_inline, fea allow(service).to receive(:can_destroy_packages?).and_raise(StandardError, 'test') end - it 'returns an error ServiceResponse' do + it 'does not mark the packages as pending destruction' do expect(::Packages::Maven::Metadata::SyncService).not_to receive(:new) expect(Gitlab::ErrorTracking).to receive(:track_exception).with( @@ -83,30 +107,25 @@ RSpec.describe Packages::MarkPackagesForDestructionService, :sidekiq_inline, fea expect { subject }.to not_change { ::Packages::Package.pending_destruction.count } .and not_change { ::Packages::PackageFile.pending_destruction.count } - - expect(subject).to be_a(ServiceResponse) - expect(subject).to be_error - expect(subject.message).to eq("Failed to mark the packages as pending destruction") - expect(subject.status).to eq(:error) end + + it_behaves_like 'returning service response', status: :error, + message: 'Failed to mark the packages as pending destruction' end end context 'when the user is not authorized' do let(:user) { nil } - it 'returns an error ServiceResponse' do + it 'does not mark the packages as pending destruction' do expect(::Packages::Maven::Metadata::SyncService).not_to receive(:new) expect { subject }.to not_change { ::Packages::Package.pending_destruction.count } .and not_change { ::Packages::PackageFile.pending_destruction.count } - - expect(subject).to be_a(ServiceResponse) - expect(subject).to be_error - expect(subject.message).to eq("You don't have the permission to perform this action") - expect(subject.status).to eq(:error) - expect(subject.reason).to eq(:unauthorized) end + + it_behaves_like 'returning service response', status: :error, reason: :unauthorized, + message: "You don't have the permission to perform this action" end end end diff --git a/spec/services/packages/ml_model/create_package_file_service_spec.rb b/spec/services/packages/ml_model/create_package_file_service_spec.rb index 30a6bedd07b..505c8038976 100644 --- a/spec/services/packages/ml_model/create_package_file_service_spec.rb +++ b/spec/services/packages/ml_model/create_package_file_service_spec.rb @@ -8,6 +8,8 @@ RSpec.describe Packages::MlModel::CreatePackageFileService, feature_category: :m let_it_be(:user) { create(:user) } let_it_be(:pipeline) { create(:ci_pipeline, user: user, project: project) } let_it_be(:file_name) { 'myfile.tar.gz.1' } + let_it_be(:model) { create(:ml_models, user: user, project: project) } + let_it_be(:model_version) { create(:ml_model_versions, :with_package, model: model, version: '0.1.0') } let(:build) { instance_double(Ci::Build, pipeline: pipeline) } @@ -26,47 +28,24 @@ RSpec.describe Packages::MlModel::CreatePackageFileService, feature_category: :m FileUtils.rm_f(temp_file) end - context 'without existing package' do + context 'when model version is nil' do let(:params) do { - package_name: 'new_model', - package_version: '1.0.0', + model_version: nil, file: file, file_name: file_name } end - it 'creates package file', :aggregate_failures do - expect { execute_service } - .to change { Packages::MlModel::Package.count }.by(1) - .and change { Packages::PackageFile.count }.by(1) - .and change { Packages::PackageFileBuildInfo.count }.by(0) - .and change { Ml::ModelVersion.count }.by(1) - - new_model = Packages::MlModel::Package.last - package_file = new_model.package_files.last - new_model_version = Ml::ModelVersion.last - - expect(new_model.name).to eq('new_model') - expect(new_model.version).to eq('1.0.0') - expect(new_model.status).to eq('default') - expect(package_file.package).to eq(new_model) - expect(package_file.file_name).to eq(file_name) - expect(package_file.size).to eq(file.size) - expect(package_file.file_sha256).to eq(sha256) - expect(new_model_version.name).to eq('new_model') - expect(new_model_version.version).to eq('1.0.0') - expect(new_model_version.package).to eq(new_model) + it 'does not create package file', :aggregate_failures do + expect(execute_service).to be(nil) end end - context 'with existing package' do - let_it_be(:model) { create(:ml_model_package, creator: user, project: project, version: '0.1.0') } - + context 'with existing model version' do let(:params) do { - package_name: model.name, - package_version: model.version, + model_version: model_version, file: file, file_name: file_name, status: :hidden, @@ -76,18 +55,16 @@ RSpec.describe Packages::MlModel::CreatePackageFileService, feature_category: :m it 'adds the package file and updates status and ci_build', :aggregate_failures do expect { execute_service } - .to change { project.packages.ml_model.count }.by(0) - .and change { model.package_files.count }.by(1) + .to change { model_version.package.package_files.count }.by(1) .and change { Packages::PackageFileBuildInfo.count }.by(1) - model.reload - - package_file = model.package_files.last + package = model_version.reload.package + package_file = package.package_files.last - expect(model.build_infos.first.pipeline).to eq(build.pipeline) - expect(model.status).to eq('hidden') + expect(package.build_infos.first.pipeline).to eq(build.pipeline) + expect(package.status).to eq('hidden') - expect(package_file.package).to eq(model) + expect(package_file.package).to eq(package) expect(package_file.file_name).to eq(file_name) expect(package_file.size).to eq(file.size) expect(package_file.file_sha256).to eq(sha256) diff --git a/spec/services/packages/npm/create_package_service_spec.rb b/spec/services/packages/npm/create_package_service_spec.rb index 867dc582771..f02e53b67cb 100644 --- a/spec/services/packages/npm/create_package_service_spec.rb +++ b/spec/services/packages/npm/create_package_service_spec.rb @@ -322,7 +322,7 @@ RSpec.describe Packages::Npm::CreatePackageService, feature_category: :package_r it { expect(subject[:message]).to eq 'Could not obtain package lease. Please try again.' } end - context 'when feature flag :packages_package_protection is disabled' do + context 'when feature flag :packages_protected_packages disabled' do let_it_be_with_reload(:package_protection_rule) { create(:package_protection_rule, package_type: :npm, project: project) } before do diff --git a/spec/services/packages/npm/generate_metadata_service_spec.rb b/spec/services/packages/npm/generate_metadata_service_spec.rb index f5d7f13d22c..ad3d4fde665 100644 --- a/spec/services/packages/npm/generate_metadata_service_spec.rb +++ b/spec/services/packages/npm/generate_metadata_service_spec.rb @@ -5,7 +5,8 @@ require 'spec_helper' RSpec.describe ::Packages::Npm::GenerateMetadataService, feature_category: :package_registry do using RSpec::Parameterized::TableSyntax - let_it_be(:project) { create(:project) } + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, namespace: group) } let_it_be(:package_name) { "@#{project.root_namespace.path}/test" } let_it_be(:package1) { create(:npm_package, version: '2.0.4', project: project, name: package_name) } let_it_be(:package2) { create(:npm_package, version: '2.0.6', project: project, name: package_name) } @@ -156,6 +157,19 @@ RSpec.describe ::Packages::Npm::GenerateMetadataService, feature_category: :pack end end end + + context 'with duplicate tags' do + let_it_be(:project2) { create(:project, namespace: group) } + let_it_be(:package2) { create(:npm_package, version: '3.0.0', project: project2, name: package_name) } + let_it_be(:package_tag1) { create(:packages_tag, package: package1, name: 'latest') } + let_it_be(:package_tag2) { create(:packages_tag, package: package2, name: 'latest') } + + let(:packages) { ::Packages::Package.for_projects([project.id, project2.id]).with_name(package_name) } + + it "returns the tag of the latest package's version" do + expect(subject['latest']).to eq(package2.version) + end + end end end diff --git a/spec/services/packages/protection/update_rule_service_spec.rb b/spec/services/packages/protection/update_rule_service_spec.rb new file mode 100644 index 00000000000..70619a1caa3 --- /dev/null +++ b/spec/services/packages/protection/update_rule_service_spec.rb @@ -0,0 +1,150 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Protection::UpdateRuleService, '#execute', feature_category: :environment_management do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:current_user) { create(:user, maintainer_projects: [project]) } + let_it_be_with_reload(:package_protection_rule) { create(:package_protection_rule, project: project) } + + let(:service) { described_class.new(package_protection_rule, current_user: current_user, params: params) } + + let(:params) do + attributes_for( + :package_protection_rule, + package_name_pattern: "#{package_protection_rule.package_name_pattern}-updated", + package_type: 'npm', + push_protected_up_to_access_level: 'owner' + ) + end + + subject(:service_execute) { service.execute } + + shared_examples 'a successful service response' do + let(:expected_attributes) { params } + + it { is_expected.to be_success } + + it do + is_expected.to have_attributes( + errors: be_blank, + message: be_blank, + payload: { package_protection_rule: be_a(Packages::Protection::Rule).and(have_attributes(expected_attributes)) } + ) + end + + it { expect { subject }.not_to change { Packages::Protection::Rule.count } } + + it { subject.tap { expect(package_protection_rule.reload).to have_attributes expected_attributes } } + end + + shared_examples 'an erroneous service response' do + it { is_expected.to be_error } + + it do + is_expected.to have_attributes( + errors: be_present, + message: be_present, + payload: { package_protection_rule: nil } + ) + end + + it { expect { subject }.not_to change { Packages::Protection::Rule.count } } + it { expect { subject }.not_to change { package_protection_rule.reload.updated_at } } + end + + it_behaves_like 'a successful service response' + + context 'with disallowed params' do + let(:params) { super().merge!(project_id: 1, unsupported_param: 'unsupported_param_value') } + + it_behaves_like 'a successful service response' do + let(:expected_attributes) { params.except(:project_id, :unsupported_param) } + end + end + + context 'when fields are invalid' do + let(:params) do + { package_name_pattern: '', package_type: 'unknown_package_type', + push_protected_up_to_access_level: 1000 } + end + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes message: /'unknown_package_type' is not a valid package_type/ } + end + + context 'with empty params' do + let(:params) { {} } + + it_behaves_like 'a successful service response' do + let(:expected_attributes) { package_protection_rule.attributes } + end + + it { expect { service_execute }.not_to change { package_protection_rule.reload.updated_at } } + end + + context 'with nil params' do + let(:params) { nil } + + it_behaves_like 'a successful service response' do + let(:expected_attributes) { package_protection_rule.attributes } + end + + it { expect { service_execute }.not_to change { package_protection_rule.reload.updated_at } } + end + + context 'when updated field `package_name_pattern` is already taken' do + let_it_be_with_reload(:other_existing_package_protection_rule) do + create(:package_protection_rule, project: project, + package_name_pattern: "#{package_protection_rule.package_name_pattern}-other") + end + + let(:params) { { package_name_pattern: other_existing_package_protection_rule.package_name_pattern } } + + it_behaves_like 'an erroneous service response' + + it do + expect { service_execute }.not_to( + change { other_existing_package_protection_rule.reload.package_name_pattern } + ) + end + + it do + is_expected.to have_attributes( + errors: match_array([/Package name pattern has already been taken/]), + message: match_array([/Package name pattern has already been taken/]) + ) + end + end + + context 'when current_user does not have permission' do + let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } + let_it_be(:reporter) { create(:user).tap { |u| project.add_reporter(u) } } + let_it_be(:guest) { create(:user).tap { |u| project.add_guest(u) } } + let_it_be(:anonymous) { create(:user) } + + where(:current_user) do + [ref(:developer), ref(:reporter), ref(:guest), ref(:anonymous)] + end + + with_them do + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes errors: match_array(/Unauthorized/), message: /Unauthorized/ } + end + end + + context 'without package protection rule' do + let(:package_protection_rule) { nil } + let(:params) { {} } + + it { expect { service_execute }.to raise_error(ArgumentError) } + end + + context 'without current_user' do + let(:current_user) { nil } + + it { expect { service_execute }.to raise_error(ArgumentError) } + end +end diff --git a/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb b/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb index 0e46391c0ad..63b5d54a18d 100644 --- a/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb +++ b/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb @@ -188,4 +188,28 @@ RSpec.describe PagesDomains::ObtainLetsEncryptCertificateService, feature_catego service.execute end end + + context 'when the domain URL is longer than 64 characters' do + let(:long_domain) { "a.b.c.#{'d' * 63}" } + let(:pages_domain) { create(:pages_domain, :without_certificate, :without_key, domain: long_domain) } + let(:service) { described_class.new(pages_domain) } + + it 'logs an error and does not proceed with certificate acquisition' do + expect(Gitlab::AppLogger).to receive(:error).with( + hash_including( + message: "Domain name too long for Let's Encrypt certificate", + pages_domain: long_domain, + pages_domain_bytesize: long_domain.bytesize, + max_allowed_bytesize: described_class::MAX_DOMAIN_LENGTH, + project_id: pages_domain.project_id + ) + ) + + # Ensure that the certificate acquisition is not attempted + expect(::PagesDomains::CreateAcmeOrderService).not_to receive(:new) + expect(PagesDomainSslRenewalWorker).not_to receive(:perform_in) + + service.execute + end + end end diff --git a/spec/services/product_analytics/build_activity_graph_service_spec.rb b/spec/services/product_analytics/build_activity_graph_service_spec.rb deleted file mode 100644 index 2eb35523da7..00000000000 --- a/spec/services/product_analytics/build_activity_graph_service_spec.rb +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ProductAnalytics::BuildActivityGraphService, feature_category: :product_analytics_data_management do - let_it_be(:project) { create(:project) } - let_it_be(:time_now) { Time.zone.now } - let_it_be(:time_ago) { Time.zone.now - 5.days } - - let_it_be(:events) do - [ - create(:product_analytics_event, project: project, collector_tstamp: time_now), - create(:product_analytics_event, project: project, collector_tstamp: time_now), - create(:product_analytics_event, project: project, collector_tstamp: time_now), - create(:product_analytics_event, project: project, collector_tstamp: time_ago), - create(:product_analytics_event, project: project, collector_tstamp: time_ago) - ] - end - - let(:params) { { timerange: 7 } } - - subject { described_class.new(project, params).execute } - - it 'returns a valid graph hash' do - expected_hash = { - id: 'collector_tstamp', - keys: [time_ago.to_date, time_now.to_date], - values: [2, 3] - } - - expect(subject).to eq(expected_hash) - end -end diff --git a/spec/services/product_analytics/build_graph_service_spec.rb b/spec/services/product_analytics/build_graph_service_spec.rb deleted file mode 100644 index a850d69e53c..00000000000 --- a/spec/services/product_analytics/build_graph_service_spec.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ProductAnalytics::BuildGraphService, feature_category: :product_analytics_data_management do - let_it_be(:project) { create(:project) } - - let_it_be(:events) do - [ - create(:product_analytics_event, project: project, platform: 'web'), - create(:product_analytics_event, project: project, platform: 'web'), - create(:product_analytics_event, project: project, platform: 'app'), - create(:product_analytics_event, project: project, platform: 'mobile'), - create(:product_analytics_event, project: project, platform: 'mobile', collector_tstamp: Time.zone.now - 60.days) - ] - end - - let(:params) { { graph: 'platform', timerange: 5 } } - - subject { described_class.new(project, params).execute } - - it 'returns a valid graph hash' do - expect(subject[:id]).to eq(:platform) - expect(subject[:keys]).to eq(%w[app mobile web]) - expect(subject[:values]).to eq([1, 1, 2]) - end -end diff --git a/spec/services/projects/after_rename_service_spec.rb b/spec/services/projects/after_rename_service_spec.rb index 4b2569f6b2d..228dff6aa0b 100644 --- a/spec/services/projects/after_rename_service_spec.rb +++ b/spec/services/projects/after_rename_service_spec.rb @@ -26,6 +26,7 @@ RSpec.describe Projects::AfterRenameService, feature_category: :groups_and_proje let(:hash) { Digest::SHA2.hexdigest(project.id.to_s) } let(:hashed_prefix) { File.join('@hashed', hash[0..1], hash[2..3]) } let(:hashed_path) { File.join(hashed_prefix, hash) } + let(:message) { "Repository #{full_path_before_rename} could not be renamed to #{full_path_after_rename}" } before do # Project#gitlab_shell returns a new instance of Gitlab::Shell on every @@ -35,6 +36,15 @@ RSpec.describe Projects::AfterRenameService, feature_category: :groups_and_proje stub_application_setting(hashed_storage_enabled: true) end + shared_examples 'logging and raising a RenameFailedError' do + it 'logs raises a RenameFailedError' do + expect_any_instance_of(described_class).to receive(:log_error).with(message) + + expect { service_execute } + .to raise_error(described_class::RenameFailedError) + end + end + it 'renames a repository' do stub_container_registry_config(enabled: false) @@ -47,8 +57,21 @@ RSpec.describe Projects::AfterRenameService, feature_category: :groups_and_proje service_execute end + context 'when renaming or migrating fails' do + before do + allow_any_instance_of(::Projects::HashedStorage::MigrationService) + .to receive(:execute).and_return(false) + end + + it_behaves_like 'logging and raising a RenameFailedError' + end + context 'container registry with images' do let(:container_repository) { create(:container_repository) } + let(:message) do + "Project #{full_path_before_rename} cannot be renamed because images are " \ + "present in its container registry" + end before do stub_container_registry_config(enabled: true) @@ -56,9 +79,36 @@ RSpec.describe Projects::AfterRenameService, feature_category: :groups_and_proje project.container_repositories << container_repository end - it 'raises a RenameFailedError' do - expect { service_execute } - .to raise_error(described_class::RenameFailedError) + context 'when Gitlab API is not supported' do + before do + allow(ContainerRegistry::GitlabApiClient).to receive(:supports_gitlab_api?).and_return(false) + end + + it_behaves_like 'logging and raising a RenameFailedError' + end + + context 'when Gitlab API Client is supported' do + before do + allow(ContainerRegistry::GitlabApiClient).to receive(:supports_gitlab_api?).and_return(true) + end + + it 'renames the base repository in the registry' do + expect(ContainerRegistry::GitlabApiClient).to receive(:rename_base_repository_path) + .with(full_path_before_rename, name: path_after_rename).and_return(:ok) + + service_execute + end + + context 'when the base repository rename in the registry fails' do + before do + allow(ContainerRegistry::GitlabApiClient) + .to receive(:rename_base_repository_path).and_return(:bad_request) + end + + let(:message) { 'Renaming the base repository in the registry failed with error bad_request.' } + + it_behaves_like 'logging and raising a RenameFailedError' + end end end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 899ed477180..5a7abf6cde8 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -812,6 +812,31 @@ RSpec.describe Projects::CreateService, '#execute', feature_category: :groups_an end end + context 'when SHA256 format is requested' do + let(:project) { create_project(user, opts) } + let(:opts) { super().merge(initialize_with_readme: true, repository_object_format: 'sha256') } + + before do + allow(Gitlab::CurrentSettings).to receive(:default_branch_name).and_return('main') + end + + it 'creates a repository with SHA256 commit hashes', :aggregate_failures do + expect(project.repository.commit_count).to be(1) + expect(project.commit.id.size).to eq 64 + end + + context 'when "support_sha256_repositories" feature flag is disabled' do + before do + stub_feature_flags(support_sha256_repositories: false) + end + + it 'creates a repository with default SHA1 commit hash' do + expect(project.repository.commit_count).to be(1) + expect(project.commit.id.size).to eq 40 + end + end + end + describe 'create integration for the project' do subject(:project) { create_project(user, opts) } diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index a0064eadf13..3aea329a45f 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -454,6 +454,12 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi expect { destroy_project(forked_project, user) } .not_to raise_error end + + it 'does not update project statistics for the deleted project' do + expect(ProjectCacheWorker).not_to receive(:perform_async) + + destroy_project(forked_project, user) + end end context 'as the root of a fork network' do diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index ceb060445ad..e6418c7b4ea 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -387,7 +387,7 @@ RSpec.describe Projects::ForkService, feature_category: :source_code_management # Stub everything required to move a project to a Gitaly shard that does not exist allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid) - stub_storage_settings('test_second_storage' => { 'path' => TestEnv::SECOND_STORAGE_PATH }) + stub_storage_settings('test_second_storage' => {}) allow_any_instance_of(Gitlab::Git::Repository).to receive(:create_repository) .and_return(true) allow_any_instance_of(Gitlab::Git::Repository).to receive(:replicate) diff --git a/spec/services/projects/group_links/create_service_spec.rb b/spec/services/projects/group_links/create_service_spec.rb index e3f170ef3fe..6bc0b86545a 100644 --- a/spec/services/projects/group_links/create_service_spec.rb +++ b/spec/services/projects/group_links/create_service_spec.rb @@ -103,11 +103,57 @@ RSpec.describe Projects::GroupLinks::CreateService, '#execute', feature_category it_behaves_like 'shareable' end end + + context 'when sharing it to a group with OWNER access' do + let(:opts) do + { + link_group_access: Gitlab::Access::OWNER, + expires_at: nil + } + end + + it 'does not share and returns a forbiden error' do + expect do + result = subject.execute + + expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(403) + end.not_to change { project.reload.project_group_links.count } + end + end + end + + context 'when the user is an OWNER in the project' do + before do + project.add_owner(user) + end + + it_behaves_like 'shareable' + + context 'when sharing it to a group with OWNER access' do + let(:opts) do + { + link_group_access: Gitlab::Access::OWNER, + expires_at: nil + } + end + + it_behaves_like 'shareable' + end end end context 'when user does not have permissions to share the project with a group' do it_behaves_like 'not shareable' + + context 'when the user has less than MAINTAINER access in the project' do + before do + group.add_guest(user) + project.add_developer(user) + end + + it_behaves_like 'not shareable' + end end context 'when group is blank' do diff --git a/spec/services/projects/group_links/destroy_service_spec.rb b/spec/services/projects/group_links/destroy_service_spec.rb index 0cd003f6142..4fc441c9e26 100644 --- a/spec/services/projects/group_links/destroy_service_spec.rb +++ b/spec/services/projects/group_links/destroy_service_spec.rb @@ -19,8 +19,10 @@ RSpec.describe Projects::GroupLinks::DestroyService, '#execute', feature_categor end end - shared_examples_for 'returns not_found' do - it do + context 'if group_link is blank' do + let!(:group_link) { nil } + + it 'returns 404 not found' do expect do result = subject.execute(group_link) @@ -30,14 +32,15 @@ RSpec.describe Projects::GroupLinks::DestroyService, '#execute', feature_categor end end - context 'if group_link is blank' do - let!(:group_link) { nil } - - it_behaves_like 'returns not_found' - end - context 'if the user does not have access to destroy the link' do - it_behaves_like 'returns not_found' + it 'returns 404 not found' do + expect do + result = subject.execute(group_link) + + expect(result[:status]).to eq(:error) + expect(result[:reason]).to eq(:not_found) + end.not_to change { project.reload.project_group_links.count } + end end context 'when the user has proper permissions to remove a group-link from a project' do @@ -111,6 +114,41 @@ RSpec.describe Projects::GroupLinks::DestroyService, '#execute', feature_categor end end end + + context 'on trying to destroy a link with OWNER access' do + let(:group_access) { Gitlab::Access::OWNER } + + it 'does not remove the group from project' do + expect do + result = subject.execute(group_link) + + expect(result[:status]).to eq(:error) + expect(result[:reason]).to eq(:forbidden) + end.not_to change { project.reload.project_group_links.count } + end + + context 'if the user is an OWNER of the group' do + before do + group.add_owner(user) + end + + it_behaves_like 'removes group from project' + end + end + end + + context 'when the user is an OWNER in the project' do + before do + project.add_owner(user) + end + + it_behaves_like 'removes group from project' + + context 'on trying to destroy a link with OWNER access' do + let(:group_access) { Gitlab::Access::OWNER } + + it_behaves_like 'removes group from project' + end end end diff --git a/spec/services/projects/group_links/update_service_spec.rb b/spec/services/projects/group_links/update_service_spec.rb index b02614fa062..86ad1bcf286 100644 --- a/spec/services/projects/group_links/update_service_spec.rb +++ b/spec/services/projects/group_links/update_service_spec.rb @@ -20,8 +20,8 @@ RSpec.describe Projects::GroupLinks::UpdateService, '#execute', feature_category subject { described_class.new(link, user).execute(group_link_params) } - shared_examples_for 'returns not_found' do - it do + context 'when the user does not have proper permissions to update a project group link' do + it 'returns 404 not found' do result = subject expect(result[:status]).to eq(:error) @@ -29,10 +29,6 @@ RSpec.describe Projects::GroupLinks::UpdateService, '#execute', feature_category end end - context 'when the user does not have proper permissions to update a project group link' do - it_behaves_like 'returns not_found' - end - context 'when user has proper permissions to update a project group link' do context 'when the user is a MAINTAINER in the project' do before do @@ -92,6 +88,86 @@ RSpec.describe Projects::GroupLinks::UpdateService, '#execute', feature_category subject end end + + context 'updating a link with OWNER access' do + let(:group_access) { Gitlab::Access::OWNER } + + shared_examples_for 'returns :forbidden' do + it do + expect do + result = subject + + expect(result[:status]).to eq(:error) + expect(result[:reason]).to eq(:forbidden) + end.to not_change { link.expires_at }.and not_change { link.group_access } + end + end + + context 'updating expires_at' do + let(:group_link_params) do + { expires_at: 7.days.from_now } + end + + it_behaves_like 'returns :forbidden' + end + + context 'updating group_access' do + let(:group_link_params) do + { group_access: Gitlab::Access::MAINTAINER } + end + + it_behaves_like 'returns :forbidden' + end + + context 'updating both expires_at and group_access' do + it_behaves_like 'returns :forbidden' + end + end + end + + context 'when the user is an OWNER in the project' do + before do + project.add_owner(user) + end + + context 'updating expires_at' do + let(:group_link_params) do + { expires_at: 7.days.from_now.to_date } + end + + it 'updates existing link' do + expect do + result = subject + + expect(result[:status]).to eq(:success) + end.to change { link.reload.expires_at }.to(group_link_params[:expires_at]) + end + end + + context 'updating group_access' do + let(:group_link_params) do + { group_access: Gitlab::Access::MAINTAINER } + end + + it 'updates existing link' do + expect do + result = subject + + expect(result[:status]).to eq(:success) + end.to change { link.reload.group_access }.to(group_link_params[:group_access]) + end + end + + context 'updating both expires_at and group_access' do + it 'updates existing link' do + expect do + result = subject + + expect(result[:status]).to eq(:success) + end.to change { link.reload.group_access }.to(group_link_params[:group_access]) + .and change { link.reload.expires_at }.to(group_link_params[:expires_at]) + end + end end end 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 e32747ad907..4834af79225 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, feature_category: 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| # rubocop:disable Rails/SaveBang + Tempfile.create do |old_path| new_path = "#{old_path}-new" expect(subject.send(:move_folder!, old_path, new_path)).to be_truthy diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index 16b9d2618ca..dd6e82b9ef2 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -183,81 +183,6 @@ RSpec.describe Projects::ImportService, feature_category: :importers do expect(result[:status]).to eq :error expect(result[:message]).to eq "Error importing repository #{project.safe_import_url} into #{project.full_path} - Failed to import the repository [FILTERED]" end - - context 'when bitbucket_parallel_importer feature flag is disabled' do - before do - stub_feature_flags(bitbucket_parallel_importer: false) - end - - it 'succeeds if repository import is successful' do - expect(project.repository).to receive(:import_repository).and_return(true) - expect_next_instance_of(Gitlab::BitbucketImport::Importer) do |importer| - expect(importer).to receive(:execute).and_return(true) - end - - expect_next_instance_of(Projects::LfsPointers::LfsImportService) do |service| - expect(service).to receive(:execute).and_return(status: :success) - end - - result = subject.execute - - expect(result[:status]).to eq :success - end - - it 'fails if repository import fails' do - expect(project.repository) - .to receive(:import_repository) - .with('https://bitbucket.org/vim/vim.git', resolved_address: '') - .and_raise(Gitlab::Git::CommandError, 'Failed to import the repository /a/b/c') - - result = subject.execute - - expect(result[:status]).to eq :error - expect(result[:message]).to eq "Error importing repository #{project.safe_import_url} into #{project.full_path} - Failed to import the repository [FILTERED]" - end - - context 'when lfs import fails' do - it 'logs the error' do - error_message = 'error message' - - expect(project.repository).to receive(:import_repository).and_return(true) - - expect_next_instance_of(Gitlab::BitbucketImport::Importer) do |importer| - expect(importer).to receive(:execute).and_return(true) - end - - expect_next_instance_of(Projects::LfsPointers::LfsImportService) do |service| - expect(service).to receive(:execute).and_return(status: :error, message: error_message) - end - - expect(Gitlab::AppLogger).to receive(:error).with("The Lfs import process failed. #{error_message}") - - subject.execute - end - end - - context 'when repository import scheduled' do - before do - expect(project.repository).to receive(:import_repository).and_return(true) - allow(subject).to receive(:import_data) - end - - it 'downloads lfs objects if lfs_enabled is enabled for project' do - allow(project).to receive(:lfs_enabled?).and_return(true) - - expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute) - - subject.execute - end - - it 'does not download lfs objects if lfs_enabled is not enabled for project' do - allow(project).to receive(:lfs_enabled?).and_return(false) - expect_any_instance_of(Projects::LfsPointers::LfsImportService).not_to receive(:execute) - - subject.execute - end - end - end end end end @@ -352,13 +277,53 @@ RSpec.describe Projects::ImportService, feature_category: :importers do end end + context 'when import is a local request' do + before do + project.import_url = "http://127.0.0.1/group/project" + end + + context 'when local network requests are enabled' do + before do + stub_application_setting(allow_local_requests_from_web_hooks_and_services: true) + end + + it 'returns an error' do + expect(project.repository).not_to receive(:import_repository) + expect(subject.execute).to include( + status: :error, + message: end_with('Requests to localhost are not allowed') + ) + end + + context 'when environment is development' do + before do + stub_rails_env('development') + end + + it 'imports successfully' do + expect(project.repository) + .to receive(:import_repository) + .and_return(true) + expect(subject.execute[:status]).to eq(:success) + end + end + end + end + context 'when DNS rebind protection is disabled' do before do allow(Gitlab::CurrentSettings).to receive(:dns_rebinding_protection_enabled?).and_return(false) project.import_url = "https://example.com/group/project" allow(Gitlab::UrlBlocker).to receive(:validate!) - .with(project.import_url, ports: Project::VALID_IMPORT_PORTS, schemes: Project::VALID_IMPORT_PROTOCOLS, dns_rebind_protection: false) + .with( + project.import_url, + ports: Project::VALID_IMPORT_PORTS, + schemes: Project::VALID_IMPORT_PROTOCOLS, + allow_local_network: false, + allow_localhost: false, + dns_rebind_protection: false + ) .and_return([Addressable::URI.parse("https://example.com/group/project"), nil]) end @@ -386,7 +351,14 @@ RSpec.describe Projects::ImportService, feature_category: :importers do project.import_url = "https://example.com/group/project" allow(Gitlab::UrlBlocker).to receive(:validate!) - .with(project.import_url, ports: Project::VALID_IMPORT_PORTS, schemes: Project::VALID_IMPORT_PROTOCOLS, dns_rebind_protection: true) + .with( + project.import_url, + ports: Project::VALID_IMPORT_PORTS, + schemes: Project::VALID_IMPORT_PROTOCOLS, + allow_local_network: false, + allow_localhost: false, + dns_rebind_protection: true + ) .and_return([Addressable::URI.parse("https://172.16.123.1/group/project"), 'example.com']) end @@ -407,7 +379,14 @@ RSpec.describe Projects::ImportService, feature_category: :importers do project.import_url = 'https://gitlab.com/gitlab-org/gitlab-development-kit' allow(Gitlab::UrlBlocker).to receive(:validate!) - .with(project.import_url, ports: Project::VALID_IMPORT_PORTS, schemes: Project::VALID_IMPORT_PROTOCOLS, dns_rebind_protection: true) + .with( + project.import_url, + ports: Project::VALID_IMPORT_PORTS, + schemes: Project::VALID_IMPORT_PROTOCOLS, + allow_local_network: false, + allow_localhost: false, + dns_rebind_protection: true + ) .and_return([Addressable::URI.parse('https://[2606:4700:90:0:f22e:fbec:5bed:a9b9]/gitlab-org/gitlab-development-kit'), 'gitlab.com']) end @@ -430,7 +409,14 @@ RSpec.describe Projects::ImportService, feature_category: :importers do project.import_url = "http://example.com/group/project" allow(Gitlab::UrlBlocker).to receive(:validate!) - .with(project.import_url, ports: Project::VALID_IMPORT_PORTS, schemes: Project::VALID_IMPORT_PROTOCOLS, dns_rebind_protection: true) + .with( + project.import_url, + ports: Project::VALID_IMPORT_PORTS, + schemes: Project::VALID_IMPORT_PROTOCOLS, + allow_local_network: false, + allow_localhost: false, + dns_rebind_protection: true + ) .and_return([Addressable::URI.parse("http://172.16.123.1/group/project"), 'example.com']) end @@ -452,7 +438,14 @@ RSpec.describe Projects::ImportService, feature_category: :importers do project.import_url = "git://example.com/group/project.git" allow(Gitlab::UrlBlocker).to receive(:validate!) - .with(project.import_url, ports: Project::VALID_IMPORT_PORTS, schemes: Project::VALID_IMPORT_PROTOCOLS, dns_rebind_protection: true) + .with( + project.import_url, + ports: Project::VALID_IMPORT_PORTS, + schemes: Project::VALID_IMPORT_PROTOCOLS, + allow_local_network: false, + allow_localhost: false, + dns_rebind_protection: true + ) .and_return([Addressable::URI.parse("git://172.16.123.1/group/project"), 'example.com']) end diff --git a/spec/services/projects/lfs_pointers/lfs_link_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_link_service_spec.rb index d3f053aaedc..5862ed15c2a 100644 --- a/spec/services/projects/lfs_pointers/lfs_link_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_link_service_spec.rb @@ -91,5 +91,23 @@ RSpec.describe Projects::LfsPointers::LfsLinkService, feature_category: :source_ # 3. Insert the lfs_objects_projects for that batch expect { subject.execute(new_oid_list.keys) }.not_to exceed_query_limit(3) end + + context 'when MAX_OIDS is 5' do + let(:max_oids) { 5 } + let(:oids) { Array.new(max_oids) { |i| "oid-#{i}" } } + + before do + stub_const("#{described_class}::MAX_OIDS", max_oids) + end + + it 'does not raise an error when trying to link exactly the OID limit' do + expect { subject.execute(oids) }.not_to raise_error + end + + it 'raises an error when trying to link more than OID limit' do + oids << 'the straw' + expect { subject.execute(oids) }.to raise_error(described_class::TooManyOidsError) + end + end end end diff --git a/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb b/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb index 591cd1cba8d..48cf963e6e1 100644 --- a/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb +++ b/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::RefreshBuildArtifactsSizeStatisticsService, :clean_gitlab_redis_shared_state, feature_category: :build_artifacts do +RSpec.describe Projects::RefreshBuildArtifactsSizeStatisticsService, :clean_gitlab_redis_buffered_counter, feature_category: :build_artifacts do let(:service) { described_class.new } describe '#execute' do @@ -79,7 +79,7 @@ RSpec.describe Projects::RefreshBuildArtifactsSizeStatisticsService, :clean_gitl end before do - allow(Gitlab::Redis::SharedState).to receive(:with).and_raise(StandardError, 'error') + allow(Gitlab::Redis::BufferedCounter).to receive(:with).and_raise(StandardError, 'error') expect { service.execute }.to raise_error(StandardError) end diff --git a/spec/services/projects/unlink_fork_service_spec.rb b/spec/services/projects/unlink_fork_service_spec.rb index 872e38aba1d..2e1a6c03c90 100644 --- a/spec/services/projects/unlink_fork_service_spec.rb +++ b/spec/services/projects/unlink_fork_service_spec.rb @@ -58,6 +58,26 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin expect(source.forks_count).to be_zero end + it 'refreshes the project statistics of the forked project' do + expect(ProjectCacheWorker).to receive(:perform_async).with(forked_project.id, [], [:repository_size]) + + subject.execute + end + + it 'does not refresh project statistics when refresh_statistics is false' do + expect(ProjectCacheWorker).not_to receive(:perform_async) + + subject.execute(refresh_statistics: false) + end + + it 'does not refresh project statistics when the feature flag is disabled' do + stub_feature_flags(refresh_statistics_on_unlink_fork: false) + + expect(ProjectCacheWorker).not_to receive(:perform_async) + + subject.execute + end + 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 diff --git a/spec/services/projects/update_repository_storage_service_spec.rb b/spec/services/projects/update_repository_storage_service_spec.rb index b81fc8bf633..de7e6ae9a40 100644 --- a/spec/services/projects/update_repository_storage_service_spec.rb +++ b/spec/services/projects/update_repository_storage_service_spec.rb @@ -13,12 +13,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService, feature_category: :sour before do allow(Time).to receive(:now).and_return(time) - stub_storage_settings( - 'test_second_storage' => { - 'gitaly_address' => Gitlab.config.repositories.storages.default.gitaly_address, - 'path' => TestEnv::SECOND_STORAGE_PATH - } - ) + stub_storage_settings('test_second_storage' => {}) end context 'without wiki and design repository' do @@ -76,6 +71,8 @@ RSpec.describe Projects::UpdateRepositoryStorageService, feature_category: :sour expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('test_second_storage') expect(project.project_repository.shard_name).to eq('test_second_storage') + expect(repository_storage_move.reload).to be_finished + expect(repository_storage_move.error_message).to be_nil end end @@ -100,6 +97,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService, feature_category: :sour expect(project).not_to be_repository_read_only expect(repository_storage_move.reload).to be_failed + expect(repository_storage_move.error_message).to eq('Boom') end end @@ -126,7 +124,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService, feature_category: :sour expect(project_repository_double).to receive(:replicate) .with(project.repository.raw) - .and_raise(Gitlab::Git::CommandError) + .and_raise(Gitlab::Git::CommandError, 'Boom') expect(project_repository_double).to receive(:remove) expect do @@ -136,6 +134,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService, feature_category: :sour expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('default') expect(repository_storage_move).to be_failed + expect(repository_storage_move.error_message).to eq('Boom') end end @@ -216,29 +215,6 @@ RSpec.describe Projects::UpdateRepositoryStorageService, feature_category: :sour expect(object_pool_double).to have_received(:link).with(project.repository.raw) end - context 'when feature flag replicate_object_pool_on_move is disabled' do - before do - stub_feature_flags(replicate_object_pool_on_move: false) - end - - it 'just moves the repository without the object pool' do - result = subject.execute - expect(result).to be_success - - project.reload.cleanup - - new_pool_repository = project.pool_repository - - expect(new_pool_repository).to eq(pool_repository) - expect(new_pool_repository.shard).to eq(shard_default) - expect(new_pool_repository.state).to eq('ready') - expect(new_pool_repository.source_project).to eq(project) - - expect(object_pool_repository_double).not_to have_received(:replicate) - expect(object_pool_double).not_to have_received(:link) - end - end - context 'when new shard has a repository pool' do let!(:new_pool_repository) { create(:pool_repository, :ready, shard: shard_to, source_project: project) } diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 7ab85d8253a..a3f6c472f3b 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -356,7 +356,7 @@ RSpec.describe Projects::UpdateService, feature_category: :groups_and_projects d context 'when changes project features' do # Using some sample features for testing. # Not using all the features because some of them must be enabled/disabled together - %w[issues wiki forking model_experiments].each do |feature_name| + %w[issues wiki forking model_experiments model_registry].each do |feature_name| context "with feature_name:#{feature_name}" do let(:feature) { "#{feature_name}_access_level" } let(:params) do @@ -415,17 +415,92 @@ RSpec.describe Projects::UpdateService, feature_category: :groups_and_projects d end context 'when updating a project that contains container images' do + let(:new_name) { 'renamed' } + before do stub_container_registry_config(enabled: true) stub_container_registry_tags(repository: /image/, tags: %w[rc1]) create(:container_repository, project: project, name: :image) end - it 'does not allow to rename the project' do - result = update_project(project, admin, path: 'renamed') + shared_examples 'renaming the project fails with message' do |error_message| + it 'does not allow to rename the project' do + result = update_project(project, admin, path: new_name) + + expect(result).to include(status: :error) + expect(result[:message]).to match(error_message) + end + end + + context 'when the GitlabAPI is not supported' do + before do + allow(ContainerRegistry::GitlabApiClient).to receive(:supports_gitlab_api?).and_return(false) + end + + it_behaves_like 'renaming the project fails with message', /contains container registry tags/ + end + + context 'when Gitlab API is supported' do + before do + allow(ContainerRegistry::GitlabApiClient).to receive(:supports_gitlab_api?).and_return(true) + end + + it 'executes a dry run of the project rename' do + stub_rename_base_repository_in_registry(dry_run: true) + + update_project(project, admin, path: new_name) + + expect_rename_of_base_repository_in_registry(dry_run: true) + end + + context 'when the dry run fails' do + before do + stub_rename_base_repository_in_registry(dry_run: true, result: :bad_request) + end + + it_behaves_like 'renaming the project fails with message', /container registry path rename validation failed/ + + it 'logs the error' do + expect_any_instance_of(described_class).to receive(:log_error).with("Dry run failed for renaming project with tags: #{project.full_path}, error: bad_request") + + update_project(project, admin, path: new_name) + end + end + + context 'when the dry run succeeds' do + before do + stub_rename_base_repository_in_registry(dry_run: true, result: :accepted) + end + + it 'continues with the project rename' do + stub_rename_base_repository_in_registry(dry_run: false, result: :ok) + old_project_full_path = project.full_path - expect(result).to include(status: :error) - expect(result[:message]).to match(/contains container registry tags/) + update_project(project, admin, path: new_name) + + expect_rename_of_base_repository_in_registry(dry_run: true, path: old_project_full_path) + expect_rename_of_base_repository_in_registry(dry_run: false, path: old_project_full_path) + end + end + + def stub_rename_base_repository_in_registry(dry_run:, result: nil) + options = { name: new_name } + options[:dry_run] = true if dry_run + + allow(ContainerRegistry::GitlabApiClient) + .to receive(:rename_base_repository_path) + .with(project.full_path, options) + .and_return(result) + end + + def expect_rename_of_base_repository_in_registry(dry_run:, path: nil) + options = { name: new_name } + options[:dry_run] = true if dry_run + + expect(ContainerRegistry::GitlabApiClient) + .to have_received(:rename_base_repository_path) + .with(path || project.full_path, options) + end end it 'allows to update other settings' do @@ -708,7 +783,7 @@ RSpec.describe Projects::UpdateService, feature_category: :groups_and_projects d let(:opts) { { repository_storage: 'test_second_storage' } } before do - stub_storage_settings('test_second_storage' => { 'path' => 'tmp/tests/extra_storage' }) + stub_storage_settings('test_second_storage' => {}) end shared_examples 'the transfer was not scheduled' do diff --git a/spec/services/protected_branches/create_service_spec.rb b/spec/services/protected_branches/create_service_spec.rb index 625aa4fa377..abfb73c147e 100644 --- a/spec/services/protected_branches/create_service_spec.rb +++ b/spec/services/protected_branches/create_service_spec.rb @@ -16,6 +16,8 @@ RSpec.describe ProtectedBranches::CreateService, feature_category: :compliance_m describe '#execute' do let(:name) { 'master' } + let(:group_cache_service_double) { instance_double(ProtectedBranches::CacheService) } + let(:project_cache_service_double) { instance_double(ProtectedBranches::CacheService) } it 'creates a new protected branch' do expect { service.execute }.to change(ProtectedBranch, :count).by(1) @@ -24,8 +26,12 @@ RSpec.describe ProtectedBranches::CreateService, feature_category: :compliance_m end it 'refreshes the cache' do - expect_next_instance_of(ProtectedBranches::CacheService) do |cache_service| - expect(cache_service).to receive(:refresh) + expect(ProtectedBranches::CacheService).to receive(:new).with(entity, user, params).and_return(group_cache_service_double) + expect(group_cache_service_double).to receive(:refresh) + + if entity.is_a?(Group) + expect(ProtectedBranches::CacheService).to receive(:new).with(project, user, params).and_return(project_cache_service_double) + expect(project_cache_service_double).to receive(:refresh) end service.execute @@ -58,14 +64,14 @@ RSpec.describe ProtectedBranches::CreateService, feature_category: :compliance_m context 'with entity project' do let_it_be_with_reload(:entity) { create(:project) } - let(:user) { entity.first_owner } it_behaves_like 'execute with entity' end context 'with entity group' do - let_it_be_with_reload(:entity) { create(:group) } + let_it_be_with_reload(:project) { create(:project, :in_group) } + let_it_be_with_reload(:entity) { project.group } let_it_be_with_reload(:user) { create(:user) } before do diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 1c9c6323e96..dc93fd96aee 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -554,14 +554,14 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning end end - shared_examples 'award command' do - it 'toggle award 100 emoji if content contains /award :100:' do + shared_examples 'react command' do |command| + it "toggle award 100 emoji if content contains #{command} :100:" do _, updates, _ = service.execute(content, issuable) expect(updates).to eq(emoji_award: "100") end - it 'returns the award message' do + it 'returns the reaction message' do _, _, message = service.execute(content, issuable) expect(message).to eq('Toggled :100: emoji award.') @@ -1861,56 +1861,59 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning end end - context '/award command' do - it_behaves_like 'award command' do - let(:content) { '/award :100:' } - let(:issuable) { issue } - end - - it_behaves_like 'award command' do - let(:content) { '/award :100:' } - let(:issuable) { merge_request } - end - - it_behaves_like 'award command' do - let(:content) { '/award :100:' } - let(:issuable) { work_item } - end - - context 'ignores command with no argument' do - it_behaves_like 'failed command' do - let(:content) { '/award' } + %w[/react /award].each do |command| + context "#{command} command" do + it_behaves_like 'react command', command do + let(:content) { "#{command} :100:" } let(:issuable) { issue } end - it_behaves_like 'failed command' do - let(:content) { '/award' } - let(:issuable) { work_item } + it_behaves_like 'react command', command do + let(:content) { "#{command} :100:" } + let(:issuable) { merge_request } end - end - context 'ignores non-existing / invalid emojis' do - it_behaves_like 'failed command' do - let(:content) { '/award noop' } - let(:issuable) { issue } + it_behaves_like 'react command', command do + let(:content) { "#{command} :100:" } + let(:issuable) { work_item } end - it_behaves_like 'failed command' do - let(:content) { '/award :lorem_ipsum:' } - let(:issuable) { issue } + context 'ignores command with no argument' do + it_behaves_like 'failed command' do + let(:content) { command } + let(:issuable) { issue } + end + + it_behaves_like 'failed command' do + let(:content) { command } + let(:issuable) { work_item } + end end - it_behaves_like 'failed command' do - let(:content) { '/award :lorem_ipsum:' } - let(:issuable) { work_item } + context 'ignores non-existing / invalid emojis' do + it_behaves_like 'failed command' do + let(:content) { "#{command} noop" } + let(:issuable) { issue } + end + + it_behaves_like 'failed command' do + let(:content) { "#{command} :lorem_ipsum:" } + let(:issuable) { issue } + end + + it_behaves_like 'failed command' do + let(:content) { "#{command} :lorem_ipsum:" } + let(:issuable) { work_item } + end end - end - context 'if issuable is a Commit' do - let(:content) { '/award :100:' } - let(:issuable) { commit } + context 'if issuable is a Commit' do + let(:content) { "#{command} :100:" } + let(:issuable) { commit } - it_behaves_like 'failed command', 'Could not apply award command.' + # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/434446 + it_behaves_like 'failed command', "Could not apply award command." + end end end @@ -2185,6 +2188,36 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning expect(message).to eq('Submitted the current review.') end end + + context 'when parameters are passed' do + context 'with approve parameter' do + it 'calls MergeRequests::ApprovalService service' do + expect_next_instance_of( + MergeRequests::ApprovalService, project: merge_request.project, current_user: current_user + ) do |service| + expect(service).to receive(:execute).with(merge_request) + end + + _, _, message = service.execute('/submit_review approve', merge_request) + + expect(message).to eq('Submitted the current review.') + end + end + + context 'with review state parameter' do + it 'calls MergeRequests::UpdateReviewerStateService service' do + expect_next_instance_of( + MergeRequests::UpdateReviewerStateService, project: merge_request.project, current_user: current_user + ) do |service| + expect(service).to receive(:execute).with(merge_request, 'requested_changes') + end + + _, _, message = service.execute('/submit_review requested_changes', merge_request) + + expect(message).to eq('Submitted the current review.') + end + end + end end context 'request_changes command' do @@ -2374,6 +2407,30 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning end end + context 'when participants limit on issue is reached' do + before do + issue.issue_email_participants.create!(email: 'user@example.com') + stub_const("IssueEmailParticipants::CreateService::MAX_NUMBER_OF_RECORDS", 1) + end + + let(:content) { '/invite_email a@gitlab.com' } + + it_behaves_like 'failed command', + "No email participants were added. Either none were provided, or they already exist." + end + + context 'when only some emails can be added because of participants limit' do + before do + stub_const("IssueEmailParticipants::CreateService::MAX_NUMBER_OF_RECORDS", 1) + end + + let(:content) { '/invite_email a@gitlab.com b@gitlab.com' } + + it 'only adds one new email' do + expect { add_emails }.to change { issue.issue_email_participants.count }.by(1) + end + end + context 'with feature flag disabled' do before do stub_feature_flags(issue_email_participants: false) @@ -2384,6 +2441,18 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning end end end + + it 'is part of the available commands' do + expect(service.available_commands(issuable)).to include(a_hash_including(name: :invite_email)) + end + + context 'with non-persisted issue' do + let(:issuable) { build(:issue) } + + it 'is not part of the available commands' do + expect(service.available_commands(issuable)).not_to include(a_hash_including(name: :invite_email)) + end + end end context 'severity command' do diff --git a/spec/services/releases/destroy_service_spec.rb b/spec/services/releases/destroy_service_spec.rb index de3ce2b6206..b7729043896 100644 --- a/spec/services/releases/destroy_service_spec.rb +++ b/spec/services/releases/destroy_service_spec.rb @@ -30,7 +30,7 @@ RSpec.describe Releases::DestroyService, feature_category: :release_orchestratio end context 'when the release is for a catalog resource' do - let!(:catalog_resource) { create(:ci_catalog_resource, project: project, state: 'published') } + let!(:catalog_resource) { create(:ci_catalog_resource, :published, project: project) } let!(:version) { create(:ci_catalog_resource_version, catalog_resource: catalog_resource, release: release) } it 'does not update the catalog resources if there are still releases' do diff --git a/spec/services/service_desk/custom_email_verifications/create_service_spec.rb b/spec/services/service_desk/custom_email_verifications/create_service_spec.rb index 0046213e0b2..03c5743434e 100644 --- a/spec/services/service_desk/custom_email_verifications/create_service_spec.rb +++ b/spec/services/service_desk/custom_email_verifications/create_service_spec.rb @@ -14,7 +14,6 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::CreateService, feature_cat let(:service) { described_class.new(project: project, current_user: user) } - let(:error_feature_flag_disabled) { 'Feature flag service_desk_custom_email is not enabled' } let(:error_user_not_authorized) { s_('ServiceDesk|User cannot manage project.') } let(:error_settings_missing) { s_('ServiceDesk|Service Desk setting missing') } let(:expected_error_message) { error_settings_missing } @@ -45,7 +44,7 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::CreateService, feature_cat end end - shared_examples 'a verification process with ramp up error' do |error, error_identifier| + shared_examples 'a verification process with ramp up error' do it 'aborts verification process', :aggregate_failures do allow(message).to receive(:deliver).and_raise(error) @@ -80,16 +79,6 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::CreateService, feature_cat it_behaves_like 'a verification process that exits early' - context 'when feature flag :service_desk_custom_email is disabled' do - let(:expected_error_message) { error_feature_flag_disabled } - - before do - stub_feature_flags(service_desk_custom_email: false) - end - - it_behaves_like 'a verification process that exits early' - end - context 'when service desk setting exists' do let(:settings) { create(:service_desk_setting, project: project, custom_email: 'user@example.com') } let(:service) { described_class.new(project: settings.project, current_user: user) } @@ -115,7 +104,7 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::CreateService, feature_cat end context 'when user has maintainer role in project' do - before do + before_all do project.add_maintainer(user) end @@ -151,10 +140,25 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::CreateService, feature_cat allow(Notify).to receive(:service_desk_verification_result_email).and_return(message_delivery) end - it_behaves_like 'a verification process with ramp up error', SocketError, 'smtp_host_issue' - it_behaves_like 'a verification process with ramp up error', OpenSSL::SSL::SSLError, 'smtp_host_issue' - it_behaves_like 'a verification process with ramp up error', - Net::SMTPAuthenticationError.new('Invalid username or password'), 'invalid_credentials' + it_behaves_like 'a verification process with ramp up error' do + let(:error) { SocketError } + let(:error_identifier) { 'smtp_host_issue' } + end + + it_behaves_like 'a verification process with ramp up error' do + let(:error) { OpenSSL::SSL::SSLError } + let(:error_identifier) { 'smtp_host_issue' } + end + + it_behaves_like 'a verification process with ramp up error' do + let(:error) { Net::SMTPAuthenticationError.new('Invalid username or password') } + let(:error_identifier) { 'invalid_credentials' } + end + + it_behaves_like 'a verification process with ramp up error' do + let(:error) { Net::ReadTimeout } + let(:error_identifier) { 'read_timeout' } + end end end end diff --git a/spec/services/service_desk/custom_email_verifications/update_service_spec.rb b/spec/services/service_desk/custom_email_verifications/update_service_spec.rb index f87952d1d0e..103caf0e6c5 100644 --- a/spec/services/service_desk/custom_email_verifications/update_service_spec.rb +++ b/spec/services/service_desk/custom_email_verifications/update_service_spec.rb @@ -14,7 +14,6 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::UpdateService, feature_cat let(:message_delivery) { instance_double(ActionMailer::MessageDelivery) } let(:service) { described_class.new(project: settings.project, params: { mail: mail_object }) } - let(:error_feature_flag_disabled) { 'Feature flag service_desk_custom_email is not enabled' } let(:error_parameter_missing) { s_('ServiceDesk|Service Desk setting or verification object missing') } let(:error_already_finished) { s_('ServiceDesk|Custom email address has already been verified.') } let(:error_already_failed) do @@ -28,6 +27,9 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::UpdateService, feature_cat before do allow(message_delivery).to receive(:deliver_later) allow(Notify).to receive(:service_desk_verification_result_email).and_return(message_delivery) + + stub_incoming_email_setting(enabled: true, address: 'support+%{key}@example.com') + stub_service_desk_email_setting(enabled: true, address: 'contact+%{key}@example.com') end shared_examples 'a failing verification process' do |expected_error_identifier| @@ -86,26 +88,6 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::UpdateService, feature_cat expect(settings).not_to be_custom_email_enabled end - context 'when feature flag :service_desk_custom_email is disabled' do - let(:expected_error_message) { error_feature_flag_disabled } - - before do - stub_feature_flags(service_desk_custom_email: false) - end - - it 'exits early' do - expect(Notify).to receive(:service_desk_verification_result_email).exactly(0).times - - expect(Gitlab::AppLogger).to receive(:warn).with(logger_params.merge( - error_message: expected_error_message - )).once - - response = service.execute - - expect(response).to be_error - end - end - context 'when verification exists' do let!(:verification) { create(:service_desk_custom_email_verification, project: project) } @@ -139,7 +121,34 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::UpdateService, feature_cat verification.update!(token: 'ZROT4ZZXA-Y6') # token from email fixture end - let(:email_raw) { email_fixture('emails/service_desk_custom_email_address_verification.eml') } + let(:service_desk_address) { project.service_desk_incoming_address } + let(:verification_address) { 'custom-support-email+verify@example.com' } + let(:verification_token) { 'ZROT4ZZXA-Y6' } + let(:shared_email_raw) do + <<~EMAIL + From: Flight Support <custom-support-email@example.com> + Subject: Verify custom email address custom-support-email@example.com for Flight + Auto-Submitted: no + + + This email is auto-generated. It verifies the ownership of the entered Service Desk custom email address and + correct functionality of email forwarding. + + Verification token: #{verification_token} + -- + + You're receiving this email because of your account on 127.0.0.1. + EMAIL + end + + let(:email_raw) do + <<~EMAIL + Delivered-To: #{service_desk_address} + To: #{verification_address} + #{shared_email_raw} + EMAIL + end + let(:mail_object) { Mail::Message.new(email_raw) } it 'verifies and sends result emails' do @@ -181,6 +190,38 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::UpdateService, feature_cat it_behaves_like 'a failing verification process', 'mail_not_received_within_timeframe' end + context 'and service desk address from service_desk_email was used as forwarding target' do + let(:service_desk_address) { project.service_desk_alias_address } + + it_behaves_like 'a failing verification process', 'incorrect_forwarding_target' + + context 'when multiple Delivered-To headers are present' do + let(:email_raw) do + <<~EMAIL + Delivered-To: other@example.com + Delivered-To: #{service_desk_address} + To: #{verification_address} + #{shared_email_raw} + EMAIL + end + + it_behaves_like 'a failing verification process', 'incorrect_forwarding_target' + end + + context 'when multiple To headers are present' do + # Microsoft Exchange forwards emails this way when forwarding + # to an external email address using a transport rule + let(:email_raw) do + <<~EMAIL + To: #{service_desk_address}, #{verification_address} + #{shared_email_raw} + EMAIL + end + + it_behaves_like 'a failing verification process', 'incorrect_forwarding_target' + end + end + context 'when already verified' do let(:expected_error_message) { error_already_finished } diff --git a/spec/services/service_desk/custom_emails/create_service_spec.rb b/spec/services/service_desk/custom_emails/create_service_spec.rb index e165131bcf9..a015817b380 100644 --- a/spec/services/service_desk/custom_emails/create_service_spec.rb +++ b/spec/services/service_desk/custom_emails/create_service_spec.rb @@ -8,7 +8,6 @@ RSpec.describe ServiceDesk::CustomEmails::CreateService, feature_category: :serv let_it_be(:user) { create(:user) } let(:service) { described_class.new(project: project, current_user: user, params: params) } - let(:error_feature_flag_disabled) { 'Feature flag service_desk_custom_email is not enabled' } let(:error_user_not_authorized) { s_('ServiceDesk|User cannot manage project.') } let(:error_cannot_create_custom_email) { s_('ServiceDesk|Cannot create custom email') } let(:error_custom_email_exists) { s_('ServiceDesk|Custom email already exists') } @@ -52,16 +51,6 @@ RSpec.describe ServiceDesk::CustomEmails::CreateService, feature_category: :serv end end - context 'when feature flag service_desk_custom_email is disabled' do - let(:expected_error_message) { error_feature_flag_disabled } - - before do - stub_feature_flags(service_desk_custom_email: false) - end - - it_behaves_like 'a service that exits with error' - end - context 'with illegitimate user' do let(:expected_error_message) { error_user_not_authorized } diff --git a/spec/services/service_desk/custom_emails/destroy_service_spec.rb b/spec/services/service_desk/custom_emails/destroy_service_spec.rb index 7f53a941d4e..d77e408c31b 100644 --- a/spec/services/service_desk/custom_emails/destroy_service_spec.rb +++ b/spec/services/service_desk/custom_emails/destroy_service_spec.rb @@ -8,7 +8,6 @@ RSpec.describe ServiceDesk::CustomEmails::DestroyService, feature_category: :ser let(:user) { build_stubbed(:user) } let(:service) { described_class.new(project: project, current_user: user) } - let(:error_feature_flag_disabled) { 'Feature flag service_desk_custom_email is not enabled' } let(:error_user_not_authorized) { s_('ServiceDesk|User cannot manage project.') } let(:error_does_not_exist) { s_('ServiceDesk|Custom email does not exist') } let(:expected_error_message) { nil } @@ -45,16 +44,6 @@ RSpec.describe ServiceDesk::CustomEmails::DestroyService, feature_category: :ser end end - context 'when feature flag service_desk_custom_email is disabled' do - let(:expected_error_message) { error_feature_flag_disabled } - - before do - stub_feature_flags(service_desk_custom_email: false) - end - - it_behaves_like 'a service that exits with error' - end - context 'with illegitimate user' do let(:expected_error_message) { error_user_not_authorized } diff --git a/spec/services/service_desk_settings/update_service_spec.rb b/spec/services/service_desk_settings/update_service_spec.rb index a9e54012075..2c310bad247 100644 --- a/spec/services/service_desk_settings/update_service_spec.rb +++ b/spec/services/service_desk_settings/update_service_spec.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true + require 'spec_helper' RSpec.describe ServiceDeskSettings::UpdateService, :aggregate_failures, feature_category: :service_desk do @@ -12,7 +13,14 @@ RSpec.describe ServiceDeskSettings::UpdateService, :aggregate_failures, feature_ let_it_be(:user) { create(:user) } context 'with valid params' do - let(:params) { { outgoing_name: 'some name', project_key: 'foo', add_external_participants_from_cc: true } } + let(:params) do + { + outgoing_name: 'some name', + project_key: 'foo', + reopen_issue_on_external_participant_note: true, + add_external_participants_from_cc: true + } + end it 'updates service desk settings' do response = described_class.new(settings.project, user, params).execute @@ -21,6 +29,7 @@ RSpec.describe ServiceDeskSettings::UpdateService, :aggregate_failures, feature_ expect(settings.reset).to have_attributes( outgoing_name: 'some name', project_key: 'foo', + reopen_issue_on_external_participant_note: true, add_external_participants_from_cc: true ) end diff --git a/spec/services/snippets/update_repository_storage_service_spec.rb b/spec/services/snippets/update_repository_storage_service_spec.rb index 66847a43335..84e687329cc 100644 --- a/spec/services/snippets/update_repository_storage_service_spec.rb +++ b/spec/services/snippets/update_repository_storage_service_spec.rb @@ -43,6 +43,8 @@ RSpec.describe Snippets::UpdateRepositoryStorageService, feature_category: :sour expect(snippet).not_to be_repository_read_only expect(snippet.repository_storage).to eq(destination) expect(snippet.snippet_repository.shard_name).to eq(destination) + expect(repository_storage_move.reload).to be_finished + expect(repository_storage_move.error_message).to be_nil end end @@ -66,7 +68,7 @@ RSpec.describe Snippets::UpdateRepositoryStorageService, feature_category: :sour it 'unmarks the repository as read-only without updating the repository storage' do expect(snippet_repository_double).to receive(:replicate) .with(snippet.repository.raw) - .and_raise(Gitlab::Git::CommandError) + .and_raise(Gitlab::Git::CommandError, 'Boom') expect(snippet_repository_double).to receive(:remove) expect do @@ -76,6 +78,7 @@ RSpec.describe Snippets::UpdateRepositoryStorageService, feature_category: :sour expect(snippet).not_to be_repository_read_only expect(snippet.repository_storage).to eq('default') expect(repository_storage_move).to be_failed + expect(repository_storage_move.error_message).to eq('Boom') end end diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index ca6feb6fde2..0ba20ee5be1 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -387,7 +387,7 @@ RSpec.describe ::SystemNotes::IssuablesService, feature_category: :team_planning describe 'note_body' do context 'cross-project' do let(:project2) { create(:project, :repository) } - let(:mentioned_in) { create(:issue, project: project2) } + let(:mentioned_in) { create(:issue, :task, project: project2) } context 'from Commit' do let(:mentioned_in) { project2.repository.commit } @@ -399,7 +399,7 @@ RSpec.describe ::SystemNotes::IssuablesService, feature_category: :team_planning context 'from non-Commit' do it 'references the mentioning object' do - expect(subject.note).to eq "mentioned in issue #{mentioned_in.to_reference(project)}" + expect(subject.note).to eq "mentioned in task #{mentioned_in.to_reference(project)}" end end end diff --git a/spec/services/users/in_product_marketing_email_records_spec.rb b/spec/services/users/in_product_marketing_email_records_spec.rb deleted file mode 100644 index d214560b2a6..00000000000 --- a/spec/services/users/in_product_marketing_email_records_spec.rb +++ /dev/null @@ -1,55 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Users::InProductMarketingEmailRecords, feature_category: :onboarding do - let_it_be(:user) { create :user } - - subject(:records) { described_class.new } - - it 'initializes records' do - expect(subject.records).to match_array [] - end - - describe '#save!' do - before do - allow(Users::InProductMarketingEmail).to receive(:bulk_insert!) - - records.add(user, track: :team_short, series: 0) - records.add(user, track: :create, series: 1) - end - - it 'bulk inserts added records' do - expect(Users::InProductMarketingEmail).to receive(:bulk_insert!).with(records.records) - records.save! - end - - it 'resets its records' do - records.save! - expect(records.records).to match_array [] - end - end - - describe '#add' do - it 'adds a Users::InProductMarketingEmail record to its records', :aggregate_failures do - freeze_time do - records.add(user, track: :team_short, series: 0) - records.add(user, track: :create, series: 1) - - first, second = records.records - - expect(first).to be_a Users::InProductMarketingEmail - expect(first.track.to_sym).to eq :team_short - expect(first.series).to eq 0 - expect(first.created_at).to eq Time.zone.now - expect(first.updated_at).to eq Time.zone.now - - expect(second).to be_a Users::InProductMarketingEmail - expect(second.track.to_sym).to eq :create - expect(second.series).to eq 1 - expect(second.created_at).to eq Time.zone.now - expect(second.updated_at).to eq Time.zone.now - end - end - end -end diff --git a/spec/services/users/migrate_records_to_ghost_user_service_spec.rb b/spec/services/users/migrate_records_to_ghost_user_service_spec.rb index d6fb7a2954d..57378c07dd7 100644 --- a/spec/services/users/migrate_records_to_ghost_user_service_spec.rb +++ b/spec/services/users/migrate_records_to_ghost_user_service_spec.rb @@ -143,6 +143,13 @@ RSpec.describe Users::MigrateRecordsToGhostUserService, feature_category: :user_ let(:created_record) { create(:release, author: user) } end end + + context 'for user achievements' do + include_examples 'migrating records to the ghost user', Achievements::UserAchievement, + [:awarded_by_user, :revoked_by_user] do + let(:created_record) { create(:user_achievement, awarded_by_user: user, revoked_by_user: user) } + end + end end context 'on post-migrate cleanups' do @@ -358,6 +365,16 @@ RSpec.describe Users::MigrateRecordsToGhostUserService, feature_category: :user_ expect(Issue).not_to exist(issue.id) end + + it 'migrates awarded and revoked fields of user achievements' do + user_achievement = create(:user_achievement, awarded_by_user: user, revoked_by_user: user) + + service.execute(hard_delete: true) + user_achievement.reload + + expect(user_achievement.revoked_by_user).to eq(Users::Internal.ghost) + expect(user_achievement.awarded_by_user).to eq(Users::Internal.ghost) + end end end end diff --git a/spec/services/work_items/delete_task_service_spec.rb b/spec/services/work_items/delete_task_service_spec.rb deleted file mode 100644 index dc01da65771..00000000000 --- a/spec/services/work_items/delete_task_service_spec.rb +++ /dev/null @@ -1,88 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe WorkItems::DeleteTaskService, feature_category: :team_planning do - let_it_be(:project) { create(:project) } - let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } - let_it_be_with_refind(:task) { create(:work_item, project: project, author: developer) } - let_it_be_with_refind(:list_work_item) do - create(:work_item, project: project, description: "- [ ] #{task.to_reference}+") - end - - let(:current_user) { developer } - let(:line_number_start) { 1 } - let(:params) do - { - line_number_start: line_number_start, - line_number_end: 1, - task: task - } - end - - before_all do - create(:issue_link, source_id: list_work_item.id, target_id: task.id) - end - - shared_examples 'failing WorkItems::DeleteTaskService' do |error_message| - it { is_expected.to be_error } - - it 'does not remove work item or issue links' do - expect do - service_result - list_work_item.reload - end.to not_change(WorkItem, :count).and( - not_change(IssueLink, :count) - ).and( - not_change(list_work_item, :description) - ) - end - - it 'returns an error message' do - expect(service_result.errors).to contain_exactly(error_message) - end - end - - describe '#execute' do - subject(:service_result) do - described_class.new( - work_item: list_work_item, - current_user: current_user, - lock_version: list_work_item.lock_version, - task_params: params - ).execute - end - - context 'when work item params are valid' do - it { is_expected.to be_success } - - it 'deletes the work item and the related issue link' do - expect do - service_result - end.to change(WorkItem, :count).by(-1).and( - change(IssueLink, :count).by(-1) - ) - end - - it 'removes the task list item with the work item reference' do - expect do - service_result - end.to change(list_work_item, :description).from(list_work_item.description).to("- [ ] #{task.title}") - end - end - - context 'when first operation fails' do - let(:line_number_start) { -1 } - - it_behaves_like 'failing WorkItems::DeleteTaskService', 'line_number_start must be greater than 0' - end - - context 'when last operation fails' do - let_it_be(:non_member_user) { create(:user) } - - let(:current_user) { non_member_user } - - it_behaves_like 'failing WorkItems::DeleteTaskService', 'User not authorized to delete work item' - end - end -end diff --git a/spec/services/work_items/task_list_reference_removal_service_spec.rb b/spec/services/work_items/task_list_reference_removal_service_spec.rb deleted file mode 100644 index 0d34aaa3c1c..00000000000 --- a/spec/services/work_items/task_list_reference_removal_service_spec.rb +++ /dev/null @@ -1,152 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe WorkItems::TaskListReferenceRemovalService, feature_category: :team_planning do - let_it_be(:developer) { create(:user) } - let_it_be(:project) { create(:project, :repository).tap { |project| project.add_developer(developer) } } - let_it_be(:task) { create(:work_item, project: project, title: 'Task title') } - let_it_be(:single_line_work_item, refind: true) do - create(:work_item, project: project, description: "- [ ] #{task.to_reference}+ single line") - end - - let_it_be(:multiple_line_work_item, refind: true) do - create( - :work_item, - project: project, - description: <<~MARKDOWN - Any text - - * [ ] Item to be converted - #{task.to_reference}+ second line - third line - * [x] task - - More text - MARKDOWN - ) - end - - let(:line_number_start) { 3 } - let(:line_number_end) { 5 } - let(:work_item) { multiple_line_work_item } - let(:lock_version) { work_item.lock_version } - - shared_examples 'successful work item task reference removal service' do |expected_description| - it { is_expected.to be_success } - - it 'removes the task list item containing the task reference' do - expect do - result - end.to change(work_item, :description).from(work_item.description).to(expected_description) - end - - it 'creates system notes' do - expect do - result - end.to change(Note, :count).by(1) - - expect(Note.last.note).to include('changed the description') - end - end - - shared_examples 'failing work item task reference removal service' do |error_message| - it { is_expected.to be_error } - - it 'does not change the work item description' do - expect do - result - work_item.reload - end.to not_change(work_item, :description) - end - - it 'returns an error message' do - expect(result.errors).to contain_exactly(error_message) - end - end - - describe '#execute' do - subject(:result) do - described_class.new( - work_item: work_item, - task: task, - line_number_start: line_number_start, - line_number_end: line_number_end, - lock_version: lock_version, - current_user: developer - ).execute - end - - context 'when task mardown spans a single line' do - let(:line_number_start) { 1 } - let(:line_number_end) { 1 } - let(:work_item) { single_line_work_item } - - it_behaves_like 'successful work item task reference removal service', '- [ ] Task title single line' - - context 'when description does not contain a task' do - let_it_be(:no_matching_work_item) { create(:work_item, project: project, description: 'no matching task') } - - let(:work_item) { no_matching_work_item } - - it_behaves_like 'failing work item task reference removal service', 'Unable to detect a task on lines 1-1' - end - - context 'when description reference does not exactly match the task reference' do - before do - work_item.update!(description: work_item.description.gsub(task.to_reference, "#{task.to_reference}200")) - end - - it_behaves_like 'failing work item task reference removal service', 'Unable to detect a task on lines 1-1' - end - end - - context 'when task mardown spans multiple lines' do - it_behaves_like 'successful work item task reference removal service', - "Any text\n\n* [ ] Item to be converted\n Task title second line\n third line\n* [x] task\n\nMore text" - end - - context 'when updating the work item fails' do - before do - work_item.title = nil - end - - it_behaves_like 'failing work item task reference removal service', "Title can't be blank" - end - - context 'when description is empty' do - let_it_be(:empty_work_item) { create(:work_item, project: project, description: '') } - - let(:work_item) { empty_work_item } - - it_behaves_like 'failing work item task reference removal service', "Work item description can't be blank" - end - - context 'when line_number_start is lower than 1' do - let(:line_number_start) { 0 } - - it_behaves_like 'failing work item task reference removal service', 'line_number_start must be greater than 0' - end - - context 'when line_number_end is lower than line_number_start' do - let(:line_number_end) { line_number_start - 1 } - - it_behaves_like 'failing work item task reference removal service', - 'line_number_end must be greater or equal to line_number_start' - end - - context 'when lock_version is older than current' do - let(:lock_version) { work_item.lock_version - 1 } - - it_behaves_like 'failing work item task reference removal service', 'Stale work item. Check lock version' - end - - context 'when work item is stale before updating' do - it_behaves_like 'failing work item task reference removal service', 'Stale work item. Check lock version' do - before do - ::WorkItem.where(id: work_item.id).update_all(lock_version: lock_version + 1) - end - end - end - end -end |