diff options
Diffstat (limited to 'spec/services')
121 files changed, 2929 insertions, 1069 deletions
diff --git a/spec/services/activity_pub/accept_follow_service_spec.rb b/spec/services/activity_pub/accept_follow_service_spec.rb new file mode 100644 index 00000000000..0f472412085 --- /dev/null +++ b/spec/services/activity_pub/accept_follow_service_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ActivityPub::AcceptFollowService, feature_category: :integrations do + let_it_be(:project) { create(:project, :public) } + let_it_be_with_reload(:existing_subscription) do + create(:activity_pub_releases_subscription, :inbox, project: project) + end + + let(:service) { described_class.new(existing_subscription, 'http://localhost/my-project/releases') } + + describe '#execute' do + context 'when third party server complies' do + before do + allow(Gitlab::HTTP).to receive(:post).and_return(true) + service.execute + end + + it 'sends an Accept activity' do + expect(Gitlab::HTTP).to have_received(:post) + end + + it 'updates subscription state to accepted' do + expect(existing_subscription.reload.status).to eq 'accepted' + end + end + + context 'when there is an error with third party server' do + before do + allow(Gitlab::HTTP).to receive(:post).and_raise(Errno::ECONNREFUSED) + end + + it 'raises a ThirdPartyError' do + expect { service.execute }.to raise_error(ActivityPub::ThirdPartyError) + end + + it 'does not update subscription state to accepted' do + begin + service.execute + rescue StandardError + end + + expect(existing_subscription.reload.status).to eq 'requested' + end + end + + context 'when subscription is already accepted' do + before do + allow(Gitlab::HTTP).to receive(:post).and_return(true) + allow(existing_subscription).to receive(:accepted!).and_return(true) + existing_subscription.status = :accepted + service.execute + end + + it 'does not send an Accept activity' do + expect(Gitlab::HTTP).not_to have_received(:post) + end + + it 'does not update subscription state' do + expect(existing_subscription).not_to have_received(:accepted!) + end + end + + context 'when inbox has not been resolved' do + before do + allow(Gitlab::HTTP).to receive(:post).and_return(true) + allow(existing_subscription).to receive(:accepted!).and_return(true) + end + + it 'raises an error' do + existing_subscription.subscriber_inbox_url = nil + expect { service.execute }.to raise_error(ActivityPub::AcceptFollowService::MissingInboxURLError) + end + end + end +end diff --git a/spec/services/activity_pub/inbox_resolver_service_spec.rb b/spec/services/activity_pub/inbox_resolver_service_spec.rb new file mode 100644 index 00000000000..29048045bb5 --- /dev/null +++ b/spec/services/activity_pub/inbox_resolver_service_spec.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ActivityPub::InboxResolverService, feature_category: :integrations do + let_it_be(:project) { create(:project, :public) } + let_it_be_with_reload(:existing_subscription) { create(:activity_pub_releases_subscription, project: project) } + let(:service) { described_class.new(existing_subscription) } + + shared_examples 'third party error' do + it 'raises a ThirdPartyError' do + expect { service.execute }.to raise_error(ActivityPub::ThirdPartyError) + end + + it 'does not update the subscription record' do + begin + service.execute + rescue StandardError + end + + expect(ActivityPub::ReleasesSubscription.last.subscriber_inbox_url).not_to eq 'https://example.com/user/inbox' + end + end + + describe '#execute' do + context 'with successful HTTP request' do + before do + allow(Gitlab::HTTP).to receive(:get) { response } + end + + let(:response) { instance_double(HTTParty::Response, body: body) } + + context 'with a JSON response' do + let(:body) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://example.com/user', + type: 'Person', + **inbox, + **entrypoints, + outbox: 'https://example.com/user/outbox' + }.to_json + end + + let(:entrypoints) { {} } + + context 'with valid response' do + let(:inbox) { { inbox: 'https://example.com/user/inbox' } } + + context 'without a shared inbox' do + it 'updates only the inbox in the subscription record' do + service.execute + + expect(ActivityPub::ReleasesSubscription.last.subscriber_inbox_url).to eq 'https://example.com/user/inbox' + expect(ActivityPub::ReleasesSubscription.last.shared_inbox_url).to be_nil + end + end + + context 'with a shared inbox' do + let(:entrypoints) { { entrypoints: { sharedInbox: 'https://example.com/shared-inbox' } } } + + it 'updates both the inbox and shared inbox in the subscription record' do + service.execute + + expect(ActivityPub::ReleasesSubscription.last.subscriber_inbox_url).to eq 'https://example.com/user/inbox' + expect(ActivityPub::ReleasesSubscription.last.shared_inbox_url).to eq 'https://example.com/shared-inbox' + end + end + end + + context 'without inbox attribute' do + let(:inbox) { {} } + + it_behaves_like 'third party error' + end + + context 'with a non string inbox attribute' do + let(:inbox) { { inbox: 27.13 } } + + it_behaves_like 'third party error' + end + end + + context 'with non JSON response' do + let(:body) { '<div>woops</div>' } + + it_behaves_like 'third party error' + end + end + + context 'with http error' do + before do + allow(Gitlab::HTTP).to receive(:get).and_raise(Errno::ECONNREFUSED) + end + + it_behaves_like 'third party error' + end + end +end diff --git a/spec/services/admin/plan_limits/update_service_spec.rb b/spec/services/admin/plan_limits/update_service_spec.rb index e57c234780c..eb9bbcf11aa 100644 --- a/spec/services/admin/plan_limits/update_service_spec.rb +++ b/spec/services/admin/plan_limits/update_service_spec.rb @@ -82,9 +82,9 @@ RSpec.describe Admin::PlanLimits::UpdateService, feature_category: :shared do response = update_plan_limits expect(response[:status]).to eq :error - expect(response[:message]).to eq ["Notification limit must be greater than or equal to " \ - "storage_size_limit (Dashboard limit): 5 " \ - "and less than or equal to enforcement_limit: 10"] + expect(response[:message]).to eq [ + "Notification limit must be greater than or equal to the dashboard limit (5)" + ] end end @@ -102,9 +102,9 @@ RSpec.describe Admin::PlanLimits::UpdateService, feature_category: :shared do response = update_plan_limits expect(response[:status]).to eq :error - expect(response[:message]).to eq ["Notification limit must be greater than or equal to " \ - "storage_size_limit (Dashboard limit): 5 " \ - "and less than or equal to enforcement_limit: 10"] + expect(response[:message]).to eq [ + "Notification limit must be less than or equal to the enforcement limit (10)" + ] end end @@ -113,8 +113,8 @@ RSpec.describe Admin::PlanLimits::UpdateService, feature_category: :shared do before do limits.update!( - storage_size_limit: 12, - notification_limit: 12 + storage_size_limit: 10, + notification_limit: 9 ) end @@ -122,9 +122,9 @@ RSpec.describe Admin::PlanLimits::UpdateService, feature_category: :shared do response = update_plan_limits expect(response[:status]).to eq :error - expect(response[:message]).to eq ["Enforcement limit must be greater than " \ - "or equal to storage_size_limit (Dashboard limit): " \ - "12 and greater than or equal to notification_limit: 12"] + expect(response[:message]).to eq [ + "Enforcement limit must be greater than or equal to the dashboard limit (10)" + ] end end @@ -133,7 +133,7 @@ RSpec.describe Admin::PlanLimits::UpdateService, feature_category: :shared do before do limits.update!( - storage_size_limit: 10, + storage_size_limit: 9, notification_limit: 10 ) end @@ -142,9 +142,9 @@ RSpec.describe Admin::PlanLimits::UpdateService, feature_category: :shared do response = update_plan_limits expect(response[:status]).to eq :error - expect(response[:message]).to eq ["Enforcement limit must be greater than or equal to " \ - "storage_size_limit (Dashboard limit): " \ - "10 and greater than or equal to notification_limit: 10"] + expect(response[:message]).to eq [ + "Enforcement limit must be greater than or equal to the notification limit (10)" + ] end end @@ -162,8 +162,9 @@ RSpec.describe Admin::PlanLimits::UpdateService, feature_category: :shared do response = update_plan_limits expect(response[:status]).to eq :error - expect(response[:message]).to eq ["Storage size limit (Dashboard limit) must be less than or " \ - "equal to enforcement_limit: 12 and notification_limit: 10"] + expect(response[:message]).to eq [ + "Dashboard limit must be less than or equal to the notification limit (10)" + ] end end @@ -181,8 +182,45 @@ RSpec.describe Admin::PlanLimits::UpdateService, feature_category: :shared do response = update_plan_limits expect(response[:status]).to eq :error - expect(response[:message]).to eq ["Storage size limit (Dashboard limit) must be less than or " \ - "equal to enforcement_limit: 10 and notification_limit: 11"] + expect(response[:message]).to eq [ + "Dashboard limit must be less than or equal to the enforcement limit (10)" + ] + end + + context 'when enforcement_limit is 0' do + before do + limits.update!( + enforcement_limit: 0 + ) + end + + it 'does not return an error' do + response = update_plan_limits + + expect(response[:status]).to eq :success + end + end + end + end + + context 'when setting limit to unlimited' do + before do + limits.update!( + notification_limit: 10, + storage_size_limit: 10, + enforcement_limit: 10 + ) + end + + [:notification_limit, :enforcement_limit, :storage_size_limit].each do |limit| + context "for #{limit}" do + let(:params) { { limit => 0 } } + + it 'is successful' do + response = update_plan_limits + + expect(response[:status]).to eq :success + end end end end diff --git a/spec/services/application_settings/update_service_spec.rb b/spec/services/application_settings/update_service_spec.rb index 0b5ba1db9d4..474d6ec4a9b 100644 --- a/spec/services/application_settings/update_service_spec.rb +++ b/spec/services/application_settings/update_service_spec.rb @@ -144,7 +144,7 @@ RSpec.describe ApplicationSettings::UpdateService, feature_category: :shared do end end - describe 'performance bar settings', feature_category: :application_performance do + describe 'performance bar settings', feature_category: :cloud_connector do using RSpec::Parameterized::TableSyntax where( @@ -321,7 +321,9 @@ RSpec.describe ApplicationSettings::UpdateService, feature_category: :shared do let(:params) { { default_branch_protection: ::Gitlab::Access::PROTECTION_DEV_CAN_MERGE } } it "updates default_branch_protection_defaults from the default_branch_protection param" do - expect { subject.execute }.to change { application_settings.default_branch_protection_defaults }.from({}).to(expected) + default_value = ::Gitlab::Access::BranchProtection.protected_fully.deep_stringify_keys + + expect { subject.execute }.to change { application_settings.default_branch_protection_defaults }.from(default_value).to(expected) end end diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb index 8cd33f8ff1e..8329c4312cd 100644 --- a/spec/services/auto_merge/base_service_spec.rb +++ b/spec/services/auto_merge/base_service_spec.rb @@ -309,26 +309,18 @@ RSpec.describe AutoMerge::BaseService, feature_category: :code_review_workflow d let(:merge_request) { create(:merge_request) } - where(:can_be_merged, :open, :broken, :discussions, :blocked, :draft, :skip_draft, :skip_blocked, - :skip_discussions, :result) do - true | true | false | true | false | false | false | false | false | true - true | true | false | true | false | false | true | true | false | true - true | true | false | true | false | true | true | false | false | true - true | true | false | true | true | false | false | true | false | true - true | true | false | false | false | false | false | false | true | true - true | true | false | true | false | true | false | false | false | false - false | true | false | true | false | false | false | false | false | false - true | false | false | true | false | false | false | false | false | false - true | true | true | true | false | false | false | false | false | false - true | true | false | false | false | false | false | false | false | false - true | true | false | true | true | false | false | false | false | false + where(:can_be_merged, :open, :broken, :discussions, :blocked, :draft, :result) do + true | true | false | true | false | false | true + false | true | false | true | false | false | false + true | false | false | true | false | false | false + true | true | true | true | false | false | false + true | true | false | false | false | false | false + true | true | false | true | true | false | false + true | true | false | true | false | true | false end with_them do before do - allow(service).to receive(:skip_draft_check).and_return(skip_draft) - allow(service).to receive(:skip_blocked_check).and_return(skip_blocked) - allow(service).to receive(:skip_discussions_check).and_return(skip_discussions) allow(merge_request).to receive(:can_be_merged_by?).and_return(can_be_merged) allow(merge_request).to receive(:open?).and_return(open) allow(merge_request).to receive(:broken?).and_return(broken) diff --git a/spec/services/award_emojis/copy_service_spec.rb b/spec/services/award_emojis/copy_service_spec.rb index 6c1d7fb21e2..81ec49d7741 100644 --- a/spec/services/award_emojis/copy_service_spec.rb +++ b/spec/services/award_emojis/copy_service_spec.rb @@ -25,7 +25,7 @@ RSpec.describe AwardEmojis::CopyService, feature_category: :team_planning do it 'copies AwardEmojis', :aggregate_failures do expect { execute_service }.to change { AwardEmoji.count }.by(2) - expect(to_awardable.award_emoji.map(&:name)).to match_array(%w(thumbsup thumbsdown)) + expect(to_awardable.award_emoji.map(&:name)).to match_array(%w[thumbsup thumbsdown]) end it 'returns success', :aggregate_failures do 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 c361dfe5052..dd85961befd 100644 --- a/spec/services/bulk_imports/batched_relation_export_service_spec.rb +++ b/spec/services/bulk_imports/batched_relation_export_service_spec.rb @@ -71,29 +71,6 @@ RSpec.describe BulkImports::BatchedRelationExportService, feature_category: :imp expect(export.batches.count).to eq(0) end end - - context 'when exception occurs' do - it 'tracks exception and marks export as failed' do - allow_next_instance_of(BulkImports::Export) do |export| - allow(export).to receive(:update!).and_call_original - - allow(export) - .to receive(:update!) - .with(status_event: 'finish', total_objects_count: 0, batched: true, batches_count: 0, jid: jid, error: nil) - .and_raise(StandardError, 'Error!') - end - - expect(Gitlab::ErrorTracking) - .to receive(:track_exception) - .with(StandardError, portable_id: portable.id, portable_type: portable.class.name) - - service.execute - - export = portable.bulk_import_exports.first - - expect(export.reload.failed?).to eq(true) - end - end end describe '.cache_key' do diff --git a/spec/services/bulk_imports/file_download_service_spec.rb b/spec/services/bulk_imports/file_download_service_spec.rb index 1734ea45507..b2971c75bce 100644 --- a/spec/services/bulk_imports/file_download_service_spec.rb +++ b/spec/services/bulk_imports/file_download_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe BulkImports::FileDownloadService, feature_category: :importers do describe '#execute' do - let_it_be(:allowed_content_types) { %w(application/gzip application/octet-stream) } + let_it_be(:allowed_content_types) { %w[application/gzip application/octet-stream] } let_it_be(:file_size_limit) { 5.gigabytes } let_it_be(:config) { build(:bulk_import_configuration) } let_it_be(:content_type) { 'application/octet-stream' } @@ -82,18 +82,17 @@ RSpec.describe BulkImports::FileDownloadService, feature_category: :importers do context 'when content-type is not valid' do let(:content_type) { 'invalid' } - let(:import_logger) { instance_double(Gitlab::Import::Logger) } + let(:import_logger) { instance_double(BulkImports::Logger) } before do - allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger) + allow(BulkImports::Logger).to receive(:build).and_return(import_logger) allow(import_logger).to receive(:warn) end it 'logs and raises an error' do expect(import_logger).to receive(:warn).once.with( message: 'Invalid content type', - response_headers: headers, - importer: 'gitlab_migration' + response_headers: headers ) expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid content type') diff --git a/spec/services/bulk_imports/lfs_objects_export_service_spec.rb b/spec/services/bulk_imports/lfs_objects_export_service_spec.rb index 587c99d9897..b65862b30d2 100644 --- a/spec/services/bulk_imports/lfs_objects_export_service_spec.rb +++ b/spec/services/bulk_imports/lfs_objects_export_service_spec.rb @@ -15,7 +15,7 @@ RSpec.describe BulkImports::LfsObjectsExportService, feature_category: :importer before do stub_lfs_object_storage - %w(wiki design).each do |repository_type| + %w[wiki design].each do |repository_type| create( :lfs_objects_project, project: project, diff --git a/spec/services/bulk_imports/process_service_spec.rb b/spec/services/bulk_imports/process_service_spec.rb index 5398e76cb67..f5566819039 100644 --- a/spec/services/bulk_imports/process_service_spec.rb +++ b/spec/services/bulk_imports/process_service_spec.rb @@ -133,23 +133,6 @@ RSpec.describe BulkImports::ProcessService, feature_category: :importers do end end end - - context 'when exception occurs' do - it 'tracks the exception & marks import as failed' do - create(:bulk_import_entity, :created, bulk_import: bulk_import) - - allow(BulkImports::ExportRequestWorker).to receive(:perform_async).and_raise(StandardError) - - expect(Gitlab::ErrorTracking).to receive(:track_exception).with( - kind_of(StandardError), - bulk_import_id: bulk_import.id - ) - - subject.execute - - expect(bulk_import.reload.failed?).to eq(true) - end - end end context 'when importing a group' do @@ -221,15 +204,14 @@ RSpec.describe BulkImports::ProcessService, feature_category: :importers do end it 'logs an info message for the skipped pipelines' do - expect_next_instance_of(Gitlab::Import::Logger) do |logger| + expect_next_instance_of(BulkImports::Logger) do |logger| 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, - importer: 'gitlab_migration', - pipeline_name: 'PipelineClass4', + pipeline_class: 'PipelineClass4', minimum_source_version: '15.1.0', maximum_source_version: nil, source_version: '15.0.0' @@ -241,8 +223,7 @@ RSpec.describe BulkImports::ProcessService, feature_category: :importers do bulk_import_id: entity.bulk_import_id, bulk_import_entity_type: entity.source_type, source_full_path: entity.source_full_path, - importer: 'gitlab_migration', - pipeline_name: 'PipelineClass5', + pipeline_class: 'PipelineClass5', minimum_source_version: '16.0.0', maximum_source_version: nil, source_version: '15.0.0' diff --git a/spec/services/bulk_imports/relation_batch_export_service_spec.rb b/spec/services/bulk_imports/relation_batch_export_service_spec.rb index 8548e01a6aa..a18099a4189 100644 --- a/spec/services/bulk_imports/relation_batch_export_service_spec.rb +++ b/spec/services/bulk_imports/relation_batch_export_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe BulkImports::RelationBatchExportService, feature_category: :impor let_it_be(:batch) { create(:bulk_import_export_batch, export: export) } let_it_be(:cache_key) { BulkImports::BatchedRelationExportService.cache_key(export.id, batch.id) } - subject(:service) { described_class.new(user.id, batch.id) } + subject(:service) { described_class.new(user, batch) } before_all do Gitlab::Cache::Import::Caching.set_add(cache_key, label.id) @@ -34,10 +34,7 @@ RSpec.describe BulkImports::RelationBatchExportService, feature_category: :impor end it 'removes exported contents after export' do - double = instance_double(BulkImports::FileTransfer::ProjectConfig, export_path: 'foo') - - allow(BulkImports::FileTransfer).to receive(:config_for).and_return(double) - allow(double).to receive(:export_service_for).and_raise(StandardError, 'Error!') + allow(subject).to receive(:export_path).and_return('foo') allow(FileUtils).to receive(:remove_entry) expect(FileUtils).to receive(:remove_entry).with('foo') @@ -53,29 +50,10 @@ RSpec.describe BulkImports::RelationBatchExportService, feature_category: :impor allow(subject).to receive(:export_path).and_return('foo') allow(FileUtils).to receive(:remove_entry) - expect(FileUtils).to receive(:touch).with('foo/milestones.ndjson') + expect(FileUtils).to receive(:touch).with('foo/milestones.ndjson').and_call_original subject.execute end end - - context 'when exception occurs' do - before do - allow(service).to receive(:gzip).and_raise(StandardError, 'Error!') - end - - it 'marks batch as failed' do - expect(Gitlab::ErrorTracking) - .to receive(:track_exception) - .with(StandardError, portable_id: project.id, portable_type: 'Project') - - service.execute - batch.reload - - expect(batch.failed?).to eq(true) - expect(batch.objects_count).to eq(0) - expect(batch.error).to eq('Error!') - end - end end end diff --git a/spec/services/bulk_imports/relation_export_service_spec.rb b/spec/services/bulk_imports/relation_export_service_spec.rb index bd8447e3d97..b7d6c424277 100644 --- a/spec/services/bulk_imports/relation_export_service_spec.rb +++ b/spec/services/bulk_imports/relation_export_service_spec.rb @@ -59,7 +59,7 @@ RSpec.describe BulkImports::RelationExportService, feature_category: :importers let(:relation) { 'milestones' } it 'creates empty file on disk' do - expect(FileUtils).to receive(:touch).with("#{export_path}/#{relation}.ndjson") + expect(FileUtils).to receive(:touch).with("#{export_path}/#{relation}.ndjson").and_call_original subject.execute end @@ -118,39 +118,6 @@ RSpec.describe BulkImports::RelationExportService, feature_category: :importers end end - context 'when exception occurs during export' do - shared_examples 'tracks exception' do |exception_class| - it 'tracks exception' do - expect(Gitlab::ErrorTracking) - .to receive(:track_exception) - .with(exception_class, portable_id: group.id, portable_type: group.class.name) - .and_call_original - - subject.execute - end - end - - before do - allow_next_instance_of(BulkImports::ExportUpload) do |upload| - allow(upload).to receive(:save!).and_raise(StandardError) - end - end - - it 'marks export as failed' do - subject.execute - - expect(export.reload.failed?).to eq(true) - end - - include_examples 'tracks exception', StandardError - - context 'when passed relation is not supported' do - let(:relation) { 'unsupported' } - - include_examples 'tracks exception', ActiveRecord::RecordInvalid - end - end - context 'when export was batched' do let(:relation) { 'milestones' } let(:export) { create(:bulk_import_export, group: group, relation: relation, batched: true, batches_count: 2) } diff --git a/spec/services/ci/cancel_pipeline_service_spec.rb b/spec/services/ci/cancel_pipeline_service_spec.rb index c4a1e1c26d1..256d2db1ed2 100644 --- a/spec/services/ci/cancel_pipeline_service_spec.rb +++ b/spec/services/ci/cancel_pipeline_service_spec.rb @@ -12,12 +12,12 @@ RSpec.describe Ci::CancelPipelineService, :aggregate_failures, feature_category: pipeline: pipeline, current_user: current_user, cascade_to_children: cascade_to_children, - auto_canceled_by_pipeline_id: auto_canceled_by_pipeline_id, + auto_canceled_by_pipeline: auto_canceled_by_pipeline, execute_async: execute_async) end let(:cascade_to_children) { true } - let(:auto_canceled_by_pipeline_id) { nil } + let(:auto_canceled_by_pipeline) { nil } let(:execute_async) { true } shared_examples 'force_execute' do @@ -58,14 +58,19 @@ RSpec.describe Ci::CancelPipelineService, :aggregate_failures, feature_category: expect(pipeline.all_jobs.pluck(:status)).to match_array(%w[canceled canceled success]) end - context 'when auto_canceled_by_pipeline_id is provided' do - let(:auto_canceled_by_pipeline_id) { create(:ci_pipeline).id } + context 'when auto_canceled_by_pipeline is provided' do + let(:auto_canceled_by_pipeline) { create(:ci_pipeline) } it 'updates the pipeline and jobs with it' do subject - expect(pipeline.auto_canceled_by_id).to eq(auto_canceled_by_pipeline_id) - expect(pipeline.all_jobs.canceled.pluck(:auto_canceled_by_id).uniq).to eq([auto_canceled_by_pipeline_id]) + expect(pipeline.auto_canceled_by_id).to eq(auto_canceled_by_pipeline.id) + + expect(pipeline.all_jobs.canceled.pluck(:auto_canceled_by_id).uniq) + .to eq([auto_canceled_by_pipeline.id]) + + expect(pipeline.all_jobs.canceled.pluck(:auto_canceled_by_partition_id).uniq) + .to eq([auto_canceled_by_pipeline.partition_id]) end end diff --git a/spec/services/ci/catalog/resources/create_service_spec.rb b/spec/services/ci/catalog/resources/create_service_spec.rb new file mode 100644 index 00000000000..202c76acaec --- /dev/null +++ b/spec/services/ci/catalog/resources/create_service_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::Catalog::Resources::CreateService, feature_category: :pipeline_composition do + let_it_be(:project) { create(:project, :catalog_resource_with_components) } + 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 }.to raise_error(Gitlab::Access::AccessDeniedError) + end + end + + context 'with an authorized user' do + before_all do + project.add_owner(user) + end + + context 'and a valid project' do + it 'creates a catalog resource' do + response = service.execute + + expect(response.payload.project).to eq(project) + end + end + + context 'with an invalid catalog resource' do + it 'does not save the catalog resource' do + catalog_resource = instance_double(::Ci::Catalog::Resource, + valid?: false, + errors: instance_double(ActiveModel::Errors, full_messages: ['not valid'])) + allow(::Ci::Catalog::Resource).to receive(:new).and_return(catalog_resource) + + response = service.execute + + expect(response.message).to eq('not valid') + end + end + end + end +end diff --git a/spec/services/ci/catalog/resources/release_service_spec.rb b/spec/services/ci/catalog/resources/release_service_spec.rb new file mode 100644 index 00000000000..60cd6cb5f96 --- /dev/null +++ b/spec/services/ci/catalog/resources/release_service_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::Catalog::Resources::ReleaseService, feature_category: :pipeline_composition do + describe '#execute' do + context 'with a valid catalog resource and release' do + it 'validates the catalog resource and creates a version' do + project = create(:project, :catalog_resource_with_components) + catalog_resource = create(:ci_catalog_resource, project: project) + release = create(:release, project: project, sha: project.repository.root_ref_sha) + + response = described_class.new(release).execute + + version = Ci::Catalog::Resources::Version.last + + expect(response).to be_success + expect(version.release).to eq(release) + expect(version.catalog_resource).to eq(catalog_resource) + expect(version.catalog_resource.project).to eq(project) + end + end + + context 'when the validation of the catalog resource fails' do + it 'returns an error and does not create a version' do + project = create(:project, :repository) + create(:ci_catalog_resource, project: project) + release = create(:release, project: project, sha: project.repository.root_ref_sha) + + response = described_class.new(release).execute + + expect(Ci::Catalog::Resources::Version.count).to be(0) + expect(response).to be_error + expect(response.message).to eq( + 'Project must have a description, ' \ + 'Project must contain components. Ensure you are using the correct directory structure') + end + end + + context 'when the creation of a version fails' do + it 'returns an error and does not create a version' do + project = + create( + :project, :custom_repo, + description: 'Component project', + files: { + 'templates/secret-detection.yml' => 'image: agent: coop', + 'README.md' => 'Read me' + } + ) + create(:ci_catalog_resource, project: project) + release = create(:release, project: project, sha: project.repository.root_ref_sha) + + response = described_class.new(release).execute + + expect(Ci::Catalog::Resources::Version.count).to be(0) + expect(response).to be_error + expect(response.message).to include('mapping values are not allowed in this context') + end + end + end +end diff --git a/spec/services/ci/catalog/resources/validate_service_spec.rb b/spec/services/ci/catalog/resources/validate_service_spec.rb index b43d98788e2..39ab758d78d 100644 --- a/spec/services/ci/catalog/resources/validate_service_spec.rb +++ b/spec/services/ci/catalog/resources/validate_service_spec.rb @@ -4,54 +4,85 @@ require 'spec_helper' RSpec.describe Ci::Catalog::Resources::ValidateService, feature_category: :pipeline_composition do describe '#execute' do - context 'with a project that has a README and a description' do + context 'when a project has a README, a description, and at least one component' do it 'is valid' do - project = create(:project, :repository, description: 'Component project') + project = create(:project, :catalog_resource_with_components) response = described_class.new(project, project.default_branch).execute expect(response).to be_success end end - context 'with a project that has neither a description nor a README' do + context 'when a project has neither a description nor a README nor components' do it 'is not valid' do - project = create(:project, :empty_repo) - project.repository.create_file( - project.creator, - 'ruby.rb', - 'I like this', - message: 'Ruby like this', - branch_name: 'master' - ) + project = create(:project, :small_repo) response = described_class.new(project, project.default_branch).execute - expect(response.message).to eq('Project must have a README , Project must have a description') + expect(response.message).to eq( + 'Project must have a README, ' \ + 'Project must have a description, ' \ + 'Project must contain components. Ensure you are using the correct directory structure') end end - context 'with a project that has a description but not a README' do + context 'when a project has components but has neither a description nor a README' do it 'is not valid' do - project = create(:project, :empty_repo, description: 'project with no README') - project.repository.create_file( - project.creator, - 'text.txt', - 'I do not like this', - message: 'only text like text', - branch_name: 'master' - ) + project = create(:project, :small_repo, files: { 'templates/dast/template.yml' => 'image: alpine' }) response = described_class.new(project, project.default_branch).execute - expect(response.message).to eq('Project must have a README') + expect(response.message).to eq('Project must have a README, Project must have a description') + end + end + + context 'when a project has a description but has neither a README nor components' do + it 'is not valid' do + project = create(:project, :small_repo, description: 'project with no README and no components') + response = described_class.new(project, project.default_branch).execute + + expect(response.message).to eq( + 'Project must have a README, ' \ + 'Project must contain components. Ensure you are using the correct directory structure') end end - context 'with a project that has a README and not a description' do + context 'when a project has a README but has neither a description nor components' do it 'is not valid' do project = create(:project, :repository) response = described_class.new(project, project.default_branch).execute + expect(response.message).to eq( + 'Project must have a description, ' \ + 'Project must contain components. Ensure you are using the correct directory structure') + end + end + + context 'when a project has components and a description but no README' do + it 'is not valid' do + project = create(:project, :small_repo, description: 'desc', files: { 'templates/dast.yml' => 'image: alpine' }) + response = described_class.new(project, project.default_branch).execute + + expect(response.message).to eq('Project must have a README') + end + end + + context 'when a project has components and a README but no description' do + it 'is not valid' do + project = create(:project, :custom_repo, + files: { 'templates/dast.yml' => 'image: alpine', 'README.md' => 'readme' }) + response = described_class.new(project, project.default_branch).execute + expect(response.message).to eq('Project must have a description') end end + + context 'when a project has a description and a README but no components' do + it 'is not valid' do + project = create(:project, :readme, description: 'project with no README and no components') + response = described_class.new(project, project.default_branch).execute + + expect(response.message).to eq( + 'Project must contain components. Ensure you are using the correct directory structure') + end + end end end diff --git a/spec/services/ci/catalog/resources/versions/create_service_spec.rb b/spec/services/ci/catalog/resources/versions/create_service_spec.rb new file mode 100644 index 00000000000..e614a74a4a1 --- /dev/null +++ b/spec/services/ci/catalog/resources/versions/create_service_spec.rb @@ -0,0 +1,180 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::Catalog::Resources::Versions::CreateService, feature_category: :pipeline_composition do + describe '#execute' do + let(:files) do + { + 'templates/secret-detection.yml' => "spec:\n inputs:\n website:\n---\nimage: alpine_1", + 'templates/dast/template.yml' => 'image: alpine_2', + 'templates/blank-yaml.yml' => '', + 'templates/dast/sub-folder/template.yml' => 'image: alpine_3', + 'templates/template.yml' => "spec:\n inputs:\n environment:\n---\nimage: alpine_6", + 'tests/test.yml' => 'image: alpine_7', + 'README.md' => 'Read me' + } + end + + let(:project) do + create( + :project, :custom_repo, + description: 'Simple and Complex components', + files: files + ) + end + + let(:release) { create(:release, project: project, sha: project.repository.root_ref_sha) } + let!(:catalog_resource) { create(:ci_catalog_resource, project: project) } + + context 'when the project is not a catalog resource' do + it 'does not create a version' do + project = create(:project, :repository) + release = create(:release, project: project, sha: project.repository.root_ref_sha) + + response = described_class.new(release).execute + + expect(response).to be_error + expect(response.message).to include('Project is not a catalog resource') + end + end + + context 'when the catalog resource has different types of components and a release' do + it 'creates a version for the release' do + response = described_class.new(release).execute + + expect(response).to be_success + + version = Ci::Catalog::Resources::Version.last + + expect(version.release).to eq(release) + expect(version.catalog_resource).to eq(catalog_resource) + expect(version.catalog_resource.project).to eq(project) + end + + it 'marks the catalog resource as published' do + described_class.new(release).execute + + expect(catalog_resource.reload.state).to eq('published') + end + + context 'when the ci_catalog_create_metadata feature flag is disabled' do + before do + stub_feature_flags(ci_catalog_create_metadata: false) + end + + it 'does not create components' do + expect(Ci::Catalog::Resources::Component).not_to receive(:bulk_insert!).and_call_original + expect(project.ci_components.count).to eq(0) + + response = described_class.new(release).execute + + expect(response).to be_success + expect(project.ci_components.count).to eq(0) + end + end + + context 'when the ci_catalog_create_metadata feature flag is enabled' do + context 'when there are more than 10 components' do + let(:files) do + { + 'templates/secret11.yml' => '', + 'templates/secret10.yml' => '', + 'templates/secret8.yml' => '', + 'templates/secret7.yml' => '', + 'templates/secret6.yml' => '', + 'templates/secret5.yml' => '', + 'templates/secret4.yml' => '', + 'templates/secret3.yml' => '', + 'templates/secret2.yml' => '', + 'templates/secret1.yml' => '', + 'templates/secret0.yml' => '', + 'README.md' => 'Read me' + } + end + + it 'does not create components' do + response = described_class.new(release).execute + + expect(response).to be_error + expect(response.message).to include('Release cannot contain more than 10 components') + expect(project.ci_components.count).to eq(0) + end + end + + it 'bulk inserts all the components' do + expect(Ci::Catalog::Resources::Component).to receive(:bulk_insert!).and_call_original + + described_class.new(release).execute + end + + it 'creates components for the catalog resource' do + expect(project.ci_components.count).to eq(0) + response = described_class.new(release).execute + + expect(response).to be_success + + version = Ci::Catalog::Resources::Version.last + + expect(project.ci_components.count).to eq(4) + expect(project.ci_components.first.name).to eq('blank-yaml') + expect(project.ci_components.first.project).to eq(version.project) + expect(project.ci_components.first.inputs).to eq({}) + expect(project.ci_components.first.catalog_resource).to eq(version.catalog_resource) + expect(project.ci_components.first.version).to eq(version) + expect(project.ci_components.first.path).to eq('templates/blank-yaml.yml') + expect(project.ci_components.second.name).to eq('dast') + expect(project.ci_components.second.project).to eq(version.project) + expect(project.ci_components.second.inputs).to eq({}) + expect(project.ci_components.second.catalog_resource).to eq(version.catalog_resource) + expect(project.ci_components.second.version).to eq(version) + expect(project.ci_components.second.path).to eq('templates/dast/template.yml') + expect(project.ci_components.third.name).to eq('secret-detection') + expect(project.ci_components.third.project).to eq(version.project) + expect(project.ci_components.third.inputs).to eq({ "website" => nil }) + expect(project.ci_components.third.catalog_resource).to eq(version.catalog_resource) + expect(project.ci_components.third.version).to eq(version) + expect(project.ci_components.third.path).to eq('templates/secret-detection.yml') + expect(project.ci_components.fourth.name).to eq('template') + expect(project.ci_components.fourth.project).to eq(version.project) + expect(project.ci_components.fourth.inputs).to eq({ "environment" => nil }) + expect(project.ci_components.fourth.catalog_resource).to eq(version.catalog_resource) + expect(project.ci_components.fourth.version).to eq(version) + expect(project.ci_components.fourth.path).to eq('templates/template.yml') + end + end + end + + context 'with invalid data' do + let_it_be(:files) do + { + 'templates/secret-detection.yml' => 'some: invalid: syntax', + 'README.md' => 'Read me' + } + end + + it 'returns an error' do + response = described_class.new(release).execute + + expect(response).to be_error + expect(response.message).to include('mapping values are not allowed in this context') + end + end + + context 'when one or more components are invalid' do + let_it_be(:files) do + { + 'templates/secret-detection.yml' => "spec:\n inputs:\n - website\n---\nimage: alpine_1", + 'README.md' => 'Read me' + } + end + + it 'returns an error' do + response = described_class.new(release).execute + + expect(response).to be_error + expect(response.message).to include('Inputs must be a valid json schema') + end + end + end +end diff --git a/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb b/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb index d935824e6cc..c0efb7cb639 100644 --- a/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb +++ b/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb @@ -39,9 +39,9 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes it 'creates a pipeline' do expect(pipeline).to be_persisted expect(pipeline.stages.map(&:name)).to contain_exactly( - *%w(.pre build .post)) + *%w[.pre build .post]) expect(pipeline.builds.map(&:name)).to contain_exactly( - *%w(validate build notify)) + *%w[validate build notify]) end end @@ -54,7 +54,7 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes # we can validate a list of stages, as they are assigned # but not persisted expect(pipeline.stages.map(&:name)).to contain_exactly( - *%w(.pre .post)) + *%w[.pre .post]) end end end diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index 05fa3cfeba3..fb448ab13dc 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -219,7 +219,7 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes let(:job1) { pipeline.builds.find_by(name: 'job1') } let(:job2) { pipeline.builds.find_by(name: 'job2') } - let(:variable_keys) { %w(VAR1 VAR2 VAR3 VAR4 VAR5 VAR6 VAR7) } + let(:variable_keys) { %w[VAR1 VAR2 VAR3 VAR4 VAR5 VAR6 VAR7] } context 'when no match' do let(:ref) { 'refs/heads/wip' } diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 11f9708f9f3..19e55c22df8 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -1549,7 +1549,7 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes stage: 'build', script: 'echo', only: { - variables: %w($CI) + variables: %w[$CI] } } } diff --git a/spec/services/ci/enqueue_job_service_spec.rb b/spec/services/ci/enqueue_job_service_spec.rb index c2bb0bb2bb5..85983651148 100644 --- a/spec/services/ci/enqueue_job_service_spec.rb +++ b/spec/services/ci/enqueue_job_service_spec.rb @@ -78,4 +78,33 @@ RSpec.describe Ci::EnqueueJobService, '#execute', feature_category: :continuous_ execute end end + + context 'when the job is manually triggered another user' do + let(:job_variables) do + [{ key: 'third', secret_value: 'third' }, + { key: 'fourth', secret_value: 'fourth' }] + end + + let(:service) do + described_class.new(build, current_user: user, variables: job_variables) + end + + it 'assigns the user and variables to the job', :aggregate_failures do + called = false + service.execute do + unless called + called = true + raise ActiveRecord::StaleObjectError + end + + build.enqueue! + end + + build.reload + + expect(called).to be true # ensure we actually entered the failure path + expect(build.user).to eq(user) + expect(build.job_variables.map(&:key)).to contain_exactly('third', 'fourth') + end + end end diff --git a/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb b/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb index 88ccda90df0..6e263e82432 100644 --- a/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb +++ b/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb @@ -139,7 +139,7 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category fail_running_or_pending - expect(builds_statuses).to eq %w(failed pending) + expect(builds_statuses).to eq %w[failed pending] fail_running_or_pending @@ -166,22 +166,22 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category succeed_running_or_pending - expect(builds_names).to eq %w(build test) - expect(builds_statuses).to eq %w(success pending) + expect(builds_names).to eq %w[build test] + expect(builds_statuses).to eq %w[success pending] succeed_running_or_pending - expect(builds_names).to eq %w(build test deploy production) - expect(builds_statuses).to eq %w(success success pending manual) + expect(builds_names).to eq %w[build test deploy production] + expect(builds_statuses).to eq %w[success success pending manual] succeed_running_or_pending - expect(builds_names).to eq %w(build test deploy production cleanup clear:cache) - expect(builds_statuses).to eq %w(success success success manual pending manual) + expect(builds_names).to eq %w[build test deploy production cleanup clear:cache] + expect(builds_statuses).to eq %w[success success success manual pending manual] succeed_running_or_pending - expect(builds_statuses).to eq %w(success success success manual success manual) + expect(builds_statuses).to eq %w[success success success manual success manual] expect(pipeline.reload.status).to eq 'success' end end @@ -194,22 +194,22 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category succeed_running_or_pending - expect(builds_names).to eq %w(build test) - expect(builds_statuses).to eq %w(success pending) + expect(builds_names).to eq %w[build test] + expect(builds_statuses).to eq %w[success pending] fail_running_or_pending - expect(builds_names).to eq %w(build test test_failure) - expect(builds_statuses).to eq %w(success failed pending) + expect(builds_names).to eq %w[build test test_failure] + expect(builds_statuses).to eq %w[success failed pending] succeed_running_or_pending - expect(builds_names).to eq %w(build test test_failure cleanup) - expect(builds_statuses).to eq %w(success failed success pending) + expect(builds_names).to eq %w[build test test_failure cleanup] + expect(builds_statuses).to eq %w[success failed success pending] succeed_running_or_pending - expect(builds_statuses).to eq %w(success failed success success) + expect(builds_statuses).to eq %w[success failed success success] expect(pipeline.reload.status).to eq 'failed' end end @@ -222,23 +222,23 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category succeed_running_or_pending - expect(builds_names).to eq %w(build test) - expect(builds_statuses).to eq %w(success pending) + expect(builds_names).to eq %w[build test] + expect(builds_statuses).to eq %w[success pending] fail_running_or_pending - expect(builds_names).to eq %w(build test test_failure) - expect(builds_statuses).to eq %w(success failed pending) + expect(builds_names).to eq %w[build test test_failure] + expect(builds_statuses).to eq %w[success failed pending] fail_running_or_pending - expect(builds_names).to eq %w(build test test_failure cleanup) - expect(builds_statuses).to eq %w(success failed failed pending) + expect(builds_names).to eq %w[build test test_failure cleanup] + expect(builds_statuses).to eq %w[success failed failed pending] succeed_running_or_pending - expect(builds_names).to eq %w(build test test_failure cleanup) - expect(builds_statuses).to eq %w(success failed failed success) + expect(builds_names).to eq %w[build test test_failure cleanup] + expect(builds_statuses).to eq %w[success failed failed success] expect(pipeline.reload.status).to eq('failed') end end @@ -251,22 +251,22 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category succeed_running_or_pending - expect(builds_names).to eq %w(build test) - expect(builds_statuses).to eq %w(success pending) + expect(builds_names).to eq %w[build test] + expect(builds_statuses).to eq %w[success pending] succeed_running_or_pending - expect(builds_names).to eq %w(build test deploy production) - expect(builds_statuses).to eq %w(success success pending manual) + expect(builds_names).to eq %w[build test deploy production] + expect(builds_statuses).to eq %w[success success pending manual] fail_running_or_pending - expect(builds_names).to eq %w(build test deploy production cleanup) - expect(builds_statuses).to eq %w(success success failed manual pending) + expect(builds_names).to eq %w[build test deploy production cleanup] + expect(builds_statuses).to eq %w[success success failed manual pending] succeed_running_or_pending - expect(builds_statuses).to eq %w(success success failed manual success) + expect(builds_statuses).to eq %w[success success failed manual success] expect(pipeline.reload).to be_failed end end @@ -280,8 +280,8 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category succeed_running_or_pending expect(builds.running_or_pending).not_to be_empty - expect(builds_names).to eq %w(build test) - expect(builds_statuses).to eq %w(success pending) + expect(builds_names).to eq %w[build test] + expect(builds_statuses).to eq %w[success pending] cancel_running_or_pending @@ -801,25 +801,25 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category it 'when linux:* finishes first it runs it out of order' do expect(process_pipeline).to be_truthy - expect(stages).to eq(%w(pending created created)) + expect(stages).to eq(%w[pending created created]) expect(builds.pending).to contain_exactly(linux_build, mac_build) # we follow the single path of linux linux_build.reset.success! - expect(stages).to eq(%w(running pending created)) + expect(stages).to eq(%w[running pending created]) expect(builds.success).to contain_exactly(linux_build) expect(builds.pending).to contain_exactly(mac_build, linux_rspec, linux_rubocop) linux_rspec.reset.success! - expect(stages).to eq(%w(running running created)) + expect(stages).to eq(%w[running running created]) expect(builds.success).to contain_exactly(linux_build, linux_rspec) expect(builds.pending).to contain_exactly(mac_build, linux_rubocop) linux_rubocop.reset.success! - expect(stages).to eq(%w(running running created)) + expect(stages).to eq(%w[running running created]) expect(builds.success).to contain_exactly(linux_build, linux_rspec, linux_rubocop) expect(builds.pending).to contain_exactly(mac_build) @@ -827,7 +827,7 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category mac_rspec.reset.success! mac_rubocop.reset.success! - expect(stages).to eq(%w(success success pending)) + expect(stages).to eq(%w[success success pending]) expect(builds.success).to contain_exactly( linux_build, linux_rspec, linux_rubocop, mac_build, mac_rspec, mac_rubocop) expect(builds.pending).to contain_exactly(deploy) @@ -866,13 +866,13 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category it 'runs deploy_pages without waiting prior stages' do expect(process_pipeline).to be_truthy - expect(stages).to eq(%w(pending created pending)) + expect(stages).to eq(%w[pending created pending]) expect(builds.pending).to contain_exactly(linux_build, mac_build, deploy_pages) linux_build.reset.success! deploy_pages.reset.success! - expect(stages).to eq(%w(running pending running)) + expect(stages).to eq(%w[running pending running]) expect(builds.success).to contain_exactly(linux_build, deploy_pages) expect(builds.pending).to contain_exactly(mac_build, linux_rspec, linux_rubocop) @@ -882,7 +882,7 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category mac_rspec.reset.success! mac_rubocop.reset.success! - expect(stages).to eq(%w(success success running)) + expect(stages).to eq(%w[success success running]) expect(builds.pending).to contain_exactly(deploy) end end @@ -900,12 +900,12 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category it 'skips the jobs depending on it' do expect(process_pipeline).to be_truthy - expect(stages).to eq(%w(pending created created)) + expect(stages).to eq(%w[pending created created]) expect(all_builds.pending).to contain_exactly(linux_build) linux_build.reset.drop! - expect(stages).to eq(%w(failed skipped skipped)) + expect(stages).to eq(%w[failed skipped skipped]) expect(all_builds.failed).to contain_exactly(linux_build) expect(all_builds.skipped).to contain_exactly(linux_rspec, deploy) end @@ -922,7 +922,7 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category it 'makes deploy DAG to be skipped' do expect(process_pipeline).to be_truthy - expect(stages).to eq(%w(skipped skipped)) + expect(stages).to eq(%w[skipped skipped]) expect(all_builds.manual).to contain_exactly(linux_build) expect(all_builds.skipped).to contain_exactly(deploy) end @@ -1460,7 +1460,7 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category end def delayed_options - { when: 'delayed', options: { script: %w(echo), start_in: '1 minute' } } + { when: 'delayed', options: { script: %w[echo], start_in: '1 minute' } } end def unschedule diff --git a/spec/services/ci/pipelines/update_metadata_service_spec.rb b/spec/services/ci/pipelines/update_metadata_service_spec.rb new file mode 100644 index 00000000000..939ce7f5785 --- /dev/null +++ b/spec/services/ci/pipelines/update_metadata_service_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::Pipelines::UpdateMetadataService, feature_category: :continuous_integration do + subject(:execute) { described_class.new(pipeline, { name: name }).execute } + + let(:name) { 'Some random pipeline name' } + + context 'when pipeline has no name' do + let(:pipeline) { create(:ci_pipeline) } + + it 'updates the name' do + expect { execute }.to change { pipeline.reload.name }.to(name) + end + end + + context 'when pipeline has a name' do + let(:pipeline) { create(:ci_pipeline, name: 'Some other name') } + + it 'updates the name' do + expect { execute }.to change { pipeline.reload.name }.to(name) + end + end + + context 'when new name is too long' do + let(:pipeline) { create(:ci_pipeline) } + let(:name) { 'a' * 256 } + + it 'does not update the name' do + expect { execute }.not_to change { pipeline.reload.name } + end + end +end diff --git a/spec/services/ci/play_build_service_spec.rb b/spec/services/ci/play_build_service_spec.rb index 46b6622d6ec..c5651dc4502 100644 --- a/spec/services/ci/play_build_service_spec.rb +++ b/spec/services/ci/play_build_service_spec.rb @@ -63,10 +63,6 @@ RSpec.describe Ci::PlayBuildService, '#execute', feature_category: :continuous_i context 'when a subsequent job is skipped' do let!(:job) { create(:ci_build, :skipped, pipeline: pipeline, stage_idx: build.stage_idx + 1) } - before do - create(:ci_build_need, build: job, name: build.name) - end - it 'marks the subsequent job as processable' do expect { service.execute(build) }.to change { job.reload.status }.from('skipped').to('created') end diff --git a/spec/services/ci/refs/enqueue_pipelines_to_unlock_service_spec.rb b/spec/services/ci/refs/enqueue_pipelines_to_unlock_service_spec.rb index 468302cb689..052be3b2587 100644 --- a/spec/services/ci/refs/enqueue_pipelines_to_unlock_service_spec.rb +++ b/spec/services/ci/refs/enqueue_pipelines_to_unlock_service_spec.rb @@ -26,34 +26,40 @@ RSpec.describe Ci::Refs::EnqueuePipelinesToUnlockService, :unlock_pipelines, :cl shared_examples_for 'unlocking pipelines' do let(:is_tag) { target_ref.ref_path.include?(::Gitlab::Git::TAG_REF_PREFIX) } - let!(:other_ref_pipeline) { create_pipeline(:locked, other_ref, tag: false) } - let!(:old_unlocked_pipeline) { create_pipeline(:unlocked, ref) } - let!(:older_locked_pipeline_1) { create_pipeline(:locked, ref) } - let!(:older_locked_pipeline_2) { create_pipeline(:locked, ref) } - let!(:older_locked_pipeline_3) { create_pipeline(:locked, ref) } - let!(:older_child_pipeline) { create_pipeline(:locked, ref, child_of: older_locked_pipeline_3) } - let!(:pipeline) { create_pipeline(:locked, ref) } - let!(:child_pipeline) { create_pipeline(:locked, ref, child_of: pipeline) } - let!(:newer_pipeline) { create_pipeline(:locked, ref) } + let!(:other_ref_pipeline) { create_pipeline(:locked, other_ref, :failed, tag: false) } + let!(:old_unlocked_pipeline) { create_pipeline(:unlocked, ref, :failed) } + let!(:old_locked_pipeline_1) { create_pipeline(:locked, ref, :failed) } + let!(:old_locked_pipeline_2) { create_pipeline(:locked, ref, :success) } + let!(:old_locked_pipeline_3) { create_pipeline(:locked, ref, :success) } + let!(:old_locked_pipeline_3_child) { create_pipeline(:locked, ref, :success, child_of: old_locked_pipeline_3) } + let!(:old_locked_pipeline_4) { create_pipeline(:locked, ref, :success) } + let!(:old_locked_pipeline_4_child) { create_pipeline(:locked, ref, :success, child_of: old_locked_pipeline_4) } + let!(:old_locked_pipeline_5) { create_pipeline(:locked, ref, :failed) } + let!(:old_locked_pipeline_5_child) { create_pipeline(:locked, ref, :success, child_of: old_locked_pipeline_5) } + let!(:pipeline) { create_pipeline(:locked, ref, :failed) } + let!(:child_pipeline) { create_pipeline(:locked, ref, :failed, child_of: pipeline) } + let!(:newer_pipeline) { create_pipeline(:locked, ref, :failed) } context 'when before_pipeline is given' do let(:before_pipeline) { pipeline } - it 'only enqueues older locked pipelines within the ref' do + it 'only enqueues old locked pipelines within the ref, excluding the last successful CI source pipeline' do expect { execute } .to change { pipeline_ids_waiting_to_be_unlocked } .from([]) .to([ - older_locked_pipeline_1.id, - older_locked_pipeline_2.id, - older_locked_pipeline_3.id, - older_child_pipeline.id + old_locked_pipeline_1.id, + old_locked_pipeline_2.id, + old_locked_pipeline_3.id, + old_locked_pipeline_3_child.id, + old_locked_pipeline_5.id, + old_locked_pipeline_5_child.id ]) expect(execute).to include( status: :success, - total_pending_entries: 4, - total_new_entries: 4 + total_pending_entries: 6, + total_new_entries: 6 ) end end @@ -66,10 +72,14 @@ RSpec.describe Ci::Refs::EnqueuePipelinesToUnlockService, :unlock_pipelines, :cl .to change { pipeline_ids_waiting_to_be_unlocked } .from([]) .to([ - older_locked_pipeline_1.id, - older_locked_pipeline_2.id, - older_locked_pipeline_3.id, - older_child_pipeline.id, + old_locked_pipeline_1.id, + old_locked_pipeline_2.id, + old_locked_pipeline_3.id, + old_locked_pipeline_3_child.id, + old_locked_pipeline_4.id, + old_locked_pipeline_4_child.id, + old_locked_pipeline_5.id, + old_locked_pipeline_5_child.id, pipeline.id, child_pipeline.id, newer_pipeline.id @@ -77,8 +87,8 @@ RSpec.describe Ci::Refs::EnqueuePipelinesToUnlockService, :unlock_pipelines, :cl expect(execute).to include( status: :success, - total_pending_entries: 7, - total_new_entries: 7 + total_pending_entries: 11, + total_new_entries: 11 ) end end @@ -96,9 +106,9 @@ RSpec.describe Ci::Refs::EnqueuePipelinesToUnlockService, :unlock_pipelines, :cl it_behaves_like 'unlocking pipelines' end - def create_pipeline(type, ref, tag: is_tag, child_of: nil) + def create_pipeline(type, ref, status, tag: is_tag, child_of: nil) trait = type == :locked ? :artifacts_locked : :unlocked - create(:ci_pipeline, trait, ref: ref, tag: tag, project: project, child_of: child_of).tap do |p| + create(:ci_pipeline, trait, status: status, ref: ref, tag: tag, project: project, child_of: child_of).tap do |p| if child_of build = create(:ci_build, pipeline: child_of) create(:ci_sources_pipeline, source_job: build, source_project: project, pipeline: p, project: project) diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 83bae16a30e..e38984281b0 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -948,7 +948,7 @@ module Ci pending_job.create_queuing_entry! end - let(:runner) { create(:ci_runner, :instance, tag_list: %w(tag1 tag2)) } + let(:runner) { create(:ci_runner, :instance, tag_list: %w[tag1 tag2]) } let(:expected_shared_runner) { true } let(:expected_shard) { ::Gitlab::Ci::Queue::Metrics::DEFAULT_METRICS_SHARD } let(:expected_jobs_running_for_project_first_job) { '0' } @@ -957,14 +957,14 @@ module Ci it_behaves_like 'metrics collector' context 'when metrics_shard tag is defined' do - let(:runner) { create(:ci_runner, :instance, tag_list: %w(tag1 metrics_shard::shard_tag tag2)) } + let(:runner) { create(:ci_runner, :instance, tag_list: %w[tag1 metrics_shard::shard_tag tag2]) } let(:expected_shard) { 'shard_tag' } it_behaves_like 'metrics collector' end context 'when multiple metrics_shard tag is defined' do - let(:runner) { create(:ci_runner, :instance, tag_list: %w(tag1 metrics_shard::shard_tag metrics_shard::shard_tag_2 tag2)) } + let(:runner) { create(:ci_runner, :instance, tag_list: %w[tag1 metrics_shard::shard_tag metrics_shard::shard_tag_2 tag2]) } let(:expected_shard) { 'shard_tag' } it_behaves_like 'metrics collector' @@ -997,7 +997,7 @@ module Ci end context 'when project runner is used' do - let(:runner) { create(:ci_runner, :project, projects: [project], tag_list: %w(tag1 metrics_shard::shard_tag tag2)) } + let(:runner) { create(:ci_runner, :project, projects: [project], tag_list: %w[tag1 metrics_shard::shard_tag tag2]) } let(:expected_shared_runner) { false } let(:expected_shard) { ::Gitlab::Ci::Queue::Metrics::DEFAULT_METRICS_SHARD } let(:expected_jobs_running_for_project_first_job) { '+Inf' } diff --git a/spec/services/ci/retry_job_service_spec.rb b/spec/services/ci/retry_job_service_spec.rb index 80fbfc04f9b..1646afde21d 100644 --- a/spec/services/ci/retry_job_service_spec.rb +++ b/spec/services/ci/retry_job_service_spec.rb @@ -270,14 +270,6 @@ RSpec.describe Ci::RetryJobService, feature_category: :continuous_integration do it_behaves_like 'creates associations for a deployable job', :ci_bridge end - context 'when `create_deployment_only_for_processable_jobs` FF is disabled' do - before do - stub_feature_flags(create_deployment_only_for_processable_jobs: false) - end - - it_behaves_like 'creates associations for a deployable job', :ci_bridge - end - context 'when given variables' do let(:new_job) { service.clone!(job, variables: job_variables_attributes) } @@ -302,14 +294,6 @@ RSpec.describe Ci::RetryJobService, feature_category: :continuous_integration do it_behaves_like 'creates associations for a deployable job', :ci_build end - context 'when `create_deployment_only_for_processable_jobs` FF is disabled' do - before do - stub_feature_flags(create_deployment_only_for_processable_jobs: false) - end - - it_behaves_like 'creates associations for a deployable job', :ci_build - end - context 'when given variables' do let(:new_job) { service.clone!(job, variables: job_variables_attributes) } diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb index 6d991baafd0..125dbc5083c 100644 --- a/spec/services/ci/retry_pipeline_service_spec.rb +++ b/spec/services/ci/retry_pipeline_service_spec.rb @@ -122,7 +122,7 @@ RSpec.describe Ci::RetryPipelineService, '#execute', feature_category: :continuo expect(build('build')).to be_success expect(build('build2')).to be_success expect(build('test')).to be_pending - expect(build('test').needs.map(&:name)).to match_array(%w(build build2)) + expect(build('test').needs.map(&:name)).to match_array(%w[build build2]) end context 'when there is a failed DAG test without needs' do diff --git a/spec/services/ci/runners/register_runner_service_spec.rb b/spec/services/ci/runners/register_runner_service_spec.rb index b5921773364..4b997855657 100644 --- a/spec/services/ci/runners/register_runner_service_spec.rb +++ b/spec/services/ci/runners/register_runner_service_spec.rb @@ -74,7 +74,7 @@ RSpec.describe ::Ci::Runners::RegisterRunnerService, '#execute', feature_categor active: false, locked: true, run_untagged: false, - tag_list: %w(tag1 tag2), + tag_list: %w[tag1 tag2], access_level: 'ref_protected', maximum_timeout: 600, name: 'some name', @@ -290,7 +290,7 @@ RSpec.describe ::Ci::Runners::RegisterRunnerService, '#execute', feature_categor let(:token) { registration_token } let(:args) do - { tag_list: %w(tag1 tag2) } + { tag_list: %w[tag1 tag2] } end it 'creates runner with tags' do 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 6d91f5098eb..9da63930057 100644 --- a/spec/services/ci/stuck_builds/drop_pending_service_spec.rb +++ b/spec/services/ci/stuck_builds/drop_pending_service_spec.rb @@ -139,7 +139,7 @@ RSpec.describe Ci::StuckBuilds::DropPendingService, feature_category: :runner_fl end end - %w(success skipped failed canceled).each do |status| + %w[success skipped failed canceled].each do |status| context "when job is #{status}" do let(:status) { status } let(:updated_at) { 2.days.ago } 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 deb807753c2..c2f8a643f24 100644 --- a/spec/services/ci/stuck_builds/drop_running_service_spec.rb +++ b/spec/services/ci/stuck_builds/drop_running_service_spec.rb @@ -51,7 +51,7 @@ RSpec.describe Ci::StuckBuilds::DropRunningService, feature_category: :runner_fl include_examples 'running builds' end - %w(success skipped failed canceled scheduled pending).each do |status| + %w[success skipped failed canceled scheduled pending].each do |status| context "when job is #{status}" do let(:status) { status } let(:updated_at) { 2.days.ago } 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 f2e658c3ae3..5560eaf9b40 100644 --- a/spec/services/ci/stuck_builds/drop_scheduled_service_spec.rb +++ b/spec/services/ci/stuck_builds/drop_scheduled_service_spec.rb @@ -23,7 +23,7 @@ RSpec.describe Ci::StuckBuilds::DropScheduledService, feature_category: :runner_ end end - %w(success skipped failed canceled running pending).each do |status| + %w[success skipped failed canceled running pending].each do |status| context "when job is #{status}" do before do job.update!(status: status) diff --git a/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb b/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb index e8e0174fe40..b5cf45e7b36 100644 --- a/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb +++ b/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb @@ -221,9 +221,9 @@ RSpec.describe Clusters::Kubernetes::CreateOrUpdateServiceAccountService, featur namespace: namespace }, rules: [{ - apiGroups: %w(serving.knative.dev), - resources: %w(configurations configurationgenerations routes revisions revisionuids autoscalers services), - verbs: %w(get list create update delete patch watch) + apiGroups: %w[serving.knative.dev], + resources: %w[configurations configurationgenerations routes revisions revisionuids autoscalers services], + verbs: %w[get list create update delete patch watch] }] ) ) @@ -239,9 +239,9 @@ RSpec.describe Clusters::Kubernetes::CreateOrUpdateServiceAccountService, featur namespace: namespace }, rules: [{ - apiGroups: %w(database.crossplane.io), - resources: %w(postgresqlinstances), - verbs: %w(get list create watch) + apiGroups: %w[database.crossplane.io], + resources: %w[postgresqlinstances], + verbs: %w[get list create watch] }] ) ) diff --git a/spec/services/container_registry/protection/create_rule_service_spec.rb b/spec/services/container_registry/protection/create_rule_service_spec.rb new file mode 100644 index 00000000000..3c319caf25c --- /dev/null +++ b/spec/services/container_registry/protection/create_rule_service_spec.rb @@ -0,0 +1,145 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ContainerRegistry::Protection::CreateRuleService, '#execute', feature_category: :container_registry do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:current_user) { create(:user, maintainer_projects: [project]) } + + let(:service) { described_class.new(project, current_user, params) } + let(:params) { attributes_for(:container_registry_protection_rule) } + + subject { service.execute } + + shared_examples 'a successful service response' do + it { is_expected.to be_success } + + it { is_expected.to have_attributes(errors: be_blank) } + + it do + is_expected.to have_attributes( + payload: { + container_registry_protection_rule: + be_a(ContainerRegistry::Protection::Rule) + .and(have_attributes( + container_path_pattern: params[:container_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 + )) + } + ) + end + + it 'creates a new container registry protection rule in the database' do + expect { subject }.to change { ContainerRegistry::Protection::Rule.count }.by(1) + + expect( + ContainerRegistry::Protection::Rule.where( + project: project, + container_path_pattern: params[:container_path_pattern], + push_protected_up_to_access_level: params[:push_protected_up_to_access_level] + ) + ).to exist + end + end + + shared_examples 'an erroneous service response' do + it { is_expected.to be_error } + it { is_expected.to have_attributes(errors: be_present, payload: include(container_registry_protection_rule: nil)) } + + it 'does not create a new container registry protection rule in the database' do + expect { subject }.not_to change { ContainerRegistry::Protection::Rule.count } + end + + it 'does not create a container registry protection rule with the given params' do + subject + + expect( + ContainerRegistry::Protection::Rule.where( + project: project, + container_path_pattern: params[:container_path_pattern], + push_protected_up_to_access_level: params[:push_protected_up_to_access_level] + ) + ).not_to exist + end + end + + 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: '') } + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes(message: match(/Container path pattern can't be blank/)) } + end + + context 'when delete_protected_up_to_access_level is invalid' do + let(:params) { super().merge(delete_protected_up_to_access_level: 1000) } + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes(message: match(/is not a valid delete_protected_up_to_access_level/)) } + end + + context 'when push_protected_up_to_access_level is invalid' do + let(:params) { super().merge(push_protected_up_to_access_level: 1000) } + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes(message: match(/is not a valid push_protected_up_to_access_level/)) } + end + end + + context 'with existing container registry protection rule in the database' do + let_it_be_with_reload(:existing_container_registry_protection_rule) do + create(:container_registry_protection_rule, project: project) + end + + 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", + push_protected_up_to_access_level: + existing_container_registry_protection_rule.push_protected_up_to_access_level + ) + end + + it_behaves_like 'a successful service response' + end + + context 'when field `container_path_pattern` is taken' do + let(:params) do + super().merge( + container_path_pattern: existing_container_registry_protection_rule.container_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 { expect { subject }.not_to change { existing_container_registry_protection_rule.updated_at } } + end + end + + context 'with disallowed params' do + let(:params) { super().merge(project_id: 1, unsupported_param: 'unsupported_param_value') } + + it_behaves_like 'a successful service response' + end + + context 'with forbidden user access level (project developer role)' do + # Because of the access level hierarchy, we can assume that + # other access levels below developer role will also not be able to + # create container registry protection rules. + let_it_be(:current_user) { create(:user).tap { |u| project.add_developer(u) } } + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes(message: match(/Unauthorized/)) } + end +end diff --git a/spec/services/deployments/update_environment_service_spec.rb b/spec/services/deployments/update_environment_service_spec.rb index 79bf0d972d4..bc6e244dc2f 100644 --- a/spec/services/deployments/update_environment_service_spec.rb +++ b/spec/services/deployments/update_environment_service_spec.rb @@ -145,7 +145,7 @@ RSpec.describe Deployments::UpdateEnvironmentService, feature_category: :continu an_instance_of(described_class::EnvironmentUpdateFailure), project_id: project.id, environment_id: environment.id, - reason: %q{External url javascript scheme is not allowed} + reason: %q(External url javascript scheme is not allowed) ) .once diff --git a/spec/services/design_management/copy_design_collection/copy_service_spec.rb b/spec/services/design_management/copy_design_collection/copy_service_spec.rb index 048327792e0..2f858e86cf1 100644 --- a/spec/services/design_management/copy_design_collection/copy_service_spec.rb +++ b/spec/services/design_management/copy_design_collection/copy_service_spec.rb @@ -267,7 +267,7 @@ RSpec.describe DesignManagement::CopyDesignCollection::CopyService, :clean_gitla let_it_be(:config_file) { Rails.root.join('lib/gitlab/design_management/copy_design_collection_model_attributes.yml') } let_it_be(:config) { YAML.load_file(config_file).symbolize_keys } - %w(Design Action Version).each do |model| + %w[Design Action Version].each do |model| specify do attributes = config["#{model.downcase}_attributes".to_sym] || [] ignored_attributes = config["ignore_#{model.downcase}_attributes".to_sym] diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb index e087f2ffc7e..fbc38f93c56 100644 --- a/spec/services/draft_notes/publish_service_spec.rb +++ b/spec/services/draft_notes/publish_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe DraftNotes::PublishService, feature_category: :code_review_workflow do include RepoHelpers - let(:merge_request) { create(:merge_request) } + let_it_be(:merge_request) { create(:merge_request, reviewers: create_list(:user, 1)) } let(:project) { merge_request.target_project } let(:user) { merge_request.author } let(:commit) { project.commit(sample_commit.id) } @@ -198,6 +198,29 @@ RSpec.describe DraftNotes::PublishService, feature_category: :code_review_workfl end end end + + it 'does not call UpdateReviewerStateService' do + publish + + expect(MergeRequests::UpdateReviewerStateService).not_to receive(:new) + end + + context 'when `mr_request_changes` feature flag is disabled' do + before do + stub_feature_flags(mr_request_changes: false) + end + + it 'calls UpdateReviewerStateService' do + expect_next_instance_of( + MergeRequests::UpdateReviewerStateService, + project: project, current_user: user + ) do |service| + expect(service).to receive(:execute).with(merge_request, "reviewed") + end + + publish + end + end end context 'draft notes with suggestions' do diff --git a/spec/services/environments/auto_recover_service_spec.rb b/spec/services/environments/auto_recover_service_spec.rb new file mode 100644 index 00000000000..9807e8f9314 --- /dev/null +++ b/spec/services/environments/auto_recover_service_spec.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Environments::AutoRecoverService, :clean_gitlab_redis_shared_state, :sidekiq_inline, + feature_category: :continuous_delivery do + include CreateEnvironmentsHelpers + include ExclusiveLeaseHelpers + + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + + let(:service) { described_class.new } + + before_all do + project.add_developer(user) + end + + describe '#execute' do + subject { service.execute } + + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + + let(:environments) { Environment.all } + + before_all do + project.add_developer(user) + project.repository.add_branch(user, 'review/feature-1', 'master') + project.repository.add_branch(user, 'review/feature-2', 'master') + end + + before do + create_review_app(user, project, 'review/feature-1') + create_review_app(user, project, 'review/feature-2') + + Environment.all.map do |e| + e.stop_actions.map(&:drop) + e.stop! + e.update!(updated_at: (Environment::LONG_STOP + 1.day).ago) + e.reload + end + end + + it 'stops environments that have been stuck stopping too long' do + expect { subject } + .to change { Environment.all.map(&:state).uniq } + .from(['stopping']).to(['available']) + end + + it 'schedules stop processes in bulk' do + args = [[Environment.find_by_name('review/feature-1').id], [Environment.find_by_name('review/feature-2').id]] + + expect(Environments::AutoRecoverWorker) + .to receive(:bulk_perform_async).with(args).once.and_call_original + + subject + end + + context 'when the other sidekiq worker has already been running' do + before do + stub_exclusive_lease_taken(described_class::EXCLUSIVE_LOCK_KEY) + end + + it 'does not execute recover_in_batch' do + expect_next_instance_of(described_class) do |service| + expect(service).not_to receive(:recover_in_batch) + end + + expect { subject }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) + end + end + + context 'when loop reached timeout' do + before do + stub_const("#{described_class}::LOOP_TIMEOUT", 0.seconds) + stub_const("#{described_class}::LOOP_LIMIT", 100_000) + allow_next_instance_of(described_class) do |service| + allow(service).to receive(:recover_in_batch).and_return(true) + end + end + + it 'returns false and does not continue the process' do + is_expected.to eq(false) + end + end + + context 'when loop reached loop limit' do + before do + stub_const("#{described_class}::LOOP_LIMIT", 1) + stub_const("#{described_class}::BATCH_SIZE", 1) + end + + it 'stops only one available environment' do + expect { subject }.to change { Environment.long_stopping.count }.by(-1) + end + end + end +end diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb index 3050d6c5eca..8fd542542ae 100644 --- a/spec/services/git/branch_hooks_service_spec.rb +++ b/spec/services/git/branch_hooks_service_spec.rb @@ -133,27 +133,14 @@ RSpec.describe Git::BranchHooksService, :clean_gitlab_redis_shared_state, featur expect(Gitlab::UsageDataCounters::HLLRedisCounter.unique_events(event_names: 'o_pipeline_authoring_unique_users_committing_ciconfigfile', start_date: time, end_date: time + 7.days)).to eq(1) end - context 'when usage ping is disabled' do - before do - allow(::ServicePing::ServicePingSettings).to receive(:enabled?).and_return(false) - end - - it 'does not track the event' do - execute_service - - expect(Gitlab::UsageDataCounters::HLLRedisCounter) - .not_to receive(:track_event).with(*tracking_params) - end - end - context 'when the branch is not the main branch' do let(:branch) { 'feature' } it 'does not track the event' do - execute_service - expect(Gitlab::UsageDataCounters::HLLRedisCounter) .not_to receive(:track_event).with(*tracking_params) + + execute_service end end @@ -163,10 +150,10 @@ RSpec.describe Git::BranchHooksService, :clean_gitlab_redis_shared_state, featur end it 'does not track the event' do - execute_service - expect(Gitlab::UsageDataCounters::HLLRedisCounter) .not_to receive(:track_event).with(*tracking_params) + + execute_service end end end diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index fe54663b983..db4f3ace64b 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -125,7 +125,7 @@ RSpec.describe Git::BranchPushService, :use_clean_rails_redis_caching, services: allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(true) expect(Sidekiq.logger).to receive(:warn) do |args| pipeline_params = args[:pipeline_params] - expect(pipeline_params.keys).to match_array(%i(before after ref variables_attributes checkout_sha)) + expect(pipeline_params.keys).to match_array(%i[before after ref variables_attributes checkout_sha]) end expect { subject }.not_to change { Ci::Pipeline.count } diff --git a/spec/services/git/process_ref_changes_service_spec.rb b/spec/services/git/process_ref_changes_service_spec.rb index 9ec13bc957b..93d65b0b344 100644 --- a/spec/services/git/process_ref_changes_service_spec.rb +++ b/spec/services/git/process_ref_changes_service_spec.rb @@ -236,7 +236,7 @@ RSpec.describe Git::ProcessRefChangesService, feature_category: :source_code_man before do allow(MergeRequests::PushedBranchesService).to receive(:new).and_return( - double(execute: %w(create1 create2)), double(execute: %w(changed1)), double(execute: %w(removed2)) + double(execute: %w[create1 create2]), double(execute: %w[changed1]), double(execute: %w[removed2]) ) allow(Gitlab::Git::Commit).to receive(:between).and_return([]) diff --git a/spec/services/google_cloud/generate_pipeline_service_spec.rb b/spec/services/google_cloud/generate_pipeline_service_spec.rb index 26a1ccb7e3b..8f49e1af901 100644 --- a/spec/services/google_cloud/generate_pipeline_service_spec.rb +++ b/spec/services/google_cloud/generate_pipeline_service_spec.rb @@ -32,7 +32,7 @@ RSpec.describe GoogleCloud::GeneratePipelineService, feature_category: :deployme response = service.execute ref = response[:commit][:result] - gitlab_ci_yml = project.repository.gitlab_ci_yml_for(ref) + gitlab_ci_yml = project.ci_config_for(ref) expect(response[:status]).to eq(:success) expect(gitlab_ci_yml).to include('https://gitlab.com/gitlab-org/incubation-engineering/five-minute-production/library/-/raw/main/gcp/cloud-run.gitlab-ci.yml') @@ -97,7 +97,7 @@ EOF response = service.execute branch_name = response[:branch_name] - gitlab_ci_yml = project.repository.gitlab_ci_yml_for(branch_name) + gitlab_ci_yml = project.ci_config_for(branch_name) pipeline = Gitlab::Config::Loader::Yaml.new(gitlab_ci_yml).load! expect(response[:status]).to eq(:success) @@ -110,7 +110,7 @@ EOF response = service.execute branch_name = response[:branch_name] - gitlab_ci_yml = project.repository.gitlab_ci_yml_for(branch_name) + gitlab_ci_yml = project.ci_config_for(branch_name) expect(YAML.safe_load(gitlab_ci_yml).keys).to eq(%w[stages build-java test-java include]) end @@ -153,7 +153,7 @@ EOF response = service.execute branch_name = response[:branch_name] - gitlab_ci_yml = project.repository.gitlab_ci_yml_for(branch_name) + gitlab_ci_yml = project.ci_config_for(branch_name) pipeline = Gitlab::Config::Loader::Yaml.new(gitlab_ci_yml).load! expect(response[:status]).to eq(:success) @@ -195,7 +195,7 @@ EOF response = service.execute branch_name = response[:branch_name] - gitlab_ci_yml = project.repository.gitlab_ci_yml_for(branch_name) + gitlab_ci_yml = project.ci_config_for(branch_name) pipeline = Gitlab::Config::Loader::Yaml.new(gitlab_ci_yml).load! expect(response[:status]).to eq(:success) @@ -235,7 +235,7 @@ EOF response = service.execute ref = response[:commit][:result] - gitlab_ci_yml = project.repository.gitlab_ci_yml_for(ref) + gitlab_ci_yml = project.ci_config_for(ref) expect(response[:status]).to eq(:success) expect(gitlab_ci_yml).to include('https://gitlab.com/gitlab-org/incubation-engineering/five-minute-production/library/-/raw/main/gcp/cloud-storage.gitlab-ci.yml') @@ -272,7 +272,7 @@ EOF response = service.execute ref = response[:commit][:result] - gitlab_ci_yml = project.repository.gitlab_ci_yml_for(ref) + gitlab_ci_yml = project.ci_config_for(ref) expect(response[:status]).to eq(:success) expect(gitlab_ci_yml).to include('https://gitlab.com/gitlab-org/incubation-engineering/five-minute-production/library/-/raw/main/gcp/vision-ai.gitlab-ci.yml') @@ -328,7 +328,7 @@ EOF response = service.execute branch_name = response[:branch_name] - gitlab_ci_yml = project.repository.gitlab_ci_yml_for(branch_name) + gitlab_ci_yml = project.ci_config_for(branch_name) pipeline = Gitlab::Config::Loader::Yaml.new(gitlab_ci_yml).load! expect(response[:status]).to eq(:success) diff --git a/spec/services/groups/update_statistics_service_spec.rb b/spec/services/groups/update_statistics_service_spec.rb index 6bab36eca89..39b9c1c234d 100644 --- a/spec/services/groups/update_statistics_service_spec.rb +++ b/spec/services/groups/update_statistics_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Groups::UpdateStatisticsService, feature_category: :groups_and_projects do let_it_be(:group, reload: true) { create(:group) } - let(:statistics) { %w(wiki_size) } + let(:statistics) { %w[wiki_size] } subject(:service) { described_class.new(group, statistics: statistics) } diff --git a/spec/services/import/gitlab_projects/create_project_service_spec.rb b/spec/services/import/gitlab_projects/create_project_service_spec.rb index a77e9bdfce1..3f5dc7a928f 100644 --- a/spec/services/import/gitlab_projects/create_project_service_spec.rb +++ b/spec/services/import/gitlab_projects/create_project_service_spec.rb @@ -144,9 +144,9 @@ RSpec.describe ::Import::GitlabProjects::CreateProjectService, :aggregate_failur ) expect(response.payload).to eq( other_errors: [ - %{Project namespace path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-', end in '.git' or end in '.atom'}, - %{Path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-', end in '.git' or end in '.atom'}, - %{Path must not start or end with a special character and must not contain consecutive special characters.} + %(Project namespace path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-', end in '.git' or end in '.atom'), + %(Path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-', end in '.git' or end in '.atom'), + %(Path must not start or end with a special character and must not contain consecutive special characters.) ]) end end diff --git a/spec/services/import/validate_remote_git_endpoint_service_spec.rb b/spec/services/import/validate_remote_git_endpoint_service_spec.rb index 1d2b3975832..15e80f2c85d 100644 --- a/spec/services/import/validate_remote_git_endpoint_service_spec.rb +++ b/spec/services/import/validate_remote_git_endpoint_service_spec.rb @@ -7,7 +7,9 @@ RSpec.describe Import::ValidateRemoteGitEndpointService, feature_category: :impo let_it_be(:base_url) { 'http://demo.host/path' } let_it_be(:endpoint_url) { "#{base_url}/info/refs?service=git-upload-pack" } - let_it_be(:error_message) { "#{base_url} is not a valid HTTP Git repository" } + let_it_be(:endpoint_error_message) { "#{base_url} endpoint error:" } + let_it_be(:body_error_message) { described_class::INVALID_BODY_MESSAGE } + let_it_be(:content_type_error_message) { described_class::INVALID_CONTENT_TYPE_MESSAGE } describe '#execute' do let(:valid_response) do @@ -70,13 +72,14 @@ RSpec.describe Import::ValidateRemoteGitEndpointService, feature_category: :impo end it 'reports error when status code is not 200' do - stub_full_request(endpoint_url, method: :get).to_return(valid_response.merge({ status: 301 })) + error_response = { status: 401 } + stub_full_request(endpoint_url, method: :get).to_return(error_response) result = subject.execute expect(result).to be_a(ServiceResponse) expect(result.error?).to be(true) - expect(result.message).to eq(error_message) + expect(result.message).to eq("#{endpoint_error_message} #{error_response[:status]}") end it 'reports error when invalid URL is provided' do @@ -94,27 +97,49 @@ RSpec.describe Import::ValidateRemoteGitEndpointService, feature_category: :impo expect(result).to be_a(ServiceResponse) expect(result.error?).to be(true) - expect(result.message).to eq(error_message) + expect(result.message).to eq(content_type_error_message) end - it 'reports error when body is in invalid format' do + it 'reports error when body is too short' do stub_full_request(endpoint_url, method: :get).to_return(valid_response.merge({ body: 'invalid content' })) result = subject.execute expect(result).to be_a(ServiceResponse) expect(result.error?).to be(true) - expect(result.message).to eq(error_message) + expect(result.message).to eq(body_error_message) + end + + it 'reports error when body is in invalid format' do + stub_full_request(endpoint_url, method: :get).to_return(valid_response.merge({ body: 'invalid long content with no git respons whatshowever' })) + + result = subject.execute + + expect(result).to be_a(ServiceResponse) + expect(result.error?).to be(true) + expect(result.message).to eq(body_error_message) + end + + it 'reports error when http exceptions are raised' do + err = SocketError.new('dummy message') + stub_full_request(endpoint_url, method: :get).to_raise(err) + + result = subject.execute + + expect(result).to be_a(ServiceResponse) + expect(result.error?).to be(true) + expect(result.message).to eq("HTTP #{err.class.name.underscore} error: #{err.message}") end - it 'reports error when exception is raised' do - stub_full_request(endpoint_url, method: :get).to_raise(SocketError.new('dummy message')) + it 'reports error when other exceptions are raised' do + err = StandardError.new('internal dummy message') + stub_full_request(endpoint_url, method: :get).to_raise(err) result = subject.execute expect(result).to be_a(ServiceResponse) expect(result.error?).to be(true) - expect(result.message).to eq(error_message) + expect(result.message).to eq("Internal #{err.class.name.underscore} error: #{err.message}") end end diff --git a/spec/services/issuable/common_system_notes_service_spec.rb b/spec/services/issuable/common_system_notes_service_spec.rb index 9306aeaac44..3d83c9ec9c2 100644 --- a/spec/services/issuable/common_system_notes_service_spec.rb +++ b/spec/services/issuable/common_system_notes_service_spec.rb @@ -42,7 +42,7 @@ RSpec.describe Issuable::CommonSystemNotesService, feature_category: :team_plann context 'on issuable update' do it_behaves_like 'system note creation', { title: 'New title' }, 'changed title' it_behaves_like 'system note creation', { description: 'New description' }, 'changed the description' - it_behaves_like 'system note creation', { discussion_locked: true }, 'locked this issue' + it_behaves_like 'system note creation', { discussion_locked: true }, 'locked the discussion in this issue' it_behaves_like 'system note creation', { time_estimate: 5 }, 'changed time estimate' context 'when new label is added' do diff --git a/spec/services/issuable/discussions_list_service_spec.rb b/spec/services/issuable/discussions_list_service_spec.rb index 446cc286e28..9c791ce9cd3 100644 --- a/spec/services/issuable/discussions_list_service_spec.rb +++ b/spec/services/issuable/discussions_list_service_spec.rb @@ -30,6 +30,12 @@ RSpec.describe Issuable::DiscussionsListService, feature_category: :team_plannin expect(discussions_service.execute).to be_empty end end + + context 'when issue exists at the group level' do + let_it_be(:issuable) { create(:issue, :group_level, namespace: group) } + + it_behaves_like 'listing issuable discussions', :guest, 1, 7 + end end describe 'fetching notes for merge requests' do diff --git a/spec/services/issuable/process_assignees_spec.rb b/spec/services/issuable/process_assignees_spec.rb index fac7ef9ce77..5484f46e955 100644 --- a/spec/services/issuable/process_assignees_spec.rb +++ b/spec/services/issuable/process_assignees_spec.rb @@ -6,11 +6,11 @@ RSpec.describe Issuable::ProcessAssignees, feature_category: :team_planning do describe '#execute' do it 'returns assignee_ids when add_assignee_ids and remove_assignee_ids are not specified' do process = described_class.new( - assignee_ids: %w(5 7 9), + assignee_ids: %w[5 7 9], add_assignee_ids: nil, remove_assignee_ids: nil, - existing_assignee_ids: %w(1 3 9), - extra_assignee_ids: %w(2 5 12) + existing_assignee_ids: %w[1 3 9], + extra_assignee_ids: %w[2 5 12] ) result = process.execute @@ -22,8 +22,8 @@ RSpec.describe Issuable::ProcessAssignees, feature_category: :team_planning do assignee_ids: nil, add_assignee_ids: nil, remove_assignee_ids: nil, - existing_assignee_ids: %w(1 3 11), - extra_assignee_ids: %w(2 5 12) + existing_assignee_ids: %w[1 3 11], + extra_assignee_ids: %w[2 5 12] ) result = process.execute @@ -32,11 +32,11 @@ RSpec.describe Issuable::ProcessAssignees, feature_category: :team_planning do it 'combines other ids when both add_assignee_ids and remove_assignee_ids are not empty' do process = described_class.new( - assignee_ids: %w(5 7 9), - add_assignee_ids: %w(2 4 6), - remove_assignee_ids: %w(4 7 11), - existing_assignee_ids: %w(1 3 11), - extra_assignee_ids: %w(2 5 12) + assignee_ids: %w[5 7 9], + add_assignee_ids: %w[2 4 6], + remove_assignee_ids: %w[4 7 11], + existing_assignee_ids: %w[1 3 11], + extra_assignee_ids: %w[2 5 12] ) result = process.execute @@ -45,11 +45,11 @@ RSpec.describe Issuable::ProcessAssignees, feature_category: :team_planning do it 'combines other ids when remove_assignee_ids is not empty' do process = described_class.new( - assignee_ids: %w(5 7 9), + assignee_ids: %w[5 7 9], add_assignee_ids: nil, - remove_assignee_ids: %w(4 7 11), - existing_assignee_ids: %w(1 3 11), - extra_assignee_ids: %w(2 5 12) + remove_assignee_ids: %w[4 7 11], + existing_assignee_ids: %w[1 3 11], + extra_assignee_ids: %w[2 5 12] ) result = process.execute @@ -58,11 +58,11 @@ RSpec.describe Issuable::ProcessAssignees, feature_category: :team_planning do it 'combines other ids when add_assignee_ids is not empty' do process = described_class.new( - assignee_ids: %w(5 7 9), - add_assignee_ids: %w(2 4 6), + assignee_ids: %w[5 7 9], + add_assignee_ids: %w[2 4 6], remove_assignee_ids: nil, - existing_assignee_ids: %w(1 3 11), - extra_assignee_ids: %w(2 5 12) + existing_assignee_ids: %w[1 3 11], + extra_assignee_ids: %w[2 5 12] ) result = process.execute @@ -71,9 +71,9 @@ RSpec.describe Issuable::ProcessAssignees, feature_category: :team_planning do it 'combines ids when existing_assignee_ids and extra_assignee_ids are omitted' do process = described_class.new( - assignee_ids: %w(5 7 9), - add_assignee_ids: %w(2 4 6), - remove_assignee_ids: %w(4 7 11) + assignee_ids: %w[5 7 9], + add_assignee_ids: %w[2 4 6], + remove_assignee_ids: %w[4 7 11] ) result = process.execute @@ -82,11 +82,11 @@ RSpec.describe Issuable::ProcessAssignees, feature_category: :team_planning do it 'handles mixed string and integer arrays' do process = described_class.new( - assignee_ids: %w(5 7 9), + assignee_ids: %w[5 7 9], add_assignee_ids: [2, 4, 6], - remove_assignee_ids: %w(4 7 11), + remove_assignee_ids: %w[4 7 11], existing_assignee_ids: [1, 3, 11], - extra_assignee_ids: %w(2 5 12) + extra_assignee_ids: %w[2 5 12] ) result = process.execute diff --git a/spec/services/issues/export_csv_service_spec.rb b/spec/services/issues/export_csv_service_spec.rb index 31eaa72255d..83dfca923fb 100644 --- a/spec/services/issues/export_csv_service_spec.rb +++ b/spec/services/issues/export_csv_service_spec.rb @@ -160,7 +160,7 @@ RSpec.describe Issues::ExportCsvService, :with_license, feature_category: :team_ context 'with issues filtered by labels and project' do subject do described_class.new( - IssuesFinder.new(user, project_id: project.id, label_name: %w(Idea Feature)).execute, + IssuesFinder.new(user, project_id: project.id, label_name: %w[Idea Feature]).execute, project ) end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index c4012e2a98f..0cb13bfb917 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -491,9 +491,9 @@ RSpec.describe Issues::UpdateService, :mailer, feature_category: :team_planning end it 'creates system note about discussion lock' do - note = find_note('locked this issue') + note = find_note('locked the discussion in this issue') - expect(note.note).to eq 'locked this issue' + expect(note.note).to eq 'locked the discussion in this issue' end end @@ -539,21 +539,6 @@ RSpec.describe Issues::UpdateService, :mailer, feature_category: :team_planning end end end - - it 'verifies the number of queries' do - update_issue(description: "- [ ] Task 1 #{user.to_reference}") - - baseline = ActiveRecord::QueryRecorder.new do - update_issue(description: "- [x] Task 1 #{user.to_reference}") - end - - recorded = ActiveRecord::QueryRecorder.new do - update_issue(description: "- [x] Task 1 #{user.to_reference}\n- [ ] Task 2 #{user.to_reference}") - end - - expect(recorded.count).to eq(baseline.count) - expect(recorded.cached_count).to eq(0) - end end context 'when description changed' do diff --git a/spec/services/jira/requests/projects/list_service_spec.rb b/spec/services/jira/requests/projects/list_service_spec.rb index f9e3a3e8510..d8e6eda2dd4 100644 --- a/spec/services/jira/requests/projects/list_service_spec.rb +++ b/spec/services/jira/requests/projects/list_service_spec.rb @@ -73,7 +73,7 @@ RSpec.describe Jira::Requests::Projects::ListService, feature_category: :groups_ payload = subject.payload expect(subject.success?).to be_truthy - expect(payload[:projects].map(&:key)).to eq(%w(pr1 pr2)) + expect(payload[:projects].map(&:key)).to eq(%w[pr1 pr2]) expect(payload[:is_last]).to be_truthy end @@ -84,7 +84,7 @@ RSpec.describe Jira::Requests::Projects::ListService, feature_category: :groups_ payload = subject.payload expect(subject.success?).to be_truthy - expect(payload[:projects].map(&:key)).to eq(%w(pr1)) + expect(payload[:projects].map(&:key)).to eq(%w[pr1]) expect(payload[:is_last]).to be_truthy end end diff --git a/spec/services/jira_connect_subscriptions/create_service_spec.rb b/spec/services/jira_connect_subscriptions/create_service_spec.rb index f9d3954b84c..2296d0fbfed 100644 --- a/spec/services/jira_connect_subscriptions/create_service_spec.rb +++ b/spec/services/jira_connect_subscriptions/create_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe JiraConnectSubscriptions::CreateService, feature_category: :integ let(:path) { group.full_path } let(:params) { { namespace_path: path, jira_user: jira_user } } - let(:jira_user) { double(:JiraUser, site_admin?: true) } + let(:jira_user) { double(:JiraUser, jira_admin?: true) } subject { described_class.new(installation, current_user, params).execute } @@ -29,11 +29,11 @@ RSpec.describe JiraConnectSubscriptions::CreateService, feature_category: :integ end context 'remote user does not have access' do - let(:jira_user) { double(site_admin?: false) } + let(:jira_user) { double(jira_admin?: false) } it_behaves_like 'a failed execution', http_status: 403, - message: 'The Jira user is not a site administrator. Check the permissions in Jira and try again.' + message: 'The Jira user is not a site or organization administrator. Check the permissions in Jira and try again.' end context 'remote user cannot be retrieved' do diff --git a/spec/services/lfs/file_transformer_spec.rb b/spec/services/lfs/file_transformer_spec.rb index c90d7af022f..398beabbeeb 100644 --- a/spec/services/lfs/file_transformer_spec.rb +++ b/spec/services/lfs/file_transformer_spec.rb @@ -218,7 +218,7 @@ RSpec.describe Lfs::FileTransformer, feature_category: :source_code_management d repository_types = project.lfs_objects_projects.order(:id).pluck(:repository_type) - expect(repository_types).to eq(%w(project wiki)) + expect(repository_types).to eq(%w[project wiki]) end end end diff --git a/spec/services/merge_requests/conflicts/resolve_service_spec.rb b/spec/services/merge_requests/conflicts/resolve_service_spec.rb index 002a07ff14e..a4245456367 100644 --- a/spec/services/merge_requests/conflicts/resolve_service_spec.rb +++ b/spec/services/merge_requests/conflicts/resolve_service_spec.rb @@ -72,8 +72,8 @@ RSpec.describe MergeRequests::Conflicts::ResolveService, feature_category: :code it 'creates a commit with the correct parents' do expect(merge_request.source_branch_head.parents.map(&:id)) - .to eq(%w(1450cd639e0bc6721eb02800169e464f212cde06 - 824be604a34828eb682305f0d963056cfac87b2d)) + .to eq(%w[1450cd639e0bc6721eb02800169e464f212cde06 + 824be604a34828eb682305f0d963056cfac87b2d]) end end @@ -169,8 +169,8 @@ RSpec.describe MergeRequests::Conflicts::ResolveService, feature_category: :code it 'creates a commit with the correct parents' do expect(merge_request.source_branch_head.parents.map(&:id)) - .to eq(%w(1450cd639e0bc6721eb02800169e464f212cde06 - 824be604a34828eb682305f0d963056cfac87b2d)) + .to eq(%w[1450cd639e0bc6721eb02800169e464f212cde06 + 824be604a34828eb682305f0d963056cfac87b2d]) end it 'sets the content to the content given' do diff --git a/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb b/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb deleted file mode 100644 index 172c2133168..00000000000 --- a/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb +++ /dev/null @@ -1,55 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe MergeRequests::MarkReviewerReviewedService, feature_category: :code_review_workflow do - let(:current_user) { create(:user) } - let(:merge_request) { create(:merge_request, reviewers: [current_user]) } - let(:reviewer) { merge_request.merge_request_reviewers.find_by(user_id: current_user.id) } - let(:project) { merge_request.project } - let(:service) { described_class.new(project: project, current_user: current_user) } - let(:result) { service.execute(merge_request) } - - before do - project.add_developer(current_user) - end - - describe '#execute' do - shared_examples_for 'failed service execution' do - it 'returns an error' do - expect(result[:status]).to eq :error - end - - it_behaves_like 'does not trigger GraphQL subscription mergeRequestReviewersUpdated' do - let(:action) { result } - end - end - - describe 'invalid permissions' do - let(:service) { described_class.new(project: project, current_user: create(:user)) } - - it_behaves_like 'failed service execution' - end - - describe 'reviewer does not exist' do - let(:service) { described_class.new(project: project, current_user: create(:user)) } - - it_behaves_like 'failed service execution' - end - - describe 'reviewer exists' do - it 'returns success' do - expect(result[:status]).to eq :success - end - - it 'updates reviewers state' do - expect(result[:status]).to eq :success - expect(reviewer.state).to eq 'reviewed' - end - - it_behaves_like 'triggers GraphQL subscription mergeRequestReviewersUpdated' do - let(:action) { result } - end - end - end -end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 6e34f4362c1..2e8f0049f28 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -569,7 +569,7 @@ RSpec.describe MergeRequests::MergeService, feature_category: :code_review_workf allow_any_instance_of(Repository).to receive(:ancestor?).and_return(nil) end - %w(semi-linear ff).each do |merge_method| + %w[semi-linear ff].each do |merge_method| it "logs and saves error if merge is #{merge_method} only" do merge_method = 'rebase_merge' if merge_method == 'semi-linear' merge_request.project.update!(merge_method: merge_method) @@ -599,6 +599,7 @@ RSpec.describe MergeRequests::MergeService, feature_category: :code_review_workf context 'with failing CI' do before do + allow(merge_request.project).to receive(:only_allow_merge_if_pipeline_succeeds) { true } allow(merge_request).to receive(:mergeable_ci_state?) { false } end @@ -616,6 +617,7 @@ RSpec.describe MergeRequests::MergeService, feature_category: :code_review_workf context 'with unresolved discussions' do before do + allow(merge_request.project).to receive(:only_allow_merge_if_all_discussions_are_resolved) { true } allow(merge_request).to receive(:mergeable_discussions_state?) { false } 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 cf835cf70a3..067e87859e7 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 @@ -5,7 +5,8 @@ require 'spec_helper' RSpec.describe MergeRequests::Mergeability::CheckCiStatusService, feature_category: :code_review_workflow do subject(:check_ci_status) { described_class.new(merge_request: merge_request, params: params) } - let(:merge_request) { build(:merge_request) } + let_it_be(:project) { build(:project) } + let_it_be(:merge_request) { build(:merge_request, source_project: project) } let(:params) { { skip_ci_check: skip_check } } let(:skip_check) { false } @@ -13,23 +14,41 @@ RSpec.describe MergeRequests::Mergeability::CheckCiStatusService, feature_catego let(:result) { check_ci_status.execute } before do - expect(merge_request).to receive(:mergeable_ci_state?).and_return(mergeable) + allow(merge_request) + .to receive(:only_allow_merge_if_pipeline_succeeds?) + .and_return(only_allow_merge_if_pipeline_succeeds?) end - context 'when the merge request is in a mergable state' do - let(:mergeable) { true } + context 'when only_allow_merge_if_pipeline_succeeds is true' do + let(:only_allow_merge_if_pipeline_succeeds?) { true } - it 'returns a check result with status success' do - expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + before do + expect(merge_request).to receive(:mergeable_ci_state?).and_return(mergeable) + end + + context 'when the merge request is in a mergeable state' do + let(:mergeable) { true } + + it 'returns a check result with status success' do + expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + + context 'when the merge request is not in a mergeable state' do + let(:mergeable) { false } + + 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 + end end end - context 'when the merge request is not in a mergeable state' do - let(:mergeable) { false } + context 'when only_allow_merge_if_pipeline_succeeds is false' do + let(:only_allow_merge_if_pipeline_succeeds?) { false } - 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 + it 'returns a check result with inactive status' do + expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::INACTIVE_STATUS 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 a3b77558ec3..4a8b28f603d 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 @@ -5,7 +5,8 @@ require 'spec_helper' RSpec.describe MergeRequests::Mergeability::CheckDiscussionsStatusService, feature_category: :code_review_workflow do subject(:check_discussions_status) { described_class.new(merge_request: merge_request, params: params) } - let(:merge_request) { build(:merge_request) } + let_it_be(:project) { build(:project) } + let_it_be(:merge_request) { build(:merge_request, source_project: project) } let(:params) { { skip_discussions_check: skip_check } } let(:skip_check) { false } @@ -13,23 +14,41 @@ RSpec.describe MergeRequests::Mergeability::CheckDiscussionsStatusService, featu let(:result) { check_discussions_status.execute } before do - expect(merge_request).to receive(:mergeable_discussions_state?).and_return(mergeable) + allow(merge_request) + .to receive(:only_allow_merge_if_all_discussions_are_resolved?) + .and_return(only_allow_merge_if_all_discussions_are_resolved?) end - context 'when the merge request is in a mergable state' do - let(:mergeable) { true } + context 'when only_allow_merge_if_all_discussions_are_resolved is true' do + let(:only_allow_merge_if_all_discussions_are_resolved?) { true } - it 'returns a check result with status success' do - expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + before do + allow(merge_request).to receive(:mergeable_discussions_state?).and_return(mergeable) + end + + context 'when the merge request is in a mergable state' do + let(:mergeable) { true } + + it 'returns a check result with status success' do + expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + + context 'when the merge request is not in a mergeable state' do + let(:mergeable) { false } + + 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) + end end end - context 'when the merge request is not in a mergeable state' do - let(:mergeable) { false } + context 'when only_allow_merge_if_all_discussions_are_resolved is false' do + let(:only_allow_merge_if_all_discussions_are_resolved?) { false } - 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) + it 'returns a check result with inactive status' do + expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::INACTIVE_STATUS 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 31ec44856b1..d6948f72c0a 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 @@ -5,7 +5,8 @@ require 'spec_helper' RSpec.describe MergeRequests::Mergeability::CheckRebaseStatusService, feature_category: :code_review_workflow do subject(:check_rebase_status) { described_class.new(merge_request: merge_request, params: params) } - let(:merge_request) { build(:merge_request) } + let_it_be(:project) { build(:project) } + let_it_be(:merge_request) { build(:merge_request, source_project: project) } let(:params) { { skip_rebase_check: skip_check } } let(:skip_check) { false } @@ -13,23 +14,41 @@ RSpec.describe MergeRequests::Mergeability::CheckRebaseStatusService, feature_ca let(:result) { check_rebase_status.execute } before do - allow(merge_request).to receive(:should_be_rebased?).and_return(should_be_rebased) + allow(project) + .to receive(:ff_merge_must_be_possible?) + .and_return(ff_merge_must_be_possible?) end - context 'when the merge request should be rebased' do - let(:should_be_rebased) { true } + context 'when ff_merge_must_be_possible is true' do + let(:ff_merge_must_be_possible?) { true } - 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 + before do + allow(merge_request).to receive(:should_be_rebased?).and_return(should_be_rebased) + end + + context 'when the merge request should be rebased' do + let(:should_be_rebased) { true } + + 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) + end + end + + context 'when the merge request should not be rebased' do + let(:should_be_rebased) { false } + + it 'returns a check result with status success' do + expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end end end - context 'when the merge request should not be rebased' do - let(:should_be_rebased) { false } + context 'when ff_merge_must_be_possible is false' do + let(:ff_merge_must_be_possible?) { false } - it 'returns a check result with status success' do - expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + it 'returns a check result with inactive status' do + expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::INACTIVE_STATUS end 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 546d583a2fb..06e15356a92 100644 --- a/spec/services/merge_requests/mergeability/run_checks_service_spec.rb +++ b/spec/services/merge_requests/mergeability/run_checks_service_spec.rb @@ -98,6 +98,26 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService, :clean_gitlab_redi let(:expected_count) { checks.count - 1 } end end + + context 'when one check is inactive' do + let(:inactive_result) { Gitlab::MergeRequests::Mergeability::CheckResult.inactive } + + before do + allow_next_instance_of(MergeRequests::Mergeability::CheckOpenStatusService) do |service| + allow(service).to receive(:skip?).and_return(false) + allow(service).to receive(:execute).and_return(inactive_result) + end + end + + it 'is still a success' do + expect(execute.success?).to eq(true) + end + + it_behaves_like 'checks are all executed' do + let(:success?) { true } + let(:expected_count) { checks.count - 1 } + end + end end context 'when a check is not skipped' do diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index 49ec8b09939..038977e4fd0 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -54,6 +54,17 @@ RSpec.describe MergeRequests::PushOptionsHandlerService, feature_category: :sour end end + shared_examples_for 'a service that can set the target project of a merge request' do + subject(:last_mr) { MergeRequest.last } + + it 'creates a merge request with the correct target project' do + project_path = push_options[:target_project] || project.default_merge_request_target.full_path + + expect { service.execute }.to change { MergeRequest.count }.by(1) + expect(last_mr.target_project.full_path).to eq(project_path) + end + end + shared_examples_for 'a service that can set the title of a merge request' do subject(:last_mr) { MergeRequest.last } @@ -347,6 +358,31 @@ RSpec.describe MergeRequests::PushOptionsHandlerService, feature_category: :sour it_behaves_like 'with the project default branch' end + describe '`target_project` push option' do + let(:changes) { new_branch_changes } + let(:double_forked_project) { fork_project(forked_project, user1, repository: true) } + let(:service) { described_class.new(project: double_forked_project, current_user: user1, changes: changes, push_options: push_options) } + let(:push_options) { { create: true, target_project: target_project.full_path } } + + context 'to self' do + let(:target_project) { double_forked_project } + + it_behaves_like 'a service that can set the target project of a merge request' + end + + context 'to intermediate project' do + let(:target_project) { forked_project } + + it_behaves_like 'a service that can set the target project of a merge request' + end + + context 'to base project' do + let(:target_project) { project } + + it_behaves_like 'a service that can set the target project of a merge request' + end + end + describe '`title` push option' do let(:push_options) { { title: title } } @@ -861,6 +897,17 @@ RSpec.describe MergeRequests::PushOptionsHandlerService, feature_category: :sour end end + describe 'when the target project does not exist' do + let(:push_options) { { create: true, target: 'my-branch', target_project: 'does-not-exist' } } + let(:changes) { default_branch_changes } + + it 'records an error', :sidekiq_inline do + service.execute + + expect(service.errors).to eq(["User access was denied"]) + end + end + describe 'when user does not have access to target project' do let(:push_options) { { create: true, target: 'my-branch' } } let(:changes) { default_branch_changes } @@ -890,6 +937,18 @@ RSpec.describe MergeRequests::PushOptionsHandlerService, feature_category: :sour end end + describe 'when projects are unrelated' do + let(:unrelated_project) { create(:project, :public, :repository, group: child_group) } + let(:push_options) { { create: true, target_project: unrelated_project.full_path } } + let(:changes) { new_branch_changes } + + it 'records an error' do + service.execute + + expect(service.errors).to eq(["Projects #{project.full_path} and #{unrelated_project.full_path} are not in the same network"]) + end + end + describe 'when MR has ActiveRecord errors' do let(:push_options) { { create: true } } let(:changes) { new_branch_changes } diff --git a/spec/services/merge_requests/pushed_branches_service_spec.rb b/spec/services/merge_requests/pushed_branches_service_spec.rb index cb5d0a6bd25..de99fb244d3 100644 --- a/spec/services/merge_requests/pushed_branches_service_spec.rb +++ b/spec/services/merge_requests/pushed_branches_service_spec.rb @@ -8,7 +8,7 @@ RSpec.describe MergeRequests::PushedBranchesService, feature_category: :source_c context 'when branches pushed' do let(:pushed_branches) do - %w(branch1 branch2 closed-branch1 closed-branch2 extra1 extra2).map do |branch| + %w[branch1 branch2 closed-branch1 closed-branch2 extra1 extra2].map do |branch| { ref: "refs/heads/#{branch}" } end end @@ -31,7 +31,7 @@ RSpec.describe MergeRequests::PushedBranchesService, feature_category: :source_c context 'when tags pushed' do let(:pushed_branches) do - %w(v10.0.0 v11.0.2 v12.1.0).map do |branch| + %w[v10.0.0 v11.0.2 v12.1.0].map do |branch| { ref: "refs/tags/#{branch}" } end end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index d5b7b56ccdd..dd50dfa49e0 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -915,7 +915,7 @@ RSpec.describe MergeRequests::RefreshService, feature_category: :code_review_wor context 'feature enabled' do it "updates merge requests' merge_commit and merged_commit values", :aggregate_failures do expect(Gitlab::BranchPushMergeCommitAnalyzer).to receive(:new).and_wrap_original do |original_method, commits| - expect(commits.map(&:id)).to eq(%w{646ece5cfed840eca0a4feb21bcd6a81bb19bda3 29284d9bcc350bcae005872d0be6edd016e2efb5 5f82584f0a907f3b30cfce5bb8df371454a90051 8a994512e8c8f0dfcf22bb16df6e876be7a61036 689600b91aabec706e657e38ea706ece1ee8268f db46a1c5a5e474aa169b6cdb7a522d891bc4c5f9}) + expect(commits.map(&:id)).to eq(%w[646ece5cfed840eca0a4feb21bcd6a81bb19bda3 29284d9bcc350bcae005872d0be6edd016e2efb5 5f82584f0a907f3b30cfce5bb8df371454a90051 8a994512e8c8f0dfcf22bb16df6e876be7a61036 689600b91aabec706e657e38ea706ece1ee8268f db46a1c5a5e474aa169b6cdb7a522d891bc4c5f9]) original_method.call(commits) end diff --git a/spec/services/merge_requests/update_reviewer_state_service_spec.rb b/spec/services/merge_requests/update_reviewer_state_service_spec.rb new file mode 100644 index 00000000000..be24d95d7f1 --- /dev/null +++ b/spec/services/merge_requests/update_reviewer_state_service_spec.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::UpdateReviewerStateService, feature_category: :code_review_workflow do + let_it_be(:current_user) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, reviewers: [current_user]) } + let(:reviewer) { merge_request.merge_request_reviewers.find_by(user_id: current_user.id) } + let(:project) { merge_request.project } + let(:service) { described_class.new(project: project, current_user: current_user) } + let(:state) { 'requested_changes' } + let(:result) { service.execute(merge_request, state) } + + before do + project.add_developer(current_user) + end + + describe '#execute' do + shared_examples_for 'failed service execution' do + it 'returns an error' do + expect(result[:status]).to eq :error + end + + it_behaves_like 'does not trigger GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { result } + end + end + + describe 'invalid permissions' do + let(:service) { described_class.new(project: project, current_user: create(:user)) } + + it_behaves_like 'failed service execution' + end + + describe 'reviewer exists' do + it 'returns success' do + expect(result[:status]).to eq :success + end + + it 'updates reviewers state' do + expect(result[:status]).to eq :success + expect(reviewer.state).to eq 'requested_changes' + end + + it 'does not call MergeRequests::RemoveApprovalService' do + expect(MergeRequests::RemoveApprovalService).not_to receive(:new) + + expect(result[:status]).to eq :success + end + + it_behaves_like 'triggers GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { result } + end + + context 'when reviewer has approved' do + before do + create(:approval, user: current_user, merge_request: merge_request) + end + + it 'removes approval when state is requested_changes' do + expect_next_instance_of( + MergeRequests::RemoveApprovalService, + project: project, current_user: current_user + ) do |service| + expect(service).to receive(:execute).with(merge_request).and_return({ success: true }) + end + + expect(result[:status]).to eq :success + end + + it 'renders error when remove approval service fails' do + expect_next_instance_of( + MergeRequests::RemoveApprovalService, + project: project, current_user: current_user + ) do |service| + expect(service).to receive(:execute).with(merge_request).and_return(nil) + end + + expect(result[:status]).to eq :error + expect(result[:message]).to eq "Failed to remove approval" + end + end + end + end +end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index f5494f429c3..53dd4263770 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -351,10 +351,10 @@ RSpec.describe MergeRequests::UpdateService, :mailer, feature_category: :code_re end it 'creates system note about discussion lock' do - note = find_note('locked this merge request') + note = find_note('locked the discussion in this merge request') expect(note).not_to be_nil - expect(note.note).to eq 'locked this merge request' + expect(note.note).to eq 'locked the discussion in this merge request' end context 'when current user cannot admin issues in the project' do diff --git a/spec/services/ml/create_candidate_service_spec.rb b/spec/services/ml/create_candidate_service_spec.rb new file mode 100644 index 00000000000..fb3456b0bcc --- /dev/null +++ b/spec/services/ml/create_candidate_service_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +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(:experiment) { create(:ml_experiments, project: model_version.project) } + + let(:params) { {} } + + subject(:candidate) { described_class.new(experiment, params).execute } + + context 'with default parameters' do + it 'creates a candidate' do + expect { candidate }.to change { experiment.candidates.count }.by(1) + end + + it 'gives a fake name' do + expect(candidate.name).to match(/[a-z]+-[a-z]+-[a-z]+-\d+/) + end + + it 'sets the correct values', :aggregate_failures do + expect(candidate.start_time).to eq(0) + expect(candidate.experiment).to be(experiment) + expect(candidate.project).to be(experiment.project) + expect(candidate.user).to be_nil + end + end + + context 'when parameters are passed' do + let(:params) do + { + start_time: 1234, + name: 'candidate_name', + model_version: model_version, + user: experiment.user + } + end + + context 'with default parameters' do + it 'creates a candidate' do + expect { candidate }.to change { experiment.candidates.count }.by(1) + end + + it 'sets the correct values', :aggregate_failures do + expect(candidate.start_time).to eq(1234) + expect(candidate.experiment).to be(experiment) + expect(candidate.project).to be(experiment.project) + expect(candidate.user).to be(experiment.user) + expect(candidate.name).to eq('candidate_name') + expect(candidate.model_version_id).to eq(model_version.id) + end + end + end + end +end diff --git a/spec/services/ml/create_model_service_spec.rb b/spec/services/ml/create_model_service_spec.rb new file mode 100644 index 00000000000..212f0940635 --- /dev/null +++ b/spec/services/ml/create_model_service_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ml::CreateModelService, feature_category: :mlops do + let_it_be(:user) { create(:user) } + let_it_be(:existing_model) { create(:ml_models) } + let_it_be(:another_project) { create(:project) } + let_it_be(:description) { 'description' } + let_it_be(:metadata) { [] } + + subject(:create_model) { described_class.new(project, name, user, description, metadata).execute } + + describe '#execute' do + context 'when model name does not exist in the project' do + let(:name) { 'new_model' } + let(:project) { existing_model.project } + + it 'creates a model', :aggregate_failures do + expect { create_model }.to change { Ml::Model.count }.by(1) + + expect(create_model.name).to eq(name) + end + end + + context 'when model name exists but project is different' do + let(:name) { existing_model.name } + let(:project) { another_project } + + it 'creates a model', :aggregate_failures do + expect { create_model }.to change { Ml::Model.count }.by(1) + + expect(create_model.name).to eq(name) + end + end + + context 'when model with name exists' do + let(:name) { existing_model.name } + let(:project) { existing_model.project } + + it 'raises an error', :aggregate_failures do + expect { create_model }.to raise_error(ActiveRecord::RecordInvalid) + end + end + + context 'when metadata are supplied, add them as metadata' do + let(:name) { 'new_model' } + let(:project) { existing_model.project } + let(:metadata) { [{ key: 'key1', value: 'value1' }, { key: 'key2', value: 'value2' }] } + + it 'creates metadata records', :aggregate_failures do + expect { create_model }.to change { Ml::Model.count }.by(1) + + expect(create_model.name).to eq(name) + expect(create_model.metadata.count).to be 2 + end + end + + # TODO: Ensure consisted error responses https://gitlab.com/gitlab-org/gitlab/-/issues/429731 + context 'for metadata with duplicate keys, it does not create duplicate records' do + let(:name) { 'new_model' } + let(:project) { existing_model.project } + let(:metadata) { [{ key: 'key1', value: 'value1' }, { key: 'key1', value: 'value2' }] } + + it 'raises an error', :aggregate_failures do + expect { create_model }.to raise_error(ActiveRecord::RecordInvalid) + end + end + + # TODO: Ensure consisted error responses https://gitlab.com/gitlab-org/gitlab/-/issues/429731 + context 'for metadata with invalid keys, it does not create invalid records' do + let(:name) { 'new_model' } + let(:project) { existing_model.project } + let(:metadata) { [{ key: 'key1', value: 'value1' }, { key: '', value: 'value2' }] } + + it 'raises an error', :aggregate_failures do + expect { create_model }.to raise_error(ActiveRecord::RecordInvalid) + end + end + end +end diff --git a/spec/services/ml/find_model_service_spec.rb b/spec/services/ml/find_model_service_spec.rb new file mode 100644 index 00000000000..027298d979a --- /dev/null +++ b/spec/services/ml/find_model_service_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ml::FindModelService, feature_category: :mlops do + let_it_be(:user) { create(:user) } + let_it_be(:existing_model) { create(:ml_models) } + let(:finder) { described_class.new(project, name) } + + describe '#execute' do + context 'when model name does not exist in the project' do + let(:name) { 'new_model' } + let(:project) { existing_model.project } + + it 'reutrns nil' do + expect(finder.execute).to be nil + end + end + + context 'when model with name exists' do + let(:name) { existing_model.name } + let(:project) { existing_model.project } + + it 'returns the existing model' do + expect(finder.execute).to eq(existing_model) + end + end + end +end diff --git a/spec/services/ml/find_or_create_model_service_spec.rb b/spec/services/ml/find_or_create_model_service_spec.rb index 6ddae20f8d6..5d5eaea0a72 100644 --- a/spec/services/ml/find_or_create_model_service_spec.rb +++ b/spec/services/ml/find_or_create_model_service_spec.rb @@ -3,10 +3,13 @@ require 'spec_helper' RSpec.describe ::Ml::FindOrCreateModelService, feature_category: :mlops do + let_it_be(:user) { create(:user) } let_it_be(:existing_model) { create(:ml_models) } let_it_be(:another_project) { create(:project) } + let_it_be(:description) { 'description' } + let_it_be(:metadata) { [] } - subject(:create_model) { described_class.new(project, name).execute } + subject(:create_model) { described_class.new(project, name, user, description, metadata).execute } describe '#execute' do context 'when model name does not exist in the project' do 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 1211a9b1165..e5ca7c3a450 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 @@ -5,14 +5,18 @@ require 'spec_helper' RSpec.describe ::Ml::FindOrCreateModelVersionService, feature_category: :mlops do let_it_be(:existing_version) { create(:ml_model_versions) } let_it_be(:another_project) { create(:project) } + let_it_be(:user) { create(:user) } let(:package) { nil } + let(:description) { nil } let(:params) do { model_name: name, version: version, - package: package + package: package, + description: description, + user: user } end @@ -26,6 +30,7 @@ RSpec.describe ::Ml::FindOrCreateModelVersionService, feature_category: :mlops d it 'returns existing model version', :aggregate_failures do expect { model_version }.to change { Ml::ModelVersion.count }.by(0) + expect { model_version }.to change { Ml::Candidate.count }.by(0) expect(model_version).to eq(existing_version) end end @@ -34,15 +39,18 @@ RSpec.describe ::Ml::FindOrCreateModelVersionService, feature_category: :mlops d let(:project) { existing_version.project } let(:name) { 'a_new_model' } 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 } + expect { model_version }.to change { Ml::ModelVersion.count }.by(1).and change { Ml::Candidate.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.description).to eq(description) end end end diff --git a/spec/services/ml/model_versions/get_model_version_service_spec.rb b/spec/services/ml/model_versions/get_model_version_service_spec.rb new file mode 100644 index 00000000000..b46a0bf6d1d --- /dev/null +++ b/spec/services/ml/model_versions/get_model_version_service_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ml::ModelVersions::GetModelVersionService, feature_category: :mlops do + let_it_be(:existing_version) { create(:ml_model_versions) } + let_it_be(:another_project) { create(:project) } + + subject(:model_version) { described_class.new(project, name, version).execute } + + describe '#execute' do + context 'when model version exists' do + let(:name) { existing_version.name } + let(:version) { existing_version.version } + let(:project) { existing_version.project } + + it { is_expected.to eq(existing_version) } + end + + context 'when model version does not exist' do + let(:project) { existing_version.project } + let(:name) { 'a_new_model' } + let(:version) { '2.0.0' } + + it { is_expected.to be_nil } + end + end +end diff --git a/spec/services/ml/update_model_service_spec.rb b/spec/services/ml/update_model_service_spec.rb new file mode 100644 index 00000000000..35df62559e6 --- /dev/null +++ b/spec/services/ml/update_model_service_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ml::UpdateModelService, feature_category: :mlops do + let_it_be(:model) { create(:ml_models) } + let_it_be(:description) { 'updated model description' } + let(:service) { described_class.new(model, description) } + + describe '#execute' do + context 'when supplied with a non-model object' do + let(:model) { nil } + + it 'returns nil' do + expect { service.execute }.to raise_error(NoMethodError) + end + end + + context 'with an existing model' do + it 'updates the description' do + updated = service.execute + expect(updated.class).to be(Ml::Model) + expect(updated.description).to eq(description) + end + end + end +end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 0cc66696184..c1b15ec7681 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -3,7 +3,8 @@ require 'spec_helper' RSpec.describe Notes::CreateService, feature_category: :team_planning do - let_it_be(:project) { create(:project, :repository) } + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :repository, group: group) } let_it_be(:issue) { create(:issue, project: project) } let_it_be(:user) { create(:user) } @@ -13,11 +14,43 @@ RSpec.describe Notes::CreateService, feature_category: :team_planning do describe '#execute' do subject(:note) { described_class.new(project, user, opts).execute } - before do - project.add_maintainer(user) + before_all do + group.add_maintainer(user) end context "valid params" do + context 'when noteable is an issue that belongs directly to a group' do + it 'creates a note without a project and correct namespace', :aggregate_failures do + group_issue = create(:issue, :group_level, namespace: group) + note_params = { note: 'test note', noteable: group_issue } + + expect do + described_class.new(nil, user, note_params).execute + end.to change { Note.count }.by(1) + + created_note = Note.last + + expect(created_note.namespace).to eq(group) + expect(created_note.project).to be_nil + end + end + + context 'when noteable is a work item that belongs directly to a group' do + it 'creates a note without a project and correct namespace', :aggregate_failures do + group_work_item = create(:work_item, :group_level, namespace: group) + note_params = { note: 'test note', noteable: group_work_item } + + expect do + described_class.new(nil, user, note_params).execute + end.to change { Note.count }.by(1) + + created_note = Note.last + + expect(created_note.namespace).to eq(group) + expect(created_note.project).to be_nil + end + end + it_behaves_like 'does not trigger GraphQL subscription mergeRequestMergeStatusUpdated' do let(:action) { note } end @@ -195,21 +228,35 @@ RSpec.describe Notes::CreateService, feature_category: :team_planning do let(:new_opts) { opts.merge(noteable_type: 'MergeRequest', noteable_id: merge_request.id) } - it 'calls MergeRequests::MarkReviewerReviewedService service' do - expect_next_instance_of( - MergeRequests::MarkReviewerReviewedService, - project: project_with_repo, current_user: user - ) do |service| - expect(service).to receive(:execute).with(merge_request) + context 'when mr_request_changes feature flag is disabled' do + before do + stub_feature_flags(mr_request_changes: false) end - described_class.new(project_with_repo, user, new_opts).execute + it 'calls MergeRequests::UpdateReviewerStateService service' do + expect_next_instance_of( + MergeRequests::UpdateReviewerStateService, + project: project_with_repo, current_user: user + ) do |service| + expect(service).to receive(:execute).with(merge_request, "reviewed") + end + + described_class.new(project_with_repo, user, new_opts).execute + end + + it 'does not call MergeRequests::UpdateReviewerStateService service when skip_set_reviewed is true' do + expect(MergeRequests::UpdateReviewerStateService).not_to receive(:new) + + described_class.new(project_with_repo, user, new_opts).execute(skip_set_reviewed: true) + end end - it 'does not call MergeRequests::MarkReviewerReviewedService service when skip_set_reviewed is true' do - expect(MergeRequests::MarkReviewerReviewedService).not_to receive(:new) + context 'when mr_request_changes feature flag is enabled' do + it 'does not call MergeRequests::UpdateReviewerStateService service when skip_set_reviewed is true' do + expect(MergeRequests::UpdateReviewerStateService).not_to receive(:new) - described_class.new(project_with_repo, user, new_opts).execute(skip_set_reviewed: true) + described_class.new(project_with_repo, user, new_opts).execute(skip_set_reviewed: true) + end end context 'noteable highlight cache clearing' do diff --git a/spec/services/organizations/create_service_spec.rb b/spec/services/organizations/create_service_spec.rb new file mode 100644 index 00000000000..7d9bf64ddd3 --- /dev/null +++ b/spec/services/organizations/create_service_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Organizations::CreateService, feature_category: :cell do + describe '#execute' do + let_it_be(:user) { create(:user) } + + let(:current_user) { user } + let(:params) { attributes_for(:organization) } + + subject(:response) { described_class.new(current_user: current_user, params: params).execute } + + 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 create organizations']) + end + end + + context 'when user has permission' do + it 'creates an organization' do + expect { response }.to change { Organizations::Organization.count } + + expect(response).to be_success + end + + it 'returns an error when the organization is not persisted' do + params[:name] = nil + + 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/packages/create_dependency_service_spec.rb b/spec/services/packages/create_dependency_service_spec.rb index 06a7a13bdd9..c50bf988899 100644 --- a/spec/services/packages/create_dependency_service_spec.rb +++ b/spec/services/packages/create_dependency_service_spec.rb @@ -33,8 +33,8 @@ RSpec.describe Packages::CreateDependencyService, feature_category: :package_reg expect { subject } .to change { Packages::Dependency.count }.by(1) .and change { Packages::DependencyLink.count }.by(1) - expect(dependency_names).to match_array(%w(express)) - expect(dependency_link_types).to match_array(%w(dependencies)) + expect(dependency_names).to match_array(%w[express]) + expect(dependency_link_types).to match_array(%w[dependencies]) end context 'with repeated packages' do @@ -49,8 +49,8 @@ RSpec.describe Packages::CreateDependencyService, feature_category: :package_reg expect { subject } .to change { Packages::Dependency.count }.by(4) .and change { Packages::DependencyLink.count }.by(6) - expect(dependency_names).to match_array(%w(d3 d3 d3 dagre-d3 dagre-d3 express)) - expect(dependency_link_types).to match_array(%w(bundleDependencies dependencies dependencies devDependencies devDependencies peerDependencies)) + expect(dependency_names).to match_array(%w[d3 d3 d3 dagre-d3 dagre-d3 express]) + expect(dependency_link_types).to match_array(%w[bundleDependencies dependencies dependencies devDependencies devDependencies peerDependencies]) end end @@ -72,8 +72,8 @@ RSpec.describe Packages::CreateDependencyService, feature_category: :package_reg expect { subject } .to change { Packages::Dependency.count }.by(1) .and change { Packages::DependencyLink.count }.by(1) - expect(dependency_names).to match_array(%w(express)) - expect(dependency_link_types).to match_array(%w(dependencies)) + expect(dependency_names).to match_array(%w[express]) + expect(dependency_link_types).to match_array(%w[dependencies]) end end @@ -105,8 +105,8 @@ RSpec.describe Packages::CreateDependencyService, feature_category: :package_reg expect { subject } .to change { Packages::Dependency.count }.by(1) .and change { Packages::DependencyLink.count }.by(1) - expect(dependency_names).to match_array(%w(express)) - expect(dependency_link_types).to match_array(%w(dependencies)) + expect(dependency_names).to match_array(%w[express]) + expect(dependency_link_types).to match_array(%w[dependencies]) end end end diff --git a/spec/services/packages/npm/create_package_service_spec.rb b/spec/services/packages/npm/create_package_service_spec.rb index 1c935c27d7f..867dc582771 100644 --- a/spec/services/packages/npm/create_package_service_spec.rb +++ b/spec/services/packages/npm/create_package_service_spec.rb @@ -2,177 +2,179 @@ require 'spec_helper' RSpec.describe Packages::Npm::CreatePackageService, feature_category: :package_registry do - include ExclusiveLeaseHelpers - - let(:namespace) { create(:namespace) } - let(:project) { create(:project, namespace: namespace) } - let(:user) { project.owner } - let(:version) { '1.0.1' } - - let(:params) do - Gitlab::Json.parse(fixture_file('packages/npm/payload.json') - .gsub('@root/npm-test', package_name) - .gsub('1.0.1', version)).with_indifferent_access - end - - let(:package_name) { "@#{namespace.path}/my-app" } - let(:version_data) { params.dig('versions', version) } - let(:lease_key) { "packages:npm:create_package_service:packages:#{project.id}_#{package_name}_#{version}" } let(:service) { described_class.new(project, user, params) } subject { service.execute } - shared_examples 'valid package' do - it 'creates a package' do - expect { subject } - .to change { Packages::Package.count }.by(1) - .and change { Packages::Package.npm.count }.by(1) - .and change { Packages::Tag.count }.by(1) - .and change { Packages::Npm::Metadatum.count }.by(1) - end - - it_behaves_like 'assigns the package creator' do - let(:package) { subject } - end + describe '#execute' do + include ExclusiveLeaseHelpers - it { is_expected.to be_valid } + let_it_be(:namespace) { create(:namespace) } + let_it_be_with_reload(:project) { create(:project, namespace: namespace) } + let_it_be(:user) { project.owner } - it 'creates a package with name and version' do - package = subject + let(:version) { '1.0.1' } - expect(package.name).to eq(package_name) - expect(package.version).to eq(version) + let(:params) do + Gitlab::Json.parse(fixture_file('packages/npm/payload.json') + .gsub('@root/npm-test', package_name) + .gsub('1.0.1', version)).with_indifferent_access end - it { expect(subject.npm_metadatum.package_json).to eq(version_data) } + let(:package_name) { "@#{namespace.path}/my-app" } + let(:version_data) { params.dig('versions', version) } + let(:lease_key) { "packages:npm:create_package_service:packages:#{project.id}_#{package_name}_#{version}" } + + shared_examples 'valid package' do + it 'creates a package' do + expect { subject } + .to change { Packages::Package.count }.by(1) + .and change { Packages::Package.npm.count }.by(1) + .and change { Packages::Tag.count }.by(1) + .and change { Packages::Npm::Metadatum.count }.by(1) + end - it { expect(subject.name).to eq(package_name) } - it { expect(subject.version).to eq(version) } + it_behaves_like 'assigns the package creator' do + let(:package) { subject } + end - context 'with build info' do - let(:job) { create(:ci_build, user: user) } - let(:params) { super().merge(build: job) } + it { is_expected.to be_valid } - it_behaves_like 'assigns build to package' - it_behaves_like 'assigns status to package' + it 'creates a package with name and version' do + package = subject - it 'creates a package file build info' do - expect { subject }.to change { Packages::PackageFileBuildInfo.count }.by(1) + expect(package.name).to eq(package_name) + expect(package.version).to eq(version) end - end - context 'when the npm metadatum creation results in a size error' do - shared_examples 'a package json structure size too large error' do - it 'does not create the package' do - expect(Gitlab::ErrorTracking).to receive(:track_exception).with( - instance_of(ActiveRecord::RecordInvalid), - field_sizes: expected_field_sizes - ) + it { expect(subject.npm_metadatum.package_json).to eq(version_data) } - expect { subject }.to raise_error(ActiveRecord::RecordInvalid, /structure is too large/) - .and not_change { Packages::Package.count } - .and not_change { Packages::Package.npm.count } - .and not_change { Packages::Tag.count } - .and not_change { Packages::Npm::Metadatum.count } - end - end + it { expect(subject.name).to eq(package_name) } + it { expect(subject.version).to eq(version) } - context 'when some of the field sizes are above the error tracking size' do - let(:package_json) do - params[:versions][version].except(*::Packages::Npm::CreatePackageService::PACKAGE_JSON_NOT_ALLOWED_FIELDS) - end + context 'with build info' do + let_it_be(:job) { create(:ci_build, user: user) } + let(:params) { super().merge(build: job) } - # Only the fields that exceed the field size limit should be passed to error tracking - let(:expected_field_sizes) do - { - 'test' => ('test' * 10000).size, - 'field2' => ('a' * (::Packages::Npm::Metadatum::MIN_PACKAGE_JSON_FIELD_SIZE_FOR_ERROR_TRACKING + 1)).size - } - end + it_behaves_like 'assigns build to package' + it_behaves_like 'assigns status to package' - before do - params[:versions][version][:test] = 'test' * 10000 - params[:versions][version][:field1] = - 'a' * (::Packages::Npm::Metadatum::MIN_PACKAGE_JSON_FIELD_SIZE_FOR_ERROR_TRACKING - 1) - params[:versions][version][:field2] = - 'a' * (::Packages::Npm::Metadatum::MIN_PACKAGE_JSON_FIELD_SIZE_FOR_ERROR_TRACKING + 1) + it 'creates a package file build info' do + expect { subject }.to change { Packages::PackageFileBuildInfo.count }.by(1) end - - it_behaves_like 'a package json structure size too large error' end - context 'when all of the field sizes are below the error tracking size' do - let(:package_json) do - params[:versions][version].except(*::Packages::Npm::CreatePackageService::PACKAGE_JSON_NOT_ALLOWED_FIELDS) + context 'when the npm metadatum creation results in a size error' do + shared_examples 'a package json structure size too large error' do + it 'does not create the package' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with( + instance_of(ActiveRecord::RecordInvalid), + field_sizes: expected_field_sizes + ) + + expect { subject }.to raise_error(ActiveRecord::RecordInvalid, /structure is too large/) + .and not_change { Packages::Package.count } + .and not_change { Packages::Package.npm.count } + .and not_change { Packages::Tag.count } + .and not_change { Packages::Npm::Metadatum.count } + end end - let(:expected_size) { ('a' * (::Packages::Npm::Metadatum::MIN_PACKAGE_JSON_FIELD_SIZE_FOR_ERROR_TRACKING - 1)).size } - # Only the five largest fields should be passed to error tracking - let(:expected_field_sizes) do - { - 'field1' => expected_size, - 'field2' => expected_size, - 'field3' => expected_size, - 'field4' => expected_size, - 'field5' => expected_size - } - end + context 'when some of the field sizes are above the error tracking size' do + let(:package_json) do + params[:versions][version].except(*::Packages::Npm::CreatePackageService::PACKAGE_JSON_NOT_ALLOWED_FIELDS) + end - before do - 5.times do |i| - params[:versions][version]["field#{i + 1}"] = + # Only the fields that exceed the field size limit should be passed to error tracking + let(:expected_field_sizes) do + { + 'test' => ('test' * 10000).size, + 'field2' => ('a' * (::Packages::Npm::Metadatum::MIN_PACKAGE_JSON_FIELD_SIZE_FOR_ERROR_TRACKING + 1)).size + } + end + + before do + params[:versions][version][:test] = 'test' * 10000 + params[:versions][version][:field1] = 'a' * (::Packages::Npm::Metadatum::MIN_PACKAGE_JSON_FIELD_SIZE_FOR_ERROR_TRACKING - 1) + params[:versions][version][:field2] = + 'a' * (::Packages::Npm::Metadatum::MIN_PACKAGE_JSON_FIELD_SIZE_FOR_ERROR_TRACKING + 1) end + + it_behaves_like 'a package json structure size too large error' end - it_behaves_like 'a package json structure size too large error' - end - end + context 'when all of the field sizes are below the error tracking size' do + let(:package_json) do + params[:versions][version].except(*::Packages::Npm::CreatePackageService::PACKAGE_JSON_NOT_ALLOWED_FIELDS) + end - context 'when the npm metadatum creation results in a different error' do - it 'does not track the error' do - error_message = 'boom' - invalid_npm_metadatum_error = ActiveRecord::RecordInvalid.new( - build(:npm_metadatum).tap do |metadatum| - metadatum.errors.add(:base, error_message) + let(:expected_size) { ('a' * (::Packages::Npm::Metadatum::MIN_PACKAGE_JSON_FIELD_SIZE_FOR_ERROR_TRACKING - 1)).size } + # Only the five largest fields should be passed to error tracking + let(:expected_field_sizes) do + { + 'field1' => expected_size, + 'field2' => expected_size, + 'field3' => expected_size, + 'field4' => expected_size, + 'field5' => expected_size + } end - ) - allow_next_instance_of(::Packages::Package) do |package| - allow(package).to receive(:create_npm_metadatum!).and_raise(invalid_npm_metadatum_error) + before do + 5.times do |i| + params[:versions][version]["field#{i + 1}"] = + 'a' * (::Packages::Npm::Metadatum::MIN_PACKAGE_JSON_FIELD_SIZE_FOR_ERROR_TRACKING - 1) + end + end + + it_behaves_like 'a package json structure size too large error' end + end - expect(Gitlab::ErrorTracking).not_to receive(:track_exception) + context 'when the npm metadatum creation results in a different error' do + it 'does not track the error' do + error_message = 'boom' + invalid_npm_metadatum_error = ActiveRecord::RecordInvalid.new( + build(:npm_metadatum).tap do |metadatum| + metadatum.errors.add(:base, error_message) + end + ) - expect { subject }.to raise_error(ActiveRecord::RecordInvalid, /#{error_message}/) - end - end + allow_next_instance_of(::Packages::Package) do |package| + allow(package).to receive(:create_npm_metadatum!).and_raise(invalid_npm_metadatum_error) + end - described_class::PACKAGE_JSON_NOT_ALLOWED_FIELDS.each do |field| - context "with not allowed #{field} field" do - before do - params[:versions][version][field] = 'test' + expect(Gitlab::ErrorTracking).not_to receive(:track_exception) + + expect { subject }.to raise_error(ActiveRecord::RecordInvalid, /#{error_message}/) end + end - it 'is persisted without the field' do - expect { subject } - .to change { Packages::Package.count }.by(1) - .and change { Packages::Package.npm.count }.by(1) - .and change { Packages::Tag.count }.by(1) - .and change { Packages::Npm::Metadatum.count }.by(1) - expect(subject.npm_metadatum.package_json[field]).to be_blank + described_class::PACKAGE_JSON_NOT_ALLOWED_FIELDS.each do |field| + context "with not allowed #{field} field" do + before do + params[:versions][version][field] = 'test' + end + + it 'is persisted without the field' do + expect { subject } + .to change { Packages::Package.count }.by(1) + .and change { Packages::Package.npm.count }.by(1) + .and change { Packages::Tag.count }.by(1) + .and change { Packages::Npm::Metadatum.count }.by(1) + expect(subject.npm_metadatum.package_json[field]).to be_blank + end end end end - end - describe '#execute' do context 'scoped package' do it_behaves_like 'valid package' end context 'when user is no project member' do - let(:user) { create(:user) } + let_it_be(:user) { create(:user) } it_behaves_like 'valid package' end @@ -320,13 +322,111 @@ 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 many of the same packages are created at the same time', :delete do - it 'only creates one package' do - expect { create_packages(project, user, params) }.to change { Packages::Package.count }.by(1) + context 'when feature flag :packages_package_protection is disabled' do + let_it_be_with_reload(:package_protection_rule) { create(:package_protection_rule, package_type: :npm, project: project) } + + before do + stub_feature_flags(packages_protected_packages: false) + end + + context 'with matching package protection rule for all roles' do + using RSpec::Parameterized::TableSyntax + + let(:package_name_pattern_no_match) { "#{package_name}_no_match" } + + where(:package_name_pattern, :push_protected_up_to_access_level) do + ref(:package_name) | :developer + ref(:package_name) | :owner + ref(:package_name_pattern_no_match) | :developer + ref(:package_name_pattern_no_match) | :owner + end + + with_them do + before do + package_protection_rule.update!(package_name_pattern: package_name_pattern, push_protected_up_to_access_level: push_protected_up_to_access_level) + end + + it_behaves_like 'valid package' + end + end + end + + context 'with package protection rule for different roles and package_name_patterns' do + using RSpec::Parameterized::TableSyntax + + let_it_be_with_reload(:package_protection_rule) { create(:package_protection_rule, package_type: :npm, project: project) } + let_it_be(:project_developer) { create(:user).tap { |u| project.add_developer(u) } } + let_it_be(:project_maintainer) { create(:user).tap { |u| project.add_maintainer(u) } } + + let(:project_owner) { project.owner } + let(:package_name_pattern_no_match) { "#{package_name}_no_match" } + + let(:service) { described_class.new(project, current_user, params) } + + shared_examples 'protected package' do + it { is_expected.to include http_status: 403, message: 'Package protected.' } + + it 'does not create any npm-related package records' do + expect { subject } + .to not_change { Packages::Package.count } + .and not_change { Packages::Package.npm.count } + .and not_change { Packages::Tag.count } + .and not_change { Packages::Npm::Metadatum.count } + end + end + + where(:package_name_pattern, :push_protected_up_to_access_level, :current_user, :shared_examples_name) do + ref(:package_name) | :developer | ref(:project_developer) | 'protected package' + ref(:package_name) | :developer | ref(:project_owner) | 'valid package' + ref(:package_name) | :maintainer | ref(:project_maintainer) | 'protected package' + ref(:package_name) | :owner | ref(:project_owner) | 'protected package' + ref(:package_name_pattern_no_match) | :developer | ref(:project_owner) | 'valid package' + ref(:package_name_pattern_no_match) | :owner | ref(:project_owner) | 'valid package' + end + + with_them do + before do + package_protection_rule.update!(package_name_pattern: package_name_pattern, push_protected_up_to_access_level: push_protected_up_to_access_level) + end + + it_behaves_like params[:shared_examples_name] + end + end + + describe '#lease_key' do + subject { service.send(:lease_key) } + + it 'returns an unique key' do + is_expected.to eq lease_key end end + end - context 'when many packages with different versions are created at the same time', :delete do + context 'when many of the same packages are created at the same time', :delete do + let(:namespace) { create(:namespace) } + let(:project) { create(:project, namespace: namespace) } + let(:user) { project.owner } + + let(:version) { '1.0.1' } + + let(:params) do + Gitlab::Json.parse( + fixture_file('packages/npm/payload.json') + .gsub('@root/npm-test', package_name) + .gsub('1.0.1', version) + ).with_indifferent_access + end + + let(:package_name) { "@#{namespace.path}/my-app" } + let(:service) { described_class.new(project, user, params) } + + subject { service.execute } + + it 'only creates one package' do + expect { create_packages(project, user, params) }.to change { Packages::Package.count }.by(1) + end + + context 'with different versions' do it 'creates all packages' do expect { create_packages_with_versions(project, user, params) }.to change { Packages::Package.count }.by(5) end @@ -367,12 +467,4 @@ RSpec.describe Packages::Npm::CreatePackageService, feature_category: :package_r threads.each(&:join) end end - - describe '#lease_key' do - subject { service.send(:lease_key) } - - it 'returns an unique key' do - is_expected.to eq lease_key - end - end end diff --git a/spec/services/packages/npm/generate_metadata_service_spec.rb b/spec/services/packages/npm/generate_metadata_service_spec.rb index d8e794405e6..f5d7f13d22c 100644 --- a/spec/services/packages/npm/generate_metadata_service_spec.rb +++ b/spec/services/packages/npm/generate_metadata_service_spec.rb @@ -44,7 +44,7 @@ RSpec.describe ::Packages::Npm::GenerateMetadataService, feature_category: :pack with_them do if params[:has_dependencies] - ::Packages::DependencyLink.dependency_types.each_key do |dependency_type| # rubocop:disable RSpec/UselessDynamicDefinition + ::Packages::DependencyLink.dependency_types.each_key do |dependency_type| # rubocop:disable RSpec/UselessDynamicDefinition -- `dependency_type` used in `let_it_be` let_it_be("package_dependency_link_for_#{dependency_type}") do create(:packages_dependency_link, package: package1, dependency_type: dependency_type) end diff --git a/spec/services/packages/nuget/check_duplicates_service_spec.rb b/spec/services/packages/nuget/check_duplicates_service_spec.rb index 9675aa5f5e2..6274036800a 100644 --- a/spec/services/packages/nuget/check_duplicates_service_spec.rb +++ b/spec/services/packages/nuget/check_duplicates_service_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' RSpec.describe Packages::Nuget::CheckDuplicatesService, feature_category: :package_registry do - include PackagesManagerApiSpecHelpers - let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } let_it_be(:file_name) { 'package.nupkg' } @@ -12,7 +10,7 @@ RSpec.describe Packages::Nuget::CheckDuplicatesService, feature_category: :packa let(:params) do { file_name: file_name, - file: temp_file(file_name) + file: File.open(expand_fixture_path('packages/nuget/package.nupkg')) } end diff --git a/spec/services/packages/nuget/create_dependency_service_spec.rb b/spec/services/packages/nuget/create_dependency_service_spec.rb index 10daec8b871..7e14779cb92 100644 --- a/spec/services/packages/nuget/create_dependency_service_spec.rb +++ b/spec/services/packages/nuget/create_dependency_service_spec.rb @@ -41,12 +41,12 @@ RSpec.describe Packages::Nuget::CreateDependencyService, feature_category: :pack subject { service.execute } - it_behaves_like 'creating dependencies, links and nuget metadata for', %w(Castle.Core Moqi Newtonsoft.Json Test.Dependency), 4, 4 + it_behaves_like 'creating dependencies, links and nuget metadata for', %w[Castle.Core Moqi Newtonsoft.Json Test.Dependency], 4, 4 context 'with existing dependencies' do let_it_be(:exisiting_dependency) { create(:packages_dependency, name: 'Moqi', version_pattern: '2.5.6') } - it_behaves_like 'creating dependencies, links and nuget metadata for', %w(Castle.Core Moqi Newtonsoft.Json Test.Dependency), 3, 4 + it_behaves_like 'creating dependencies, links and nuget metadata for', %w[Castle.Core Moqi Newtonsoft.Json Test.Dependency], 3, 4 end context 'with dependencies with no target framework' do @@ -59,7 +59,7 @@ RSpec.describe Packages::Nuget::CreateDependencyService, feature_category: :pack ] end - it_behaves_like 'creating dependencies, links and nuget metadata for', %w(Castle.Core Moqi Newtonsoft.Json Test.Dependency), 4, 4 + it_behaves_like 'creating dependencies, links and nuget metadata for', %w[Castle.Core Moqi Newtonsoft.Json Test.Dependency], 4, 4 end context 'with empty dependencies' do diff --git a/spec/services/packages/nuget/extract_metadata_file_service_spec.rb b/spec/services/packages/nuget/extract_metadata_file_service_spec.rb index 4c761826b53..ac5749c2dc4 100644 --- a/spec/services/packages/nuget/extract_metadata_file_service_spec.rb +++ b/spec/services/packages/nuget/extract_metadata_file_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Packages::Nuget::ExtractMetadataFileService, feature_category: :package_registry do + include PackagesManagerApiSpecHelpers + let_it_be(:package_file) { build(:package_file, :nuget) } let_it_be(:package_zip_file) { Zip::File.new(package_file.file) } @@ -38,6 +40,18 @@ RSpec.describe Packages::Nuget::ExtractMetadataFileService, feature_category: :p it 'returns the nuspec file content' do expect(subject.payload.squish).to include(expected_metadata) end + + context 'with InputStream zip' do + let(:package_zip_file) do + Zip::InputStream.open( + temp_file('package.nupkg', content: File.open(package_file.file.path)) + ) + end + + it 'returns the nuspec file content' do + expect(subject.payload.squish).to include(expected_metadata) + end + end end context 'without the nuspec file' do diff --git a/spec/services/packages/nuget/metadata_extraction_service_spec.rb b/spec/services/packages/nuget/metadata_extraction_service_spec.rb index 81a4e4a430b..46d5449d52b 100644 --- a/spec/services/packages/nuget/metadata_extraction_service_spec.rb +++ b/spec/services/packages/nuget/metadata_extraction_service_spec.rb @@ -4,7 +4,8 @@ require 'spec_helper' RSpec.describe Packages::Nuget::MetadataExtractionService, feature_category: :package_registry do let_it_be(:package_file) { build(:package_file, :nuget) } - let(:service) { described_class.new(package_file) } + let(:package_zip_file) { Zip::File.new(package_file.file) } + let(:service) { described_class.new(package_zip_file) } describe '#execute' do subject { service.execute } @@ -50,8 +51,8 @@ RSpec.describe Packages::Nuget::MetadataExtractionService, feature_category: :pa end it 'calls the necessary services and executes the metadata extraction' do - expect_next_instance_of(Packages::Nuget::ProcessPackageFileService, package_file) do |service| - expect(service).to receive(:execute).and_return(ServiceResponse.success(payload: { nuspec_file_content: nuspec_file_content })) + expect_next_instance_of(Packages::Nuget::ExtractMetadataFileService, package_zip_file) do |service| + expect(service).to receive(:execute).and_return(ServiceResponse.success(payload: nuspec_file_content)) end expect_next_instance_of(Packages::Nuget::ExtractMetadataContentService, nuspec_file_content) do |service| diff --git a/spec/services/packages/nuget/process_package_file_service_spec.rb b/spec/services/packages/nuget/process_package_file_service_spec.rb index cdeb5b32737..70e8bcb8c5c 100644 --- a/spec/services/packages/nuget/process_package_file_service_spec.rb +++ b/spec/services/packages/nuget/process_package_file_service_spec.rb @@ -14,25 +14,14 @@ RSpec.describe Packages::Nuget::ProcessPackageFileService, feature_category: :pa it { expect { subject }.to raise_error(described_class::ExtractionError, error_message) } end - shared_examples 'not creating a symbol file' do - it 'does not call the CreateSymbolFilesService' do - expect(Packages::Nuget::Symbols::CreateSymbolFilesService).not_to receive(:new) - - expect(subject).to be_success - end - end - context 'with valid package file' do - it 'calls the ExtractMetadataFileService' do - expect_next_instance_of(Packages::Nuget::ExtractMetadataFileService, instance_of(Zip::File)) do |service| - expect(service).to receive(:execute) do - instance_double(ServiceResponse).tap do |response| - expect(response).to receive(:payload).and_return(instance_of(String)) - end - end + it 'calls the UpdatePackageFromMetadataService' do + expect_next_instance_of(Packages::Nuget::UpdatePackageFromMetadataService, package_file, + instance_of(Zip::File)) do |service| + expect(service).to receive(:execute) end - expect(subject).to be_success + subject end end @@ -59,25 +48,5 @@ RSpec.describe Packages::Nuget::ProcessPackageFileService, feature_category: :pa it_behaves_like 'raises an error', 'invalid package file' end - - context 'with a symbol package file' do - let(:package_file) { build(:package_file, :snupkg) } - - it 'calls the CreateSymbolFilesService' do - expect_next_instance_of( - Packages::Nuget::Symbols::CreateSymbolFilesService, package_file.package, instance_of(Zip::File) - ) do |service| - expect(service).to receive(:execute) - end - - expect(subject).to be_success - end - end - - context 'with a non symbol package file' do - let(:package_file) { build(:package_file, :nuget) } - - it_behaves_like 'not creating a symbol file' - end end end diff --git a/spec/services/packages/nuget/symbols/create_symbol_files_service_spec.rb b/spec/services/packages/nuget/symbols/create_symbol_files_service_spec.rb index 97bfc3e06a8..96cc46884af 100644 --- a/spec/services/packages/nuget/symbols/create_symbol_files_service_spec.rb +++ b/spec/services/packages/nuget/symbols/create_symbol_files_service_spec.rb @@ -15,9 +15,9 @@ RSpec.describe Packages::Nuget::Symbols::CreateSymbolFilesService, feature_categ describe '#execute' do subject { service.execute } - shared_examples 'logs an error' do |error_class| - it 'logs an error' do - expect(Gitlab::ErrorTracking).to receive(:log_exception).with( + shared_examples 'logging an error' do |error_class| + it 'logs the error' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with( an_instance_of(error_class), class: described_class.name, package_id: package.id @@ -29,8 +29,8 @@ RSpec.describe Packages::Nuget::Symbols::CreateSymbolFilesService, feature_categ context 'when symbol files are found' do it 'creates a symbol record and extracts the signature' do - expect_next_instance_of(Packages::Nuget::Symbols::ExtractSymbolSignatureService, - instance_of(String)) do |service| + expect_next_instance_of(Packages::Nuget::Symbols::ExtractSignatureAndChecksumService, + instance_of(File)) do |service| expect(service).to receive(:execute).and_call_original end @@ -47,13 +47,13 @@ RSpec.describe Packages::Nuget::Symbols::CreateSymbolFilesService, feature_categ expect { subject }.not_to change { package.nuget_symbols.count } end - it_behaves_like 'logs an error', described_class::ExtractionError + it_behaves_like 'logging an error', described_class::ExtractionError end - context 'when creating a symbol record without a signature' do + context 'without a signature' do before do - allow_next_instance_of(Packages::Nuget::Symbols::ExtractSymbolSignatureService) do |instance| - allow(instance).to receive(:execute).and_return(ServiceResponse.success(payload: nil)) + allow_next_instance_of(Packages::Nuget::Symbols::ExtractSignatureAndChecksumService) do |instance| + allow(instance).to receive(:execute).and_return(ServiceResponse.success(payload: { signature: nil })) end end @@ -64,12 +64,28 @@ RSpec.describe Packages::Nuget::Symbols::CreateSymbolFilesService, feature_categ end end - context 'when creating duplicate symbol records' do + context 'without a checksum' do + before do + allow_next_instance_of(Packages::Nuget::Symbols::ExtractSignatureAndChecksumService) do |instance| + allow(instance).to receive(:execute).and_return(ServiceResponse.success(payload: { checksum: nil })) + end + end + + it 'does not call create! on the symbol record' do + expect(::Packages::Nuget::Symbol).not_to receive(:create!) + + subject + end + end + + context 'with existing duplicate symbol records' do let_it_be(:symbol) { create(:nuget_symbol, package: package) } before do - allow_next_instance_of(Packages::Nuget::Symbols::ExtractSymbolSignatureService) do |instance| - allow(instance).to receive(:execute).and_return(ServiceResponse.success(payload: symbol.signature)) + allow_next_instance_of(Packages::Nuget::Symbols::ExtractSignatureAndChecksumService) do |instance| + allow(instance).to receive(:execute).and_return( + ServiceResponse.success(payload: { signature: symbol.signature, checksum: symbol.file_sha256 }) + ) end end @@ -77,7 +93,7 @@ RSpec.describe Packages::Nuget::Symbols::CreateSymbolFilesService, feature_categ expect { subject }.not_to change { package.nuget_symbols.count } end - it_behaves_like 'logs an error', ActiveRecord::RecordInvalid + it_behaves_like 'logging an error', ActiveRecord::RecordInvalid end context 'when a symbol file has the wrong entry size' do @@ -87,7 +103,7 @@ RSpec.describe Packages::Nuget::Symbols::CreateSymbolFilesService, feature_categ end end - it_behaves_like 'logs an error', described_class::ExtractionError + it_behaves_like 'logging an error', described_class::ExtractionError end context 'when a symbol file has the wrong entry name' do @@ -97,7 +113,7 @@ RSpec.describe Packages::Nuget::Symbols::CreateSymbolFilesService, feature_categ end end - it_behaves_like 'logs an error', described_class::ExtractionError + it_behaves_like 'logging an error', described_class::ExtractionError end end end diff --git a/spec/services/packages/nuget/symbols/extract_signature_and_checksum_service_spec.rb b/spec/services/packages/nuget/symbols/extract_signature_and_checksum_service_spec.rb new file mode 100644 index 00000000000..210b92dfce5 --- /dev/null +++ b/spec/services/packages/nuget/symbols/extract_signature_and_checksum_service_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Nuget::Symbols::ExtractSignatureAndChecksumService, feature_category: :package_registry do + let_it_be(:symbol_file_path) { expand_fixture_path('packages/nuget/symbol/package.pdb') } + let(:symbol_file) { File.new(symbol_file_path) } + + let(:service) { described_class.new(symbol_file) } + + after do + symbol_file.close + end + + describe '#execute' do + subject { service.execute } + + context 'with a valid symbol file' do + it 'returns the signature and checksum' do + payload = subject.payload + + expect(payload[:signature]).to eq('b91a152048fc4b3883bf3cf73fbc03f1FFFFFFFF') + expect(payload[:checksum]).to eq('20151ab9fc48384b83bf3cf73fbc03f1d49166cc356139845f290d1d315256c0') + end + + it 'reads the file in chunks' do + expect(symbol_file).to receive(:read).with(described_class::GUID_CHUNK_SIZE).and_call_original + expect(symbol_file).to receive(:read).with(described_class::SHA_CHUNK_SIZE, instance_of(String)) + .at_least(:once).and_call_original + + subject + end + end + + context 'with an invalid symbol file' do + before do + allow(symbol_file).to receive(:read).and_return('invalid') + end + + it 'returns an error' do + expect(subject).to be_error + expect(subject.message).to eq('Could not find the signature in the symbol file') + end + end + end +end diff --git a/spec/services/packages/nuget/symbols/extract_symbol_signature_service_spec.rb b/spec/services/packages/nuget/symbols/extract_symbol_signature_service_spec.rb deleted file mode 100644 index 87b0d00a0a7..00000000000 --- a/spec/services/packages/nuget/symbols/extract_symbol_signature_service_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Packages::Nuget::Symbols::ExtractSymbolSignatureService, feature_category: :package_registry do - let_it_be(:symbol_file) { fixture_file('packages/nuget/symbol/package.pdb') } - - let(:service) { described_class.new(symbol_file) } - - describe '#execute' do - subject { service.execute } - - context 'with a valid symbol file' do - it { expect(subject.payload).to eq('b91a152048fc4b3883bf3cf73fbc03f1FFFFFFFF') } - end - - context 'with corrupted data' do - let(:symbol_file) { 'corrupted data' } - - it { expect(subject).to be_error } - end - end -end diff --git a/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb b/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb index cb70176ee61..0e19f2ac3f9 100644 --- a/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb +++ b/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb @@ -7,7 +7,8 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ let!(:package) { create(:nuget_package, :processing, :with_symbol_package, :with_build) } let(:package_file) { package.package_files.first } - let(:service) { described_class.new(package_file) } + let(:package_zip_file) { Zip::File.new(package_file.file) } + let(:service) { described_class.new(package_file, package_zip_file) } let(:package_name) { 'DummyProject.DummyPackage' } let(:package_version) { '1.0.0' } let(:package_file_name) { 'dummyproject.dummypackage.1.0.0.nupkg' } @@ -127,7 +128,7 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ context 'with a nuspec file with metadata' do let(:nuspec_filepath) { 'packages/nuget/with_metadata.nuspec' } - let(:expected_tags) { %w(foo bar test tag1 tag2 tag3 tag4 tag5) } + let(:expected_tags) { %w[foo bar test tag1 tag2 tag3 tag4 tag5] } before do allow_next_instance_of(Packages::Nuget::MetadataExtractionService) do |service| @@ -182,13 +183,15 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ context 'without authors or description' do %i[authors description].each do |property| - let(:metadata) { { package_name: package_name, package_version: package_version, property => nil } } + context "for #{property}" do + let(:metadata) { { package_name: package_name, package_version: package_version, property => nil } } - before do - allow(service).to receive(:metadata).and_return(metadata) - end + before do + allow(service).to receive(:metadata).and_return(metadata) + end - it_behaves_like 'raising an', described_class::InvalidMetadataError, with_message: described_class::INVALID_METADATA_ERROR_MESSAGE + it_behaves_like 'raising an', described_class::InvalidMetadataError, with_message: described_class::INVALID_METADATA_ERROR_MESSAGE + end end end end @@ -245,11 +248,20 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ context 'with existing package' do let!(:existing_package) { create(:nuget_package, project: package.project, name: package_name, version: package_version) } let(:package_id) { existing_package.id } + let(:package_zip_file) do + Zip::File.open(package_file.file.path) do |zipfile| + zipfile.add('package.pdb', expand_fixture_path('packages/nuget/symbol/package.pdb')) + zipfile + end + end it 'link existing package and updates package file', :aggregate_failures do expect(service).to receive(:try_obtain_lease).and_call_original expect(::Packages::Nuget::SyncMetadatumService).not_to receive(:new) expect(::Packages::UpdateTagsService).not_to receive(:new) + expect_next_instance_of(Packages::Nuget::Symbols::CreateSymbolFilesService, existing_package, package_zip_file) do |service| + expect(service).to receive(:execute).and_call_original + end expect { subject } .to change { ::Packages::Package.count }.by(-1) @@ -257,20 +269,11 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ .and change { Packages::DependencyLink.count }.by(0) .and change { Packages::Nuget::DependencyLinkMetadatum.count }.by(0) .and change { ::Packages::Nuget::Metadatum.count }.by(0) + .and change { existing_package.nuget_symbols.count }.by(1) expect(package_file.reload.file_name).to eq(package_file_name) expect(package_file.package).to eq(existing_package) end - context 'with packages_nuget_symbols records' do - before do - create_list(:nuget_symbol, 2, package: package) - end - - it 'links the symbol records to the existing package' do - expect { subject }.to change { existing_package.nuget_symbols.count }.by(2) - end - end - it_behaves_like 'taking the lease' it_behaves_like 'not updating the package if the lease is taken' diff --git a/spec/services/packages/protection/create_rule_service_spec.rb b/spec/services/packages/protection/create_rule_service_spec.rb index 67835479473..ed8d21f4344 100644 --- a/spec/services/packages/protection/create_rule_service_spec.rb +++ b/spec/services/packages/protection/create_rule_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Packages::Protection::CreateRuleService, '#execute', feature_category: :environment_management do +RSpec.describe Packages::Protection::CreateRuleService, '#execute', feature_category: :package_registry do let_it_be(:project) { create(:project, :repository) } let_it_be(:maintainer) { create(:user).tap { |u| project.add_maintainer(u) } } diff --git a/spec/services/packages/protection/delete_rule_service_spec.rb b/spec/services/packages/protection/delete_rule_service_spec.rb new file mode 100644 index 00000000000..d64609d4df1 --- /dev/null +++ b/spec/services/packages/protection/delete_rule_service_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Protection::DeleteRuleService, '#execute', feature_category: :package_registry do + let_it_be(:project) { create(:project) } + let_it_be(:current_user) { create(:user, maintainer_projects: [project]) } + let_it_be_with_refind(:package_protection_rule) { create(:package_protection_rule, project: project) } + + subject { described_class.new(package_protection_rule, current_user: current_user).execute } + + shared_examples 'a successful service response' do + it { is_expected.to be_success } + + it { + is_expected.to have_attributes( + errors: be_blank, + message: be_blank, + payload: { package_protection_rule: package_protection_rule } + ) + } + + it { subject.tap { expect { package_protection_rule.reload }.to raise_error ActiveRecord::RecordNotFound } } + end + + shared_examples 'an erroneous service response' do + it { is_expected.to be_error } + it { is_expected.to have_attributes(message: be_present, payload: { package_protection_rule: be_blank }) } + + it do + expect { subject }.not_to change { Packages::Protection::Rule.count } + + expect { package_protection_rule.reload }.not_to raise_error + end + end + + it_behaves_like 'a successful service response' + + it 'deletes the package protection rule in the database' do + expect { subject } + .to change { project.reload.package_protection_rules }.from([package_protection_rule]).to([]) + .and change { ::Packages::Protection::Rule.count }.from(1).to(0) + end + + context 'with deleted package protection rule' do + let!(:package_protection_rule) do + create(:package_protection_rule, project: project, package_name_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(package_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 package protection rule/ } + end + end + + context 'without package protection rule' do + let(:package_protection_rule) { nil } + + it { expect { subject }.to raise_error(ArgumentError) } + end + + context 'without current_user' do + let(:current_user) { nil } + let(:package_protection_rule) { build_stubbed(:package_protection_rule, project: project) } + + it { expect { subject }.to raise_error(ArgumentError) } + end +end diff --git a/spec/services/packages/pypi/create_package_service_spec.rb b/spec/services/packages/pypi/create_package_service_spec.rb index 0d278e32e89..abff91d1878 100644 --- a/spec/services/packages/pypi/create_package_service_spec.rb +++ b/spec/services/packages/pypi/create_package_service_spec.rb @@ -69,6 +69,30 @@ RSpec.describe Packages::Pypi::CreatePackageService, :aggregate_failures, featur end end + context 'with additional metadata' do + before do + params.merge!( + metadata_version: '2.3', + author_email: 'cschultz@example.com, snoopy@peanuts.com', + description: 'Example description', + description_content_type: 'text/plain', + summary: 'A module for collecting votes from beagles.', + keywords: 'dog,puppy,voting,election' + ) + end + + it 'creates the package' do + expect { subject }.to change { Packages::Package.pypi.count }.by(1) + + expect(created_package.pypi_metadatum.metadata_version).to eq('2.3') + expect(created_package.pypi_metadatum.author_email).to eq('cschultz@example.com, snoopy@peanuts.com') + expect(created_package.pypi_metadatum.description).to eq('Example description') + expect(created_package.pypi_metadatum.description_content_type).to eq('text/plain') + expect(created_package.pypi_metadatum.summary).to eq('A module for collecting votes from beagles.') + expect(created_package.pypi_metadatum.keywords).to eq('dog,puppy,voting,election') + end + end + context 'with an invalid metadata' do let(:requires_python) { 'x' * 256 } diff --git a/spec/services/packages/update_tags_service_spec.rb b/spec/services/packages/update_tags_service_spec.rb index d8f572fff32..ec4d68ba5a0 100644 --- a/spec/services/packages/update_tags_service_spec.rb +++ b/spec/services/packages/update_tags_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Packages::UpdateTagsService, feature_category: :package_registry do let_it_be(:package, reload: true) { create(:nuget_package) } - let(:tags) { %w(test-tag tag1 tag2 tag3) } + let(:tags) { %w[test-tag tag1 tag2 tag3] } let(:service) { described_class.new(package, tags) } describe '#execute' do diff --git a/spec/services/pages/delete_service_spec.rb b/spec/services/pages/delete_service_spec.rb index 488f29f6b7e..86b1bd5bde2 100644 --- a/spec/services/pages/delete_service_spec.rb +++ b/spec/services/pages/delete_service_spec.rb @@ -8,14 +8,12 @@ RSpec.describe Pages::DeleteService, feature_category: :pages do let(:project) { create(:project, path: "my.project") } let(:service) { described_class.new(project, admin) } - before do - project.mark_pages_as_deployed - end - it 'marks pages as not deployed' do - expect do - service.execute - end.to change { project.reload.pages_deployed? }.from(true).to(false) + create(:pages_deployment, project: project) + + expect { service.execute } + .to change { project.reload.pages_deployed? } + .from(true).to(false) end it 'deletes all domains' do @@ -29,9 +27,8 @@ RSpec.describe Pages::DeleteService, feature_category: :pages do end it 'schedules a destruction of pages deployments' do - expect(DestroyPagesDeploymentsWorker).to( - receive(:perform_async).with(project.id) - ) + expect(DestroyPagesDeploymentsWorker) + .to(receive(:perform_async).with(project.id)) service.execute end @@ -39,9 +36,8 @@ RSpec.describe Pages::DeleteService, feature_category: :pages do it 'removes pages deployments', :sidekiq_inline do create(:pages_deployment, project: project) - expect do - service.execute - end.to change { PagesDeployment.count }.by(-1) + expect { service.execute } + .to change { PagesDeployment.count }.by(-1) end it 'publishes a ProjectDeleted event with project id and namespace id' do 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 7f8992e8bbc..0e46391c0ad 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 @@ -65,7 +65,7 @@ RSpec.describe PagesDomains::ObtainLetsEncryptCertificateService, feature_catego end end - %w(pending processing).each do |status| + %w[pending processing].each do |status| context "there is an order in '#{status}' status" do let(:existing_order) do create(:pages_domain_acme_order, pages_domain: pages_domain) diff --git a/spec/services/product_analytics/build_graph_service_spec.rb b/spec/services/product_analytics/build_graph_service_spec.rb index 13c7206241c..a850d69e53c 100644 --- a/spec/services/product_analytics/build_graph_service_spec.rb +++ b/spec/services/product_analytics/build_graph_service_spec.rb @@ -21,7 +21,7 @@ RSpec.describe ProductAnalytics::BuildGraphService, feature_category: :product_a it 'returns a valid graph hash' do expect(subject[:id]).to eq(:platform) - expect(subject[:keys]).to eq(%w(app mobile web)) + 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/branches_by_mode_service_spec.rb b/spec/services/projects/branches_by_mode_service_spec.rb index bfe76b34310..c87787346b9 100644 --- a/spec/services/projects/branches_by_mode_service_spec.rb +++ b/spec/services/projects/branches_by_mode_service_spec.rb @@ -50,7 +50,7 @@ RSpec.describe Projects::BranchesByModeService, feature_category: :source_code_m branches, prev_page, next_page = subject - expect(branches.map(&:name)).to match_array(%w(feature feature_conflict)) + expect(branches.map(&:name)).to match_array(%w[feature feature_conflict]) expect(next_page).to be_nil expect(prev_page).to be_nil end diff --git a/spec/services/projects/container_repository/delete_tags_service_spec.rb b/spec/services/projects/container_repository/delete_tags_service_spec.rb index 5b67d614dfb..942e244e3d2 100644 --- a/spec/services/projects/container_repository/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/delete_tags_service_spec.rb @@ -77,7 +77,7 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService, feature_categor before do expect(::Projects::ContainerRepository::Gitlab::DeleteTagsService).not_to receive(:new) expect(::Projects::ContainerRepository::ThirdParty::DeleteTagsService).not_to receive(:new) - expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name) + expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_digest) end context 'when no params are specified' do @@ -107,7 +107,7 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService, feature_categor context 'with the real service' do before do stub_delete_reference_requests(tags) - expect_delete_tag_by_names(tags) + expect_delete_tags(tags) end it { is_expected.to include(status: :success) } diff --git a/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb b/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb index 0d7d1254428..676c7ca8e99 100644 --- a/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb @@ -15,7 +15,7 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService, feature RSpec.shared_examples 'deleting tags' do it 'deletes the tags by name' do stub_delete_reference_requests(tags) - expect_delete_tag_by_names(tags) + expect_delete_tags(tags) is_expected.to eq(status: :success, deleted: tags) end @@ -59,7 +59,7 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService, feature tags.each do |tag| stub_delete_reference_requests(tag => 500) end - allow(container_repository).to receive(:delete_tag_by_name).and_return(false) + allow(container_repository).to receive(:delete_tag).and_return(false) end it 'truncates the log message' do @@ -119,7 +119,7 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService, feature let_it_be(:tags) { [] } it 'does not remove anything' do - expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name) + expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_digest) is_expected.to eq(status: :success, deleted: []) end diff --git a/spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb b/spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb index 0c297b6e1f7..d3d3f3bb7ce 100644 --- a/spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb @@ -18,7 +18,7 @@ RSpec.describe Projects::ContainerRepository::ThirdParty::DeleteTagsService, fea tags.each { |tag| stub_put_manifest_request(tag) } - expect_delete_tag_by_digest('sha256:dummy') + expect_delete_tags(['sha256:dummy']) is_expected.to eq(status: :success, deleted: tags) end @@ -92,7 +92,7 @@ RSpec.describe Projects::ContainerRepository::ThirdParty::DeleteTagsService, fea let_it_be(:tags) { [] } it 'does not remove anything' do - expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name) + expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_digest) is_expected.to eq(status: :success, deleted: []) end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index ce7e5188c7b..899ed477180 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -320,7 +320,7 @@ RSpec.describe Projects::CreateService, '#execute', feature_category: :groups_an it 'cannot create a project' do expect(project.errors.errors.length).to eq 1 - expect(project.errors.messages[:limit_reached].first).to eq(_('Personal project creation is not allowed. Please contact your administrator with questions')) + expect(project.errors.messages[:limit_reached].first).to eq(_('You cannot create projects in your personal namespace. Contact your GitLab administrator.')) end end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 0210e101e5f..a0064eadf13 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -472,6 +472,31 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi end end + context 'with related storage move records' do + context 'when project has active repository storage move records' do + let!(:project_repository_storage_move) { create(:project_repository_storage_move, :scheduled, container: project) } + + it 'does not delete the project' do + expect(destroy_project(project, user)).to be_falsey + + expect(project.delete_error).to eq "Couldn't remove the project. A project repository storage move is in progress. Try again when it's complete." + expect(project.pending_delete).to be_falsey + end + end + + context 'when project has active snippet storage move records' do + let(:project_snippet) { create(:project_snippet, project: project) } + let!(:snippet_repository_storage_move) { create(:snippet_repository_storage_move, :started, container: project_snippet) } + + it 'does not delete the project' do + expect(destroy_project(project, user)).to be_falsey + + expect(project.delete_error).to eq "Couldn't remove the project. A related snippet repository storage move is in progress. Try again when it's complete." + expect(project.pending_delete).to be_falsey + end + end + end + context 'repository removal' do describe '.trash_project_repositories!' do let(:trash_project_repositories!) { described_class.new(project, user, {}).send(:trash_project_repositories!) } diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index 4d55f310974..ceb060445ad 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -510,6 +510,26 @@ RSpec.describe Projects::ForkService, feature_category: :source_code_management end end + describe '#valid_fork_branch?' do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :small_repo, creator_id: user.id) } + let_it_be(:branch) { nil } + + subject { described_class.new(project, user).valid_fork_branch?(branch) } + + context 'when branch exists' do + let(:branch) { project.default_branch_or_main } + + it { is_expected.to be_truthy } + end + + context 'when branch does not exist' do + let(:branch) { 'branch-that-does-not-exist' } + + it { is_expected.to be_falsey } + end + end + describe '#valid_fork_target?' do let(:project) { Project.new } let(:params) { {} } diff --git a/spec/services/projects/group_links/create_service_spec.rb b/spec/services/projects/group_links/create_service_spec.rb index ca2902af472..e3f170ef3fe 100644 --- a/spec/services/projects/group_links/create_service_spec.rb +++ b/spec/services/projects/group_links/create_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Projects::GroupLinks::CreateService, '#execute', feature_category let_it_be(:user) { create :user } let_it_be(:group) { create :group } let_it_be(:project) { create(:project, namespace: create(:namespace, :with_namespace_settings)) } + let_it_be(:group_user) { create(:user).tap { |user| group.add_guest(user) } } let(:opts) do { @@ -37,67 +38,75 @@ RSpec.describe Projects::GroupLinks::CreateService, '#execute', feature_category end end - context 'when user has proper membership to share a group' do + context 'when user has proper permissions to share a project with a group' do before do group.add_guest(user) end - it_behaves_like 'shareable' - - it 'updates authorization', :sidekiq_inline do - expect { subject.execute }.to( - change { Ability.allowed?(user, :read_project, project) } - .from(false).to(true)) - end - - context 'with specialized project_authorization workers' do - let_it_be(:other_user) { create(:user) } - + context 'when the user is a MAINTAINER in the project' do before do - group.add_developer(other_user) + project.add_maintainer(user) end - it 'schedules authorization update for users with access to group' do - stub_feature_flags(do_not_run_safety_net_auth_refresh_jobs: false) - - expect(AuthorizedProjectsWorker).not_to( - receive(:bulk_perform_async) - ) - expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).to( - receive(:perform_async) - .with(project.id) - .and_call_original - ) - expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to( - receive(:bulk_perform_in).with( - 1.hour, - array_including([user.id], [other_user.id]), - batch_delay: 30.seconds, batch_size: 100 - ).and_call_original - ) - - subject.execute + it_behaves_like 'shareable' + + it 'updates authorization', :sidekiq_inline do + expect { subject.execute }.to( + change { Ability.allowed?(group_user, :read_project, project) } + .from(false).to(true)) end - end - context 'when sharing outside the hierarchy is disabled' do - let_it_be(:shared_group_parent) do - create(:group, namespace_settings: create(:namespace_settings, prevent_sharing_groups_outside_hierarchy: true)) + context 'with specialized project_authorization workers' do + let_it_be(:other_user) { create(:user) } + + before do + group.add_developer(other_user) + end + + it 'schedules authorization update for users with access to group' do + stub_feature_flags(do_not_run_safety_net_auth_refresh_jobs: false) + + expect(AuthorizedProjectsWorker).not_to( + receive(:bulk_perform_async) + ) + expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).to( + receive(:perform_async) + .with(project.id) + .and_call_original + ) + expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to( + receive(:bulk_perform_in).with( + 1.hour, + array_including([user.id], [other_user.id]), + batch_delay: 30.seconds, batch_size: 100 + ).and_call_original + ) + + subject.execute + end end - let_it_be(:project, reload: true) { create(:project, group: shared_group_parent) } + context 'when sharing outside the hierarchy is disabled' do + let_it_be(:shared_group_parent) do + create(:group, + namespace_settings: create(:namespace_settings, prevent_sharing_groups_outside_hierarchy: true) + ) + end + + let_it_be(:project, reload: true) { create(:project, group: shared_group_parent) } - it_behaves_like 'not shareable' + it_behaves_like 'not shareable' - context 'when group is inside hierarchy' do - let(:group) { create(:group, :private, parent: shared_group_parent) } + context 'when group is inside hierarchy' do + let(:group) { create(:group, :private, parent: shared_group_parent) } - it_behaves_like 'shareable' + it_behaves_like 'shareable' + end end end end - context 'when user does not have permissions for the group' do + context 'when user does not have permissions to share the project with a group' do it_behaves_like 'not shareable' end diff --git a/spec/services/projects/group_links/destroy_service_spec.rb b/spec/services/projects/group_links/destroy_service_spec.rb index 103aff8c659..0cd003f6142 100644 --- a/spec/services/projects/group_links/destroy_service_spec.rb +++ b/spec/services/projects/group_links/destroy_service_spec.rb @@ -6,83 +6,120 @@ RSpec.describe Projects::GroupLinks::DestroyService, '#execute', feature_categor let_it_be(:user) { create :user } let_it_be(:project) { create(:project, :private) } let_it_be(:group) { create(:group) } + let_it_be(:group_user) { create(:user).tap { |user| group.add_guest(user) } } - let!(:group_link) { create(:project_group_link, project: project, group: group) } + let(:group_access) { Gitlab::Access::DEVELOPER } + let!(:group_link) { create(:project_group_link, project: project, group: group, group_access: group_access) } subject { described_class.new(project, user) } - it 'removes group from project' do - expect { subject.execute(group_link) }.to change { project.project_group_links.count }.from(1).to(0) - end - - context 'project authorizations refresh' do - before do - group.add_maintainer(user) + shared_examples_for 'removes group from project' do + it 'removes group from project' do + expect { subject.execute(group_link) }.to change { project.reload.project_group_links.count }.from(1).to(0) end + end - it 'calls AuthorizedProjectUpdate::ProjectRecalculateWorker to update project authorizations' do - expect(AuthorizedProjectUpdate::ProjectRecalculateWorker) - .to receive(:perform_async).with(group_link.project.id) + shared_examples_for 'returns not_found' do + it do + expect do + result = subject.execute(group_link) - 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 - it 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations' do - stub_feature_flags(do_not_run_safety_net_auth_refresh_jobs: false) - - expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to( - receive(:bulk_perform_in).with( - 1.hour, - [[user.id]], - batch_delay: 30.seconds, batch_size: 100 - ) - ) - - subject.execute(group_link) - end + context 'if group_link is blank' do + let!(:group_link) { nil } - it 'updates project authorizations of users who had access to the project via the group share', :sidekiq_inline do - expect { subject.execute(group_link) }.to( - change { Ability.allowed?(user, :read_project, project) } - .from(true).to(false)) - end + it_behaves_like 'returns not_found' end - it 'returns false if group_link is blank' do - expect { subject.execute(nil) }.not_to change { project.project_group_links.count } + context 'if the user does not have access to destroy the link' do + it_behaves_like 'returns not_found' end - describe 'todos cleanup' do - context 'when project is private' do - it 'triggers todos cleanup' do - expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) - expect(project.private?).to be true - - subject.execute(group_link) + context 'when the user has proper permissions to remove a group-link from a project' do + context 'when the user is a MAINTAINER in the project' do + before do + project.add_maintainer(user) end - end - context 'when project is public or internal' do - shared_examples_for 'removes confidential todos' do - it 'does not trigger todos cleanup' do - expect(TodosDestroyer::ProjectPrivateWorker).not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) - expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, nil, project.id) - expect(project.private?).to be false + it_behaves_like 'removes group from project' + + context 'project authorizations refresh' do + it 'calls AuthorizedProjectUpdate::ProjectRecalculateWorker to update project authorizations' do + expect(AuthorizedProjectUpdate::ProjectRecalculateWorker) + .to receive(:perform_async).with(group_link.project.id) subject.execute(group_link) end - end - context 'when project is public' do - let(:project) { create(:project, :public) } + it 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations' do + stub_feature_flags(do_not_run_safety_net_auth_refresh_jobs: false) - it_behaves_like 'removes confidential todos' + expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to( + receive(:bulk_perform_in).with( + 1.hour, + [[group_user.id]], + batch_delay: 30.seconds, batch_size: 100 + ) + ) + + subject.execute(group_link) + end + + it 'updates project authorizations of users who had access to the project via the group share', :sidekiq_inline do + expect { subject.execute(group_link) }.to( + change { Ability.allowed?(group_user, :read_project, project) } + .from(true).to(false)) + end end - context 'when project is internal' do - let(:project) { create(:project, :public) } + describe 'todos cleanup' do + context 'when project is private' do + it 'triggers todos cleanup' do + expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) + expect(project.private?).to be true + + subject.execute(group_link) + end + end + + context 'when project is public or internal' do + shared_examples_for 'removes confidential todos' do + it 'does not trigger todos cleanup' do + expect(TodosDestroyer::ProjectPrivateWorker).not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) + expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, nil, project.id) + expect(project.private?).to be false + + subject.execute(group_link) + end + end + + context 'when project is public' do + let(:project) { create(:project, :public) } + + it_behaves_like 'removes confidential todos' + end + + context 'when project is internal' do + let(:project) { create(:project, :public) } + + it_behaves_like 'removes confidential todos' + end + end + end + end + end - it_behaves_like 'removes confidential todos' + context 'when skipping authorization' do + context 'without providing a user' do + it 'destroys the link' do + expect do + described_class.new(project, nil).execute(group_link, skip_authorization: true) + end.to change { project.reload.project_group_links.count }.by(-1) 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 f7607deef04..b02614fa062 100644 --- a/spec/services/projects/group_links/update_service_spec.rb +++ b/spec/services/projects/group_links/update_service_spec.rb @@ -6,8 +6,11 @@ RSpec.describe Projects::GroupLinks::UpdateService, '#execute', feature_category let_it_be(:user) { create :user } let_it_be(:group) { create :group } let_it_be(:project) { create :project } + let_it_be(:group_user) { create(:user).tap { |user| group.add_developer(user) } } - let!(:link) { create(:project_group_link, project: project, group: group) } + let(:group_access) { Gitlab::Access::DEVELOPER } + + let!(:link) { create(:project_group_link, project: project, group: group, group_access: group_access) } let(:expiry_date) { 1.month.from_now.to_date } let(:group_link_params) do @@ -17,60 +20,78 @@ RSpec.describe Projects::GroupLinks::UpdateService, '#execute', feature_category subject { described_class.new(link, user).execute(group_link_params) } - before do - group.add_developer(user) - end - - it 'updates existing link' do - expect(link.group_access).to eq(Gitlab::Access::DEVELOPER) - expect(link.expires_at).to be_nil - - subject - - link.reload + shared_examples_for 'returns not_found' do + it do + result = subject - expect(link.group_access).to eq(Gitlab::Access::GUEST) - expect(link.expires_at).to eq(expiry_date) - end - - context 'project authorizations update' do - it 'calls AuthorizedProjectUpdate::ProjectRecalculateWorker to update project authorizations' do - expect(AuthorizedProjectUpdate::ProjectRecalculateWorker) - .to receive(:perform_async).with(link.project.id) - - subject - end - - it 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations' do - stub_feature_flags(do_not_run_safety_net_auth_refresh_jobs: false) - - expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to( - receive(:bulk_perform_in).with( - 1.hour, - [[user.id]], - batch_delay: 30.seconds, batch_size: 100 - ) - ) - - subject - end - - it 'updates project authorizations of users who had access to the project via the group share', :sidekiq_inline do - group.add_maintainer(user) - - expect { subject }.to( - change { Ability.allowed?(user, :create_release, project) } - .from(true).to(false)) + expect(result[:status]).to eq(:error) + expect(result[:reason]).to eq(:not_found) end end - context 'with only param not requiring authorization refresh' do - let(:group_link_params) { { expires_at: Date.tomorrow } } - - it 'does not perform any project authorizations update using `AuthorizedProjectUpdate::ProjectRecalculateWorker`' do - expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).not_to receive(:perform_async) + context 'when the user does not have proper permissions to update a project group link' do + it_behaves_like 'returns not_found' + end - subject + 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 + project.add_maintainer(user) + end + + it 'updates existing link' do + expect(link.group_access).to eq(Gitlab::Access::DEVELOPER) + expect(link.expires_at).to be_nil + + subject + + link.reload + + expect(link.group_access).to eq(Gitlab::Access::GUEST) + expect(link.expires_at).to eq(expiry_date) + end + + context 'project authorizations update' do + it 'calls AuthorizedProjectUpdate::ProjectRecalculateWorker to update project authorizations' do + expect(AuthorizedProjectUpdate::ProjectRecalculateWorker) + .to receive(:perform_async).with(link.project.id) + + subject + end + + it 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker ' \ + 'with a delay to update project authorizations' do + stub_feature_flags(do_not_run_safety_net_auth_refresh_jobs: false) + + expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to( + receive(:bulk_perform_in).with( + 1.hour, + [[group_user.id]], + batch_delay: 30.seconds, batch_size: 100 + ) + ) + + subject + end + + it 'updates project authorizations of users who had access to the project via the group share', + :sidekiq_inline do + expect { subject }.to( + change { Ability.allowed?(group_user, :developer_access, project) } + .from(true).to(false)) + end + end + + context 'with only param not requiring authorization refresh' do + let(:group_link_params) { { expires_at: Date.tomorrow } } + + it 'does not perform any project authorizations update using ' \ + '`AuthorizedProjectUpdate::ProjectRecalculateWorker`' do + expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).not_to receive(:perform_async) + + subject + end + end end end 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 fb3cc9bdac9..d3f053aaedc 100644 --- a/spec/services/projects/lfs_pointers/lfs_link_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_link_service_spec.rb @@ -73,10 +73,10 @@ RSpec.describe Projects::LfsPointers::LfsLinkService, feature_category: :source_ it 'only queries for the batch that will be processed', :aggregate_failures do stub_const("#{described_class}::BATCH_SIZE", 1) - oids = %w(one two) + oids = %w[one two] - expect(LfsObject).to receive(:for_oids).with(%w(one)).once.and_call_original - expect(LfsObject).to receive(:for_oids).with(%w(two)).once.and_call_original + expect(LfsObject).to receive(:for_oids).with(%w[one]).once.and_call_original + expect(LfsObject).to receive(:for_oids).with(%w[two]).once.and_call_original subject.execute(oids) end diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb index 5f9b1a59bf9..03508a9732e 100644 --- a/spec/services/projects/operations/update_service_spec.rb +++ b/spec/services/projects/operations/update_service_spec.rb @@ -325,7 +325,7 @@ RSpec.describe Projects::Operations::UpdateService, feature_category: :groups_an expect(project_arg).to eq project expect(user_arg).to eq user expect(prometheus_attrs).to have_key('encrypted_properties') - expect(prometheus_attrs.keys).not_to include(*%w(id project_id created_at updated_at properties)) + expect(prometheus_attrs.keys).not_to include(*%w[id project_id created_at updated_at properties]) expect(prometheus_attrs['encrypted_properties']).not_to eq(prometheus_integration.encrypted_properties) end.and_call_original diff --git a/spec/services/projects/record_target_platforms_service_spec.rb b/spec/services/projects/record_target_platforms_service_spec.rb index bf87b763341..40ade386847 100644 --- a/spec/services/projects/record_target_platforms_service_spec.rb +++ b/spec/services/projects/record_target_platforms_service_spec.rb @@ -21,11 +21,11 @@ RSpec.describe Projects::RecordTargetPlatformsService, '#execute', feature_categ it 'creates a new setting record for the project', :aggregate_failures do expect { execute }.to change { ProjectSetting.count }.from(0).to(1) - expect(ProjectSetting.last.target_platforms).to match_array(%w(ios osx)) + expect(ProjectSetting.last.target_platforms).to match_array(%w[ios osx]) end it 'returns array of detected target platforms' do - expect(execute).to match_array %w(ios osx) + expect(execute).to match_array %w[ios osx] end context 'when a project has an existing setting record' do @@ -34,17 +34,17 @@ RSpec.describe Projects::RecordTargetPlatformsService, '#execute', feature_categ end context 'when target platforms changed' do - let(:saved_target_platforms) { %w(tvos) } + let(:saved_target_platforms) { %w[tvos] } it 'updates' do - expect { execute }.to change { project_setting.target_platforms }.from(%w(tvos)).to(%w(ios osx)) + expect { execute }.to change { project_setting.target_platforms }.from(%w[tvos]).to(%w[ios osx]) end - it { is_expected.to match_array %w(ios osx) } + it { is_expected.to match_array %w[ios osx] } end context 'when target platforms are the same' do - let(:saved_target_platforms) { %w(osx ios) } + let(:saved_target_platforms) { %w[osx ios] } it 'does not update' do expect { execute }.not_to change { project_setting.updated_at } diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 0ad7693a047..b5d1276988f 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -48,7 +48,7 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do deploy_status = GenericCommitStatus.last expect(deploy_status.description).not_to be_present - expect(project.pages_metadatum).to be_deployed + expect(project.pages_deployed?).to eq(true) end it_behaves_like 'old deployments' @@ -116,15 +116,14 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do it "doesn't delete artifacts after deploying" do expect(service.execute[:status]).to eq(:success) - expect(project.pages_metadatum).to be_deployed + expect(project.pages_deployed?).to eq(true) expect(build.artifacts?).to eq(true) end it 'succeeds' do - expect(project.pages_deployed?).to be_falsey - expect(service.execute[:status]).to eq(:success) - expect(project.pages_metadatum).to be_deployed - expect(project.pages_deployed?).to be_truthy + expect { expect(service.execute[:status]).to eq(:success) } + .to change { project.pages_deployed? } + .from(false).to(true) end it 'publishes a PageDeployedEvent event with project id and namespace id' do @@ -137,10 +136,10 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do expect { service.execute }.to publish_event(Pages::PageDeployedEvent).with(expected_data) end - it 'creates pages_deployment and saves it in the metadata' do - expect do - expect(service.execute[:status]).to eq(:success) - end.to change { project.pages_deployments.count }.by(1) + it 'creates pages_deployment' do + expect { expect(service.execute[:status]).to eq(:success) } + .to change { project.pages_deployments.count } + .by(1) deployment = project.pages_deployments.last @@ -148,7 +147,6 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do expect(deployment.file).to be_present expect(deployment.file_count).to eq(3) expect(deployment.file_sha256).to eq(artifacts_archive.file_sha256) - expect(project.pages_metadatum.reload.pages_deployment_id).to eq(deployment.id) expect(deployment.ci_build_id).to eq(build.id) expect(deployment.root_directory).to be_nil end @@ -157,11 +155,9 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do project.pages_metadatum.destroy! project.reload - expect do - expect(service.execute[:status]).to eq(:success) - end.to change { project.pages_deployments.count }.by(1) - - expect(project.pages_metadatum.reload.pages_deployment).to eq(project.pages_deployments.last) + expect { expect(service.execute[:status]).to eq(:success) } + .to change { project.pages_deployments.count } + .by(1) end context 'when archive does not have pages directory' do @@ -171,7 +167,10 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do it 'returns an error' do expect(service.execute[:status]).not_to eq(:success) - expect(GenericCommitStatus.last.description).to eq("Error: You need to either include a `public/` folder in your artifacts, or specify which one to use for Pages using `publish` in `.gitlab-ci.yml`") + expect(GenericCommitStatus.last.description) + .to eq( + "Error: You need to either include a `public/` folder in your artifacts, " \ + "or specify which one to use for Pages using `publish` in `.gitlab-ci.yml`") end end @@ -196,7 +195,10 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do it 'returns an error' do expect(service.execute[:status]).not_to eq(:success) - expect(GenericCommitStatus.last.description).to eq("Error: You need to either include a `public/` folder in your artifacts, or specify which one to use for Pages using `publish` in `.gitlab-ci.yml`") + expect(GenericCommitStatus.last.description) + .to eq( + "Error: You need to either include a `public/` folder in your artifacts, " \ + "or specify which one to use for Pages using `publish` in `.gitlab-ci.yml`") end end @@ -208,7 +210,10 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do it 'returns an error' do expect(service.execute[:status]).not_to eq(:success) - expect(GenericCommitStatus.last.description).to eq("Error: You need to either include a `public/` folder in your artifacts, or specify which one to use for Pages using `publish` in `.gitlab-ci.yml`") + expect(GenericCommitStatus.last.description) + .to eq( + "Error: You need to either include a `public/` folder in your artifacts, " \ + "or specify which one to use for Pages using `publish` in `.gitlab-ci.yml`") end end end @@ -223,7 +228,8 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do expect(service.execute[:status]).not_to eq(:success) - expect(GenericCommitStatus.last.description).to eq("pages site contains 3 file entries, while limit is set to 2") + expect(GenericCommitStatus.last.description) + .to eq("pages site contains 3 file entries, while limit is set to 2") end context 'when timeout happens by DNS error' do @@ -240,13 +246,13 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do deploy_status = GenericCommitStatus.last expect(deploy_status).to be_failed - expect(project.pages_metadatum).not_to be_deployed + expect(project.pages_deployed?).to eq(false) end end context 'when missing artifacts metadata' do before do - expect(build).to receive(:artifacts_metadata?).and_return(false) + allow(build).to receive(:artifacts_metadata?).and_return(false) end it 'does not raise an error as failed job' do @@ -256,7 +262,7 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do deploy_status = GenericCommitStatus.last expect(deploy_status).to be_failed - expect(project.pages_metadatum).not_to be_deployed + expect(project.pages_deployed?).to eq(false) end end @@ -275,10 +281,9 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do end end - it 'creates a new pages deployment and mark it as deployed' do - expect do - expect(service.execute[:status]).to eq(:success) - end.to change { project.pages_deployments.count }.by(1) + it 'creates a new pages deployment' do + expect { expect(service.execute[:status]).to eq(:success) } + .to change { project.pages_deployments.count }.by(1) deployment = project.pages_deployments.last expect(deployment.ci_build_id).to eq(build.id) @@ -287,16 +292,12 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do it_behaves_like 'old deployments' context 'when newer deployment present' do - before do + it 'fails with outdated reference message' do new_pipeline = create(:ci_pipeline, project: project, sha: project.commit('HEAD').sha) new_build = create(:ci_build, name: 'pages', pipeline: new_pipeline, ref: 'HEAD') - new_deployment = create(:pages_deployment, ci_build: new_build, project: project) - project.update_pages_deployment!(new_deployment) - end + create(:pages_deployment, project: project, ci_build: new_build) - it 'fails with outdated reference message' do expect(service.execute[:status]).to eq(:error) - expect(project.reload.pages_metadatum).not_to be_deployed deploy_status = GenericCommitStatus.last expect(deploy_status).to be_failed @@ -308,16 +309,14 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do it 'fails when uploaded deployment size is wrong' do allow_next_instance_of(PagesDeployment) do |deployment| allow(deployment) - .to receive(:size) - .and_return(file.size + 1) + .to receive(:file) + .and_return(instance_double(Pages::DeploymentUploader, size: file.size + 1)) end expect(service.execute[:status]).not_to eq(:success) - expect(GenericCommitStatus.last.description).to eq('The uploaded artifact size does not match the expected value') - project.pages_metadatum.reload - expect(project.pages_metadatum).not_to be_deployed - expect(project.pages_metadatum.pages_deployment).to be_nil + expect(GenericCommitStatus.last.description) + .to eq('The uploaded artifact size does not match the expected value') end end end @@ -335,9 +334,8 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do end it 'fails with exception raised' do - expect do - service.execute - end.to raise_error("Validation failed: File sha256 can't be blank") + expect { service.execute } + .to raise_error("Validation failed: File sha256 can't be blank") end end diff --git a/spec/services/projects/update_repository_storage_service_spec.rb b/spec/services/projects/update_repository_storage_service_spec.rb index d173d23a1d6..b81fc8bf633 100644 --- a/spec/services/projects/update_repository_storage_service_spec.rb +++ b/spec/services/projects/update_repository_storage_service_spec.rb @@ -79,6 +79,30 @@ RSpec.describe Projects::UpdateRepositoryStorageService, feature_category: :sour end end + context 'when touch raises an exception' do + let(:exception) { RuntimeError.new('Boom') } + + it 'marks the storage move as failed and restores read-write access' do + allow(repository_storage_move).to receive(:container).and_return(project) + + allow(project).to receive(:touch).and_wrap_original do + project.assign_attributes(updated_at: 1.second.ago) + raise exception + end + + expect(project_repository_double).to receive(:replicate) + .with(project.repository.raw) + expect(project_repository_double).to receive(:checksum) + .and_return(checksum) + + expect { subject.execute }.to raise_error(exception) + project.reload + + expect(project).not_to be_repository_read_only + expect(repository_storage_move.reload).to be_failed + end + end + context 'when the filesystems are the same' do before do expect(Gitlab::GitalyClient).to receive(:filesystem_id).twice.and_return(SecureRandom.uuid) diff --git a/spec/services/projects/update_statistics_service_spec.rb b/spec/services/projects/update_statistics_service_spec.rb index f6565853460..5311b8daeb1 100644 --- a/spec/services/projects/update_statistics_service_spec.rb +++ b/spec/services/projects/update_statistics_service_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Projects::UpdateStatisticsService, feature_category: :groups_and_ using RSpec::Parameterized::TableSyntax let(:service) { described_class.new(project, nil, statistics: statistics) } - let(:statistics) { %w(repository_size) } + let(:statistics) { %w[repository_size] } describe '#execute' do context 'with a non-existing project' do @@ -23,13 +23,13 @@ RSpec.describe Projects::UpdateStatisticsService, feature_category: :groups_and_ let_it_be(:project) { create(:project) } where(:statistics, :method_caches) do - [] | %i(size recent_objects_size commit_count) - ['repository_size'] | %i(size recent_objects_size) - [:repository_size] | %i(size recent_objects_size) + [] | %i[size recent_objects_size commit_count] + ['repository_size'] | %i[size recent_objects_size] + [:repository_size] | %i[size recent_objects_size] [:lfs_objects_size] | nil [:commit_count] | [:commit_count] - [:repository_size, :commit_count] | %i(size recent_objects_size commit_count) - [:repository_size, :commit_count, :lfs_objects_size] | %i(size recent_objects_size commit_count) + [:repository_size, :commit_count] | %i[size recent_objects_size commit_count] + [:repository_size, :commit_count, :lfs_objects_size] | %i[size recent_objects_size commit_count] end with_them do @@ -59,7 +59,7 @@ RSpec.describe Projects::UpdateStatisticsService, feature_category: :groups_and_ it 'invalidates and refreshes Wiki size' do expect(project.statistics).to receive(:refresh!).with(only: statistics).and_call_original - expect(project.wiki.repository).to receive(:expire_method_caches).with(%i(size)).and_call_original + expect(project.wiki.repository).to receive(:expire_method_caches).with(%i[size]).and_call_original service.execute end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 2c34d6a59be..1c9c6323e96 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -80,7 +80,7 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning it 'returns the title message' do _, _, message = service.execute(content, issuable) - expect(message).to eq(%{Changed the title to "A brand new title".}) + expect(message).to eq(%(Changed the title to "A brand new title".)) end end @@ -695,7 +695,7 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning _, _, message = service.execute(content, issuable) if tag_message.present? - expect(message).to eq(%{Tagged this commit to #{tag_name} with "#{tag_message}".}) + expect(message).to eq(%(Tagged this commit to #{tag_name} with "#{tag_message}".)) else expect(message).to eq("Tagged this commit to #{tag_name}.") end @@ -1979,7 +1979,7 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning context '/board_move command' do let_it_be(:todo) { create(:label, project: project, title: 'To Do') } let_it_be(:inreview) { create(:label, project: project, title: 'In Review') } - let(:content) { %{/board_move ~"#{inreview.title}"} } + let(:content) { %(/board_move ~"#{inreview.title}") } let_it_be(:board) { create(:board, project: project) } let_it_be(:todo_list) { create(:list, board: board, label: todo) } @@ -2043,14 +2043,14 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning context 'if multiple labels are given' do let(:issuable) { issue } - let(:content) { %{/board_move ~"#{inreview.title}" ~"#{todo.title}"} } + let(:content) { %(/board_move ~"#{inreview.title}" ~"#{todo.title}") } it_behaves_like 'failed command', 'Failed to move this issue because only a single label can be provided.' end context 'if the given label is not a list on the board' do let(:issuable) { issue } - let(:content) { %{/board_move ~"#{bug.title}"} } + let(:content) { %(/board_move ~"#{bug.title}") } it_behaves_like 'failed command', 'Failed to move this issue because label was not found.' end @@ -2187,6 +2187,67 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning end end + context 'request_changes command' do + let(:merge_request) { create(:merge_request, source_project: project) } + let(:content) { '/request_changes' } + + context "when `mr_request_changes` feature flag is disabled" do + before do + stub_feature_flags(mr_request_changes: false) + end + + it 'does not call MergeRequests::UpdateReviewerStateService' do + expect(MergeRequests::UpdateReviewerStateService).not_to receive(:new) + + service.execute(content, merge_request) + end + end + + context "when the user is a reviewer" do + before do + create(:merge_request_reviewer, merge_request: merge_request, reviewer: current_user) + end + + it 'calls MergeRequests::UpdateReviewerStateService with requested_changes' do + expect_next_instance_of( + MergeRequests::UpdateReviewerStateService, + project: project, current_user: current_user + ) do |service| + expect(service).to receive(:execute).with(merge_request, "requested_changes").and_return({ status: :success }) + end + + _, _, message = service.execute(content, merge_request) + + expect(message).to eq('Changes requested to the current merge request.') + end + + it 'returns error message from MergeRequests::UpdateReviewerStateService' do + expect_next_instance_of( + MergeRequests::UpdateReviewerStateService, + project: project, current_user: current_user + ) do |service| + expect(service).to receive(:execute).with(merge_request, "requested_changes").and_return({ status: :error, message: 'Error' }) + end + + _, _, message = service.execute(content, merge_request) + + expect(message).to eq('Error') + end + end + + context "when the user is not a reviewer" do + it 'does not call MergeRequests::UpdateReviewerStateService' do + expect(MergeRequests::UpdateReviewerStateService).not_to receive(:new) + + service.execute(content, merge_request) + end + end + + it_behaves_like 'approve command unavailable' do + let(:issuable) { issue } + end + end + it_behaves_like 'issues link quick action', :relate do let(:user) { developer } end @@ -2422,6 +2483,17 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning expect(merge_request.approved_by_users).to be_empty end + it 'calls MergeRequests::UpdateReviewerStateService' do + expect_next_instance_of( + MergeRequests::UpdateReviewerStateService, + project: project, current_user: current_user + ) do |service| + expect(service).to receive(:execute).with(merge_request, "unreviewed") + end + + service.execute(content, merge_request) + end + context "when the user can't unapprove" do before do project.team.truncate diff --git a/spec/services/releases/create_service_spec.rb b/spec/services/releases/create_service_spec.rb index 0170c3abcaf..3504f00412c 100644 --- a/spec/services/releases/create_service_spec.rb +++ b/spec/services/releases/create_service_spec.rb @@ -56,21 +56,25 @@ RSpec.describe Releases::CreateService, feature_category: :continuous_integratio end context 'when project is a catalog resource' do - let(:ref) { 'master' } + let(:project) { create(:project, :catalog_resource_with_components, create_tag: 'final') } let!(:ci_catalog_resource) { create(:ci_catalog_resource, project: project) } + let(:ref) { 'master' } context 'and it is valid' do - let_it_be(:project) { create(:project, :repository, description: 'our components') } - it_behaves_like 'a successful release creation' end - context 'and it is invalid' do + context 'and it is an invalid resource' do + let_it_be(:project) { create(:project, :repository) } + it 'raises an error and does not update the release' do result = service.execute expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Project must have a description') + expect(result[:http_status]).to eq(422) + expect(result[:message]).to eq( + 'Project must have a description, ' \ + 'Project must contain components. Ensure you are using the correct directory structure') end end end @@ -104,6 +108,7 @@ RSpec.describe Releases::CreateService, feature_category: :continuous_integratio result = service.execute expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(403) end end @@ -139,6 +144,7 @@ RSpec.describe Releases::CreateService, feature_category: :continuous_integratio it 'raises an error and does not update the release' do result = service.execute expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(409) expect(project.releases.find_by(tag: tag_name).description).to eq(description) end end @@ -150,6 +156,7 @@ RSpec.describe Releases::CreateService, feature_category: :continuous_integratio result = service.execute expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(400) expect(result[:message]).to eq("Milestone(s) not found: #{inexistent_milestone_tag}") end @@ -159,6 +166,7 @@ RSpec.describe Releases::CreateService, feature_category: :continuous_integratio result = service.execute expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(400) expect(result[:message]).to eq("Milestone id(s) not found: #{inexistent_milestone_id}") end end @@ -244,6 +252,7 @@ RSpec.describe Releases::CreateService, feature_category: :continuous_integratio result = service.execute expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(400) expect(result[:message]).to eq("Milestone(s) not found: #{inexistent_title}") end @@ -260,6 +269,7 @@ RSpec.describe Releases::CreateService, feature_category: :continuous_integratio result = service.execute expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(400) expect(result[:message]).to eq("Milestone id(s) not found: #{non_existing_record_id}") 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 d882cd8635a..f87952d1d0e 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 @@ -22,6 +22,7 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::UpdateService, feature_cat end let(:expected_error_message) { error_parameter_missing } + let(:expected_custom_email_enabled) { false } let(:logger_params) { { category: 'custom_email_verification' } } before do @@ -30,7 +31,7 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::UpdateService, feature_cat end shared_examples 'a failing verification process' do |expected_error_identifier| - it 'refuses to verify and sends result emails' do + it 'refuses to verify and sends result emails', :aggregate_failures do expect(Notify).to receive(:service_desk_verification_result_email).twice expect(Gitlab::AppLogger).to receive(:info).with(logger_params.merge( @@ -52,7 +53,7 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::UpdateService, feature_cat end shared_examples 'an early exit from the verification process' do |expected_state| - it 'exits early' do + it 'exits early', :aggregate_failures do expect(Notify).to receive(:service_desk_verification_result_email).exactly(0).times expect(Gitlab::AppLogger).to receive(:warn).with(logger_params.merge( @@ -65,7 +66,7 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::UpdateService, feature_cat verification.reset expect(response).to be_error - expect(settings).not_to be_custom_email_enabled + expect(settings.custom_email_enabled).to eq expected_custom_email_enabled expect(verification.state).to eq expected_state end end @@ -179,6 +180,26 @@ RSpec.describe ServiceDesk::CustomEmailVerifications::UpdateService, feature_cat it_behaves_like 'a failing verification process', 'mail_not_received_within_timeframe' end + + context 'when already verified' do + let(:expected_error_message) { error_already_finished } + + before do + verification.mark_as_finished! + end + + it_behaves_like 'an early exit from the verification process', 'finished' + + context 'when enabled' do + let(:expected_custom_email_enabled) { true } + + before do + settings.update!(custom_email_enabled: true) + end + + it_behaves_like 'an early exit from the verification process', 'finished' + end + end end end end 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 2029c9a0c3f..e165131bcf9 100644 --- a/spec/services/service_desk/custom_emails/create_service_spec.rb +++ b/spec/services/service_desk/custom_emails/create_service_spec.rb @@ -156,7 +156,7 @@ RSpec.describe ServiceDesk::CustomEmails::CreateService, feature_category: :serv } end - it 'creates all records returns a successful response' do + it 'creates all records and returns a successful response' do # Because we also log in ServiceDesk::CustomEmailVerifications::CreateService expect(Gitlab::AppLogger).to receive(:info).with({ category: 'custom_email_verification' }).once expect(Gitlab::AppLogger).to receive(:info).with(logger_params).once @@ -174,7 +174,8 @@ RSpec.describe ServiceDesk::CustomEmails::CreateService, feature_category: :serv smtp_address: params[:smtp_address], smtp_port: params[:smtp_port].to_i, smtp_username: params[:smtp_username], - smtp_password: params[:smtp_password] + smtp_password: params[:smtp_password], + smtp_authentication: nil ) expect(project.service_desk_custom_email_verification).to have_attributes( state: 'started', @@ -183,6 +184,30 @@ RSpec.describe ServiceDesk::CustomEmails::CreateService, feature_category: :serv ) end + context 'with optional smtp_authentication parameter' do + before do + params[:smtp_authentication] = 'login' + end + + it 'sets authentication and returns a successful response' do + response = service.execute + project.reset + + expect(response).to be_success + expect(project.service_desk_custom_email_credential.smtp_authentication).to eq 'login' + end + + context 'with unsupported value' do + let(:expected_error_message) { error_cannot_create_custom_email } + + before do + params[:smtp_authentication] = 'unsupported' + end + + it_behaves_like 'a failing service that does not create records' + end + end + context 'when custom email aready exists' do let!(:settings) { create(:service_desk_setting, project: project, custom_email: 'user@example.com') } let!(:credential) { create(:service_desk_custom_email_credential, project: project) } diff --git a/spec/services/service_desk_settings/update_service_spec.rb b/spec/services/service_desk_settings/update_service_spec.rb index 27978225bcf..a9e54012075 100644 --- a/spec/services/service_desk_settings/update_service_spec.rb +++ b/spec/services/service_desk_settings/update_service_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'spec_helper' -RSpec.describe ServiceDeskSettings::UpdateService, feature_category: :service_desk do +RSpec.describe ServiceDeskSettings::UpdateService, :aggregate_failures, feature_category: :service_desk do describe '#execute' do let_it_be(:settings) do create(:service_desk_setting, outgoing_name: 'original name', custom_email: 'user@example.com') @@ -12,14 +12,17 @@ RSpec.describe ServiceDeskSettings::UpdateService, feature_category: :service_de let_it_be(:user) { create(:user) } context 'with valid params' do - let(:params) { { outgoing_name: 'some name', project_key: 'foo' } } + let(:params) { { outgoing_name: 'some name', project_key: 'foo', add_external_participants_from_cc: true } } it 'updates service desk settings' do response = described_class.new(settings.project, user, params).execute expect(response).to be_success - expect(settings.reload.outgoing_name).to eq 'some name' - expect(settings.reload.project_key).to eq 'foo' + expect(settings.reset).to have_attributes( + outgoing_name: 'some name', + project_key: 'foo', + add_external_participants_from_cc: true + ) end context 'with custom email verification in finished state' do @@ -39,6 +42,23 @@ RSpec.describe ServiceDeskSettings::UpdateService, feature_category: :service_de expect(Gitlab::AppLogger).to have_received(:info).with({ category: 'custom_email' }) end end + + context 'when issue_email_participants feature flag is disabled' do + before do + stub_feature_flags(issue_email_participants: false) + end + + it 'updates service desk setting but not add_external_participants_from_cc value' do + response = described_class.new(settings.project, user, params).execute + + expect(response).to be_success + expect(settings.reset).to have_attributes( + outgoing_name: 'some name', + project_key: 'foo', + add_external_participants_from_cc: false + ) + end + end end context 'when project_key is an empty string' do diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb index 4133609d9ae..d8fd09ebd07 100644 --- a/spec/services/spam/spam_action_service_spec.rb +++ b/spec/services/spam/spam_action_service_spec.rb @@ -85,6 +85,26 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d end end + shared_examples 'calls SpamAbuseEventsWorker with correct arguments' do + let(:params) do + { + user_id: user.id, + title: target.title, + description: target.spam_description, + source_ip: fake_ip, + user_agent: fake_user_agent, + noteable_type: target_type, + verdict: verdict + } + end + + it do + expect(::Abuse::SpamAbuseEventsWorker).to receive(:perform_async).with(params) + + subject + end + end + shared_examples 'execute spam action service' do |target_type| let(:fake_captcha_verification_service) { double(:captcha_verification_service) } let(:fake_verdict_service) { double(:spam_verdict_service) } @@ -161,6 +181,12 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d it 'does not create a spam log' do expect { subject }.not_to change(SpamLog, :count) end + + it 'does not call SpamAbuseEventsWorker' do + expect(::Abuse::SpamAbuseEventsWorker).not_to receive(:perform_async) + + subject + end end context 'when spammable attributes have changed' do @@ -213,6 +239,11 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d it_behaves_like 'creates a spam log', target_type + it_behaves_like 'calls SpamAbuseEventsWorker with correct arguments' do + let(:verdict) { DISALLOW } + let(:target_type) { target_type } + end + it 'marks as spam' do response = subject @@ -231,6 +262,11 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d it_behaves_like 'creates a spam log', target_type + it_behaves_like 'calls SpamAbuseEventsWorker with correct arguments' do + let(:verdict) { BLOCK_USER } + let(:target_type) { target_type } + end + it 'marks as spam' do response = subject @@ -254,6 +290,11 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d it_behaves_like 'creates a spam log', target_type + it_behaves_like 'calls SpamAbuseEventsWorker with correct arguments' do + let(:verdict) { CONDITIONAL_ALLOW } + let(:target_type) { target_type } + end + it 'does not mark as spam' do response = subject @@ -276,6 +317,12 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d it_behaves_like 'creates a spam log', target_type + it 'does not call SpamAbuseEventsWorker' do + expect(::Abuse::SpamAbuseEventsWorker).not_to receive(:perform_async) + + subject + end + it 'does not mark as spam' do response = subject @@ -300,6 +347,12 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d expect { subject }.not_to change(SpamLog, :count) end + it 'does not call SpamAbuseEventsWorker' do + expect(::Abuse::SpamAbuseEventsWorker).not_to receive(:perform_async) + + subject + end + it 'clears spam flags' do expect(target).to receive(:clear_spam_flags!) @@ -316,6 +369,12 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d expect { subject }.not_to change(SpamLog, :count) end + it 'does not call SpamAbuseEventsWorker' do + expect(::Abuse::SpamAbuseEventsWorker).not_to receive(:perform_async) + + subject + end + it 'clears spam flags' do expect(target).to receive(:clear_spam_flags!) diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index bcca1ed0b23..ca6feb6fde2 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -784,7 +784,7 @@ RSpec.describe ::SystemNotes::IssuablesService, feature_category: :team_planning service = described_class.new(noteable: issuable, author: author) expect(service.discussion_lock.note) - .to eq("unlocked this #{type.to_s.titleize.downcase}") + .to eq("unlocked the discussion in this #{type.to_s.titleize.downcase}") end end end @@ -804,7 +804,7 @@ RSpec.describe ::SystemNotes::IssuablesService, feature_category: :team_planning service = described_class.new(noteable: issuable, author: author) expect(service.discussion_lock.note) - .to eq("locked this #{type.to_s.titleize.downcase}") + .to eq("locked the discussion in this #{type.to_s.titleize.downcase}") end end end diff --git a/spec/services/upload_service_spec.rb b/spec/services/upload_service_spec.rb index 518d12d5b41..4a8cd46172d 100644 --- a/spec/services/upload_service_spec.rb +++ b/spec/services/upload_service_spec.rb @@ -81,7 +81,7 @@ RSpec.describe UploadService, feature_category: :shared do it 'allows the upload' do service.override_max_attachment_size = 101.megabytes - expect(subject.keys).to eq(%i(alt url markdown)) + expect(subject.keys).to eq(%i[alt url markdown]) end it 'disallows the upload' do diff --git a/spec/services/users/refresh_authorized_projects_service_spec.rb b/spec/services/users/refresh_authorized_projects_service_spec.rb index b36152f81c3..3d88618711b 100644 --- a/spec/services/users/refresh_authorized_projects_service_spec.rb +++ b/spec/services/users/refresh_authorized_projects_service_spec.rb @@ -98,6 +98,13 @@ RSpec.describe Users::RefreshAuthorizedProjectsService, feature_category: :user_ service.execute_without_lease end + it 'updates project_authorizations_recalculated_at', :freeze_time do + default_date = Time.zone.local('2010') + expect do + service.execute_without_lease + end.to change { user.project_authorizations_recalculated_at }.from(default_date).to(Time.zone.now) + end + it 'returns a User' do expect(service.execute_without_lease).to be_an_instance_of(User) end diff --git a/spec/services/users/upsert_credit_card_validation_service_spec.rb b/spec/services/users/upsert_credit_card_validation_service_spec.rb index 4e23b51cae2..e1c5b30115d 100644 --- a/spec/services/users/upsert_credit_card_validation_service_spec.rb +++ b/spec/services/users/upsert_credit_card_validation_service_spec.rb @@ -3,20 +3,29 @@ require 'spec_helper' RSpec.describe Users::UpsertCreditCardValidationService, feature_category: :user_profile do + include CryptoHelpers + let_it_be(:user) { create(:user) } let(:user_id) { user.id } - let(:credit_card_validated_time) { Time.utc(2020, 1, 1) } + + let(:network) { 'American Express' } + let(:holder_name) { 'John Smith' } + let(:last_digits) { '1111' } let(:expiration_year) { Date.today.year + 10 } + let(:expiration_month) { 1 } + let(:expiration_date) { Date.new(expiration_year, expiration_month, -1) } + let(:credit_card_validated_at) { Time.utc(2020, 1, 1) } + let(:params) do { user_id: user_id, - credit_card_validated_at: credit_card_validated_time, + credit_card_validated_at: credit_card_validated_at, credit_card_expiration_year: expiration_year, - credit_card_expiration_month: 1, - credit_card_holder_name: 'John Smith', - credit_card_type: 'AmericanExpress', - credit_card_mask_number: '1111' + credit_card_expiration_month: expiration_month, + credit_card_holder_name: holder_name, + credit_card_type: network, + credit_card_mask_number: last_digits } end @@ -25,82 +34,97 @@ RSpec.describe Users::UpsertCreditCardValidationService, feature_category: :user context 'successfully set credit card validation record for the user' do context 'when user does not have credit card validation record' do - it 'creates the credit card validation and returns a success' do + it 'creates the credit card validation and returns a success', :aggregate_failures do expect(user.credit_card_validated_at).to be nil - result = service.execute + service_result = service.execute - expect(result.status).to eq(:success) + expect(service_result.status).to eq(:success) + expect(service_result.message).to eq(_('Credit card validation record saved')) user.reload expect(user.credit_card_validation).to have_attributes( - credit_card_validated_at: credit_card_validated_time, - network: 'AmericanExpress', - holder_name: 'John Smith', - last_digits: 1111, - expiration_date: Date.new(expiration_year, 1, 31) + credit_card_validated_at: credit_card_validated_at, + network_hash: sha256(network.downcase), + holder_name_hash: sha256(holder_name.downcase), + last_digits_hash: sha256(last_digits), + expiration_date_hash: sha256(expiration_date.to_s) ) end end context 'when user has credit card validation record' do - let(:old_time) { Time.utc(1999, 2, 2) } + let(:previous_credit_card_validated_at) { Time.utc(1999, 2, 2) } before do - create(:credit_card_validation, user: user, credit_card_validated_at: old_time) + create(:credit_card_validation, user: user, credit_card_validated_at: previous_credit_card_validated_at) end - it 'updates the credit card validation and returns a success' do - expect(user.credit_card_validated_at).to eq(old_time) + it 'updates the credit card validation record and returns a success', :aggregate_failures do + expect(user.credit_card_validated_at).to eq(previous_credit_card_validated_at) + + service_result = service.execute - result = service.execute + expect(service_result.status).to eq(:success) + expect(service_result.message).to eq(_('Credit card validation record saved')) - expect(result.status).to eq(:success) - expect(user.reload.credit_card_validated_at).to eq(credit_card_validated_time) + user.reload + + expect(user.credit_card_validated_at).to eq(credit_card_validated_at) end end end shared_examples 'returns an error without tracking the exception' do - it do + it 'does not send an exception to Gitlab::ErrorTracking' do expect(Gitlab::ErrorTracking).not_to receive(:track_exception) - result = service.execute + service.execute + end + + it 'returns an error', :aggregate_failures do + service_result = service.execute - expect(result.status).to eq(:error) + expect(service_result.status).to eq(:error) + expect(service_result.message).to eq(_('Error saving credit card validation record')) end end - shared_examples 'returns an error, tracking the exception' do - it do + shared_examples 'returns an error and tracks the exception' do + it 'sends an exception to Gitlab::ErrorTracking' do expect(Gitlab::ErrorTracking).to receive(:track_exception) - result = service.execute + service.execute + end + + it 'returns an error', :aggregate_failures do + service_result = service.execute - expect(result.status).to eq(:error) + expect(service_result.status).to eq(:error) + expect(service_result.message).to eq(_('Error saving credit card validation record')) end end - context 'when user id does not exist' do + context 'when the user_id does not exist' do let(:user_id) { non_existing_record_id } it_behaves_like 'returns an error without tracking the exception' end - context 'when missing credit_card_validated_at' do + context 'when the request is missing the credit_card_validated_at field' do let(:params) { { user_id: user_id } } - it_behaves_like 'returns an error, tracking the exception' + it_behaves_like 'returns an error and tracks the exception' end - context 'when missing user id' do - let(:params) { { credit_card_validated_at: credit_card_validated_time } } + context 'when the request is missing the user_id field' do + let(:params) { { credit_card_validated_at: credit_card_validated_at } } - it_behaves_like 'returns an error, tracking the exception' + it_behaves_like 'returns an error and tracks the exception' end - context 'when unexpected exception happen' do + context 'when there is an unexpected error' do let(:exception) { StandardError.new } before do @@ -109,22 +133,7 @@ RSpec.describe Users::UpsertCreditCardValidationService, feature_category: :user end end - it 'tracks the exception and returns an error' do - logged_params = { - credit_card_validated_at: credit_card_validated_time, - expiration_date: Date.new(expiration_year, 1, 31), - holder_name: "John Smith", - last_digits: 1111, - network: "AmericanExpress", - user_id: user_id - } - - expect(Gitlab::ErrorTracking).to receive(:track_exception).with(exception, class: described_class.to_s, params: logged_params) - - result = service.execute - - expect(result.status).to eq(:error) - end + it_behaves_like 'returns an error and tracks the exception' end end end diff --git a/spec/services/vs_code/settings/delete_service_spec.rb b/spec/services/vs_code/settings/delete_service_spec.rb new file mode 100644 index 00000000000..fd19c01569f --- /dev/null +++ b/spec/services/vs_code/settings/delete_service_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe VsCode::Settings::DeleteService, feature_category: :web_ide do + describe '#execute' do + let_it_be(:user) { create(:user) } + let_it_be(:other_user) { create(:user) } + let_it_be(:setting_one) { create(:vscode_setting, user: user) } + let_it_be(:setting_two) { create(:vscode_setting, setting_type: 'extensions', user: user) } + let_it_be(:setting_three) { create(:vscode_setting, setting_type: 'extensions', user: other_user) } + + subject { described_class.new(current_user: user).execute } + + it 'deletes all vscode_settings belonging to the current user' do + expect { subject } + .to change { User.find(user.id).vscode_settings.count }.from(2).to(0) + .and not_change { User.find(other_user.id).vscode_settings.count } + end + end +end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 89346353db2..c33273348f6 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -13,6 +13,8 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state, { before: 'oldrev', after: 'newrev', ref: 'ref' } end + let(:serialized_data) { data.deep_stringify_keys } + let(:service_instance) { described_class.new(project_hook, data, :push_hooks) } describe '#initialize' do @@ -426,9 +428,9 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state, expect(WebHooks::LogExecutionWorker).to receive(:perform_async) .with( project_hook.id, - hash_including(default_log_data), - :ok, - nil + hash_including(default_log_data.deep_stringify_keys), + 'ok', + '' ) service_instance.execute @@ -456,10 +458,10 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state, default_log_data.merge( response_body: 'Bad request', response_status: 400 - ) + ).deep_stringify_keys ), - :failed, - nil + 'failed', + '' ) service_instance.execute @@ -480,10 +482,10 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state, response_body: '', response_status: 'internal error', internal_error_message: 'Some HTTP Post error' - ) + ).deep_stringify_keys ), - :error, - nil + 'error', + '' ) service_instance.execute @@ -499,9 +501,9 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state, expect(WebHooks::LogExecutionWorker).to receive(:perform_async) .with( project_hook.id, - hash_including(default_log_data.merge(response_body: '')), - :ok, - nil + hash_including(default_log_data.merge(response_body: '').deep_stringify_keys), + 'ok', + '' ) service_instance.execute @@ -520,9 +522,9 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state, expect(WebHooks::LogExecutionWorker).to receive(:perform_async) .with( project_hook.id, - hash_including(default_log_data.merge(response_body: stripped_body)), - :ok, - nil + hash_including(default_log_data.merge(response_body: stripped_body).deep_stringify_keys), + 'ok', + '' ) service_instance.execute @@ -553,9 +555,9 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state, expect(WebHooks::LogExecutionWorker).to receive(:perform_async) .with( project_hook.id, - hash_including(default_log_data.merge(response_headers: expected_response_headers)), - :ok, - nil + hash_including(default_log_data.merge(response_headers: expected_response_headers).deep_stringify_keys), + 'ok', + '' ) service_instance.execute @@ -578,9 +580,9 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state, expect(WebHooks::LogExecutionWorker).to receive(:perform_async) .with( project_hook.id, - hash_including(default_log_data.merge(response_headers: expected_response_headers)), - :ok, - nil + hash_including(default_log_data.merge(response_headers: expected_response_headers).deep_stringify_keys), + 'ok', + '' ) service_instance.execute @@ -596,9 +598,9 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state, expect(WebHooks::LogExecutionWorker).to receive(:perform_async) .with( project_hook.id, - hash_including(default_log_data), - :ok, - nil + hash_including(default_log_data.deep_stringify_keys), + 'ok', + '' ) .and_raise( Gitlab::SidekiqMiddleware::SizeLimiter::ExceedLimitError.new(WebHooks::LogExecutionWorker, 100, 50) @@ -607,9 +609,11 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state, expect(WebHooks::LogExecutionWorker).to receive(:perform_async) .with( project_hook.id, - hash_including(default_log_data.merge(request_data: WebHookLog::OVERSIZE_REQUEST_DATA)), - :ok, - nil + hash_including(default_log_data.merge( + request_data: WebHookLog::OVERSIZE_REQUEST_DATA + ).deep_stringify_keys), + 'ok', + '' ) .and_call_original .ordered @@ -636,7 +640,9 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state, describe '#async_execute' do def expect_to_perform_worker(hook) - expect(WebHookWorker).to receive(:perform_async).with(hook.id, data, 'push_hooks', an_instance_of(Hash)) + expect(WebHookWorker).to receive(:perform_async).with( + hook.id, serialized_data, 'push_hooks', an_instance_of(Hash) + ) end def expect_to_rate_limit(hook, threshold:, throttled: false) |