diff options
Diffstat (limited to 'spec/services')
173 files changed, 4195 insertions, 1397 deletions
diff --git a/spec/services/achievements/destroy_user_achievement_service_spec.rb b/spec/services/achievements/destroy_user_achievement_service_spec.rb new file mode 100644 index 00000000000..c5ff43fa1b2 --- /dev/null +++ b/spec/services/achievements/destroy_user_achievement_service_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Achievements::DestroyUserAchievementService, feature_category: :user_profile do + describe '#execute' do + let_it_be(:maintainer) { create(:user) } + let_it_be(:owner) { create(:user) } + let_it_be(:group) { create(:group) } + + let_it_be(:achievement) { create(:achievement, namespace: group) } + let_it_be(:user_achievement) { create(:user_achievement, achievement: achievement) } + + subject(:response) { described_class.new(current_user, user_achievement).execute } + + before_all do + group.add_maintainer(maintainer) + group.add_owner(owner) + end + + context 'when user does not have permission' do + let(:current_user) { maintainer } + + it 'returns an error' do + expect(response).to be_error + expect(response.message).to match_array( + ['You have insufficient permissions to delete this user achievement']) + end + end + + context 'when user has permission' do + let(:current_user) { owner } + + it 'deletes the achievement' do + expect(response).to be_success + expect(Achievements::UserAchievement.find_by(id: user_achievement.id)).to be_nil + end + end + end +end diff --git a/spec/services/admin/abuse_report_update_service_spec.rb b/spec/services/admin/abuse_report_update_service_spec.rb index e85b516b87f..7069d8ee5c1 100644 --- a/spec/services/admin/abuse_report_update_service_spec.rb +++ b/spec/services/admin/abuse_report_update_service_spec.rb @@ -52,6 +52,10 @@ RSpec.describe Admin::AbuseReportUpdateService, feature_category: :instance_resi comment: params[:comment] ) end + + it 'returns the event success message' do + expect(subject.message).to eq(abuse_report.events.last.success_message) + end end context 'when invalid parameters are given' do @@ -194,6 +198,15 @@ RSpec.describe Admin::AbuseReportUpdateService, feature_category: :instance_resi it_behaves_like 'closes the report' it_behaves_like 'records an event', action: 'close_report' + + context 'when report is already closed' do + before do + abuse_report.closed! + end + + it_behaves_like 'returns an error response', 'Report already closed' + it_behaves_like 'does not record an event' + end 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 new file mode 100644 index 00000000000..4a384b98299 --- /dev/null +++ b/spec/services/admin/plan_limits/update_service_spec.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Admin::PlanLimits::UpdateService, feature_category: :shared do + let_it_be(:user) { create(:admin) } + let_it_be(:plan) { create(:plan, name: 'free') } + let_it_be(:limits) { plan.actual_limits } + let_it_be(:params) do + { + ci_pipeline_size: 101, + ci_active_jobs: 102, + ci_project_subscriptions: 104, + ci_pipeline_schedules: 105, + ci_needs_size_limit: 106, + ci_registered_group_runners: 107, + ci_registered_project_runners: 108, + conan_max_file_size: 10, + enforcement_limit: 15, + generic_packages_max_file_size: 20, + helm_max_file_size: 25, + notification_limit: 30, + maven_max_file_size: 40, + npm_max_file_size: 60, + nuget_max_file_size: 60, + pypi_max_file_size: 70, + terraform_module_max_file_size: 80, + storage_size_limit: 90, + pipeline_hierarchy_size: 250 + } + end + + subject(:update_plan_limits) { described_class.new(params, current_user: user, plan: plan).execute } + + context 'when current_user is an admin', :enable_admin_mode do + context 'when the update is successful' do + it 'updates all attributes' do + expect_next_instance_of(described_class) do |instance| + expect(instance).to receive(:parsed_params).and_call_original + end + + update_plan_limits + + params.each do |key, value| + expect(limits.send(key)).to eq value + end + end + + it 'returns success' do + response = update_plan_limits + + expect(response[:status]).to eq :success + end + end + + context 'when the update is unsuccessful' do + let(:params) { { notification_limit: 'abc' } } + + it 'returns an error' do + response = update_plan_limits + + expect(response[:status]).to eq :error + expect(response[:message]).to include 'Notification limit is not a number' + end + end + end + + context 'when the user is not an admin' do + let(:user) { create(:user) } + + it 'returns an error' do + response = update_plan_limits + + expect(response[:status]).to eq :error + expect(response[:message]).to eq 'Access denied' + end + end +end diff --git a/spec/services/alert_management/http_integrations/create_service_spec.rb b/spec/services/alert_management/http_integrations/create_service_spec.rb index 5200ec27dd1..bced09044eb 100644 --- a/spec/services/alert_management/http_integrations/create_service_spec.rb +++ b/spec/services/alert_management/http_integrations/create_service_spec.rb @@ -38,12 +38,6 @@ RSpec.describe AlertManagement::HttpIntegrations::CreateService, feature_categor it_behaves_like 'error response', 'You have insufficient permissions to create an HTTP integration for this project' end - context 'when an integration already exists' do - let_it_be(:existing_integration) { create(:alert_management_http_integration, project: project) } - - it_behaves_like 'error response', 'Multiple HTTP integrations are not supported for this project' - end - context 'when an error occurs during update' do it_behaves_like 'error response', "Name can't be blank" end @@ -61,6 +55,38 @@ RSpec.describe AlertManagement::HttpIntegrations::CreateService, feature_categor expect(integration.token).to be_present expect(integration.endpoint_identifier).to be_present end + + context 'with an existing HTTP integration' do + let_it_be(:http_integration) { create(:alert_management_http_integration, project: project) } + + it_behaves_like 'error response', 'Multiple integrations of a single type are not supported for this project' + + context 'when creating a different type of integration' do + let(:params) { { type_identifier: :prometheus, name: 'Prometheus' } } + + it 'is successful' do + expect(response).to be_success + expect(response.payload[:integration]).to be_a(::AlertManagement::HttpIntegration) + end + end + end + + context 'with an existing Prometheus integration' do + let_it_be(:http_integration) { create(:alert_management_prometheus_integration, project: project) } + + context 'when creating a different type of integration' do + it 'is successful' do + expect(response).to be_success + expect(response.payload[:integration]).to be_a(::AlertManagement::HttpIntegration) + end + end + + context 'when creating the same time of integration' do + let(:params) { { type_identifier: :prometheus, name: 'Prometheus' } } + + it_behaves_like 'error response', 'Multiple integrations of a single type are not supported for this project' + end + end end end end diff --git a/spec/services/alert_management/http_integrations/destroy_service_spec.rb b/spec/services/alert_management/http_integrations/destroy_service_spec.rb index a8e9746cb85..e3d9ddfbad8 100644 --- a/spec/services/alert_management/http_integrations/destroy_service_spec.rb +++ b/spec/services/alert_management/http_integrations/destroy_service_spec.rb @@ -47,6 +47,13 @@ RSpec.describe AlertManagement::HttpIntegrations::DestroyService, feature_catego it_behaves_like 'error response', 'Name cannot be removed' end + context 'when destroying a legacy Prometheus integration' do + let_it_be(:existing_integration) { create(:alert_management_prometheus_integration, :legacy, project: project) } + let!(:integration) { existing_integration } + + it_behaves_like 'error response', 'Legacy Prometheus integrations cannot currently be removed' + end + it 'successfully returns the integration' do expect(response).to be_success diff --git a/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb b/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb index e8f86b4d7c5..ca766590ada 100644 --- a/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb +++ b/spec/services/authorized_project_update/find_records_due_for_refresh_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService, feature_category: :projects do +RSpec.describe AuthorizedProjectUpdate::FindRecordsDueForRefreshService, feature_category: :groups_and_projects do # We're using let! here so that any expectations for the service class are not # triggered twice. let!(:project) { create(:project) } diff --git a/spec/services/authorized_project_update/periodic_recalculate_service_spec.rb b/spec/services/authorized_project_update/periodic_recalculate_service_spec.rb index 51cab6d188b..88099e76a8c 100644 --- a/spec/services/authorized_project_update/periodic_recalculate_service_spec.rb +++ b/spec/services/authorized_project_update/periodic_recalculate_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe AuthorizedProjectUpdate::PeriodicRecalculateService, feature_category: :projects do +RSpec.describe AuthorizedProjectUpdate::PeriodicRecalculateService, feature_category: :groups_and_projects do subject(:service) { described_class.new } describe '#execute' do diff --git a/spec/services/authorized_project_update/project_access_changed_service_spec.rb b/spec/services/authorized_project_update/project_access_changed_service_spec.rb index 7c09d7755ca..12c2f7cb5d5 100644 --- a/spec/services/authorized_project_update/project_access_changed_service_spec.rb +++ b/spec/services/authorized_project_update/project_access_changed_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe AuthorizedProjectUpdate::ProjectAccessChangedService, feature_category: :projects do +RSpec.describe AuthorizedProjectUpdate::ProjectAccessChangedService, feature_category: :groups_and_projects do describe '#execute' do it 'executes projects_authorizations refresh' do expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).to receive(:bulk_perform_async) diff --git a/spec/services/authorized_project_update/project_recalculate_per_user_service_spec.rb b/spec/services/authorized_project_update/project_recalculate_per_user_service_spec.rb index 7b2dd52810f..9cc31810b84 100644 --- a/spec/services/authorized_project_update/project_recalculate_per_user_service_spec.rb +++ b/spec/services/authorized_project_update/project_recalculate_per_user_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe AuthorizedProjectUpdate::ProjectRecalculatePerUserService, '#execute', feature_category: :projects do +RSpec.describe AuthorizedProjectUpdate::ProjectRecalculatePerUserService, '#execute', feature_category: :groups_and_projects do let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } let_it_be(:another_user) { create(:user) } diff --git a/spec/services/authorized_project_update/project_recalculate_service_spec.rb b/spec/services/authorized_project_update/project_recalculate_service_spec.rb index 8360f3c67ab..4ccbaa3185d 100644 --- a/spec/services/authorized_project_update/project_recalculate_service_spec.rb +++ b/spec/services/authorized_project_update/project_recalculate_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe AuthorizedProjectUpdate::ProjectRecalculateService, '#execute', feature_category: :projects do +RSpec.describe AuthorizedProjectUpdate::ProjectRecalculateService, '#execute', feature_category: :groups_and_projects do let_it_be(:project) { create(:project) } subject(:execute) { described_class.new(project).execute } diff --git a/spec/services/bulk_imports/archive_extraction_service_spec.rb b/spec/services/bulk_imports/archive_extraction_service_spec.rb index 40f8d8718ae..5593218c259 100644 --- a/spec/services/bulk_imports/archive_extraction_service_spec.rb +++ b/spec/services/bulk_imports/archive_extraction_service_spec.rb @@ -53,7 +53,7 @@ RSpec.describe BulkImports::ArchiveExtractionService, feature_category: :importe context 'when filepath is being traversed' do it 'raises an error' do expect { described_class.new(tmpdir: File.join(Dir.mktmpdir, 'test', '..'), filename: 'name').execute } - .to raise_error(Gitlab::Utils::PathTraversalAttackError, 'Invalid path') + .to raise_error(Gitlab::PathTraversal::PathTraversalAttackError, 'Invalid path') end end end diff --git a/spec/services/bulk_imports/file_decompression_service_spec.rb b/spec/services/bulk_imports/file_decompression_service_spec.rb index 9b8320aeac5..9d80ab3cd8f 100644 --- a/spec/services/bulk_imports/file_decompression_service_spec.rb +++ b/spec/services/bulk_imports/file_decompression_service_spec.rb @@ -66,7 +66,7 @@ RSpec.describe BulkImports::FileDecompressionService, feature_category: :importe subject { described_class.new(tmpdir: File.join(Dir.mktmpdir, 'test', '..'), filename: 'filename') } it 'raises an error' do - expect { subject.execute }.to raise_error(Gitlab::Utils::PathTraversalAttackError, 'Invalid path') + expect { subject.execute }.to raise_error(Gitlab::PathTraversal::PathTraversalAttackError, 'Invalid path') end end diff --git a/spec/services/bulk_imports/file_download_service_spec.rb b/spec/services/bulk_imports/file_download_service_spec.rb index 7c64d6efc65..cbeea5b0f46 100644 --- a/spec/services/bulk_imports/file_download_service_spec.rb +++ b/spec/services/bulk_imports/file_download_service_spec.rb @@ -95,7 +95,7 @@ RSpec.describe BulkImports::FileDownloadService, feature_category: :importers do it 'raises an error' do expect { subject.execute }.to raise_error( described_class::ServiceError, - 'File size 1000 Bytes exceeds limit of 1 Byte' + 'File size 1000 B exceeds limit of 1 B' ) end end @@ -128,7 +128,7 @@ RSpec.describe BulkImports::FileDownloadService, feature_category: :importers do it 'raises an error' do expect { subject.execute }.to raise_error( described_class::ServiceError, - 'File size 151 Bytes exceeds limit of 150 Bytes' + 'File size 151 B exceeds limit of 150 B' ) end end @@ -281,7 +281,7 @@ RSpec.describe BulkImports::FileDownloadService, feature_category: :importers do it 'raises an error' do expect { subject.execute }.to raise_error( - Gitlab::Utils::PathTraversalAttackError, + Gitlab::PathTraversal::PathTraversalAttackError, 'Invalid path' ) end diff --git a/spec/services/ci/cancel_pipeline_service_spec.rb b/spec/services/ci/cancel_pipeline_service_spec.rb new file mode 100644 index 00000000000..c4a1e1c26d1 --- /dev/null +++ b/spec/services/ci/cancel_pipeline_service_spec.rb @@ -0,0 +1,191 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::CancelPipelineService, :aggregate_failures, feature_category: :continuous_integration do + let_it_be(:project) { create(:project) } + let_it_be(:current_user) { project.owner } + let_it_be(:pipeline) { create(:ci_pipeline, project: project) } + + let(:service) do + described_class.new( + pipeline: pipeline, + current_user: current_user, + cascade_to_children: cascade_to_children, + auto_canceled_by_pipeline_id: auto_canceled_by_pipeline_id, + execute_async: execute_async) + end + + let(:cascade_to_children) { true } + let(:auto_canceled_by_pipeline_id) { nil } + let(:execute_async) { true } + + shared_examples 'force_execute' do + context 'when pipeline is not cancelable' do + it 'returns an error' do + expect(response).to be_error + expect(response.reason).to eq(:pipeline_not_cancelable) + end + end + + context 'when pipeline is cancelable' do + before do + create(:ci_build, :running, pipeline: pipeline) + create(:ci_build, :created, pipeline: pipeline) + create(:ci_build, :success, pipeline: pipeline) + end + + it 'logs the event' do + allow(Gitlab::AppJsonLogger).to receive(:info) + + subject + + expect(Gitlab::AppJsonLogger) + .to have_received(:info) + .with( + a_hash_including( + event: 'pipeline_cancel_running', + pipeline_id: pipeline.id, + auto_canceled_by_pipeline_id: nil, + cascade_to_children: true, + execute_async: true + ) + ) + end + + it 'cancels all cancelable jobs' do + expect(response).to be_success + 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 } + + 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]) + end + end + + context 'when pipeline has child pipelines' do + let(:child_pipeline) { create(:ci_pipeline, child_of: pipeline) } + let!(:child_job) { create(:ci_build, :running, pipeline: child_pipeline) } + let(:grandchild_pipeline) { create(:ci_pipeline, child_of: child_pipeline) } + let!(:grandchild_job) { create(:ci_build, :running, pipeline: grandchild_pipeline) } + + before do + child_pipeline.source_bridge.update!(status: :running) + grandchild_pipeline.source_bridge.update!(status: :running) + end + + context 'when execute_async: false' do + let(:execute_async) { false } + + it 'cancels the bridge jobs and child jobs' do + expect(response).to be_success + + expect(pipeline.bridges.pluck(:status)).to be_all('canceled') + expect(child_pipeline.bridges.pluck(:status)).to be_all('canceled') + expect(child_job.reload).to be_canceled + expect(grandchild_job.reload).to be_canceled + end + end + + context 'when execute_async: true' do + it 'schedules the child pipelines for async cancelation' do + expect(::Ci::CancelPipelineWorker) + .to receive(:perform_async) + .with(child_pipeline.id, nil) + + expect(::Ci::CancelPipelineWorker) + .to receive(:perform_async) + .with(grandchild_pipeline.id, nil) + + expect(response).to be_success + + expect(pipeline.bridges.pluck(:status)).to be_all('canceled') + end + end + + context 'when cascade_to_children: false' do + let(:execute_async) { true } + let(:cascade_to_children) { false } + + it 'does not cancel child pipelines' do + expect(::Ci::CancelPipelineWorker) + .not_to receive(:perform_async) + + expect(response).to be_success + + expect(pipeline.bridges.pluck(:status)).to be_all('canceled') + expect(child_job.reload).to be_running + end + end + end + + context 'when preloading relations' do + let(:pipeline1) { create(:ci_pipeline, :created) } + let(:pipeline2) { create(:ci_pipeline, :created) } + + before do + create(:ci_build, :pending, pipeline: pipeline1) + create(:generic_commit_status, :pending, pipeline: pipeline1) + + create(:ci_build, :pending, pipeline: pipeline2) + create(:ci_build, :pending, pipeline: pipeline2) + create(:generic_commit_status, :pending, pipeline: pipeline2) + create(:generic_commit_status, :pending, pipeline: pipeline2) + create(:generic_commit_status, :pending, pipeline: pipeline2) + end + + it 'preloads relations for each build to avoid N+1 queries' do + control1 = ActiveRecord::QueryRecorder.new do + described_class.new(pipeline: pipeline1, current_user: current_user).force_execute + end + + control2 = ActiveRecord::QueryRecorder.new do + described_class.new(pipeline: pipeline2, current_user: current_user).force_execute + end + + extra_update_queries = 4 # transition ... => :canceled, queue pop + extra_generic_commit_status_validation_queries = 2 # name_uniqueness_across_types + + expect(control2.count) + .to eq(control1.count + extra_update_queries + extra_generic_commit_status_validation_queries) + end + end + end + end + + describe '#execute' do + subject(:response) { service.execute } + + it_behaves_like 'force_execute' + + context 'when user does not have permissions to cancel the pipeline' do + let(:current_user) { create(:user) } + + it 'returns an error when user does not have permissions to cancel pipeline' do + expect(response).to be_error + expect(response.reason).to eq(:insufficient_permissions) + end + end + end + + describe '#force_execute' do + subject(:response) { service.force_execute } + + it_behaves_like 'force_execute' + + context 'when pipeline is not provided' do + let(:pipeline) { nil } + + it 'returns an error' do + expect(response).to be_error + expect(response.reason).to eq(:no_pipeline) + end + end + end +end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index b08dda72a69..f75c95c66f9 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -810,32 +810,6 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes end end - context 'when FF `ci_remove_legacy_predefined_variables` is disabled' do - before do - stub_feature_flags(ci_remove_legacy_predefined_variables: false) - end - - context 'with environment name including persisted variables' do - before do - config = YAML.dump( - deploy: { - environment: { name: "review/id1$CI_PIPELINE_ID/id2$CI_BUILD_ID" }, - script: 'ls' - } - ) - - stub_ci_pipeline_yaml_file(config) - end - - it 'skips persisted variables in environment name' do - result = execute_service.payload - - expect(result).to be_persisted - expect(Environment.find_by(name: "review/id1/id2")).to be_present - end - end - end - context 'environment with Kubernetes configuration' do let(:kubernetes_namespace) { 'custom-namespace' } diff --git a/spec/services/ci/destroy_pipeline_service_spec.rb b/spec/services/ci/destroy_pipeline_service_spec.rb index a1883d90b0a..eff9b9e4b63 100644 --- a/spec/services/ci/destroy_pipeline_service_spec.rb +++ b/spec/services/ci/destroy_pipeline_service_spec.rb @@ -96,17 +96,15 @@ RSpec.describe ::Ci::DestroyPipelineService, feature_category: :continuous_integ let!(:child_build) { create(:ci_build, :running, pipeline: child_pipeline) } it 'cancels the pipelines sync' do - # turn off deletion for all instances of pipeline to allow for testing cancellation - allow(pipeline).to receive_message_chain(:reset, :destroy!) - allow_next_found_instance_of(Ci::Pipeline) { |p| allow(p).to receive_message_chain(:reset, :destroy!) } + cancel_pipeline_service = instance_double(::Ci::CancelPipelineService) + expect(::Ci::CancelPipelineService) + .to receive(:new) + .with(pipeline: pipeline, current_user: user, cascade_to_children: true, execute_async: false) + .and_return(cancel_pipeline_service) - # ensure cancellation happens sync so we accumulate minutes - expect(::Ci::CancelPipelineWorker).not_to receive(:perform) + expect(cancel_pipeline_service).to receive(:force_execute) subject - - expect(build.reload.status).to eq('canceled') - expect(child_build.reload.status).to eq('canceled') end end end diff --git a/spec/services/ci/job_artifacts/create_service_spec.rb b/spec/services/ci/job_artifacts/create_service_spec.rb index f71d7feb04a..7e471bf39a1 100644 --- a/spec/services/ci/job_artifacts/create_service_spec.rb +++ b/spec/services/ci/job_artifacts/create_service_spec.rb @@ -82,7 +82,11 @@ RSpec.describe Ci::JobArtifacts::CreateService, :clean_gitlab_redis_shared_state before do stub_artifacts_object_storage(JobArtifactUploader, direct_upload: true) - allow(JobArtifactUploader).to receive(:generate_final_store_path).and_return(final_store_path) + + allow(JobArtifactUploader) + .to receive(:generate_final_store_path) + .with(root_id: project.id) + .and_return(final_store_path) end it 'includes the authorize headers' do @@ -103,14 +107,6 @@ RSpec.describe Ci::JobArtifacts::CreateService, :clean_gitlab_redis_shared_state it_behaves_like 'handling lsif artifact' it_behaves_like 'validating requirements' - - context 'with ci_artifacts_upload_to_final_location feature flag disabled' do - before do - stub_feature_flags(ci_artifacts_upload_to_final_location: false) - end - - it_behaves_like 'uploading to temp location', :object_storage - end end context 'and direct upload is disabled' do diff --git a/spec/services/ci/job_token_scope/remove_project_service_spec.rb b/spec/services/ci/job_token_scope/remove_project_service_spec.rb index 5b39f8908f2..c1f28ea4523 100644 --- a/spec/services/ci/job_token_scope/remove_project_service_spec.rb +++ b/spec/services/ci/job_token_scope/remove_project_service_spec.rb @@ -52,6 +52,16 @@ RSpec.describe Ci::JobTokenScope::RemoveProjectService, feature_category: :conti it_behaves_like 'returns error', "Source project cannot be removed from the job token scope" end + + context 'when target project is not in the job token scope' do + let_it_be(:target_project) { create(:project, :public) } + + before do + project.add_maintainer(current_user) + end + + it_behaves_like 'returns error', 'Target project is not in the job token scope' + end end end end diff --git a/spec/services/ci/pipeline_creation/cancel_redundant_pipelines_service_spec.rb b/spec/services/ci/pipeline_creation/cancel_redundant_pipelines_service_spec.rb index 402bc2faa81..905ccf164ca 100644 --- a/spec/services/ci/pipeline_creation/cancel_redundant_pipelines_service_spec.rb +++ b/spec/services/ci/pipeline_creation/cancel_redundant_pipelines_service_spec.rb @@ -56,14 +56,6 @@ RSpec.describe Ci::PipelineCreation::CancelRedundantPipelinesService, feature_ca ) end - it 'cancels the builds with 2 queries to avoid query timeout' do - second_query_regex = /WHERE "ci_pipelines"\."id" = \d+ AND \(NOT EXISTS/ - recorder = ActiveRecord::QueryRecorder.new { execute } - second_query = recorder.occurrences.keys.filter { |occ| occ =~ second_query_regex } - - expect(second_query).to be_one - end - context 'when the previous pipeline has a child pipeline' do let(:child_pipeline) { create(:ci_pipeline, child_of: prev_pipeline) } @@ -240,6 +232,241 @@ RSpec.describe Ci::PipelineCreation::CancelRedundantPipelinesService, feature_ca expect(build_statuses(pipeline)).to contain_exactly('pending') end end + + context 'when enable_cancel_redundant_pipelines_service FF is enabled' do + before do + stub_feature_flags(disable_cancel_redundant_pipelines_service: true) + end + + it 'does not cancel any build' do + subject + + expect(build_statuses(prev_pipeline)).to contain_exactly('running', 'success', 'created') + expect(build_statuses(pipeline)).to contain_exactly('pending') + end + end + end + + context 'when the use_offset_pagination_for_canceling_redundant_pipelines FF is off' do + # copy-paste from above + + before do + stub_feature_flags(use_offset_pagination_for_canceling_redundant_pipelines: false) + end + + describe '#execute!' do + subject(:execute) { service.execute } + + context 'when build statuses are set up correctly' do + it 'has builds of all statuses' do + expect(build_statuses(prev_pipeline)).to contain_exactly('running', 'success', 'created') + expect(build_statuses(pipeline)).to contain_exactly('pending') + end + end + + context 'when auto-cancel is enabled' do + before do + project.update!(auto_cancel_pending_pipelines: 'enabled') + end + + it 'cancels only previous interruptible builds' do + execute + + expect(build_statuses(prev_pipeline)).to contain_exactly('canceled', 'success', 'canceled') + expect(build_statuses(pipeline)).to contain_exactly('pending') + end + + it 'logs canceled pipelines' do + allow(Gitlab::AppLogger).to receive(:info) + + execute + + expect(Gitlab::AppLogger).to have_received(:info).with( + class: described_class.name, + message: "Pipeline #{pipeline.id} auto-canceling pipeline #{prev_pipeline.id}", + canceled_pipeline_id: prev_pipeline.id, + canceled_by_pipeline_id: pipeline.id, + canceled_by_pipeline_source: pipeline.source + ) + end + + context 'when the previous pipeline has a child pipeline' do + let(:child_pipeline) { create(:ci_pipeline, child_of: prev_pipeline) } + + context 'with another nested child pipeline' do + let(:another_child_pipeline) { create(:ci_pipeline, child_of: child_pipeline) } + + before do + create(:ci_build, :interruptible, :running, pipeline: another_child_pipeline) + create(:ci_build, :interruptible, :running, pipeline: another_child_pipeline) + end + + it 'cancels all nested child pipeline builds' do + expect(build_statuses(another_child_pipeline)).to contain_exactly('running', 'running') + + execute + + expect(build_statuses(another_child_pipeline)).to contain_exactly('canceled', 'canceled') + end + end + + context 'when started after pipeline was finished' do + before do + create(:ci_build, :interruptible, :running, pipeline: child_pipeline) + prev_pipeline.update!(status: "success") + end + + it 'cancels child pipeline builds' do + expect(build_statuses(child_pipeline)).to contain_exactly('running') + + execute + + expect(build_statuses(child_pipeline)).to contain_exactly('canceled') + end + end + + context 'when the child pipeline has interruptible running jobs' do + before do + create(:ci_build, :interruptible, :running, pipeline: child_pipeline) + create(:ci_build, :interruptible, :running, pipeline: child_pipeline) + end + + it 'cancels all child pipeline builds' do + expect(build_statuses(child_pipeline)).to contain_exactly('running', 'running') + + execute + + expect(build_statuses(child_pipeline)).to contain_exactly('canceled', 'canceled') + end + + context 'when the child pipeline includes completed interruptible jobs' do + before do + create(:ci_build, :interruptible, :failed, pipeline: child_pipeline) + create(:ci_build, :interruptible, :success, pipeline: child_pipeline) + end + + it 'cancels all child pipeline builds with a cancelable_status' do + expect(build_statuses(child_pipeline)).to contain_exactly('running', 'running', 'failed', 'success') + + execute + + expect(build_statuses(child_pipeline)).to contain_exactly('canceled', 'canceled', 'failed', 'success') + end + end + end + + context 'when the child pipeline has started non-interruptible job' do + before do + create(:ci_build, :interruptible, :running, pipeline: child_pipeline) + # non-interruptible started + create(:ci_build, :success, pipeline: child_pipeline) + end + + it 'does not cancel any child pipeline builds' do + expect(build_statuses(child_pipeline)).to contain_exactly('running', 'success') + + execute + + expect(build_statuses(child_pipeline)).to contain_exactly('running', 'success') + end + end + + context 'when the child pipeline has non-interruptible non-started job' do + before do + create(:ci_build, :interruptible, :running, pipeline: child_pipeline) + end + + not_started_statuses = Ci::HasStatus::AVAILABLE_STATUSES - Ci::HasStatus::STARTED_STATUSES + context 'when the jobs are cancelable' do + cancelable_not_started_statuses = + Set.new(not_started_statuses).intersection(Ci::HasStatus::CANCELABLE_STATUSES) + cancelable_not_started_statuses.each do |status| + it "cancels all child pipeline builds when build status #{status} included" do + # non-interruptible but non-started + create(:ci_build, status.to_sym, pipeline: child_pipeline) + + expect(build_statuses(child_pipeline)).to contain_exactly('running', status) + + execute + + expect(build_statuses(child_pipeline)).to contain_exactly('canceled', 'canceled') + end + end + end + + context 'when the jobs are not cancelable' do + not_cancelable_not_started_statuses = not_started_statuses - Ci::HasStatus::CANCELABLE_STATUSES + not_cancelable_not_started_statuses.each do |status| + it "does not cancel child pipeline builds when build status #{status} included" do + # non-interruptible but non-started + create(:ci_build, status.to_sym, pipeline: child_pipeline) + + expect(build_statuses(child_pipeline)).to contain_exactly('running', status) + + execute + + expect(build_statuses(child_pipeline)).to contain_exactly('canceled', status) + end + end + end + end + end + + context 'when the pipeline is a child pipeline' do + let!(:parent_pipeline) { create(:ci_pipeline, project: project, sha: new_commit.sha) } + let(:pipeline) { create(:ci_pipeline, child_of: parent_pipeline) } + + before do + create(:ci_build, :interruptible, :running, pipeline: parent_pipeline) + create(:ci_build, :interruptible, :running, pipeline: parent_pipeline) + end + + it 'does not cancel any builds' do + expect(build_statuses(prev_pipeline)).to contain_exactly('running', 'success', 'created') + expect(build_statuses(parent_pipeline)).to contain_exactly('running', 'running') + + execute + + expect(build_statuses(prev_pipeline)).to contain_exactly('running', 'success', 'created') + expect(build_statuses(parent_pipeline)).to contain_exactly('running', 'running') + end + end + + context 'when the previous pipeline source is webide' do + let(:prev_pipeline) { create(:ci_pipeline, :webide, project: project) } + + it 'does not cancel builds of the previous pipeline' do + execute + + expect(build_statuses(prev_pipeline)).to contain_exactly('created', 'running', 'success') + expect(build_statuses(pipeline)).to contain_exactly('pending') + end + end + + it 'does not cancel future pipelines' do + expect(prev_pipeline.id).to be < pipeline.id + expect(build_statuses(pipeline)).to contain_exactly('pending') + expect(build_statuses(prev_pipeline)).to contain_exactly('running', 'success', 'created') + + described_class.new(prev_pipeline).execute + + expect(build_statuses(pipeline.reload)).to contain_exactly('pending') + end + end + + context 'when auto-cancel is disabled' do + before do + project.update!(auto_cancel_pending_pipelines: 'disabled') + end + + it 'does not cancel any build' do + subject + + expect(build_statuses(prev_pipeline)).to contain_exactly('running', 'success', 'created') + expect(build_statuses(pipeline)).to contain_exactly('pending') + end + end + end end private diff --git a/spec/services/ci/pipeline_processing/atomic_processing_service/status_collection_spec.rb b/spec/services/ci/pipeline_processing/atomic_processing_service/status_collection_spec.rb index 89b3c45485b..8f8c7b5ce08 100644 --- a/spec/services/ci/pipeline_processing/atomic_processing_service/status_collection_spec.rb +++ b/spec/services/ci/pipeline_processing/atomic_processing_service/status_collection_spec.rb @@ -106,4 +106,11 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService::StatusCollection .to contain_exactly(build_a.id, build_b.id, test_a.id, test_b.id, deploy.id) end end + + describe '#stopped_job_names' do + it 'returns names of jobs that have a stopped status' do + expect(collection.stopped_job_names) + .to contain_exactly(build_a.name, build_b.name) + 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 8c52603e769..c43f1e5264e 100644 --- a/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb +++ b/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb @@ -927,6 +927,203 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category end end + context 'when jobs change from stopped to alive status during pipeline processing' do + around do |example| + Sidekiq::Testing.fake! { example.run } + end + + let(:config) do + <<-YAML + stages: [test, deploy] + + manual1: + stage: test + when: manual + script: exit 0 + + manual2: + stage: test + when: manual + script: exit 0 + + test1: + stage: test + needs: [manual1] + script: exit 0 + + test2: + stage: test + needs: [manual2] + script: exit 0 + + deploy1: + stage: deploy + needs: [manual1, manual2] + script: exit 0 + + deploy2: + stage: deploy + needs: [test2] + script: exit 0 + YAML + end + + let(:pipeline) do + Ci::CreatePipelineService.new(project, user, { ref: 'master' }).execute(:push).payload + end + + let(:manual1) { all_builds.find_by(name: 'manual1') } + let(:manual2) { all_builds.find_by(name: 'manual2') } + + let(:statuses_0) do + { 'manual1': 'created', 'manual2': 'created', 'test1': 'created', 'test2': 'created', 'deploy1': 'created', 'deploy2': 'created' } + end + + let(:statuses_1) do + { 'manual1': 'manual', 'manual2': 'manual', 'test1': 'skipped', 'test2': 'skipped', 'deploy1': 'skipped', 'deploy2': 'skipped' } + end + + let(:statuses_2) do + { 'manual1': 'pending', 'manual2': 'pending', 'test1': 'skipped', 'test2': 'skipped', 'deploy1': 'skipped', 'deploy2': 'skipped' } + end + + let(:statuses_3) do + { 'manual1': 'pending', 'manual2': 'pending', 'test1': 'created', 'test2': 'created', 'deploy1': 'created', 'deploy2': 'created' } + end + + let(:log_info) do + { + class: described_class.name.to_s, + message: 'Running ResetSkippedJobsService on new alive jobs', + project_id: project.id, + pipeline_id: pipeline.id, + user_id: user.id, + jobs_count: 2 + } + end + + before do + stub_ci_pipeline_yaml_file(config) + pipeline # Create the pipeline + end + + # Since this is a test for a race condition, we are calling internal method `enqueue!` + # instead of `play` and stubbing `new_alive_jobs` of the service class. + it 'runs ResetSkippedJobsService on the new alive jobs and logs event' do + # Initial control without any pipeline processing + expect(all_builds_names_and_statuses).to eq(statuses_0) + + process_pipeline + + # Initial control after the first pipeline processing + expect(all_builds_names_and_statuses).to eq(statuses_1) + + # Change the manual jobs from stopped to alive status. + # We don't use `play` to avoid running `ResetSkippedJobsService`. + manual1.enqueue! + manual2.enqueue! + + # Statuses after playing the manual jobs + expect(all_builds_names_and_statuses).to eq(statuses_2) + + mock_play_jobs_during_processing([manual1, manual2]) + + expect(Ci::ResetSkippedJobsService).to receive(:new).once.and_call_original + + process_pipeline + + expect(all_builds_names_and_statuses).to eq(statuses_3) + end + + it 'logs event' do + expect(Gitlab::AppJsonLogger).to receive(:info).once.with(log_info) + + mock_play_jobs_during_processing([manual1, manual2]) + process_pipeline + end + + context 'when the new alive jobs belong to different users' do + let_it_be(:user2) { create(:user) } + + before do + process_pipeline # First pipeline processing + + # Change the manual jobs from stopped to alive status + manual1.enqueue! + manual2.enqueue! + + manual2.update!(user: user2) + + mock_play_jobs_during_processing([manual1, manual2]) + end + + it 'runs ResetSkippedJobsService on the new alive jobs' do + # Statuses after playing the manual jobs + expect(all_builds_names_and_statuses).to eq(statuses_2) + + # Since there are two different users, we expect this service to be called twice. + expect(Ci::ResetSkippedJobsService).to receive(:new).twice.and_call_original + + process_pipeline + + expect(all_builds_names_and_statuses).to eq(statuses_3) + end + + # In this scenario, the new alive jobs (manual1 and manual2) have different users. + # We can only know for certain the assigned user of dependent jobs that are exclusive + # to either manual1 or manual2. Otherwise, the assigned user will depend on which of + # the new alive jobs get processed first by ResetSkippedJobsService. + it 'assigns the correct user to the dependent jobs' do + test1 = all_builds.find_by(name: 'test1') + test2 = all_builds.find_by(name: 'test2') + + expect(test1.user).to eq(user) + expect(test2.user).to eq(user) + + process_pipeline + + expect(test1.reset.user).to eq(user) + expect(test2.reset.user).to eq(user2) + end + + it 'logs event' do + expect(Gitlab::AppJsonLogger).to receive(:info).once.with(log_info.merge(jobs_count: 1)) + expect(Gitlab::AppJsonLogger).to receive(:info).once.with(log_info.merge(user_id: user2.id, jobs_count: 1)) + + mock_play_jobs_during_processing([manual1, manual2]) + process_pipeline + end + end + + context 'when FF `ci_reset_skipped_jobs_in_atomic_processing` is disabled' do + before do + stub_feature_flags(ci_reset_skipped_jobs_in_atomic_processing: false) + + process_pipeline # First pipeline processing + + # Change the manual jobs from stopped to alive status + manual1.enqueue! + manual2.enqueue! + + mock_play_jobs_during_processing([manual1, manual2]) + end + + it 'does not run ResetSkippedJobsService' do + expect(Ci::ResetSkippedJobsService).not_to receive(:new) + + process_pipeline + + expect(all_builds_names_and_statuses).to eq(statuses_2) + end + + it 'does not log event' do + expect(Gitlab::AppJsonLogger).not_to receive(:info) + + process_pipeline + end + end + end + context 'when a bridge job has parallel:matrix config', :sidekiq_inline do let(:parent_config) do <<-EOY @@ -1085,7 +1282,12 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category def builds_names_and_statuses builds.each_with_object({}) do |b, h| h[b.name.to_sym] = b.status - h + end + end + + def all_builds_names_and_statuses + all_builds.each_with_object({}) do |b, h| + h[b.name.to_sym] = b.status end end @@ -1167,4 +1369,18 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService, feature_category def process_pipeline described_class.new(pipeline).execute end + + # A status collection is initialized at the start of pipeline processing and then again at the + # end of processing. Here we simulate "playing" the given jobs during pipeline processing by + # stubbing stopped_job_names so that they appear to have been stopped at the beginning of + # processing and then later changed to alive status at the end. + def mock_play_jobs_during_processing(jobs) + collection = Ci::PipelineProcessing::AtomicProcessingService::StatusCollection.new(pipeline) + + allow(collection).to receive(:stopped_job_names).and_return(jobs.map(&:name), []) + + # Return the same collection object for every instance of StatusCollection + allow(Ci::PipelineProcessing::AtomicProcessingService::StatusCollection).to receive(:new) + .and_return(collection) + end end diff --git a/spec/services/ci/pipelines/add_job_service_spec.rb b/spec/services/ci/pipelines/add_job_service_spec.rb index 6380a6a5ec3..9fb1d6933c6 100644 --- a/spec/services/ci/pipelines/add_job_service_spec.rb +++ b/spec/services/ci/pipelines/add_job_service_spec.rb @@ -86,15 +86,5 @@ RSpec.describe Ci::Pipelines::AddJobService, feature_category: :continuous_integ expect(execute.payload[:job]).to eq(job) end end - - it 'locks pipelines and stages before persisting builds', :aggregate_failures do - expect(job).not_to be_persisted - - recorder = ActiveRecord::QueryRecorder.new(skip_cached: false) { execute } - entries = recorder.log.select { |query| query.match(/LOCK|INSERT INTO ".{0,2}ci_builds"/) } - - expect(entries.size).to eq(2) - expect(entries.first).to match(/LOCK "ci_pipelines", "ci_stages" IN ROW SHARE MODE;/) - end end end diff --git a/spec/services/ci/reset_skipped_jobs_service_spec.rb b/spec/services/ci/reset_skipped_jobs_service_spec.rb index ba6a4a4e822..88c6f56dd41 100644 --- a/spec/services/ci/reset_skipped_jobs_service_spec.rb +++ b/spec/services/ci/reset_skipped_jobs_service_spec.rb @@ -406,294 +406,6 @@ RSpec.describe Ci::ResetSkippedJobsService, :sidekiq_inline, feature_category: : it_behaves_like 'with same-stage needs' end - context 'when FF is `ci_support_reset_skipped_jobs_for_multiple_jobs` disabled' do - before do - stub_feature_flags(ci_support_reset_skipped_jobs_for_multiple_jobs: false) - end - - context 'with a stage-dag mixed pipeline' do - let(:config) do - <<-YAML - stages: [a, b, c] - - a1: - stage: a - script: exit $(($RANDOM % 2)) - - a2: - stage: a - script: exit 0 - needs: [a1] - - a3: - stage: a - script: exit 0 - needs: [a2] - - b1: - stage: b - script: exit 0 - needs: [] - - b2: - stage: b - script: exit 0 - needs: [a2] - - c1: - stage: c - script: exit 0 - needs: [b2] - - c2: - stage: c - script: exit 0 - YAML - end - - let(:pipeline) do - Ci::CreatePipelineService.new(project, user, { ref: 'master' }).execute(:push).payload - end - - let(:a1) { find_job('a1') } - let(:b1) { find_job('b1') } - - before do - stub_ci_pipeline_yaml_file(config) - check_jobs_statuses( - a1: 'pending', - a2: 'created', - a3: 'created', - b1: 'pending', - b2: 'created', - c1: 'created', - c2: 'created' - ) - - b1.success! - check_jobs_statuses( - a1: 'pending', - a2: 'created', - a3: 'created', - b1: 'success', - b2: 'created', - c1: 'created', - c2: 'created' - ) - - a1.drop! - check_jobs_statuses( - a1: 'failed', - a2: 'skipped', - a3: 'skipped', - b1: 'success', - b2: 'skipped', - c1: 'skipped', - c2: 'skipped' - ) - - new_a1 = Ci::RetryJobService.new(project, user).clone!(a1) - new_a1.enqueue! - check_jobs_statuses( - a1: 'pending', - a2: 'skipped', - a3: 'skipped', - b1: 'success', - b2: 'skipped', - c1: 'skipped', - c2: 'skipped' - ) - end - - it 'marks subsequent skipped jobs as processable' do - execute_after_requeue_service(a1) - - check_jobs_statuses( - a1: 'pending', - a2: 'created', - a3: 'created', - b1: 'success', - b2: 'created', - c1: 'created', - c2: 'created' - ) - end - - context 'when executed by a different user than the original owner' do - let(:retryer) { create(:user).tap { |u| project.add_maintainer(u) } } - let(:service) { described_class.new(project, retryer) } - - it 'reassigns jobs with updated statuses to the retryer' do - expect(jobs_name_status_owner_needs).to contain_exactly( - { 'name' => 'a1', 'status' => 'pending', 'user_id' => user.id, 'needs' => [] }, - { 'name' => 'a2', 'status' => 'skipped', 'user_id' => user.id, 'needs' => ['a1'] }, - { 'name' => 'a3', 'status' => 'skipped', 'user_id' => user.id, 'needs' => ['a2'] }, - { 'name' => 'b1', 'status' => 'success', 'user_id' => user.id, 'needs' => [] }, - { 'name' => 'b2', 'status' => 'skipped', 'user_id' => user.id, 'needs' => ['a2'] }, - { 'name' => 'c1', 'status' => 'skipped', 'user_id' => user.id, 'needs' => ['b2'] }, - { 'name' => 'c2', 'status' => 'skipped', 'user_id' => user.id, 'needs' => [] } - ) - - execute_after_requeue_service(a1) - - expect(jobs_name_status_owner_needs).to contain_exactly( - { 'name' => 'a1', 'status' => 'pending', 'user_id' => user.id, 'needs' => [] }, - { 'name' => 'a2', 'status' => 'created', 'user_id' => retryer.id, 'needs' => ['a1'] }, - { 'name' => 'a3', 'status' => 'created', 'user_id' => retryer.id, 'needs' => ['a2'] }, - { 'name' => 'b1', 'status' => 'success', 'user_id' => user.id, 'needs' => [] }, - { 'name' => 'b2', 'status' => 'created', 'user_id' => retryer.id, 'needs' => ['a2'] }, - { 'name' => 'c1', 'status' => 'created', 'user_id' => retryer.id, 'needs' => ['b2'] }, - { 'name' => 'c2', 'status' => 'created', 'user_id' => retryer.id, 'needs' => [] } - ) - end - end - end - - context 'with stage-dag mixed pipeline with some same-stage needs' do - let(:config) do - <<-YAML - stages: [a, b, c] - - a1: - stage: a - script: exit $(($RANDOM % 2)) - - a2: - stage: a - script: exit 0 - needs: [a1] - - b1: - stage: b - script: exit 0 - needs: [b2] - - b2: - stage: b - script: exit 0 - - c1: - stage: c - script: exit 0 - needs: [b2] - - c2: - stage: c - script: exit 0 - YAML - end - - let(:pipeline) do - Ci::CreatePipelineService.new(project, user, { ref: 'master' }).execute(:push).payload - end - - let(:a1) { find_job('a1') } - - before do - stub_ci_pipeline_yaml_file(config) - check_jobs_statuses( - a1: 'pending', - a2: 'created', - b1: 'created', - b2: 'created', - c1: 'created', - c2: 'created' - ) - - a1.drop! - check_jobs_statuses( - a1: 'failed', - a2: 'skipped', - b1: 'skipped', - b2: 'skipped', - c1: 'skipped', - c2: 'skipped' - ) - - new_a1 = Ci::RetryJobService.new(project, user).clone!(a1) - new_a1.enqueue! - check_jobs_statuses( - a1: 'pending', - a2: 'skipped', - b1: 'skipped', - b2: 'skipped', - c1: 'skipped', - c2: 'skipped' - ) - end - - it 'marks subsequent skipped jobs as processable' do - execute_after_requeue_service(a1) - - check_jobs_statuses( - a1: 'pending', - a2: 'created', - b1: 'created', - b2: 'created', - c1: 'created', - c2: 'created' - ) - end - end - - context 'with same-stage needs' do - let(:config) do - <<-YAML - a: - script: exit $(($RANDOM % 2)) - - b: - script: exit 0 - needs: [a] - - c: - script: exit 0 - needs: [b] - YAML - end - - let(:pipeline) do - Ci::CreatePipelineService.new(project, user, { ref: 'master' }).execute(:push).payload - end - - let(:a) { find_job('a') } - - before do - stub_ci_pipeline_yaml_file(config) - check_jobs_statuses( - a: 'pending', - b: 'created', - c: 'created' - ) - - a.drop! - check_jobs_statuses( - a: 'failed', - b: 'skipped', - c: 'skipped' - ) - - new_a = Ci::RetryJobService.new(project, user).clone!(a) - new_a.enqueue! - check_jobs_statuses( - a: 'pending', - b: 'skipped', - c: 'skipped' - ) - end - - it 'marks subsequent skipped jobs as processable' do - execute_after_requeue_service(a) - - check_jobs_statuses( - a: 'pending', - b: 'created', - c: 'created' - ) - end - end - end - private def find_job(name) @@ -713,9 +425,4 @@ RSpec.describe Ci::ResetSkippedJobsService, :sidekiq_inline, feature_category: : job.attributes.slice('name', 'status', 'user_id').merge('needs' => job.needs.map(&:name)) end end - - # Remove this method when FF is `ci_support_reset_skipped_jobs_for_multiple_jobs` is removed - def execute_after_requeue_service(processable) - service.execute(processable) - end end diff --git a/spec/services/ci/runners/assign_runner_service_spec.rb b/spec/services/ci/runners/assign_runner_service_spec.rb index 92f6db2bdfb..00fbb5e2d26 100644 --- a/spec/services/ci/runners/assign_runner_service_spec.rb +++ b/spec/services/ci/runners/assign_runner_service_spec.rb @@ -3,10 +3,12 @@ require 'spec_helper' RSpec.describe ::Ci::Runners::AssignRunnerService, '#execute', feature_category: :runner_fleet do - subject(:execute) { described_class.new(runner, project, user).execute } + subject(:execute) { described_class.new(runner, new_project, user).execute } - let_it_be(:runner) { create(:ci_runner, :project, projects: [project]) } - let_it_be(:project) { create(:project) } + let_it_be(:owner_group) { create(:group) } + let_it_be(:owner_project) { create(:project, group: owner_group) } + let_it_be(:new_project) { create(:project) } + let_it_be(:runner) { create(:ci_runner, :project, projects: [owner_project]) } context 'without user' do let(:user) { nil } @@ -30,11 +32,54 @@ RSpec.describe ::Ci::Runners::AssignRunnerService, '#execute', feature_category: end end + context 'with authorized user' do + let(:user) { create(:user) } + + context 'with user owning runner and being maintainer of new project' do + before do + owner_project.group.add_owner(user) + new_project.add_maintainer(user) + end + + it 'calls assign_to on runner and returns success response' do + expect(runner).to receive(:assign_to).with(new_project, user).once.and_call_original + + is_expected.to be_success + end + end + + context 'with user owning runner' do + before do + owner_project.add_maintainer(user) + end + + it 'does not call assign_to on runner and returns error message', :aggregate_failures do + expect(runner).not_to receive(:assign_to) + + is_expected.to be_error + expect(execute.message).to eq('user not allowed to add runners to project') + end + end + + context 'with user being maintainer of new project', :aggregate_failures do + before do + new_project.add_maintainer(user) + end + + it 'does not call assign_to on runner and returns error message' do + expect(runner).not_to receive(:assign_to) + + is_expected.to be_error + expect(execute.message).to eq('user not allowed to assign runner') + end + end + end + context 'with admin user', :enable_admin_mode do - let(:user) { create_default(:user, :admin) } + let(:user) { create(:user, :admin) } it 'calls assign_to on runner and returns success response' do - expect(runner).to receive(:assign_to).with(project, user).once.and_call_original + expect(runner).to receive(:assign_to).with(new_project, user).once.and_call_original is_expected.to be_success end diff --git a/spec/services/ci/runners/stale_managers_cleanup_service_spec.rb b/spec/services/ci/runners/stale_managers_cleanup_service_spec.rb index a78506ca5f7..0a20c12bc15 100644 --- a/spec/services/ci/runners/stale_managers_cleanup_service_spec.rb +++ b/spec/services/ci/runners/stale_managers_cleanup_service_spec.rb @@ -12,7 +12,7 @@ RSpec.describe Ci::Runners::StaleManagersCleanupService, feature_category: :runn it 'does not clean any runner managers and returns :success status' do expect do expect(response).to be_success - expect(response.payload).to match({ deleted_managers: false }) + expect(response.payload).to match({ total_deleted: 0, batch_counts: [0] }) end.not_to change { Ci::RunnerManager.count }.from(1) end end @@ -25,10 +25,22 @@ RSpec.describe Ci::Runners::StaleManagersCleanupService, feature_category: :runn it 'only leaves non-stale runners' do expect(response).to be_success - expect(response.payload).to match({ deleted_managers: true }) + expect(response.payload).to match({ total_deleted: 2, batch_counts: [2, 0] }) expect(Ci::RunnerManager.all).to contain_exactly(runner_manager3) end + context 'with more stale runners than SUB_BATCH_LIMIT' do + before do + stub_const("#{described_class}::SUB_BATCH_LIMIT", 1) + end + + it 'only leaves non-stale runners' do + expect(response).to be_success + expect(response.payload).to match({ total_deleted: 2, batch_counts: [1, 1, 0] }) + expect(Ci::RunnerManager.all).to contain_exactly(runner_manager3) + end + end + context 'with more stale runners than MAX_DELETIONS' do before do stub_const("#{described_class}::MAX_DELETIONS", 1) @@ -37,7 +49,10 @@ RSpec.describe Ci::Runners::StaleManagersCleanupService, feature_category: :runn it 'only leaves non-stale runners' do expect do expect(response).to be_success - expect(response.payload).to match({ deleted_managers: true }) + expect(response.payload).to match({ + total_deleted: Ci::Runners::StaleManagersCleanupService::MAX_DELETIONS, + batch_counts: [1] + }) end.to change { Ci::RunnerManager.count }.by(-Ci::Runners::StaleManagersCleanupService::MAX_DELETIONS) end end diff --git a/spec/services/ci/unlock_artifacts_service_spec.rb b/spec/services/ci/unlock_artifacts_service_spec.rb index 0d6ac333587..c149eaf41e5 100644 --- a/spec/services/ci/unlock_artifacts_service_spec.rb +++ b/spec/services/ci/unlock_artifacts_service_spec.rb @@ -207,6 +207,8 @@ RSpec.describe Ci::UnlockArtifactsService, feature_category: :continuous_integra describe '#unlock_job_artifacts_query' do subject { described_class.new(pipeline.project, pipeline.user).unlock_job_artifacts_query(pipeline_ids) } + let(:builds_table) { Ci::Build.quoted_table_name } + context 'when given a single pipeline ID' do let(:pipeline_ids) { [older_pipeline.id] } @@ -219,12 +221,12 @@ RSpec.describe Ci::UnlockArtifactsService, feature_category: :continuous_integra WHERE "ci_job_artifacts"."job_id" IN (SELECT - "ci_builds"."id" + #{builds_table}."id" FROM - "ci_builds" + #{builds_table} WHERE - "ci_builds"."type" = 'Ci::Build' - AND "ci_builds"."commit_id" = #{older_pipeline.id}) + #{builds_table}."type" = 'Ci::Build' + AND #{builds_table}."commit_id" = #{older_pipeline.id}) RETURNING ("ci_job_artifacts"."id") SQL @@ -243,12 +245,12 @@ RSpec.describe Ci::UnlockArtifactsService, feature_category: :continuous_integra WHERE "ci_job_artifacts"."job_id" IN (SELECT - "ci_builds"."id" + #{builds_table}."id" FROM - "ci_builds" + #{builds_table} WHERE - "ci_builds"."type" = 'Ci::Build' - AND "ci_builds"."commit_id" IN (#{pipeline_ids.join(', ')})) + #{builds_table}."type" = 'Ci::Build' + AND #{builds_table}."commit_id" IN (#{pipeline_ids.join(', ')})) RETURNING ("ci_job_artifacts"."id") SQL diff --git a/spec/services/clusters/agent_tokens/create_service_spec.rb b/spec/services/clusters/agent_tokens/create_service_spec.rb index 803bd947629..431d7ce2079 100644 --- a/spec/services/clusters/agent_tokens/create_service_spec.rb +++ b/spec/services/clusters/agent_tokens/create_service_spec.rb @@ -78,6 +78,33 @@ RSpec.describe Clusters::AgentTokens::CreateService, feature_category: :deployme expect(subject.message).to eq(["Name can't be blank"]) end end + + context 'when the active agent tokens limit is reached' do + before do + create(:cluster_agent_token, agent: cluster_agent) + create(:cluster_agent_token, agent: cluster_agent) + end + + it 'returns an error' do + expect(subject.status).to eq(:error) + expect(subject.message).to eq('An agent can have only two active tokens at a time') + end + + context 'when cluster_agents_limit_tokens_created feature flag is disabled' do + before do + stub_feature_flags(cluster_agents_limit_tokens_created: false) + end + + it 'creates a new token' do + expect { subject }.to change { ::Clusters::AgentToken.count }.by(1) + end + + it 'returns success status', :aggregate_failures do + expect(subject.status).to eq(:success) + expect(subject.message).to be_nil + end + end + end end end end diff --git a/spec/services/database/mark_migration_service_spec.rb b/spec/services/database/mark_migration_service_spec.rb new file mode 100644 index 00000000000..5fd2268484e --- /dev/null +++ b/spec/services/database/mark_migration_service_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Database::MarkMigrationService, feature_category: :database do + let(:service) { described_class.new(connection: connection, version: version) } + let(:version) { 1 } + let(:connection) { ApplicationRecord.connection } + + let(:migrations) do + [ + instance_double( + ActiveRecord::MigrationProxy, + version: 1, + name: 'migration_pending', + filename: 'db/migrate/1_migration_pending.rb' + ) + ] + end + + before do + ctx = instance_double(ActiveRecord::MigrationContext, migrations: migrations) + allow(connection).to receive(:migration_context).and_return(ctx) + end + + describe '#execute' do + subject(:execute) { service.execute } + + it 'marks the migration as successful' do + expect { execute } + .to change { ActiveRecord::SchemaMigration.where(version: version).count } + .by(1) + + is_expected.to be_success + end + + context 'when the migration does not exist' do + let(:version) { 123 } + + it { is_expected.to be_error } + it { expect(execute.reason).to eq(:not_found) } + + it 'does not insert records' do + expect { execute } + .not_to change { ActiveRecord::SchemaMigration.where(version: version).count } + end + end + + context 'when the migration was already executed' do + before do + allow(service).to receive(:all_versions).and_return([version]) + end + + it { is_expected.to be_error } + it { expect(execute.reason).to eq(:invalid) } + + it 'does not insert records' do + expect { execute } + .not_to change { ActiveRecord::SchemaMigration.where(version: version).count } + end + end + + context 'when the insert fails' do + it 'returns an error response' do + expect(service).to receive(:create_version).with(version).and_return(false) + + is_expected.to be_error + end + end + end +end diff --git a/spec/services/dependency_proxy/group_settings/update_service_spec.rb b/spec/services/dependency_proxy/group_settings/update_service_spec.rb index 38f837a828a..101eee35ca5 100644 --- a/spec/services/dependency_proxy/group_settings/update_service_spec.rb +++ b/spec/services/dependency_proxy/group_settings/update_service_spec.rb @@ -41,7 +41,8 @@ RSpec.describe ::DependencyProxy::GroupSettings::UpdateService, feature_category end where(:user_role, :shared_examples_name) do - :maintainer | 'updating the dependency proxy group settings' + :owner | 'updating the dependency proxy group settings' + :maintainer | 'denying access to dependency proxy group settings' :developer | 'denying access to dependency proxy group settings' :reporter | 'denying access to dependency proxy group settings' :guest | 'denying access to dependency proxy group settings' @@ -55,6 +56,14 @@ RSpec.describe ::DependencyProxy::GroupSettings::UpdateService, feature_category end it_behaves_like params[:shared_examples_name] + + context 'with disabled admin_package feature flag' do + before do + stub_feature_flags(raise_group_admin_package_permission_to_owner: false) + end + + it_behaves_like 'updating the dependency proxy group settings' if params[:user_role] == :maintainer + end end end end diff --git a/spec/services/dependency_proxy/image_ttl_group_policies/update_service_spec.rb b/spec/services/dependency_proxy/image_ttl_group_policies/update_service_spec.rb index f58434222a5..6a5df7358eb 100644 --- a/spec/services/dependency_proxy/image_ttl_group_policies/update_service_spec.rb +++ b/spec/services/dependency_proxy/image_ttl_group_policies/update_service_spec.rb @@ -62,6 +62,15 @@ RSpec.describe ::DependencyProxy::ImageTtlGroupPolicies::UpdateService, feature_ end end + # To be removed when raise_group_admin_package_permission_to_owner FF is removed + shared_examples 'disabling admin_package feature flag' do |action:| + before do + stub_feature_flags(raise_group_admin_package_permission_to_owner: false) + end + + it_behaves_like "#{action} the dependency proxy image ttl policy" + end + before do stub_config(dependency_proxy: { enabled: true }) end @@ -71,7 +80,8 @@ RSpec.describe ::DependencyProxy::ImageTtlGroupPolicies::UpdateService, feature_ let_it_be(:params) { { enabled: false, ttl: 2 } } where(:user_role, :shared_examples_name) do - :maintainer | 'updating the dependency proxy image ttl policy' + :owner | 'updating the dependency proxy image ttl policy' + :maintainer | 'denying access to dependency proxy image ttl policy' :developer | 'denying access to dependency proxy image ttl policy' :reporter | 'denying access to dependency proxy image ttl policy' :guest | 'denying access to dependency proxy image ttl policy' @@ -84,6 +94,7 @@ RSpec.describe ::DependencyProxy::ImageTtlGroupPolicies::UpdateService, feature_ end it_behaves_like params[:shared_examples_name] + it_behaves_like 'disabling admin_package feature flag', action: :updating if params[:user_role] == :maintainer end end @@ -91,7 +102,8 @@ RSpec.describe ::DependencyProxy::ImageTtlGroupPolicies::UpdateService, feature_ let_it_be(:ttl_policy) { group.dependency_proxy_image_ttl_policy } where(:user_role, :shared_examples_name) do - :maintainer | 'creating the dependency proxy image ttl policy' + :owner | 'creating the dependency proxy image ttl policy' + :maintainer | 'denying access to dependency proxy image ttl policy' :developer | 'denying access to dependency proxy image ttl policy' :reporter | 'denying access to dependency proxy image ttl policy' :guest | 'denying access to dependency proxy image ttl policy' @@ -104,15 +116,21 @@ RSpec.describe ::DependencyProxy::ImageTtlGroupPolicies::UpdateService, feature_ end it_behaves_like params[:shared_examples_name] + it_behaves_like 'disabling admin_package feature flag', action: :creating if params[:user_role] == :maintainer end context 'when the policy is not found' do - before do - group.add_maintainer(user) - expect(group).to receive(:dependency_proxy_image_ttl_policy).and_return nil + %i[owner maintainer].each do |role| + context "when user is #{role}" do + before do + group.send("add_#{role}", user) + stub_feature_flags(raise_group_admin_package_permission_to_owner: false) + expect(group).to receive(:dependency_proxy_image_ttl_policy).and_return nil + end + + it_behaves_like 'returning an error', 'Dependency proxy image TTL Policy not found', 404 + end end - - it_behaves_like 'returning an error', 'Dependency proxy image TTL Policy not found', 404 end end end diff --git a/spec/services/environments/create_service_spec.rb b/spec/services/environments/create_service_spec.rb new file mode 100644 index 00000000000..d7fdfd2a38e --- /dev/null +++ b/spec/services/environments/create_service_spec.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Environments::CreateService, feature_category: :environment_management do + let_it_be(:project) { create(:project, :repository) } + 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(:service) { described_class.new(project, current_user, params) } + let(:current_user) { developer } + let(:params) { {} } + + describe '#execute' do + subject { service.execute } + + let(:params) { { name: 'production', external_url: 'https://gitlab.com', tier: :production } } + + it 'creates an environment' do + expect { subject }.to change { ::Environment.count }.by(1) + end + + it 'returns successful response' do + response = subject + + expect(response).to be_success + expect(response.payload[:environment].name).to eq('production') + expect(response.payload[:environment].external_url).to eq('https://gitlab.com') + expect(response.payload[:environment].tier).to eq('production') + end + + context 'with a cluster agent' do + let_it_be(:agent_management_project) { create(:project) } + let_it_be(:cluster_agent) { create(:cluster_agent, project: agent_management_project) } + + let!(:authorization) { create(:agent_user_access_project_authorization, project: project, agent: cluster_agent) } + let(:params) { { name: 'production', cluster_agent: cluster_agent } } + + it 'returns successful response' do + response = subject + + expect(response).to be_success + expect(response.payload[:environment].cluster_agent).to eq(cluster_agent) + end + + context 'when user does not have permission to read the agent' do + let!(:authorization) { nil } + + it 'returns an error' do + response = subject + + expect(response).to be_error + expect(response.message).to eq('Unauthorized to access the cluster agent in this project') + expect(response.payload[:environment]).to be_nil + end + end + end + + context 'when params contain invalid value' do + let(:params) { { name: 'production', external_url: 'http://${URL}' } } + + it 'does not create an environment' do + expect { subject }.not_to change { ::Environment.count } + end + + it 'returns an error' do + response = subject + + expect(response).to be_error + expect(response.message).to match_array("External url URI is invalid") + expect(response.payload[:environment]).to be_nil + end + end + + context 'when disallowed parameter is passed' do + let(:params) { { name: 'production', slug: 'prod' } } + + it 'ignores the parameter' do + response = subject + + expect(response).to be_success + expect(response.payload[:environment].name).to eq('production') + expect(response.payload[:environment].slug).not_to eq('prod') + end + end + + context 'when user is reporter' do + let(:current_user) { reporter } + + it 'does not create an environment' do + expect { subject }.not_to change { ::Environment.count } + end + + it 'returns an error' do + response = subject + + expect(response).to be_error + expect(response.message).to eq('Unauthorized to create an environment') + expect(response.payload[:environment]).to be_nil + end + end + end +end diff --git a/spec/services/environments/destroy_service_spec.rb b/spec/services/environments/destroy_service_spec.rb new file mode 100644 index 00000000000..26efb93718b --- /dev/null +++ b/spec/services/environments/destroy_service_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Environments::DestroyService, feature_category: :continuous_delivery do + include CreateEnvironmentsHelpers + + let_it_be(:project) { create(:project, :private, :repository) } + let_it_be(:user) { create(:user) } + + let(:service) { described_class.new(project, user) } + + describe '#execute' do + subject { service.execute(environment) } + + let_it_be(:project) { create(:project, :private, :repository) } + 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(:user) { developer } + + let!(:environment) { create(:environment, project: project, state: :stopped) } + + context "when destroy is authorized" do + it 'destroys the environment' do + expect { subject }.to change { environment.destroyed? }.from(false).to(true) + end + end + + context "when destroy is not authorized" do + let(:user) { reporter } + + it 'does not destroy the environment' do + expect { subject }.not_to change { environment.destroyed? } + end + end + + context "when destroy fails" do + before do + allow(environment) + .to receive(:destroy) + .and_return(false) + end + + it 'returns errors' do + expect(subject.message).to include("Attemped to destroy the environment but failed") + end + end + end +end diff --git a/spec/services/environments/update_service_spec.rb b/spec/services/environments/update_service_spec.rb new file mode 100644 index 00000000000..84220c0930b --- /dev/null +++ b/spec/services/environments/update_service_spec.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Environments::UpdateService, feature_category: :environment_management do + let_it_be(:project) { create(:project, :repository) } + 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(:environment) { create(:environment, project: project) } + + let(:service) { described_class.new(project, current_user, params) } + let(:current_user) { developer } + let(:params) { {} } + + describe '#execute' do + subject { service.execute(environment) } + + let(:params) { { external_url: 'https://gitlab.com/' } } + + it 'updates the external URL' do + expect { subject }.to change { environment.reload.external_url }.to('https://gitlab.com/') + end + + it 'returns successful response' do + response = subject + + expect(response).to be_success + expect(response.payload[:environment]).to eq(environment) + end + + context 'when setting a cluster agent to the environment' do + let_it_be(:agent_management_project) { create(:project) } + let_it_be(:cluster_agent) { create(:cluster_agent, project: agent_management_project) } + + let!(:authorization) { create(:agent_user_access_project_authorization, project: project, agent: cluster_agent) } + let(:params) { { cluster_agent: cluster_agent } } + + it 'returns successful response' do + response = subject + + expect(response).to be_success + expect(response.payload[:environment].cluster_agent).to eq(cluster_agent) + end + + context 'when user does not have permission to read the agent' do + let!(:authorization) { nil } + + it 'returns an error' do + response = subject + + expect(response).to be_error + expect(response.message).to eq('Unauthorized to access the cluster agent in this project') + expect(response.payload[:environment]).to eq(environment) + end + end + end + + context 'when unsetting a cluster agent of the environment' do + let_it_be(:cluster_agent) { create(:cluster_agent, project: project) } + + let(:params) { { cluster_agent: nil } } + + before do + environment.update!(cluster_agent: cluster_agent) + end + + it 'returns successful response' do + response = subject + + expect(response).to be_success + expect(response.payload[:environment].cluster_agent).to be_nil + end + end + + context 'when params contain invalid value' do + let(:params) { { external_url: 'http://${URL}' } } + + it 'returns an error' do + response = subject + + expect(response).to be_error + expect(response.message).to match_array("External url URI is invalid") + expect(response.payload[:environment]).to eq(environment) + end + end + + context 'when disallowed parameter is passed' do + let(:params) { { external_url: 'https://gitlab.com/', slug: 'prod' } } + + it 'ignores the parameter' do + response = subject + + expect(response).to be_success + expect(response.payload[:environment].external_url).to eq('https://gitlab.com/') + expect(response.payload[:environment].slug).not_to eq('prod') + end + end + + context 'when user is reporter' do + let(:current_user) { reporter } + + it 'returns an error' do + response = subject + + expect(response).to be_error + expect(response.message).to eq('Unauthorized to update the environment') + expect(response.payload[:environment]).to eq(environment) + end + end + end +end diff --git a/spec/services/error_tracking/collect_error_service_spec.rb b/spec/services/error_tracking/collect_error_service_spec.rb deleted file mode 100644 index 3ff753e8c65..00000000000 --- a/spec/services/error_tracking/collect_error_service_spec.rb +++ /dev/null @@ -1,140 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ErrorTracking::CollectErrorService, feature_category: :error_tracking do - let_it_be(:project) { create(:project) } - - let(:parsed_event_file) { 'error_tracking/parsed_event.json' } - let(:parsed_event) { parse_valid_event(parsed_event_file) } - - subject { described_class.new(project, nil, event: parsed_event) } - - describe '#execute' do - it 'creates Error and creates ErrorEvent' do - expect { subject.execute } - .to change { ErrorTracking::Error.count }.by(1) - .and change { ErrorTracking::ErrorEvent.count }.by(1) - end - - it 'updates Error and created ErrorEvent on second hit' do - subject.execute - - expect { subject.execute }.not_to change { ErrorTracking::Error.count } - expect { subject.execute }.to change { ErrorTracking::ErrorEvent.count }.by(1) - end - - it 'has correct values set' do - subject.execute - - event = ErrorTracking::ErrorEvent.last - error = event.error - - expect(error.name).to eq 'ActionView::MissingTemplate' - expect(error.description).to start_with 'Missing template posts/error2' - expect(error.actor).to eq 'PostsController#error2' - expect(error.platform).to eq 'ruby' - expect(error.last_seen_at).to eq '2021-07-08T12:59:16Z' - - expect(event.description).to start_with 'Missing template posts/error2' - expect(event.occurred_at).to eq '2021-07-08T12:59:16Z' - expect(event.level).to eq 'error' - expect(event.environment).to eq 'development' - expect(event.payload).to eq parsed_event - end - - context 'python sdk event' do - let(:parsed_event_file) { 'error_tracking/python_event.json' } - - it 'creates a valid event' do - expect { subject.execute }.to change { ErrorTracking::ErrorEvent.count }.by(1) - end - end - - context 'with unusual payload' do - let(:event) { ErrorTracking::ErrorEvent.last! } - - context 'when transaction is missing' do - it 'builds actor from stacktrace' do - parsed_event.delete('transaction') - - subject.execute - - expect(event.error.actor).to eq 'find()' - end - end - - context 'when transaction is an empty string' do \ - it 'builds actor from stacktrace' do - parsed_event['transaction'] = '' - - subject.execute - - expect(event.error.actor).to eq 'find()' - end - end - - context 'when timestamp is numeric' do - it 'parses timestamp' do - parsed_event['timestamp'] = '1631015580.50' - - subject.execute - - expect(event.occurred_at).to eq '2021-09-07T11:53:00.5' - end - end - end - - context 'go payload' do - let(:parsed_event_file) { 'error_tracking/go_parsed_event.json' } - - it 'has correct values set' do - subject.execute - - event = ErrorTracking::ErrorEvent.last - error = event.error - - expect(error.name).to eq '*errors.errorString' - expect(error.description).to start_with 'Hello world' - expect(error.platform).to eq 'go' - - expect(event.description).to start_with 'Hello world' - expect(event.level).to eq 'error' - expect(event.environment).to eq 'Accumulate' - expect(event.payload).to eq parsed_event - end - - context 'with two exceptions' do - let(:parsed_event_file) { 'error_tracking/go_two_exception_event.json' } - - it 'reports using second exception', :aggregate_failures do - subject.execute - - event = ErrorTracking::ErrorEvent.last - error = event.error - - expect(error.name).to eq '*url.Error' - expect(error.description).to eq(%(Get \"foobar\": unsupported protocol scheme \"\")) - expect(error.platform).to eq 'go' - expect(error.actor).to eq('main(main)') - - expect(event.description).to eq(%(Get \"foobar\": unsupported protocol scheme \"\")) - expect(event.payload).to eq parsed_event - end - end - end - end - - private - - def parse_valid_event(parsed_event_file) - parsed_event = Gitlab::Json.parse(fixture_file(parsed_event_file)) - - validator = ErrorTracking::Collector::PayloadValidator.new - # This a precondition for all specs to verify that - # submitted JSON payload is valid. - expect(validator).to be_valid(parsed_event) - - parsed_event - end -end diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb index e991b5bd842..f567624068a 100644 --- a/spec/services/git/branch_hooks_service_spec.rb +++ b/spec/services/git/branch_hooks_service_spec.rb @@ -571,105 +571,4 @@ RSpec.describe Git::BranchHooksService, :clean_gitlab_redis_shared_state, featur end end end - - describe 'Metrics dashboard sync' do - shared_examples 'trigger dashboard sync' do - it 'imports metrics to database' do - expect(Metrics::Dashboard::SyncDashboardsWorker).to receive(:perform_async) - - service.execute - end - end - - shared_examples 'no dashboard sync' do - it 'does not sync metrics to database' do - expect(Metrics::Dashboard::SyncDashboardsWorker).not_to receive(:perform_async) - - service.execute - end - end - - def change_repository(**changes) - actions = changes.flat_map do |(action, paths)| - Array(paths).flat_map do |file_path| - { action: action, file_path: file_path, content: SecureRandom.hex } - end - end - - project.repository.commit_files( - user, message: 'message', branch_name: branch, actions: actions - ) - end - - let(:charts) { '.gitlab/dashboards/charts.yml' } - let(:readme) { 'README.md' } - let(:commit_id) { change_repository(**commit_changes) } - - context 'with default branch' do - context 'when adding files' do - let(:new_file) { 'somenewfile.md' } - - context 'also related' do - let(:commit_changes) { { create: [charts, new_file] } } - - include_examples 'trigger dashboard sync' - end - - context 'only unrelated' do - let(:commit_changes) { { create: new_file } } - - include_examples 'no dashboard sync' - end - end - - context 'when deleting files' do - before do - change_repository(create: charts) - end - - context 'also related' do - let(:commit_changes) { { delete: [charts, readme] } } - - include_examples 'trigger dashboard sync' - end - - context 'only unrelated' do - let(:commit_changes) { { delete: readme } } - - include_examples 'no dashboard sync' - end - end - - context 'when updating files' do - before do - change_repository(create: charts) - end - - context 'also related' do - let(:commit_changes) { { update: [charts, readme] } } - - include_examples 'trigger dashboard sync' - end - - context 'only unrelated' do - let(:commit_changes) { { update: readme } } - - include_examples 'no dashboard sync' - end - end - - context 'without changes' do - let(:commit_changes) { {} } - - include_examples 'no dashboard sync' - end - end - - context 'with other branch' do - let(:branch) { 'fix' } - let(:commit_changes) { { create: charts } } - - include_examples 'no dashboard sync' - end - end end diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index aa534777f3e..5e43426b9dd 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -14,15 +14,17 @@ RSpec.describe Git::BranchPushService, :use_clean_rails_redis_caching, services: let(:branch) { 'master' } let(:ref) { "refs/heads/#{branch}" } let(:push_options) { nil } + let(:service) do + described_class + .new(project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref }, push_options: push_options) + end before do project.add_maintainer(user) end subject(:execute_service) do - described_class - .new(project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref }, push_options: push_options) - .execute + service.execute end describe 'Push branches' do @@ -683,14 +685,44 @@ RSpec.describe Git::BranchPushService, :use_clean_rails_redis_caching, services: let(:commits_to_sync) { [] } shared_examples 'enqueues Jira sync worker' do - specify :aggregate_failures do - Sidekiq::Testing.fake! do - expect(JiraConnect::SyncBranchWorker) - .to receive(:perform_async) - .with(project.id, branch_to_sync, commits_to_sync, kind_of(Numeric)) - .and_call_original + context "batch_delay_jira_branch_sync_worker feature flag is enabled" do + before do + stub_feature_flags(batch_delay_jira_branch_sync_worker: true) + end + + specify :aggregate_failures do + Sidekiq::Testing.fake! do + if commits_to_sync.any? + expect(JiraConnect::SyncBranchWorker) + .to receive(:perform_in) + .with(kind_of(Numeric), project.id, branch_to_sync, commits_to_sync, kind_of(Numeric)) + .and_call_original + else + expect(JiraConnect::SyncBranchWorker) + .to receive(:perform_async) + .with(project.id, branch_to_sync, commits_to_sync, kind_of(Numeric)) + .and_call_original + end + + expect { subject }.to change(JiraConnect::SyncBranchWorker.jobs, :size).by(1) + end + end + end - expect { subject }.to change(JiraConnect::SyncBranchWorker.jobs, :size).by(1) + context "batch_delay_jira_branch_sync_worker feature flag is disabled" do + before do + stub_feature_flags(batch_delay_jira_branch_sync_worker: false) + end + + specify :aggregate_failures do + Sidekiq::Testing.fake! do + expect(JiraConnect::SyncBranchWorker) + .to receive(:perform_async) + .with(project.id, branch_to_sync, commits_to_sync, kind_of(Numeric)) + .and_call_original + + expect { subject }.to change(JiraConnect::SyncBranchWorker.jobs, :size).by(1) + end end end end @@ -723,6 +755,29 @@ RSpec.describe Git::BranchPushService, :use_clean_rails_redis_caching, services: end it_behaves_like 'enqueues Jira sync worker' + + describe 'batch requests' do + let(:commits_to_sync) { [sample_commit.id, another_sample_commit.id] } + + it 'enqueues multiple jobs' do + # We have to stub this as we only have two valid commits to use + stub_const('Git::BranchHooksService::JIRA_SYNC_BATCH_SIZE', 1) + + expect_any_instance_of(Git::BranchHooksService).to receive(:filtered_commit_shas).and_return(commits_to_sync) + + expect(JiraConnect::SyncBranchWorker) + .to receive(:perform_in) + .with(0.seconds, project.id, branch_to_sync, [commits_to_sync.first], kind_of(Numeric)) + .and_call_original + + expect(JiraConnect::SyncBranchWorker) + .to receive(:perform_in) + .with(10.seconds, project.id, branch_to_sync, [commits_to_sync.last], kind_of(Numeric)) + .and_call_original + + subject + end + end end context 'branch name and commit message does not contain Jira issue key' do diff --git a/spec/services/google_cloud/enable_vision_ai_service_spec.rb b/spec/services/google_cloud/enable_vision_ai_service_spec.rb new file mode 100644 index 00000000000..5adafcffe69 --- /dev/null +++ b/spec/services/google_cloud/enable_vision_ai_service_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GoogleCloud::EnableVisionAiService, feature_category: :deployment_management do + describe 'when a project does not have any gcp projects' do + let_it_be(:project) { create(:project) } + + it 'returns error' do + result = described_class.new(project).execute + message = 'No GCP projects found. Configure a service account or GCP_PROJECT_ID ci variable.' + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq(message) + end + end + + describe 'when a project has 3 gcp projects' do + let_it_be(:project) { create(:project) } + + before do + project.variables.build(environment_scope: 'production', key: 'GCP_PROJECT_ID', value: 'prj-prod') + project.variables.build(environment_scope: 'staging', key: 'GCP_PROJECT_ID', value: 'prj-staging') + project.save! + end + + it 'enables cloud run, artifacts registry and cloud build', :aggregate_failures do + expect_next_instance_of(GoogleApi::CloudPlatform::Client) do |instance| + expect(instance).to receive(:enable_vision_api).with('prj-prod') + expect(instance).to receive(:enable_vision_api).with('prj-staging') + end + + result = described_class.new(project).execute + + expect(result[:status]).to eq(:success) + end + end +end diff --git a/spec/services/google_cloud/generate_pipeline_service_spec.rb b/spec/services/google_cloud/generate_pipeline_service_spec.rb index c18514884ca..b363b7b17b6 100644 --- a/spec/services/google_cloud/generate_pipeline_service_spec.rb +++ b/spec/services/google_cloud/generate_pipeline_service_spec.rb @@ -236,4 +236,98 @@ EOF end end end + + describe 'for vision ai' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:maintainer) { create(:user) } + let_it_be(:service_params) { { action: described_class::ACTION_VISION_AI_PIPELINE } } + let_it_be(:service) { described_class.new(project, maintainer, service_params) } + + describe 'when there is no existing pipeline' do + before do + project.add_maintainer(maintainer) + end + + it 'creates a new branch with commit for cloud-run deployment' do + response = service.execute + + branch_name = response[:branch_name] + commit = response[:commit] + local_branches = project.repository.local_branches + created_branch = local_branches.find { |branch| branch.name == branch_name } + + expect(response[:status]).to eq(:success) + expect(branch_name).to start_with('vision-ai-pipeline-') + expect(created_branch).to be_present + expect(created_branch.target).to eq(commit[:result]) + end + + it 'generated pipeline includes vision ai deployment' do + response = service.execute + + ref = response[:commit][:result] + gitlab_ci_yml = project.repository.gitlab_ci_yml_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') + end + + context 'simulate errors' do + it 'fails to create branch' do + allow_next_instance_of(Branches::CreateService) do |create_service| + allow(create_service).to receive(:execute) + .and_return({ status: :error }) + end + + response = service.execute + expect(response[:status]).to eq(:error) + end + + it 'fails to commit changes' do + allow_next_instance_of(Files::CreateService) do |create_service| + allow(create_service).to receive(:execute) + .and_return({ status: :error }) + end + + response = service.execute + expect(response[:status]).to eq(:error) + end + end + end + + describe 'when there is an existing pipeline with `includes`' do + before do + project.add_maintainer(maintainer) + + file_name = '.gitlab-ci.yml' + file_content = <<EOF +stages: + - validate + - detect + - render + +include: + local: 'some-pipeline.yml' +EOF + project.repository.create_file(maintainer, + file_name, + file_content, + message: 'Pipeline with three stages and two jobs', + branch_name: project.default_branch) + end + + it 'includes the vision ai pipeline' do + response = service.execute + + branch_name = response[:branch_name] + gitlab_ci_yml = project.repository.gitlab_ci_yml_for(branch_name) + pipeline = Gitlab::Config::Loader::Yaml.new(gitlab_ci_yml).load! + + expect(response[:status]).to eq(:success) + expect(pipeline[:stages]).to eq(%w[validate detect render]) + expect(pipeline[:include]).to be_present + 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') + end + end + end end diff --git a/spec/services/groups/autocomplete_service_spec.rb b/spec/services/groups/autocomplete_service_spec.rb index 9f55322e72d..4fb14b525ac 100644 --- a/spec/services/groups/autocomplete_service_spec.rb +++ b/spec/services/groups/autocomplete_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::AutocompleteService, feature_category: :subgroups do +RSpec.describe Groups::AutocompleteService, feature_category: :groups_and_projects do let_it_be(:group, refind: true) { create(:group, :nested, :private, avatar: fixture_file_upload('spec/fixtures/dk.png')) } let_it_be(:sub_group) { create(:group, :private, parent: group) } diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index 84794b5f6f8..2317c6fba61 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::CreateService, '#execute', feature_category: :subgroups do +RSpec.describe Groups::CreateService, '#execute', feature_category: :groups_and_projects do let!(:user) { create(:user) } let!(:group_params) { { path: "group_path", visibility_level: Gitlab::VisibilityLevel::PUBLIC } } diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index 7c3710aeeb2..929f7d5b4e3 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::DestroyService, feature_category: :subgroups do +RSpec.describe Groups::DestroyService, feature_category: :groups_and_projects do let!(:user) { create(:user) } let!(:group) { create(:group) } let!(:nested_group) { create(:group, parent: group) } diff --git a/spec/services/groups/group_links/create_service_spec.rb b/spec/services/groups/group_links/create_service_spec.rb index ced87421858..8acbcdc77af 100644 --- a/spec/services/groups/group_links/create_service_spec.rb +++ b/spec/services/groups/group_links/create_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::GroupLinks::CreateService, '#execute', feature_category: :subgroups do +RSpec.describe Groups::GroupLinks::CreateService, '#execute', feature_category: :groups_and_projects do let_it_be(:shared_with_group_parent) { create(:group, :private) } let_it_be(:shared_with_group) { create(:group, :private, parent: shared_with_group_parent) } let_it_be(:shared_with_group_child) { create(:group, :private, parent: shared_with_group) } diff --git a/spec/services/groups/group_links/destroy_service_spec.rb b/spec/services/groups/group_links/destroy_service_spec.rb index 5821ec44192..65f24323f8b 100644 --- a/spec/services/groups/group_links/destroy_service_spec.rb +++ b/spec/services/groups/group_links/destroy_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::GroupLinks::DestroyService, '#execute', feature_category: :subgroups do +RSpec.describe Groups::GroupLinks::DestroyService, '#execute', feature_category: :groups_and_projects do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group, :private) } let_it_be(:shared_group) { create(:group, :private) } diff --git a/spec/services/groups/group_links/update_service_spec.rb b/spec/services/groups/group_links/update_service_spec.rb index f17d2f50a02..79fc25e111a 100644 --- a/spec/services/groups/group_links/update_service_spec.rb +++ b/spec/services/groups/group_links/update_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::GroupLinks::UpdateService, '#execute', feature_category: :subgroups do +RSpec.describe Groups::GroupLinks::UpdateService, '#execute', feature_category: :groups_and_projects do let(:user) { create(:user) } let_it_be(:group) { create(:group, :private) } diff --git a/spec/services/groups/merge_requests_count_service_spec.rb b/spec/services/groups/merge_requests_count_service_spec.rb index 32c4c618eda..cdcb344bd40 100644 --- a/spec/services/groups/merge_requests_count_service_spec.rb +++ b/spec/services/groups/merge_requests_count_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::MergeRequestsCountService, :use_clean_rails_memory_store_caching, feature_category: :subgroups do +RSpec.describe Groups::MergeRequestsCountService, :use_clean_rails_memory_store_caching, feature_category: :groups_and_projects do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group, :public) } let_it_be(:project) { create(:project, :repository, namespace: group) } diff --git a/spec/services/groups/nested_create_service_spec.rb b/spec/services/groups/nested_create_service_spec.rb index 476bc2aa23c..1efb5bf0c9c 100644 --- a/spec/services/groups/nested_create_service_spec.rb +++ b/spec/services/groups/nested_create_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::NestedCreateService, feature_category: :subgroups do +RSpec.describe Groups::NestedCreateService, feature_category: :groups_and_projects do let(:user) { create(:user) } subject(:service) { described_class.new(user, params) } diff --git a/spec/services/groups/open_issues_count_service_spec.rb b/spec/services/groups/open_issues_count_service_spec.rb index 725b913bf15..ef3c86869a0 100644 --- a/spec/services/groups/open_issues_count_service_spec.rb +++ b/spec/services/groups/open_issues_count_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_caching, feature_category: :subgroups do +RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_caching, feature_category: :groups_and_projects do let_it_be(:group) { create(:group, :public) } let_it_be(:project) { create(:project, :public, namespace: group) } let_it_be(:user) { create(:user) } diff --git a/spec/services/groups/participants_service_spec.rb b/spec/services/groups/participants_service_spec.rb index 37966a523c2..eee9cfce1b1 100644 --- a/spec/services/groups/participants_service_spec.rb +++ b/spec/services/groups/participants_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::ParticipantsService, feature_category: :subgroups do +RSpec.describe Groups::ParticipantsService, feature_category: :groups_and_projects do describe '#group_members' do let(:user) { create(:user) } let(:parent_group) { create(:group) } diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index d6eb060ea7e..a3020241377 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::TransferService, :sidekiq_inline, feature_category: :subgroups do +RSpec.describe Groups::TransferService, :sidekiq_inline, feature_category: :groups_and_projects do shared_examples 'project namespace path is in sync with project path' do it 'keeps project and project namespace attributes in sync' do projects_with_project_namespace.each do |project| @@ -907,7 +907,7 @@ RSpec.describe Groups::TransferService, :sidekiq_inline, feature_category: :subg let(:subsub_project) { create(:project, group: subsubgroup) } let!(:contacts) { create_list(:contact, 4, group: root_group) } - let!(:organizations) { create_list(:crm_organization, 2, group: root_group) } + let!(:crm_organizations) { create_list(:crm_organization, 2, group: root_group) } before do create(:issue_customer_relations_contact, contact: contacts[0], issue: create(:issue, project: root_project)) @@ -966,7 +966,7 @@ RSpec.describe Groups::TransferService, :sidekiq_inline, feature_category: :subg it 'moves all crm objects' do expect { transfer_service.execute(new_parent_group) } .to change { root_group.contacts.count }.by(-4) - .and change { root_group.organizations.count }.by(-2) + .and change { root_group.crm_organizations.count }.by(-2) end it 'retains issue contacts' do @@ -991,7 +991,7 @@ RSpec.describe Groups::TransferService, :sidekiq_inline, feature_category: :subg it 'moves all crm objects' do expect { transfer_service.execute(subgroup_in_new_parent_group) } .to change { root_group.contacts.count }.by(-4) - .and change { root_group.organizations.count }.by(-2) + .and change { root_group.crm_organizations.count }.by(-2) end it 'retains issue contacts' do diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 6baa8e5d6b6..2842097199f 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::UpdateService, feature_category: :subgroups do +RSpec.describe Groups::UpdateService, feature_category: :groups_and_projects do let!(:user) { create(:user) } let!(:private_group) { create(:group, :private) } let!(:internal_group) { create(:group, :internal) } diff --git a/spec/services/groups/update_shared_runners_service_spec.rb b/spec/services/groups/update_shared_runners_service_spec.rb index 48c81f109aa..0acf1ec3d35 100644 --- a/spec/services/groups/update_shared_runners_service_spec.rb +++ b/spec/services/groups/update_shared_runners_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::UpdateSharedRunnersService, feature_category: :subgroups do +RSpec.describe Groups::UpdateSharedRunnersService, feature_category: :groups_and_projects do let(:user) { create(:user) } let(:group) { create(:group) } let(:params) { {} } diff --git a/spec/services/groups/update_statistics_service_spec.rb b/spec/services/groups/update_statistics_service_spec.rb index 13a88839de0..6bab36eca89 100644 --- a/spec/services/groups/update_statistics_service_spec.rb +++ b/spec/services/groups/update_statistics_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::UpdateStatisticsService, feature_category: :subgroups do +RSpec.describe Groups::UpdateStatisticsService, feature_category: :groups_and_projects do let_it_be(:group, reload: true) { create(:group) } let(:statistics) { %w(wiki_size) } diff --git a/spec/services/import/github_service_spec.rb b/spec/services/import/github_service_spec.rb index fa8b2489599..21dc24e28f6 100644 --- a/spec/services/import/github_service_spec.rb +++ b/spec/services/import/github_service_spec.rb @@ -268,7 +268,7 @@ RSpec.describe Import::GithubService, feature_category: :importers do { status: :error, http_status: :unprocessable_entity, - message: '"repository" size (101 Bytes) is larger than the limit of 100 Bytes.' + message: '"repository" size (101 B) is larger than the limit of 100 B.' } end diff --git a/spec/services/import/gitlab_projects/file_acquisition_strategies/remote_file_s3_spec.rb b/spec/services/import/gitlab_projects/file_acquisition_strategies/remote_file_s3_spec.rb index 411e2ec5286..147bfccbfb7 100644 --- a/spec/services/import/gitlab_projects/file_acquisition_strategies/remote_file_s3_spec.rb +++ b/spec/services/import/gitlab_projects/file_acquisition_strategies/remote_file_s3_spec.rb @@ -64,7 +64,7 @@ RSpec.describe ::Import::GitlabProjects::FileAcquisitionStrategies::RemoteFileS3 it 'validates the remote content-length' do expect(subject).not_to be_valid expect(subject.errors.full_messages) - .to include('Content length is too big (should be at most 10 GB)') + .to include('Content length is too big (should be at most 10 GiB)') end end diff --git a/spec/services/import/gitlab_projects/file_acquisition_strategies/remote_file_spec.rb b/spec/services/import/gitlab_projects/file_acquisition_strategies/remote_file_spec.rb index a28a552746f..0807a0e9d05 100644 --- a/spec/services/import/gitlab_projects/file_acquisition_strategies/remote_file_spec.rb +++ b/spec/services/import/gitlab_projects/file_acquisition_strategies/remote_file_spec.rb @@ -58,7 +58,7 @@ RSpec.describe ::Import::GitlabProjects::FileAcquisitionStrategies::RemoteFile, expect(subject).not_to be_valid expect(subject.errors.full_messages) - .to include('Content length is too big (should be at most 10 GB)') + .to include('Content length is too big (should be at most 10 GiB)') end it 'validates the remote content-type' do diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 548d9455ebf..3dfc9571c9c 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -10,8 +10,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do let_it_be(:user) { create(:user) } let(:opts) { { title: 'title' } } - let(:spam_params) { double } - let(:service) { described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params) } + let(:service) { described_class.new(container: project, current_user: user, params: opts) } it_behaves_like 'rate limited service' do let(:key) { :issues_create } @@ -27,10 +26,6 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do let(:result) { service.execute } let(:issue) { result[:issue] } - before do - stub_spam_services - end - context 'when params are invalid' do let(:opts) { { title: '' } } @@ -155,7 +150,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do end context 'when a build_service is provided' do - let(:result) { described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params, build_service: build_service).execute } + let(:result) { described_class.new(container: project, current_user: user, params: opts, build_service: build_service).execute } let(:issue_from_builder) { build(:work_item, project: project, title: 'Issue from builder') } let(:build_service) { double(:build_service, execute: issue_from_builder) } @@ -168,7 +163,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do end context 'when skip_system_notes is true' do - let(:issue) { described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params).execute(skip_system_notes: true) } + let(:issue) { described_class.new(container: project, current_user: user, params: opts).execute(skip_system_notes: true) } it 'does not call Issuable::CommonSystemNotesService' do expect(Issuable::CommonSystemNotesService).not_to receive(:new) @@ -264,7 +259,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do let_it_be(:non_member) { create(:user) } it 'filters out params that cannot be set without the :set_issue_metadata permission' do - result = described_class.new(container: project, current_user: non_member, params: opts, spam_params: spam_params).execute + result = described_class.new(container: project, current_user: non_member, params: opts).execute issue = result[:issue] expect(result).to be_success @@ -278,7 +273,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do end it 'can create confidential issues' do - result = described_class.new(container: project, current_user: non_member, params: opts.merge(confidential: true), spam_params: spam_params).execute + result = described_class.new(container: project, current_user: non_member, params: opts.merge(confidential: true)).execute issue = result[:issue] expect(result).to be_success @@ -289,7 +284,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do it 'moves the issue to the end, in an asynchronous worker' do expect(Issues::PlacementWorker).to receive(:perform_async).with(be_nil, Integer) - described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params).execute + described_class.new(container: project, current_user: user, params: opts).execute end context 'when label belongs to project group' do @@ -376,13 +371,13 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do it 'invalidates open issues counter for assignees when issue is assigned' do project.add_maintainer(assignee) - described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params).execute + described_class.new(container: project, current_user: user, params: opts).execute expect(assignee.assigned_open_issues_count).to eq 1 end it 'records the assignee assignment event' do - result = described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params).execute + result = described_class.new(container: project, current_user: user, params: opts).execute issue = result.payload[:issue] expect(issue.assignment_events).to match([have_attributes(user_id: assignee.id, action: 'add')]) @@ -454,7 +449,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do expect(project.project_namespace).to receive(:execute_hooks).with(expected_payload, :issue_hooks) expect(project.project_namespace).to receive(:execute_integrations).with(expected_payload, :issue_hooks) - described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params).execute + described_class.new(container: project, current_user: user, params: opts).execute end context 'when issue is confidential' do @@ -477,7 +472,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do expect(project.project_namespace).to receive(:execute_hooks).with(expected_payload, :confidential_issue_hooks) expect(project.project_namespace).to receive(:execute_integrations).with(expected_payload, :confidential_issue_hooks) - described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params).execute + described_class.new(container: project, current_user: user, params: opts).execute end end end @@ -523,7 +518,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do it 'removes assignee when user id is invalid' do opts = { title: 'Title', description: 'Description', assignee_ids: [-1] } - result = described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params).execute + result = described_class.new(container: project, current_user: user, params: opts).execute issue = result[:issue] expect(result).to be_success @@ -533,7 +528,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do it 'removes assignee when user id is 0' do opts = { title: 'Title', description: 'Description', assignee_ids: [0] } - result = described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params).execute + result = described_class.new(container: project, current_user: user, params: opts).execute issue = result[:issue] expect(result).to be_success @@ -544,7 +539,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do project.add_maintainer(assignee) opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } - result = described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params).execute + result = described_class.new(container: project, current_user: user, params: opts).execute issue = result[:issue] expect(result).to be_success @@ -564,7 +559,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do project.update!(visibility_level: level) opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } - result = described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params).execute + result = described_class.new(container: project, current_user: user, params: opts).execute issue = result[:issue] expect(result).to be_success @@ -576,7 +571,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do end it_behaves_like 'issuable record that supports quick actions' do - let(:issuable) { described_class.new(container: project, current_user: user, params: params, spam_params: spam_params).execute[:issue] } + let(:issuable) { described_class.new(container: project, current_user: user, params: params).execute[:issue] } end context 'Quick actions' do @@ -703,14 +698,14 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do let(:opts) { { discussion_to_resolve: discussion.id, merge_request_to_resolve_discussions_of: merge_request.iid } } it 'resolves the discussion' do - described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params).execute + described_class.new(container: project, current_user: user, params: opts).execute discussion.first_note.reload expect(discussion.resolved?).to be(true) end it 'added a system note to the discussion' do - described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params).execute + described_class.new(container: project, current_user: user, params: opts).execute reloaded_discussion = MergeRequest.find(merge_request.id).discussions.first @@ -720,8 +715,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do it 'sets default title and description values if not provided' do result = described_class.new( container: project, current_user: user, - params: opts, - spam_params: spam_params + params: opts ).execute issue = result[:issue] @@ -738,8 +732,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do params: opts.merge( description: 'Custom issue description', title: 'My new issue' - ), - spam_params: spam_params + ) ).execute issue = result[:issue] @@ -754,14 +747,14 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do let(:opts) { { merge_request_to_resolve_discussions_of: merge_request.iid } } it 'resolves the discussion' do - described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params).execute + described_class.new(container: project, current_user: user, params: opts).execute discussion.first_note.reload expect(discussion.resolved?).to be(true) end it 'added a system note to the discussion' do - described_class.new(container: project, current_user: user, params: opts, spam_params: spam_params).execute + described_class.new(container: project, current_user: user, params: opts).execute reloaded_discussion = MergeRequest.find(merge_request.id).discussions.first @@ -771,8 +764,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do it 'sets default title and description values if not provided' do result = described_class.new( container: project, current_user: user, - params: opts, - spam_params: spam_params + params: opts ).execute issue = result[:issue] @@ -789,8 +781,7 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do params: opts.merge( description: 'Custom issue description', title: 'My new issue' - ), - spam_params: spam_params + ) ).execute issue = result[:issue] @@ -836,25 +827,31 @@ RSpec.describe Issues::CreateService, feature_category: :team_planning do } end + let(:perform_spam_check) { true } + subject do - described_class.new(container: project, current_user: user, params: params, spam_params: spam_params) + described_class.new(container: project, current_user: user, params: params, perform_spam_check: perform_spam_check) end - it 'executes SpamActionService' do - expect_next_instance_of( - Spam::SpamActionService, - { - spammable: kind_of(Issue), - spam_params: spam_params, - user: an_instance_of(User), - action: :create - } - ) do |instance| - expect(instance).to receive(:execute) + it 'checks for spam' do + expect_next_instance_of(Issue) do |instance| + expect(instance).to receive(:check_for_spam).with(user: user, action: :create) end subject.execute end + + context 'when `perform_spam_check` is set to `false`' do + let(:perform_spam_check) { false } + + it 'does not execute the SpamActionService' do + expect_next_instance_of(Issue) do |instance| + expect(instance).not_to receive(:check_for_spam) + end + + subject.execute + end + end end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index f96fbf54f08..a5151925c52 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -104,6 +104,10 @@ RSpec.describe Issues::UpdateService, :mailer, feature_category: :team_planning expect(issue.issue_customer_relations_contacts.last.contact).to eq contact end + it_behaves_like 'update service that triggers GraphQL work_item_updated subscription' do + subject(:execute_service) { update_issue(opts) } + end + context 'when updating milestone' do before do update_issue({ milestone_id: nil }) @@ -897,7 +901,7 @@ RSpec.describe Issues::UpdateService, :mailer, feature_category: :team_planning } service = described_class.new(container: project, current_user: user, params: params) - expect(Spam::SpamActionService).not_to receive(:new) + expect(issue).not_to receive(:check_for_spam) service.execute(issue) end diff --git a/spec/services/jira/requests/projects/list_service_spec.rb b/spec/services/jira/requests/projects/list_service_spec.rb index 37e9f66d273..f9e3a3e8510 100644 --- a/spec/services/jira/requests/projects/list_service_spec.rb +++ b/spec/services/jira/requests/projects/list_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Jira::Requests::Projects::ListService, feature_category: :projects do +RSpec.describe Jira::Requests::Projects::ListService, feature_category: :groups_and_projects do include AfterNextHelpers let(:jira_integration) { create(:jira_integration) } diff --git a/spec/services/jira_connect_installations/update_service_spec.rb b/spec/services/jira_connect_installations/update_service_spec.rb index 15f3b485b20..cb45865f6fe 100644 --- a/spec/services/jira_connect_installations/update_service_spec.rb +++ b/spec/services/jira_connect_installations/update_service_spec.rb @@ -137,11 +137,7 @@ RSpec.describe JiraConnectInstallations::UpdateService, feature_category: :integ it 'returns an error message' do expect(execute_service[:status]).to eq(:error) - expect(execute_service[:message]).to eq( - { - instance_url: ["Could not be installed on the instance. Error response code 422"] - } - ) + expect(execute_service[:message]).to eq("Could not be installed on the instance. Error response code 422") end context 'and the installation had a previous instance_url' do @@ -175,11 +171,7 @@ RSpec.describe JiraConnectInstallations::UpdateService, feature_category: :integ it 'returns an error message' do expect(execute_service[:status]).to eq(:error) - expect(execute_service[:message]).to eq( - { - instance_url: ["Could not be installed on the instance. Network error"] - } - ) + expect(execute_service[:message]).to eq("Could not be installed on the instance. Network error") end end end diff --git a/spec/services/markup/rendering_service_spec.rb b/spec/services/markup/rendering_service_spec.rb index 952ee33da98..1a30fcb648d 100644 --- a/spec/services/markup/rendering_service_spec.rb +++ b/spec/services/markup/rendering_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Markup::RenderingService, feature_category: :projects do +RSpec.describe Markup::RenderingService, feature_category: :groups_and_projects do describe '#execute' do let_it_be(:project) { create(:project, :repository) } let_it_be(:user) do diff --git a/spec/services/members/approve_access_request_service_spec.rb b/spec/services/members/approve_access_request_service_spec.rb index 6c0d47e98ba..460b1caad5b 100644 --- a/spec/services/members/approve_access_request_service_spec.rb +++ b/spec/services/members/approve_access_request_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::ApproveAccessRequestService, feature_category: :subgroups do +RSpec.describe Members::ApproveAccessRequestService, feature_category: :groups_and_projects do let(:project) { create(:project, :public) } let(:group) { create(:group, :public) } let(:current_user) { create(:user) } diff --git a/spec/services/members/base_service_spec.rb b/spec/services/members/base_service_spec.rb index 514c25fbc03..09c903bb82b 100644 --- a/spec/services/members/base_service_spec.rb +++ b/spec/services/members/base_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::BaseService, feature_category: :projects do +RSpec.describe Members::BaseService, feature_category: :groups_and_projects do let_it_be(:access_requester) { create(:group_member) } describe '#resolve_access_request_todos' do diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb index 13f233162cd..c9dee0aadda 100644 --- a/spec/services/members/create_service_spec.rb +++ b/spec/services/members/create_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_cache, :clean_gitlab_redis_shared_state, :sidekiq_inline, - feature_category: :subgroups do + feature_category: :groups_and_projects do let_it_be(:source, reload: true) { create(:project) } let_it_be(:user) { create(:user) } let_it_be(:member) { create(:user) } diff --git a/spec/services/members/creator_service_spec.rb b/spec/services/members/creator_service_spec.rb index 8191eefbe95..e58d501d02d 100644 --- a/spec/services/members/creator_service_spec.rb +++ b/spec/services/members/creator_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::CreatorService, feature_category: :subgroups do +RSpec.describe Members::CreatorService, feature_category: :groups_and_projects do let_it_be(:source, reload: true) { create(:group, :public) } let_it_be(:member_type) { GroupMember } let_it_be(:user) { create(:user) } diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 498b9576875..4218a3297b3 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::DestroyService, feature_category: :subgroups do +RSpec.describe Members::DestroyService, feature_category: :groups_and_projects do let(:current_user) { create(:user) } let(:member_user) { create(:user) } let(:group) { create(:group, :public) } diff --git a/spec/services/members/groups/creator_service_spec.rb b/spec/services/members/groups/creator_service_spec.rb index 4c13106145e..4716bc7485b 100644 --- a/spec/services/members/groups/creator_service_spec.rb +++ b/spec/services/members/groups/creator_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::Groups::CreatorService, feature_category: :subgroups do +RSpec.describe Members::Groups::CreatorService, feature_category: :groups_and_projects do let_it_be(:source, reload: true) { create(:group, :public) } let_it_be(:source2, reload: true) { create(:group, :public) } let_it_be(:user) { create(:user) } diff --git a/spec/services/members/import_project_team_service_spec.rb b/spec/services/members/import_project_team_service_spec.rb index af9b30aa0b3..7dcdb70f2cd 100644 --- a/spec/services/members/import_project_team_service_spec.rb +++ b/spec/services/members/import_project_team_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::ImportProjectTeamService, feature_category: :subgroups do +RSpec.describe Members::ImportProjectTeamService, feature_category: :groups_and_projects do describe '#execute' do let_it_be(:source_project) { create(:project) } let_it_be(:target_project) { create(:project) } diff --git a/spec/services/members/invitation_reminder_email_service_spec.rb b/spec/services/members/invitation_reminder_email_service_spec.rb index da23965eabe..2b72a4919b4 100644 --- a/spec/services/members/invitation_reminder_email_service_spec.rb +++ b/spec/services/members/invitation_reminder_email_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::InvitationReminderEmailService, feature_category: :subgroups do +RSpec.describe Members::InvitationReminderEmailService, feature_category: :groups_and_projects do describe 'sending invitation reminders' do subject { described_class.new(invitation).execute } diff --git a/spec/services/members/invite_member_builder_spec.rb b/spec/services/members/invite_member_builder_spec.rb index e7bbec4e0ef..62c33b42fa2 100644 --- a/spec/services/members/invite_member_builder_spec.rb +++ b/spec/services/members/invite_member_builder_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::InviteMemberBuilder, feature_category: :subgroups do +RSpec.describe Members::InviteMemberBuilder, feature_category: :groups_and_projects do let_it_be(:source) { create(:group) } let_it_be(:existing_member) { create(:group_member) } diff --git a/spec/services/members/invite_service_spec.rb b/spec/services/members/invite_service_spec.rb index 22294b3fda5..1c0466980f4 100644 --- a/spec/services/members/invite_service_spec.rb +++ b/spec/services/members/invite_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_shared_state, :sidekiq_inline, - feature_category: :subgroups do + feature_category: :groups_and_projects do let_it_be(:project, reload: true) { create(:project) } let_it_be(:user) { project.first_owner } let_it_be(:project_user) { create(:user) } diff --git a/spec/services/members/projects/creator_service_spec.rb b/spec/services/members/projects/creator_service_spec.rb index 7ec7361a285..7f2b1869847 100644 --- a/spec/services/members/projects/creator_service_spec.rb +++ b/spec/services/members/projects/creator_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::Projects::CreatorService, feature_category: :projects do +RSpec.describe Members::Projects::CreatorService, feature_category: :groups_and_projects do let_it_be(:source, reload: true) { create(:project, :public) } let_it_be(:source2, reload: true) { create(:project, :public) } let_it_be(:user) { create(:user) } diff --git a/spec/services/members/request_access_service_spec.rb b/spec/services/members/request_access_service_spec.rb index ef8ee6492ab..68eef253452 100644 --- a/spec/services/members/request_access_service_spec.rb +++ b/spec/services/members/request_access_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::RequestAccessService, feature_category: :subgroups do +RSpec.describe Members::RequestAccessService, feature_category: :groups_and_projects do let(:user) { create(:user) } shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do diff --git a/spec/services/members/standard_member_builder_spec.rb b/spec/services/members/standard_member_builder_spec.rb index 69b764f3f16..96dda83fe54 100644 --- a/spec/services/members/standard_member_builder_spec.rb +++ b/spec/services/members/standard_member_builder_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::StandardMemberBuilder, feature_category: :subgroups do +RSpec.describe Members::StandardMemberBuilder, feature_category: :groups_and_projects do let_it_be(:source) { create(:group) } let_it_be(:existing_member) { create(:group_member) } diff --git a/spec/services/members/unassign_issuables_service_spec.rb b/spec/services/members/unassign_issuables_service_spec.rb index 37dfbd16c56..9623cef868b 100644 --- a/spec/services/members/unassign_issuables_service_spec.rb +++ b/spec/services/members/unassign_issuables_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::UnassignIssuablesService, feature_category: :subgroups do +RSpec.describe Members::UnassignIssuablesService, feature_category: :groups_and_projects do let_it_be(:group) { create(:group, :private) } let_it_be(:project) { create(:project, group: group) } let_it_be(:user, reload: true) { create(:user) } diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb index b94b44c8485..1c4b1abcfdb 100644 --- a/spec/services/members/update_service_spec.rb +++ b/spec/services/members/update_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Members::UpdateService, feature_category: :subgroups do +RSpec.describe Members::UpdateService, feature_category: :groups_and_projects do let_it_be(:project) { create(:project, :public) } let_it_be(:group) { create(:group, :public) } let_it_be(:current_user) { create(:user) } diff --git a/spec/services/merge_requests/after_create_service_spec.rb b/spec/services/merge_requests/after_create_service_spec.rb index 50a3d49d4a3..7255d19ef8a 100644 --- a/spec/services/merge_requests/after_create_service_spec.rb +++ b/spec/services/merge_requests/after_create_service_spec.rb @@ -231,5 +231,30 @@ RSpec.describe MergeRequests::AfterCreateService, feature_category: :code_review expect(service).to have_received(:execute).with(merge_request) end + + describe 'logging' do + it 'logs specific events' do + ::Gitlab::ApplicationContext.push(caller_id: 'NewMergeRequestWorker') + + allow(Gitlab::AppLogger).to receive(:info).and_call_original + + [ + 'Executing hooks', + 'Executed hooks', + 'Creating pipeline', + 'Pipeline created' + ].each do |message| + expect(Gitlab::AppLogger).to receive(:info).with( + hash_including( + 'meta.caller_id' => 'NewMergeRequestWorker', + message: message, + merge_request_id: merge_request.id + ) + ).and_call_original + end + + execute_service + end + end end end diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 7705278f30d..51b1bed1dd3 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -66,6 +66,24 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state, f expect(merge_request.reload).to be_preparing end + describe 'checking for spam' do + it 'checks for spam' do + expect_next_instance_of(MergeRequest) do |instance| + expect(instance).to receive(:check_for_spam).with(user: user, action: :create) + end + + service.execute + end + + it 'does not persist when spam' do + allow_next_instance_of(MergeRequest) do |instance| + allow(instance).to receive(:spam?).and_return(true) + end + + expect(merge_request).not_to be_persisted + end + end + describe 'when marked with /draft' do context 'in title and in description' do let(:opts) do diff --git a/spec/services/merge_requests/mergeability/logger_spec.rb b/spec/services/merge_requests/mergeability/logger_spec.rb index 1f56b6bebdb..7863b69abf6 100644 --- a/spec/services/merge_requests/mergeability/logger_spec.rb +++ b/spec/services/merge_requests/mergeability/logger_spec.rb @@ -40,6 +40,37 @@ RSpec.describe MergeRequests::Mergeability::Logger, :request_store, feature_cate logger.commit end + context 'when block value responds to #success?' do + let(:success?) { true } + let(:check_result) { instance_double(Gitlab::MergeRequests::Mergeability::CheckResult, success?: success?) } + + let(:extra_data) do + { + 'mergeability.expensive_operation.successful.values' => [success?] + } + end + + shared_examples_for 'success state logger' do + it 'records operation success state' do + expect_next_instance_of(Gitlab::AppJsonLogger) do |app_logger| + expect(app_logger).to receive(:info).with(match(a_hash_including(loggable_data(**extra_data)))) + end + + expect(logger.instrument(mergeability_name: :expensive_operation) { check_result }).to eq(check_result) + + logger.commit + end + end + + it_behaves_like 'success state logger' + + context 'when not successful' do + let(:success?) { false } + + it_behaves_like 'success state logger' + end + end + context 'with multiple observations' do let(:operation_count) { 2 } diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 012eb5f6fca..52999b5a1ea 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -425,6 +425,22 @@ RSpec.describe MergeRequests::UpdateService, :mailer, feature_category: :code_re expect(update_merge_request(opts).reviewers).to eq [] end end + + describe 'checking for spam' do + it 'checks for spam' do + expect(merge_request).to receive(:check_for_spam).with(user: user, action: :update) + + update_merge_request(opts) + end + + it 'marks the merge request invalid' do + merge_request.spam! + + update_merge_request(title: 'New title') + + expect(merge_request).to be_invalid + end + end end context 'after_save callback to store_mentions' do @@ -496,13 +512,11 @@ RSpec.describe MergeRequests::UpdateService, :mailer, feature_category: :code_re before do merge_request.merge_error = 'Error' - perform_enqueued_jobs do - service.execute(merge_request) - @merge_request = MergeRequest.find(merge_request.id) - end + service.execute(merge_request) + @merge_request = MergeRequest.find(merge_request.id) end - it 'merges the MR', :sidekiq_might_not_need_inline do + it 'merges the MR', :sidekiq_inline do expect(@merge_request).to be_valid expect(@merge_request.state).to eq('merged') expect(@merge_request.merge_error).to be_nil @@ -517,13 +531,11 @@ RSpec.describe MergeRequests::UpdateService, :mailer, feature_category: :code_re sha: merge_request.diff_head_sha, status: :success) - perform_enqueued_jobs do - @merge_request = service.execute(merge_request) - @merge_request = MergeRequest.find(merge_request.id) - end + @merge_request = service.execute(merge_request) + @merge_request = MergeRequest.find(merge_request.id) end - it 'merges the MR', :sidekiq_might_not_need_inline do + it 'merges the MR', :sidekiq_inline do expect(@merge_request).to be_valid expect(@merge_request.state).to eq('merged') end @@ -674,7 +686,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer, feature_category: :code_re expect(Todo.where(attributes).count).to eq 1 end - it 'sends email reviewer change notifications to old and new reviewers', :sidekiq_might_not_need_inline do + it 'sends email reviewer change notifications to old and new reviewers', :sidekiq_inline do merge_request.reviewers = [user2] perform_enqueued_jobs do @@ -719,7 +731,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer, feature_category: :code_re end end - it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do + it 'sends notifications for subscribers of changed milestone', :sidekiq_inline do merge_request.milestone = create(:milestone, project: project) merge_request.save! @@ -751,7 +763,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer, feature_category: :code_re update_merge_request(milestone_id: create(:milestone, project: project).id) end - it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do + it 'sends notifications for subscribers of changed milestone', :sidekiq_inline do perform_enqueued_jobs do update_merge_request(milestone_id: create(:milestone, project: project).id) end @@ -867,7 +879,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer, feature_category: :code_re merge_request.update_attribute(:title, draft_title) end - it 'sends notifications for subscribers', :sidekiq_might_not_need_inline do + it 'sends notifications for subscribers', :sidekiq_inline do opts = { title: 'New title' } perform_enqueued_jobs do @@ -899,7 +911,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer, feature_category: :code_re merge_request.update_attribute(:title, title) end - it 'does not send notifications', :sidekiq_might_not_need_inline do + it 'does not send notifications', :sidekiq_inline do opts = { title: 'Draft: New title' } perform_enqueued_jobs do @@ -936,7 +948,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer, feature_category: :code_re project.add_developer(subscriber) end - it 'sends notifications for subscribers of newly added labels', :sidekiq_might_not_need_inline do + it 'sends notifications for subscribers of newly added labels', :sidekiq_inline do opts = { label_ids: [label.id] } perform_enqueued_jobs do diff --git a/spec/services/namespace_settings/update_service_spec.rb b/spec/services/namespace_settings/update_service_spec.rb index 5f1ff6746bc..daffae1dda7 100644 --- a/spec/services/namespace_settings/update_service_spec.rb +++ b/spec/services/namespace_settings/update_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe NamespaceSettings::UpdateService, feature_category: :subgroups do +RSpec.describe NamespaceSettings::UpdateService, feature_category: :groups_and_projects do let(:user) { create(:user) } let(:group) { create(:group) } let(:settings) { {} } diff --git a/spec/services/namespaces/package_settings/update_service_spec.rb b/spec/services/namespaces/package_settings/update_service_spec.rb index e21c9a8f1b9..385fd7c130e 100644 --- a/spec/services/namespaces/package_settings/update_service_spec.rb +++ b/spec/services/namespaces/package_settings/update_service_spec.rb @@ -81,6 +81,15 @@ RSpec.describe ::Namespaces::PackageSettings::UpdateService, feature_category: : end end + # To be removed when raise_group_admin_package_permission_to_owner FF is removed + shared_examples 'disabling admin_package feature flag' do |action:| + before do + stub_feature_flags(raise_group_admin_package_permission_to_owner: false) + end + + it_behaves_like "#{action} the namespace package setting" + end + context 'with existing namespace package setting' do let_it_be(:package_settings) { create(:namespace_package_setting, namespace: namespace) } let_it_be(:params) do @@ -99,7 +108,8 @@ RSpec.describe ::Namespaces::PackageSettings::UpdateService, feature_category: : end where(:user_role, :shared_examples_name) do - :maintainer | 'updating the namespace package setting' + :owner | 'updating the namespace package setting' + :maintainer | 'denying access to namespace package setting' :developer | 'denying access to namespace package setting' :reporter | 'denying access to namespace package setting' :guest | 'denying access to namespace package setting' @@ -112,6 +122,7 @@ RSpec.describe ::Namespaces::PackageSettings::UpdateService, feature_category: : end it_behaves_like params[:shared_examples_name] + it_behaves_like 'disabling admin_package feature flag', action: :updating if params[:user_role] == :maintainer end end @@ -119,7 +130,8 @@ RSpec.describe ::Namespaces::PackageSettings::UpdateService, feature_category: : let_it_be(:package_settings) { namespace.package_settings } where(:user_role, :shared_examples_name) do - :maintainer | 'creating the namespace package setting' + :owner | 'creating the namespace package setting' + :maintainer | 'denying access to namespace package setting' :developer | 'denying access to namespace package setting' :reporter | 'denying access to namespace package setting' :guest | 'denying access to namespace package setting' @@ -132,6 +144,7 @@ RSpec.describe ::Namespaces::PackageSettings::UpdateService, feature_category: : end it_behaves_like params[:shared_examples_name] + it_behaves_like 'disabling admin_package feature flag', action: :creating if params[:user_role] == :maintainer end end end diff --git a/spec/services/namespaces/statistics_refresher_service_spec.rb b/spec/services/namespaces/statistics_refresher_service_spec.rb index 750f98615cc..4c33e5e80d6 100644 --- a/spec/services/namespaces/statistics_refresher_service_spec.rb +++ b/spec/services/namespaces/statistics_refresher_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Namespaces::StatisticsRefresherService, '#execute', feature_category: :subgroups do +RSpec.describe Namespaces::StatisticsRefresherService, '#execute', feature_category: :groups_and_projects do let(:group) { create(:group) } let(:subgroup) { create(:group, parent: group) } let(:projects) { create_list(:project, 5, namespace: group) } diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 240d81bb485..22509885c92 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -30,6 +30,24 @@ RSpec.describe Notes::CreateService, feature_category: :team_planning do expect(note).to be_persisted end + it 'checks for spam' do + expect_next_instance_of(Note) do |instance| + expect(instance).to receive(:check_for_spam).with(action: :create, user: user) + end + + note + end + + it 'does not persist when spam' do + expect_next_instance_of(Note) do |instance| + expect(instance).to receive(:check_for_spam).with(action: :create, user: user) do + instance.spam! + end + end + + expect(note).not_to be_persisted + end + context 'with internal parameter' do context 'when confidential' do let(:opts) { base_opts.merge(internal: true) } @@ -482,7 +500,7 @@ RSpec.describe Notes::CreateService, feature_category: :team_planning do expect(noteable.target_branch == "fix").to eq(can_use_quick_action) } ), - # Set WIP status + # Set Draft status QuickAction.new( action_text: "/draft", before_action: -> { @@ -511,7 +529,7 @@ RSpec.describe Notes::CreateService, feature_category: :team_planning do end end - context 'when note only have commands' do + context 'when note only has commands' do it 'adds commands applied message to note errors' do note_text = %(/close) service = double(:service) @@ -540,6 +558,28 @@ RSpec.describe Notes::CreateService, feature_category: :team_planning do expect(note.errors[:commands_only]).to contain_exactly('Closed this issue. Could not apply reopen command.') end + + it 'does not check for spam' do + expect_next_instance_of(Note) do |instance| + expect(instance).not_to receive(:check_for_spam).with(action: :create, user: user) + end + + note_text = %(/close) + described_class.new(project, user, opts.merge(note: note_text)).execute + end + + it 'generates failed update error messages' do + note_text = %(/confidential) + service = double(:service) + issue.errors.add(:confidential, 'an error occurred') + allow(Issues::UpdateService).to receive(:new).and_return(service) + allow_next_instance_of(Issues::UpdateService) do |service_instance| + allow(service_instance).to receive(:execute).and_return(issue) + end + + note = described_class.new(project, user, opts.merge(note: note_text)).execute + expect(note.errors[:commands_only]).to contain_exactly('Confidential an error occurred') + end end end diff --git a/spec/services/notes/quick_actions_service_spec.rb b/spec/services/notes/quick_actions_service_spec.rb index c65a077f907..cd3a4e8a395 100644 --- a/spec/services/notes/quick_actions_service_spec.rb +++ b/spec/services/notes/quick_actions_service_spec.rb @@ -248,6 +248,46 @@ RSpec.describe Notes::QuickActionsService, feature_category: :team_planning do end end end + + describe '/promote_to' do + shared_examples 'promotes work item' do |from:, to:| + it 'leaves the note empty' do + expect(execute(note)).to be_empty + end + + it 'promotes to provided type' do + expect { execute(note) }.to change { noteable.work_item_type.base_type }.from(from).to(to) + end + end + + context 'on a task' do + let_it_be_with_reload(:noteable) { create(:work_item, :task, project: project) } + let_it_be(:note_text) { '/promote_to Issue' } + let_it_be(:note) { create(:note, noteable: noteable, project: project, note: note_text) } + + it_behaves_like 'promotes work item', from: 'task', to: 'issue' + + context 'when type name is lower case' do + let_it_be(:note_text) { '/promote_to issue' } + + it_behaves_like 'promotes work item', from: 'task', to: 'issue' + end + end + + context 'on an issue' do + let_it_be_with_reload(:noteable) { create(:work_item, :issue, project: project) } + let_it_be(:note_text) { '/promote_to Incident' } + let_it_be(:note) { create(:note, noteable: noteable, project: project, note: note_text) } + + it_behaves_like 'promotes work item', from: 'issue', to: 'incident' + + context 'when type name is lower case' do + let_it_be(:note_text) { '/promote_to incident' } + + it_behaves_like 'promotes work item', from: 'issue', to: 'incident' + end + end + end end describe '.supported?' do @@ -380,6 +420,87 @@ RSpec.describe Notes::QuickActionsService, feature_category: :team_planning do end end + describe '#apply_updates' do + include_context 'note on noteable' + + let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:work_item, reload: true) { create(:work_item, :issue, project: project) } + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + let_it_be(:issue_note) { create(:note_on_issue, project: project, noteable: issue) } + let_it_be(:work_item_note) { create(:note, project: project, noteable: work_item) } + let_it_be(:mr_note) { create(:note_on_merge_request, project: project, noteable: merge_request) } + let_it_be(:commit_note) { create(:note_on_commit, project: project) } + let(:update_params) { {} } + + subject(:apply_updates) { described_class.new(project, maintainer).apply_updates(update_params, note) } + + context 'with a note on an issue' do + let(:note) { issue_note } + + it 'returns successful service response if update returned no errors' do + update_params[:confidential] = true + expect(apply_updates.success?).to be true + end + + it 'returns service response with errors if update failed' do + update_params[:title] = "" + expect(apply_updates.success?).to be false + expect(apply_updates.message).to include("Title can't be blank") + end + end + + context 'with a note on a merge request' do + let(:note) { mr_note } + + it 'returns successful service response if update returned no errors' do + update_params[:title] = 'New title' + expect(apply_updates.success?).to be true + end + + it 'returns service response with errors if update failed' do + update_params[:title] = "" + expect(apply_updates.success?).to be false + expect(apply_updates.message).to include("Title can't be blank") + end + end + + context 'with a note on a work item' do + let(:note) { work_item_note } + + before do + update_params[:confidential] = true + end + + it 'returns successful service response if update returned no errors' do + expect(apply_updates.success?).to be true + end + + it 'returns service response with errors if update failed' do + task = create(:work_item, :task, project: project) + create(:parent_link, work_item: task, work_item_parent: work_item) + + expect(apply_updates.success?).to be false + expect(apply_updates.message) + .to include("A confidential work item cannot have a parent that already has non-confidential children.") + end + end + + context 'with a note on a commit' do + let(:note) { commit_note } + + it 'returns successful service response if update returned no errors' do + update_params[:tag_name] = 'test' + expect(apply_updates.success?).to be true + end + + it 'returns service response with errors if update failed' do + update_params[:tag_name] = '-test' + expect(apply_updates.success?).to be false + expect(apply_updates.message).to include('Tag name invalid') + end + end + end + context 'CE restriction for issue assignees' do describe '/assign' do let(:project) { create(:project) } diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index 245cc046775..e109bfbcd0b 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -47,6 +47,24 @@ RSpec.describe Notes::UpdateService, feature_category: :team_planning do end end + context 'when the note is invalid' do + let(:edit_note_text) { { note: 'new text' } } + + before do + allow(note).to receive(:valid?).and_return(false) + end + + it 'does not update the note' do + travel_to(1.day.from_now) do + expect { update_note(edit_note_text) }.not_to change { note.reload.updated_at } + end + end + + it 'returns the note' do + expect(update_note(edit_note_text)).to eq(note) + end + end + describe 'event tracking', :snowplow do let(:event) { Gitlab::UsageDataCounters::IssueActivityUniqueCounter::ISSUE_COMMENT_EDITED } @@ -89,6 +107,23 @@ RSpec.describe Notes::UpdateService, feature_category: :team_planning do expect { edit_note_text }.to change { note.reload.updated_by } end end + + it 'checks for spam' do + expect(note).to receive(:check_for_spam).with(action: :update, user: user) + edit_note_text + end + + context 'when quick action only update' do + it "delete note and return commands_only error" do + updated_note = described_class.new(project, user, { note: "/close\n" }).execute(note) + + expect(updated_note.destroyed?).to eq(true) + expect(updated_note.errors).to match_array([ + "Note can't be blank", + "Commands only Closed this issue." + ]) + end + end end context 'when note text was not changed' do @@ -106,6 +141,11 @@ RSpec.describe Notes::UpdateService, feature_category: :team_planning do expect { does_not_edit_note_text }.not_to change { note.reload.updated_by } end end + + it 'does not check for spam' do + expect(note).not_to receive(:check_for_spam) + does_not_edit_note_text + end end context 'when the notable is a merge request' do diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index f63f982708d..99f3134f06f 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -798,9 +798,24 @@ RSpec.describe NotificationService, :mailer, feature_category: :team_planning do context 'issue note mention', :deliver_mails_inline do let_it_be(:issue) { create(:issue, project: project, assignees: [assignee]) } let_it_be(:mentioned_issue) { create(:issue, assignees: issue.assignees) } + let_it_be(:user_to_exclude) { create(:user) } let_it_be(:author) { create(:user) } - let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@all mentioned') } + let(:user_mentions) do + other_members = [ + @unsubscribed_mentioned, + @u_guest_watcher, + @pg_watcher, + @u_mentioned, + @u_not_mentioned, + @u_disabled, + @pg_disabled + ] + + (issue.project.team.members + other_members).map(&:to_reference).join(' ') + end + + let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: note_content) } before_all do build_team(project) @@ -815,108 +830,231 @@ RSpec.describe NotificationService, :mailer, feature_category: :team_planning do end describe '#new_note' do - it 'notifies the team members' do + it 'notifies parent group members with mention level' do + note = create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: "@#{@pg_mention.username}") + notification.new_note(note) - # Make sure @unsubscribed_mentioned is part of the team - expect(note.project.team.members).to include(@unsubscribed_mentioned) + should_email_nested_group_user(@pg_mention) + end + + shared_examples 'correct team members are notified' do + it 'notifies the team members' do + notification.new_note(note) - # Notify all team members - note.project.team.members.each do |member| - # User with disabled notification should not be notified - next if member.id == @u_disabled.id - # Author should not be notified - next if member.id == note.author.id + # Make sure @unsubscribed_mentioned is part of the team + expect(note.project.team.members).to include(@unsubscribed_mentioned) - should_email(member) + # Notify all team members + note.project.team.members.each do |member| + # User with disabled notification should not be notified + next if member.id == @u_disabled.id + # Author should not be notified + next if member.id == note.author.id + + should_email(member) + end + + should_email(@u_guest_watcher) + should_email(note.noteable.author) + should_email(note.noteable.assignees.first) + should_email_nested_group_user(@pg_watcher) + should_email(@u_mentioned) + should_email(@u_not_mentioned) + should_not_email(note.author) + should_not_email(@u_disabled) + should_not_email_nested_group_user(@pg_disabled) end - should_email(@u_guest_watcher) - should_email(note.noteable.author) - should_email(note.noteable.assignees.first) - should_email_nested_group_user(@pg_watcher) - should_email(@u_mentioned) - should_email(@u_not_mentioned) - should_not_email(note.author) - should_not_email(@u_disabled) - should_not_email_nested_group_user(@pg_disabled) - end + it 'filters out "mentioned in" notes' do + mentioned_note = SystemNoteService.cross_reference(mentioned_issue, issue, issue.author) - it 'notifies parent group members with mention level' do - note = create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: "@#{@pg_mention.username}") + expect(Notify).not_to receive(:note_issue_email) + notification.new_note(mentioned_note) + end - notification.new_note(note) + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end - should_email_nested_group_user(@pg_mention) - end + context 'when note is confidential' do + let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: note_content, confidential: true) } + let(:guest) { create(:user) } - it 'filters out "mentioned in" notes' do - mentioned_note = SystemNoteService.cross_reference(mentioned_issue, issue, issue.author) + it 'does not notify users that cannot read note' do + project.add_guest(guest) + reset_delivered_emails! - expect(Notify).not_to receive(:note_issue_email) - notification.new_note(mentioned_note) + notification.new_note(note) + + should_not_email(guest) + end + end end - it_behaves_like 'project emails are disabled' do - let(:notification_target) { note } - let(:notification_trigger) { notification.new_note(note) } + context 'when `disable_all_mention` FF is disabled' do + before do + stub_feature_flags(disable_all_mention: false) + end + + context 'when `@all` mention is used' do + let(:note_content) { "@all mentioned" } + + it_behaves_like 'correct team members are notified' + end + + context 'when users are individually mentioned' do + # `user_mentions` is concatenanting individual user mentions + # so that the end result is the same as `@all`. + let(:note_content) { "#{user_mentions} mentioned" } + + it_behaves_like 'correct team members are notified' + end end - context 'when note is confidential' do - let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@all mentioned', confidential: true) } - let(:guest) { create(:user) } + context 'when `disable_all_mention` FF is enabled' do + before do + stub_feature_flags(disable_all_mention: true) + end - it 'does not notify users that cannot read note' do - project.add_guest(guest) - reset_delivered_emails! + context 'when `@all` mention is used' do + before_all do + # user_to_exclude is in the note's project but is neither mentioned nor participating. + project.add_maintainer(user_to_exclude) + end - notification.new_note(note) + let(:note_content) { "@all mentioned" } - should_not_email(guest) + it "does not notify users who are not participating or mentioned" do + reset_delivered_emails! + + notification.new_note(note) + + should_email(note.noteable.author) + should_not_email(user_to_exclude) + end + end + + context 'when users are individually mentioned' do + # `user_mentions` is concatenanting individual user mentions + # so that the end result is the same as `@all`. + let(:note_content) { "#{user_mentions} mentioned" } + + it_behaves_like 'correct team members are notified' end end end end context 'project snippet note', :deliver_mails_inline do + let(:user_mentions) do + other_members = [ + @u_custom_global, + @u_guest_watcher, + snippet.author, # snippet = note.noteable's author + author, # note's author + @u_disabled, + @u_mentioned, + @u_not_mentioned + ] + + (snippet.project.team.members + other_members).map(&:to_reference).join(' ') + end + let(:snippet) { create(:project_snippet, project: project, author: create(:user)) } let(:author) { create(:user) } - let(:note) { create(:note_on_project_snippet, author: author, noteable: snippet, project_id: project.id, note: '@all mentioned') } + let(:note) { create(:note_on_project_snippet, author: author, noteable: snippet, project_id: project.id, note: note_content) } - before do - build_team(project) - build_group(project) - project.add_maintainer(author) + describe '#new_note' do + shared_examples 'correct team members are notified' do + before do + build_team(project) + build_group(project) + project.add_maintainer(author) + + # make sure these users can read the project snippet! + project.add_guest(@u_guest_watcher) + project.add_guest(@u_guest_custom) + add_member_for_parent_group(@pg_watcher, project) + reset_delivered_emails! + end - # make sure these users can read the project snippet! - project.add_guest(@u_guest_watcher) - project.add_guest(@u_guest_custom) - add_member_for_parent_group(@pg_watcher, project) - reset_delivered_emails! - end + it 'notifies the team members' do + notification.new_note(note) + # Notify all team members + note.project.team.members.each do |member| + # User with disabled notification should not be notified + next if member.id == @u_disabled.id + # Author should not be notified + next if member.id == note.author.id + + should_email(member) + end - describe '#new_note' do - it 'notifies the team members' do - notification.new_note(note) - # Notify all team members - note.project.team.members.each do |member| - # User with disabled notification should not be notified - next if member.id == @u_disabled.id - # Author should not be notified - next if member.id == note.author.id + # it emails custom global users on mention + should_email(@u_custom_global) - should_email(member) + should_email(@u_guest_watcher) + should_email(note.noteable.author) + should_not_email(note.author) + should_email(@u_mentioned) + should_not_email(@u_disabled) + should_email(@u_not_mentioned) end + end - # it emails custom global users on mention - should_email(@u_custom_global) + context 'when `disable_all_mention` FF is disabled' do + before do + stub_feature_flags(disable_all_mention: false) + end - should_email(@u_guest_watcher) - should_email(note.noteable.author) - should_not_email(note.author) - should_email(@u_mentioned) - should_not_email(@u_disabled) - should_email(@u_not_mentioned) + context 'when `@all` mention is used' do + let(:note_content) { "@all mentioned" } + + it_behaves_like 'correct team members are notified' + end + + context 'when users are individually mentioned' do + # `user_mentions` is concatenanting individual user mentions + # so that the end result is the same as `@all`. + let(:note_content) { "#{user_mentions} mentioned" } + + it_behaves_like 'correct team members are notified' + end + end + + context 'when `disable_all_mention` FF is enabled' do + before do + stub_feature_flags(disable_all_mention: true) + end + + context 'when `@all` mention is used' do + let(:user_to_exclude) { create(:user) } + let(:note_content) { "@all mentioned" } + + before do + project.add_maintainer(author) + project.add_maintainer(user_to_exclude) + + reset_delivered_emails! + end + + it "does not notify users who are not participating or mentioned" do + notification.new_note(note) + + should_email(note.noteable.author) + should_not_email(user_to_exclude) + end + end + + context 'when users are individually mentioned' do + # `user_mentions` is concatenanting individual user mentions + # so that the end result is the same as `@all`. + let(:note_content) { "#{user_mentions} mentioned" } + + it_behaves_like 'correct team members are notified' + end end end end diff --git a/spec/services/object_storage/delete_stale_direct_uploads_service_spec.rb b/spec/services/object_storage/delete_stale_direct_uploads_service_spec.rb new file mode 100644 index 00000000000..e44d57e9bb5 --- /dev/null +++ b/spec/services/object_storage/delete_stale_direct_uploads_service_spec.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ObjectStorage::DeleteStaleDirectUploadsService, :direct_uploads, :clean_gitlab_redis_shared_state, feature_category: :shared do + let(:service) { described_class.new } + + describe '#execute', :aggregate_failures do + subject(:execute_result) { service.execute } + + let(:location_identifier) { JobArtifactUploader.storage_location_identifier } + let(:fog_connection) { stub_artifacts_object_storage(JobArtifactUploader, direct_upload: true) } + + let(:stale_path_1) { 'stale/path/123' } + let!(:stale_object_1) do + fog_connection.directories + .new(key: location_identifier.to_s) + .files + .create( # rubocop:disable Rails/SaveBang + key: stale_path_1, + body: 'something' + ) + end + + let(:stale_path_2) { 'stale/path/456' } + let!(:stale_object_2) do + fog_connection.directories + .new(key: location_identifier.to_s) + .files + .create( # rubocop:disable Rails/SaveBang + key: stale_path_2, + body: 'something' + ) + end + + let(:non_stale_path) { 'nonstale/path/123' } + let!(:non_stale_object) do + fog_connection.directories + .new(key: location_identifier.to_s) + .files + .create( # rubocop:disable Rails/SaveBang + key: non_stale_path, + body: 'something' + ) + end + + it 'only deletes stale entries', :aggregate_failures do + prepare_pending_direct_upload(stale_path_1, 5.hours.ago) + prepare_pending_direct_upload(stale_path_2, 4.hours.ago) + prepare_pending_direct_upload(non_stale_path, 3.minutes.ago) + + expect(execute_result).to eq( + status: :success, + total_pending_entries: 3, + total_deleted_stale_entries: 2, + execution_timeout: false + ) + + expect_not_to_have_pending_direct_upload(stale_path_1) + expect_pending_uploaded_object_not_to_exist(stale_path_1) + + expect_not_to_have_pending_direct_upload(stale_path_2) + expect_pending_uploaded_object_not_to_exist(stale_path_2) + + expect_to_have_pending_direct_upload(non_stale_path) + expect_pending_uploaded_object_to_exist(non_stale_path) + end + + context 'when a stale entry does not have a matching object in the storage' do + it 'does not fail and still remove the stale entry' do + stale_no_object_path = 'some/other/path' + prepare_pending_direct_upload(stale_path_1, 5.hours.ago) + prepare_pending_direct_upload(stale_no_object_path, 5.hours.ago) + + expect(execute_result[:status]).to eq(:success) + + expect_not_to_have_pending_direct_upload(stale_path_1) + expect_pending_uploaded_object_not_to_exist(stale_path_1) + + expect_not_to_have_pending_direct_upload(stale_no_object_path) + end + end + + context 'when timeout happens' do + before do + stub_const("#{described_class}::MAX_EXEC_DURATION", 0.seconds) + + prepare_pending_direct_upload(stale_path_1, 5.hours.ago) + prepare_pending_direct_upload(stale_path_2, 4.hours.ago) + end + + it 'completes the current iteration and reports information about total entries' do + expect(execute_result).to eq( + status: :success, + total_pending_entries: 2, + total_deleted_stale_entries: 1, + execution_timeout: true + ) + + expect_not_to_have_pending_direct_upload(stale_path_1) + expect_pending_uploaded_object_not_to_exist(stale_path_1) + + expect_to_have_pending_direct_upload(stale_path_2) + expect_pending_uploaded_object_to_exist(stale_path_2) + end + end + end +end diff --git a/spec/services/packages/debian/create_package_file_service_spec.rb b/spec/services/packages/debian/create_package_file_service_spec.rb index b527bf8c1de..7dfbfa0b429 100644 --- a/spec/services/packages/debian/create_package_file_service_spec.rb +++ b/spec/services/packages/debian/create_package_file_service_spec.rb @@ -35,7 +35,6 @@ RSpec.describe Packages::Debian::CreatePackageFileService, feature_category: :pa expect(::Packages::Debian::ProcessPackageFileWorker).not_to receive(:perform_async) end - expect(::Packages::Debian::ProcessChangesWorker).not_to receive(:perform_async) expect(package_file).to be_valid expect(package_file.file.read).to start_with('!<arch>') expect(package_file.size).to eq(1124) @@ -52,8 +51,8 @@ RSpec.describe Packages::Debian::CreatePackageFileService, feature_category: :pa shared_examples 'a valid changes' do it 'creates a new package file', :aggregate_failures do - expect(::Packages::Debian::ProcessChangesWorker) - .to receive(:perform_async).with(an_instance_of(Integer), current_user.id) + expect(::Packages::Debian::ProcessPackageFileWorker) + .to receive(:perform_async).with(an_instance_of(Integer), nil, nil) expect(package_file).to be_valid expect(package_file.file.read).to start_with('Format: 1.8') diff --git a/spec/services/packages/debian/extract_changes_metadata_service_spec.rb b/spec/services/packages/debian/extract_changes_metadata_service_spec.rb index a22c1fc7acc..68137c3abe1 100644 --- a/spec/services/packages/debian/extract_changes_metadata_service_spec.rb +++ b/spec/services/packages/debian/extract_changes_metadata_service_spec.rb @@ -1,13 +1,17 @@ # frozen_string_literal: true + require 'spec_helper' RSpec.describe Packages::Debian::ExtractChangesMetadataService, feature_category: :package_registry do describe '#execute' do let_it_be(:incoming) { create(:debian_incoming) } + let_it_be(:temp_package) do + create(:debian_package, without_package_files: true, with_changes_file: true, project: incoming.project) + end let(:source_file) { incoming.package_files.first } let(:dsc_file) { incoming.package_files.second } - let(:changes_file) { incoming.package_files.last } + let(:changes_file) { temp_package.package_files.first } let(:service) { described_class.new(changes_file) } subject { service.execute } diff --git a/spec/services/packages/debian/process_changes_service_spec.rb b/spec/services/packages/debian/process_changes_service_spec.rb index dbfcc359f9c..39b917cf1bc 100644 --- a/spec/services/packages/debian/process_changes_service_spec.rb +++ b/spec/services/packages/debian/process_changes_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Packages::Debian::ProcessChangesService, feature_category: :packa let_it_be(:user) { create(:user) } let_it_be_with_reload(:distribution) { create(:debian_project_distribution, :with_file, suite: 'unstable') } - let!(:incoming) { create(:debian_incoming, project: distribution.project) } + let!(:incoming) { create(:debian_incoming, project: distribution.project, with_changes_file: true) } let(:package_file) { incoming.package_files.with_file_name('sample_1.2.3~alpha2_amd64.changes').first } diff --git a/spec/services/packages/debian/process_package_file_service_spec.rb b/spec/services/packages/debian/process_package_file_service_spec.rb index 7782b5fc1a6..d4e37403b87 100644 --- a/spec/services/packages/debian/process_package_file_service_spec.rb +++ b/spec/services/packages/debian/process_package_file_service_spec.rb @@ -3,211 +3,408 @@ require 'spec_helper' RSpec.describe Packages::Debian::ProcessPackageFileService, feature_category: :package_registry do + include ExclusiveLeaseHelpers + + let_it_be(:distribution) { create(:debian_project_distribution, :with_file, suite: 'unstable') } + + let(:debian_file_metadatum) { package_file.debian_file_metadatum } + let(:service) { described_class.new(package_file, distribution_name, component_name) } + describe '#execute' do - let_it_be_with_reload(:distribution) { create(:debian_project_distribution, :with_suite, :with_file) } + using RSpec::Parameterized::TableSyntax + + subject { service.execute } + + shared_examples 'common validations' do + context 'with package file without Debian metadata' do + let!(:package_file) { create(:debian_package_file, without_loaded_metadatum: true) } + + let(:expected_error) { ArgumentError } + let(:expected_message) { 'package file without Debian metadata' } + + it_behaves_like 'raises error' + end + + context 'with already processed package file' do + let!(:package_file) { create(:debian_package_file) } + + let(:expected_error) { ArgumentError } + let(:expected_message) { 'already processed package file' } + + it_behaves_like 'raises error' + end + + context 'without a distribution' do + let(:expected_error) { ActiveRecord::RecordNotFound } + let(:expected_message) { /^Couldn't find Packages::Debian::ProjectDistribution with / } + + before do + # Workaround ActiveRecord cache + Packages::Debian::ProjectDistribution.find(distribution.id).delete + end + + it_behaves_like 'raises error' + end + + context 'when there is a matching published package in another distribution' do + let!(:matching_package) do + create( + :debian_package, + project: distribution.project, + name: 'sample', + version: '1.2.3~alpha2' + ) + end + + let(:expected_error) { ArgumentError } + + let(:expected_message) do + "Debian package sample 1.2.3~alpha2 exists in distribution #{matching_package.debian_distribution.codename}" + end + + it_behaves_like 'raises error' + end + end + + shared_examples 'raises error' do + it 'raises error', :aggregate_failures do + expect(::Packages::Debian::GenerateDistributionWorker).not_to receive(:perform_async) + expect { subject } + .to not_change(Packages::Package, :count) + .and not_change(Packages::PackageFile, :count) + .and not_change { Packages::Debian::Publication.count } + .and not_change(package.package_files, :count) + .and not_change { package.reload.name } + .and not_change { package.version } + .and not_change { package.status } + .and not_change { debian_file_metadatum&.reload&.file_type } + .and not_change { debian_file_metadatum&.component } + .and raise_error(expected_error, expected_message) + end + end - let!(:package) { create(:debian_package, :processing, project: distribution.project, published_in: nil) } - let(:distribution_name) { distribution.codename } - let(:component_name) { 'main' } - let(:debian_file_metadatum) { package_file.debian_file_metadatum } + shared_examples 'does nothing' do + it 'does nothing', :aggregate_failures do + expect(::Packages::Debian::GenerateDistributionWorker).not_to receive(:perform_async) + expect { subject } + .to not_change(Packages::Package, :count) + .and not_change(Packages::PackageFile, :count) + .and not_change { Packages::Debian::Publication.count } + .and not_change(package.package_files, :count) + .and not_change { package.reload.name } + .and not_change { package.version } + .and not_change { package.status } + .and not_change { debian_file_metadatum&.reload&.file_type } + .and not_change { debian_file_metadatum&.component } + end + end - subject { described_class.new(package_file, distribution_name, component_name) } + shared_examples 'updates package and changes file' do + it 'updates package and changes file', :aggregate_failures do + expect(::Packages::Debian::GenerateDistributionWorker) + .to receive(:perform_async).with(:project, distribution.id) + expect { subject } + .to not_change(Packages::Package, :count) + .and not_change(Packages::PackageFile, :count) + .and change { Packages::Debian::Publication.count }.by(1) + .and change { package.package_files.count }.from(1).to(8) + .and change { package.reload.name }.to('sample') + .and change { package.version }.to('1.2.3~alpha2') + .and change { package.status }.from('processing').to('default') + .and change { package.debian_publication }.from(nil) + .and change { debian_file_metadatum.file_type }.from('unknown').to('changes') + .and not_change { debian_file_metadatum.component } + end + end shared_examples 'updates package and package file' do it 'updates package and package file', :aggregate_failures do expect(::Packages::Debian::GenerateDistributionWorker) .to receive(:perform_async).with(:project, distribution.id) - expect { subject.execute } + expect { subject } .to not_change(Packages::Package, :count) .and not_change(Packages::PackageFile, :count) .and change { Packages::Debian::Publication.count }.by(1) .and not_change(package.package_files, :count) .and change { package.reload.name }.to('sample') - .and change { package.reload.version }.to('1.2.3~alpha2') - .and change { package.reload.status }.from('processing').to('default') - .and change { package.reload.debian_publication }.from(nil) + .and change { package.version }.to('1.2.3~alpha2') + .and change { package.status }.from('processing').to('default') + .and change { package.debian_publication }.from(nil) .and change { debian_file_metadatum.file_type }.from('unknown').to(expected_file_type) .and change { debian_file_metadatum.component }.from(nil).to(component_name) end end - using RSpec::Parameterized::TableSyntax + context 'with a changes file' do + let!(:incoming) { create(:debian_incoming, project: distribution.project) } + let!(:temporary_with_changes) { create(:debian_temporary_with_changes, project: distribution.project) } + let(:package) { temporary_with_changes } - where(:case_name, :expected_file_type, :file_name, :component_name) do - 'with a deb' | 'deb' | 'libsample0_1.2.3~alpha2_amd64.deb' | 'main' - 'with an udeb' | 'udeb' | 'sample-udeb_1.2.3~alpha2_amd64.udeb' | 'contrib' - 'with an ddeb' | 'ddeb' | 'sample-ddeb_1.2.3~alpha2_amd64.ddeb' | 'main' - end + let(:package_file) { temporary_with_changes.package_files.first } + let(:distribution_name) { nil } + let(:component_name) { nil } - with_them do - context 'with Debian package file' do - let(:package_file) { package.package_files.with_file_name(file_name).first } + it_behaves_like 'common validations' - context 'when there is no matching published package' do - it_behaves_like 'updates package and package file' + context 'with distribution_name' do + let(:distribution_name) { distribution.codename } + let(:expected_error) { ArgumentError } + let(:expected_message) { 'unwanted distribution name' } - context 'with suite as distribution name' do - let(:distribution_name) { distribution.suite } + it_behaves_like 'raises error' + end - it_behaves_like 'updates package and package file' - end + context 'with component_name' do + let(:component_name) { 'main' } + let(:expected_error) { ArgumentError } + let(:expected_message) { 'unwanted component name' } + + it_behaves_like 'raises error' + end + + context 'with crafted file_metadata' do + let(:complete_file_metadata) do + { + file_type: :changes, + fields: { + 'Source' => 'abc', + 'Version' => '1.0', + 'Distribution' => 'sid' + } + } end - context 'when there is a matching published package' do - let!(:matching_package) do - create( - :debian_package, - project: distribution.project, - published_in: distribution, - name: 'sample', - version: '1.2.3~alpha2' - ) - end + let(:expected_error) { ArgumentError } - it 'reuses existing package and update package file', :aggregate_failures do - expect(::Packages::Debian::GenerateDistributionWorker) - .to receive(:perform_async).with(:project, distribution.id) - expect { subject.execute } - .to change { Packages::Package.count }.from(2).to(1) - .and change { Packages::PackageFile.count }.from(16).to(9) - .and not_change(Packages::Debian::Publication, :count) - .and change { package.package_files.count }.from(8).to(0) - .and change { package_file.package }.from(package).to(matching_package) - .and not_change(matching_package, :name) - .and not_change(matching_package, :version) - .and change { debian_file_metadatum.file_type }.from('unknown').to(expected_file_type) - .and change { debian_file_metadatum.component }.from(nil).to(component_name) - - expect { package.reload } - .to raise_error(ActiveRecord::RecordNotFound) + before do + allow_next_instance_of(::Packages::Debian::ExtractChangesMetadataService) do |extract_changes_metadata_svc| + allow(extract_changes_metadata_svc).to receive(:execute).and_return(file_metadata) end end - context 'when there is a matching published package in another distribution' do - let!(:matching_package) do - create( - :debian_package, - project: distribution.project, - name: 'sample', - version: '1.2.3~alpha2' - ) - end + context 'with missing Source field' do + let(:file_metadata) { complete_file_metadata.tap { |m| m[:fields].delete 'Source' } } + let(:expected_message) { 'missing Source field' } - it 'raise ArgumentError', :aggregate_failures do - expect(::Packages::Debian::GenerateDistributionWorker).not_to receive(:perform_async) - expect { subject.execute } - .to not_change(Packages::Package, :count) - .and not_change(Packages::PackageFile, :count) - .and not_change(package.package_files, :count) - .and raise_error(ArgumentError, "Debian package sample 1.2.3~alpha2 exists " \ - "in distribution #{matching_package.debian_distribution.codename}") - end + it_behaves_like 'raises error' end - context 'when there is a matching published package pending destruction' do - let!(:matching_package) do - create( - :debian_package, - :pending_destruction, - project: distribution.project, - published_in: distribution, - name: 'sample', - version: '1.2.3~alpha2' - ) - end + context 'with missing Version field' do + let(:file_metadata) { complete_file_metadata.tap { |m| m[:fields].delete 'Version' } } + let(:expected_message) { 'missing Version field' } + + it_behaves_like 'raises error' + end + + context 'with missing Distribution field' do + let(:file_metadata) { complete_file_metadata.tap { |m| m[:fields].delete 'Distribution' } } + let(:expected_message) { 'missing Distribution field' } - it_behaves_like 'updates package and package file' + it_behaves_like 'raises error' end end - end - context 'without a distribution' do - let(:package_file) { package.package_files.with_file_name('libsample0_1.2.3~alpha2_amd64.deb').first } - let(:component_name) { 'main' } + context 'when lease is already taken' do + before do + stub_exclusive_lease_taken( + "packages:debian:process_package_file_service:#{distribution.project_id}_sample_1.2.3~alpha2", + timeout: Packages::Debian::ProcessPackageFileService::DEFAULT_LEASE_TIMEOUT) + end - before do - distribution.destroy! + it_behaves_like 'does nothing' end - it 'raise ActiveRecord::RecordNotFound', :aggregate_failures do - expect(::Packages::Debian::GenerateDistributionWorker).not_to receive(:perform_async) - expect { subject.execute } - .to not_change(Packages::Package, :count) - .and not_change(Packages::PackageFile, :count) - .and not_change(package.package_files, :count) - .and raise_error(ActiveRecord::RecordNotFound) + context 'when there is no matching published package' do + it_behaves_like 'updates package and changes file' end - end - context 'without distribution name' do - let!(:package_file) { create(:debian_package_file, without_loaded_metadatum: true) } - let(:distribution_name) { '' } + context 'when there is a matching published package' do + let!(:matching_package) do + create( + :debian_package, + project: distribution.project, + published_in: distribution, + name: 'sample', + version: '1.2.3~alpha2' + ) + end - it 'raise ArgumentError', :aggregate_failures do - expect(::Packages::Debian::GenerateDistributionWorker).not_to receive(:perform_async) - expect { subject.execute } - .to not_change(Packages::Package, :count) - .and not_change(Packages::PackageFile, :count) - .and not_change(package.package_files, :count) - .and raise_error(ArgumentError, 'missing distribution name') + it 'reuses existing package and update package file', :aggregate_failures do + expect(::Packages::Debian::GenerateDistributionWorker) + .to receive(:perform_async).with(:project, distribution.id) + expect { subject } + .to change { Packages::Package.count }.from(3).to(2) + .and not_change { Packages::PackageFile.count } + .and not_change(Packages::Debian::Publication, :count) + .and change { package.package_files.count }.from(1).to(0) + .and change { incoming.package_files.count }.from(7).to(0) + .and change { matching_package.package_files.count }.from(7).to(15) + .and change { package_file.package }.from(package).to(matching_package) + .and not_change(matching_package, :name) + .and not_change(matching_package, :version) + .and change { debian_file_metadatum.file_type }.from('unknown').to('changes') + .and not_change { debian_file_metadatum.component } + + expect { package.reload } + .to raise_error(ActiveRecord::RecordNotFound) + end end - end - context 'without component name' do - let!(:package_file) { create(:debian_package_file, without_loaded_metadatum: true) } - let(:component_name) { '' } + context 'when there is a matching published package pending destruction' do + let!(:matching_package) do + create( + :debian_package, + :pending_destruction, + project: distribution.project, + published_in: distribution, + name: 'sample', + version: '1.2.3~alpha2' + ) + end - it 'raise ArgumentError', :aggregate_failures do - expect(::Packages::Debian::GenerateDistributionWorker).not_to receive(:perform_async) - expect { subject.execute } - .to not_change(Packages::Package, :count) - .and not_change(Packages::PackageFile, :count) - .and not_change(package.package_files, :count) - .and raise_error(ArgumentError, 'missing component name') + it_behaves_like 'updates package and changes file' end end - context 'with package file without Debian metadata' do - let!(:package_file) { create(:debian_package_file, without_loaded_metadatum: true) } + context 'with a package file' do + let!(:temporary_with_files) { create(:debian_temporary_with_files, project: distribution.project) } + let(:package) { temporary_with_files } + + let(:package_file) { package.package_files.with_file_name('libsample0_1.2.3~alpha2_amd64.deb').first } + let(:distribution_name) { distribution.codename } let(:component_name) { 'main' } - it 'raise ArgumentError', :aggregate_failures do - expect(::Packages::Debian::GenerateDistributionWorker).not_to receive(:perform_async) - expect { subject.execute } - .to not_change(Packages::Package, :count) - .and not_change(Packages::PackageFile, :count) - .and not_change(package.package_files, :count) - .and raise_error(ArgumentError, 'package file without Debian metadata') + where(:case_name, :expected_file_type, :file_name, :component_name) do + 'with a deb' | 'deb' | 'libsample0_1.2.3~alpha2_amd64.deb' | 'main' + 'with an udeb' | 'udeb' | 'sample-udeb_1.2.3~alpha2_amd64.udeb' | 'contrib' + 'with an ddeb' | 'ddeb' | 'sample-ddeb_1.2.3~alpha2_amd64.ddeb' | 'main' end - end - context 'with already processed package file' do - let_it_be(:package_file) { create(:debian_package_file) } + with_them do + context 'with Debian package file' do + let(:package_file) { package.package_files.with_file_name(file_name).first } - let(:component_name) { 'main' } + it_behaves_like 'common validations' - it 'raise ArgumentError', :aggregate_failures do - expect(::Packages::Debian::GenerateDistributionWorker).not_to receive(:perform_async) - expect { subject.execute } - .to not_change(Packages::Package, :count) - .and not_change(Packages::PackageFile, :count) - .and not_change(package.package_files, :count) - .and raise_error(ArgumentError, 'already processed package file') + context 'without distribution name' do + let(:distribution_name) { '' } + let(:expected_error) { ArgumentError } + let(:expected_message) { 'missing distribution name' } + + it_behaves_like 'raises error' + end + + context 'without component name' do + let(:component_name) { '' } + let(:expected_error) { ArgumentError } + let(:expected_message) { 'missing component name' } + + it_behaves_like 'raises error' + end + + context 'with invalid package file type' do + let(:package_file) { package.package_files.with_file_name('sample_1.2.3~alpha2.tar.xz').first } + let(:expected_error) { ArgumentError } + let(:expected_message) { 'invalid package file type: source' } + + it_behaves_like 'raises error' + end + + context 'when lease is already taken' do + before do + stub_exclusive_lease_taken( + "packages:debian:process_package_file_service:#{distribution.project_id}_sample_1.2.3~alpha2", + timeout: Packages::Debian::ProcessPackageFileService::DEFAULT_LEASE_TIMEOUT) + end + + it_behaves_like 'does nothing' + end + + context 'when there is no matching published package' do + it_behaves_like 'updates package and package file' + + context 'with suite as distribution name' do + let(:distribution_name) { distribution.suite } + + it_behaves_like 'updates package and package file' + end + end + + context 'when there is a matching published package' do + let!(:matching_package) do + create( + :debian_package, + project: distribution.project, + published_in: distribution, + name: 'sample', + version: '1.2.3~alpha2' + ) + end + + it 'reuses existing package and update package file', :aggregate_failures do + expect(::Packages::Debian::GenerateDistributionWorker) + .to receive(:perform_async).with(:project, distribution.id) + expect { subject } + .to change { Packages::Package.count }.from(2).to(1) + .and change { Packages::PackageFile.count }.from(14).to(8) + .and not_change(Packages::Debian::Publication, :count) + .and change { package.package_files.count }.from(7).to(0) + .and change { package_file.package }.from(package).to(matching_package) + .and not_change(matching_package, :name) + .and not_change(matching_package, :version) + .and change { debian_file_metadatum.file_type }.from('unknown').to(expected_file_type) + .and change { debian_file_metadatum.component }.from(nil).to(component_name) + + expect { package.reload } + .to raise_error(ActiveRecord::RecordNotFound) + end + end + + context 'when there is a matching published package pending destruction' do + let!(:matching_package) do + create( + :debian_package, + :pending_destruction, + project: distribution.project, + published_in: distribution, + name: 'sample', + version: '1.2.3~alpha2' + ) + end + + it_behaves_like 'updates package and package file' + end + end end end + end + + describe '#lease_key' do + let(:prefix) { 'packages:debian:process_package_file_service' } + + subject { service.send(:lease_key) } + + context 'with a changes file' do + let!(:incoming) { create(:debian_incoming, project: distribution.project) } + let!(:temporary_with_changes) { create(:debian_temporary_with_changes, project: distribution.project) } + let(:package) { temporary_with_changes } + + let(:package_file) { temporary_with_changes.package_files.first } + let(:distribution_name) { nil } + let(:component_name) { nil } + + it { is_expected.to eq("#{prefix}:#{distribution.project_id}_sample_1.2.3~alpha2") } + end + + context 'with a package file' do + let!(:temporary_with_files) { create(:debian_temporary_with_files, project: distribution.project) } + let(:package) { temporary_with_files } - context 'with invalid package file type' do - let(:package_file) { package.package_files.with_file_name('sample_1.2.3~alpha2.tar.xz').first } + let(:package_file) { package.package_files.with_file_name('libsample0_1.2.3~alpha2_amd64.deb').first } + let(:distribution_name) { distribution.codename } let(:component_name) { 'main' } - it 'raise ArgumentError', :aggregate_failures do - expect(::Packages::Debian::GenerateDistributionWorker).not_to receive(:perform_async) - expect { subject.execute } - .to not_change(Packages::Package, :count) - .and not_change(Packages::PackageFile, :count) - .and not_change(package.package_files, :count) - .and raise_error(ArgumentError, 'invalid package file type: source') - end + it { is_expected.to eq("#{prefix}:#{distribution.project_id}_sample_1.2.3~alpha2") } end end end diff --git a/spec/services/packages/generic/find_or_create_package_service_spec.rb b/spec/services/packages/generic/find_or_create_package_service_spec.rb index 07054fe3651..d0ffd297069 100644 --- a/spec/services/packages/generic/find_or_create_package_service_spec.rb +++ b/spec/services/packages/generic/find_or_create_package_service_spec.rb @@ -27,7 +27,7 @@ RSpec.describe Packages::Generic::FindOrCreatePackageService, feature_category: expect(package.creator).to eq(user) expect(package.name).to eq('mypackage') expect(package.version).to eq('0.0.1') - expect(package.original_build_info).to be_nil + expect(package.last_build_info).to be_nil end end @@ -42,7 +42,7 @@ RSpec.describe Packages::Generic::FindOrCreatePackageService, feature_category: expect(package.creator).to eq(user) expect(package.name).to eq('mypackage') expect(package.version).to eq('0.0.1') - expect(package.original_build_info.pipeline).to eq(ci_build.pipeline) + expect(package.last_build_info.pipeline).to eq(ci_build.pipeline) end end end @@ -60,7 +60,7 @@ RSpec.describe Packages::Generic::FindOrCreatePackageService, feature_category: expect(found_package).to eq(package) end.not_to change { project.packages.generic.count } - expect(package.reload.original_build_info).to be_nil + expect(package.reload.last_build_info).to be_nil end end @@ -80,7 +80,7 @@ RSpec.describe Packages::Generic::FindOrCreatePackageService, feature_category: expect(found_package).to eq(package) end.not_to change { project.packages.generic.count } - expect(package.reload.original_build_info.pipeline).to eq(pipeline) + expect(package.reload.last_build_info.pipeline).to eq(pipeline) end end @@ -97,7 +97,7 @@ RSpec.describe Packages::Generic::FindOrCreatePackageService, feature_category: expect(package.creator).to eq(user) expect(package.name).to eq('mypackage') expect(package.version).to eq('0.0.1') - expect(package.original_build_info).to be_nil + expect(package.last_build_info).to be_nil end end end diff --git a/spec/services/packages/ml_model/create_package_file_service_spec.rb b/spec/services/packages/ml_model/create_package_file_service_spec.rb new file mode 100644 index 00000000000..d749aee227a --- /dev/null +++ b/spec/services/packages/ml_model/create_package_file_service_spec.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::MlModel::CreatePackageFileService, feature_category: :mlops do + describe '#execute' do + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be(:pipeline) { create(:ci_pipeline, user: user, project: project) } + let_it_be(:file_name) { 'myfile.tar.gz.1' } + + let(:build) { instance_double(Ci::Build, pipeline: pipeline) } + + let(:sha256) { '440e5e148a25331bbd7991575f7d54933c0ebf6cc735a18ee5066ac1381bb590' } + let(:temp_file) { Tempfile.new("test") } + let(:file) { UploadedFile.new(temp_file.path, sha256: sha256) } + let(:package_service) { double } + + subject(:execute_service) { described_class.new(project, user, params).execute } + + before do + FileUtils.touch(temp_file) + end + + after do + FileUtils.rm_f(temp_file) + end + + context 'without existing package' do + let(:params) do + { + package_name: 'new_model', + package_version: '1.0.0', + file: file, + file_name: file_name + } + end + + it 'creates package file', :aggregate_failures do + expect { execute_service } + .to change { project.packages.ml_model.count }.by(1) + .and change { Packages::PackageFile.count }.by(1) + .and change { Packages::PackageFileBuildInfo.count }.by(0) + + new_model = project.packages.ml_model.last + package_file = new_model.package_files.last + + aggregate_failures do + expect(new_model.name).to eq('new_model') + expect(new_model.version).to eq('1.0.0') + expect(new_model.status).to eq('default') + expect(package_file.package).to eq(new_model) + expect(package_file.file_name).to eq(file_name) + expect(package_file.size).to eq(file.size) + expect(package_file.file_sha256).to eq(sha256) + end + end + end + + context 'with existing package' do + let_it_be(:model) { create(:ml_model_package, creator: user, project: project, version: '0.1.0') } + + let(:params) do + { + package_name: model.name, + package_version: model.version, + file: file, + file_name: file_name, + status: :hidden, + build: build + } + end + + it 'adds the package file and updates status and ci_build', :aggregate_failures do + expect { execute_service } + .to change { project.packages.ml_model.count }.by(0) + .and change { model.package_files.count }.by(1) + .and change { Packages::PackageFileBuildInfo.count }.by(1) + + model.reload + + package_file = model.package_files.last + + expect(model.build_infos.first.pipeline).to eq(build.pipeline) + expect(model.status).to eq('hidden') + + expect(package_file.package).to eq(model) + expect(package_file.file_name).to eq(file_name) + expect(package_file.size).to eq(file.size) + expect(package_file.file_sha256).to eq(sha256) + end + end + end +end diff --git a/spec/services/packages/ml_model/find_or_create_package_service_spec.rb b/spec/services/packages/ml_model/find_or_create_package_service_spec.rb new file mode 100644 index 00000000000..6e1e17da0e6 --- /dev/null +++ b/spec/services/packages/ml_model/find_or_create_package_service_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::MlModel::FindOrCreatePackageService, feature_category: :mlops do + let_it_be(:project) { create(:project) } + let_it_be(:user) { project.creator } + let_it_be(:ci_build) { create(:ci_build, :running, user: user, project: project) } + + let(:base_params) do + { + name: 'mymodel', + version: '0.0.1' + } + end + + let(:params) { base_params } + + describe '#execute' do + subject(:execute_service) { described_class.new(project, user, params).execute } + + context 'when model does not exist' do + it 'creates the model' do + expect { subject }.to change { project.packages.ml_model.count }.by(1) + + package = project.packages.ml_model.last + + aggregate_failures do + expect(package.creator).to eq(user) + expect(package.package_type).to eq('ml_model') + expect(package.name).to eq('mymodel') + expect(package.version).to eq('0.0.1') + expect(package.build_infos.count).to eq(0) + end + end + + context 'when build is provided' do + let(:params) { base_params.merge(build: ci_build) } + + it 'creates package and package build info' do + expect { subject }.to change { project.packages.ml_model.count }.by(1) + + package = project.packages.ml_model.last + + aggregate_failures do + expect(package.creator).to eq(user) + expect(package.package_type).to eq('ml_model') + expect(package.name).to eq('mymodel') + expect(package.version).to eq('0.0.1') + expect(package.build_infos.first.pipeline).to eq(ci_build.pipeline) + end + end + end + end + + context 'when model already exists' do + it 'does not create a new model', :aggregate_failures do + model = project.packages.ml_model.create!(params) + + expect do + new_model = subject + expect(new_model).to eq(model) + end.not_to change { project.packages.ml_model.count } + end + end + end +end diff --git a/spec/services/packages/npm/create_metadata_cache_service_spec.rb b/spec/services/packages/npm/create_metadata_cache_service_spec.rb index 75f822f0ddb..02f29dd94df 100644 --- a/spec/services/packages/npm/create_metadata_cache_service_spec.rb +++ b/spec/services/packages/npm/create_metadata_cache_service_spec.rb @@ -9,9 +9,8 @@ RSpec.describe Packages::Npm::CreateMetadataCacheService, :clean_gitlab_redis_sh let_it_be(:package_name) { "@#{project.root_namespace.path}/npm-test" } let_it_be(:package) { create(:npm_package, version: '1.0.0', project: project, name: package_name) } - let(:packages) { project.packages } let(:lease_key) { "packages:npm:create_metadata_cache_service:metadata_caches:#{project.id}_#{package_name}" } - let(:service) { described_class.new(project, package_name, packages) } + let(:service) { described_class.new(project, package_name) } describe '#execute' do let(:npm_metadata_cache) { Packages::Npm::MetadataCache.last } diff --git a/spec/services/packages/nuget/metadata_extraction_service_spec.rb b/spec/services/packages/nuget/metadata_extraction_service_spec.rb index 9177a5379d9..8954b89971e 100644 --- a/spec/services/packages/nuget/metadata_extraction_service_spec.rb +++ b/spec/services/packages/nuget/metadata_extraction_service_spec.rb @@ -10,10 +10,16 @@ RSpec.describe Packages::Nuget::MetadataExtractionService, feature_category: :pa describe '#execute' do subject { service.execute } + shared_examples 'raises an error' do |error_message| + it { expect { subject }.to raise_error(described_class::ExtractionError, error_message) } + end + context 'with valid package file id' do expected_metadata = { package_name: 'DummyProject.DummyPackage', package_version: '1.0.0', + authors: 'Test', + description: 'This is a dummy project', package_dependencies: [ { name: 'Newtonsoft.Json', @@ -72,15 +78,23 @@ RSpec.describe Packages::Nuget::MetadataExtractionService, feature_category: :pa allow(service).to receive(:nuspec_file_content).and_return(fixture_file(nuspec_filepath)) end - it { expect(subject[:license_url]).to eq('https://opensource.org/licenses/MIT') } - it { expect(subject[:project_url]).to eq('https://gitlab.com/gitlab-org/gitlab') } - it { expect(subject[:icon_url]).to eq('https://opensource.org/files/osi_keyhole_300X300_90ppi_0.png') } + it 'returns the correct metadata' do + expected_metadata = { + authors: 'Author Test', + description: 'Description Test', + license_url: 'https://opensource.org/licenses/MIT', + project_url: 'https://gitlab.com/gitlab-org/gitlab', + icon_url: 'https://opensource.org/files/osi_keyhole_300X300_90ppi_0.png' + } + + expect(subject.slice(*expected_metadata.keys)).to eq(expected_metadata) + end end context 'with invalid package file id' do let(:package_file) { double('file', id: 555) } - it { expect { subject }.to raise_error(::Packages::Nuget::MetadataExtractionService::ExtractionError, 'invalid package file') } + it_behaves_like 'raises an error', 'invalid package file' end context 'linked to a non nuget package' do @@ -88,7 +102,7 @@ RSpec.describe Packages::Nuget::MetadataExtractionService, feature_category: :pa package_file.package.maven! end - it { expect { subject }.to raise_error(::Packages::Nuget::MetadataExtractionService::ExtractionError, 'invalid package file') } + it_behaves_like 'raises an error', 'invalid package file' end context 'with a 0 byte package file id' do @@ -96,7 +110,7 @@ RSpec.describe Packages::Nuget::MetadataExtractionService, feature_category: :pa allow_any_instance_of(Packages::PackageFileUploader).to receive(:size).and_return(0) end - it { expect { subject }.to raise_error(::Packages::Nuget::MetadataExtractionService::ExtractionError, 'invalid package file') } + it_behaves_like 'raises an error', 'invalid package file' end context 'without the nuspec file' do @@ -104,7 +118,7 @@ RSpec.describe Packages::Nuget::MetadataExtractionService, feature_category: :pa allow_any_instance_of(Zip::File).to receive(:glob).and_return([]) end - it { expect { subject }.to raise_error(::Packages::Nuget::MetadataExtractionService::ExtractionError, 'nuspec file not found') } + it_behaves_like 'raises an error', 'nuspec file not found' end context 'with a too big nuspec file' do @@ -112,18 +126,17 @@ RSpec.describe Packages::Nuget::MetadataExtractionService, feature_category: :pa allow_any_instance_of(Zip::File).to receive(:glob).and_return([double('file', size: 6.megabytes)]) end - it { expect { subject }.to raise_error(::Packages::Nuget::MetadataExtractionService::ExtractionError, 'nuspec file too big') } + it_behaves_like 'raises an error', 'nuspec file too big' end context 'with a corrupted nupkg file with a wrong entry size' do let(:nupkg_fixture_path) { expand_fixture_path('packages/nuget/corrupted_package.nupkg') } - let(:expected_error) { "nuspec file has the wrong entry size: entry 'DummyProject.DummyPackage.nuspec' should be 255B, but is larger when inflated." } before do allow(Zip::File).to receive(:new).and_return(Zip::File.new(nupkg_fixture_path, false, false)) end - it { expect { subject }.to raise_error(::Packages::Nuget::MetadataExtractionService::ExtractionError, expected_error) } + it_behaves_like 'raises an error', "nuspec file has the wrong entry size: entry 'DummyProject.DummyPackage.nuspec' should be 255B, but is larger when inflated." end end end diff --git a/spec/services/packages/nuget/sync_metadatum_service_spec.rb b/spec/services/packages/nuget/sync_metadatum_service_spec.rb index ae07f312fcc..bf352d134c0 100644 --- a/spec/services/packages/nuget/sync_metadatum_service_spec.rb +++ b/spec/services/packages/nuget/sync_metadatum_service_spec.rb @@ -6,6 +6,8 @@ RSpec.describe Packages::Nuget::SyncMetadatumService, feature_category: :package let_it_be(:package, reload: true) { create(:nuget_package) } let_it_be(:metadata) do { + authors: 'Package authors', + description: 'Package description', project_url: 'https://test.org/test', license_url: 'https://test.org/MIT', icon_url: 'https://test.org/icon.png' @@ -53,5 +55,39 @@ RSpec.describe Packages::Nuget::SyncMetadatumService, feature_category: :package end end end + + context 'with metadata containing only authors and description' do + let_it_be(:metadata) { { authors: 'Package authors 2', description: 'Package description 2' } } + + it 'updates the nuget metadatum' do + subject + + expect(nuget_metadatum.authors).to eq('Package authors 2') + expect(nuget_metadatum.description).to eq('Package description 2') + end + end + + context 'with too long metadata' do + let(:metadata) { super().merge(authors: 'a' * 260, description: 'a' * 4010) } + let(:max_authors_length) { ::Packages::Nuget::Metadatum::MAX_AUTHORS_LENGTH } + let(:max_description_length) { ::Packages::Nuget::Metadatum::MAX_DESCRIPTION_LENGTH } + + it 'truncates authors and description to the maximum length and logs its info' do + %i[authors description].each do |field| + expect(Gitlab::AppLogger).to receive(:info).with( + class: described_class.name, + package_id: package.id, + project_id: package.project_id, + message: "#{field.capitalize} is too long (maximum is #{send("max_#{field}_length")} characters)", + field => metadata[field] + ) + end + + subject + + expect(nuget_metadatum.authors.size).to eq(max_authors_length) + expect(nuget_metadatum.description.size).to eq(max_description_length) + end + 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 c35863030b0..fa7d994c13c 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 @@ -12,13 +12,15 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ let(:package_version) { '1.0.0' } let(:package_file_name) { 'dummyproject.dummypackage.1.0.0.nupkg' } - shared_examples 'raising an' do |error_class| + shared_examples 'raising an' do |error_class, with_message:| it "raises an #{error_class}" do - expect { subject }.to raise_error(error_class) + expect { subject }.to raise_error(error_class, with_message) end end describe '#execute' do + using RSpec::Parameterized::TableSyntax + subject { service.execute } shared_examples 'taking the lease' do @@ -38,7 +40,7 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ shared_examples 'not updating the package if the lease is taken' do context 'without obtaining the exclusive lease' do let(:lease_key) { "packages:nuget:update_package_from_metadata_service:package:#{package_id}" } - let(:metadata) { { package_name: package_name, package_version: package_version } } + let(:metadata) { { package_name: package_name, package_version: package_version, authors: 'author1, author2', description: 'test description' } } let(:package_from_package_file) { package_file.package } before do @@ -66,12 +68,12 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ context 'with no existing package' do let(:package_id) { package.id } - it 'updates package and package file', :aggregate_failures do + it 'updates package and package file and creates metadatum', :aggregate_failures do expect { subject } .to not_change { ::Packages::Package.count } .and change { Packages::Dependency.count }.by(1) .and change { Packages::DependencyLink.count }.by(1) - .and change { ::Packages::Nuget::Metadatum.count }.by(0) + .and change { ::Packages::Nuget::Metadatum.count }.by(1) expect(package.reload.name).to eq(package_name) expect(package.version).to eq(package_version) @@ -98,7 +100,7 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ .and change { Packages::Dependency.count }.by(0) .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 { ::Packages::Nuget::Metadatum.count }.by(1) expect(package_file.reload.file_name).to eq(package_file_name) expect(package_file.package).to eq(existing_package) end @@ -117,7 +119,7 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ .to not_change { ::Packages::Package.count } .and change { Packages::Dependency.count }.by(1) .and change { Packages::DependencyLink.count }.by(1) - .and change { ::Packages::Nuget::Metadatum.count }.by(0) + .and change { ::Packages::Nuget::Metadatum.count }.by(1) end end end @@ -158,6 +160,8 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ .and change { ::Packages::Nuget::Metadatum.count }.by(1) metadatum = package_file.reload.package.nuget_metadatum + expect(metadatum.authors).to eq('Author Test') + expect(metadatum.description).to eq('Description Test') expect(metadatum.license_url).to eq('https://opensource.org/licenses/MIT') expect(metadatum.project_url).to eq('https://gitlab.com/gitlab-org/gitlab') expect(metadatum.icon_url).to eq('https://opensource.org/files/osi_keyhole_300X300_90ppi_0.png') @@ -166,13 +170,25 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ context 'with too long url' do let_it_be(:too_long_url) { "http://localhost/#{'bananas' * 50}" } - let(:metadata) { { package_name: package_name, package_version: package_version, license_url: too_long_url } } + let(:metadata) { { package_name: package_name, package_version: package_version, authors: 'Author Test', description: 'Description Test', license_url: too_long_url } } before do allow(service).to receive(:metadata).and_return(metadata) end - it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError + it_behaves_like 'raising an', described_class::InvalidMetadataError, with_message: /Validation failed: License url is too long/ + end + + context 'without authors or description' do + %i[authors description].each do |property| + let(:metadata) { { package_name: package_name, package_version: package_version, property => nil } } + + 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 + end end end @@ -212,7 +228,7 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ end end - it_behaves_like 'raising an', ::Packages::Nuget::MetadataExtractionService::ExtractionError + it_behaves_like 'raising an', ::Packages::Nuget::MetadataExtractionService::ExtractionError, with_message: 'nuspec file not found' end context 'with a symbol package' do @@ -222,7 +238,7 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ context 'with no existing package' do let(:package_id) { package.id } - it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError + it_behaves_like 'raising an', described_class::InvalidMetadataError, with_message: described_class::MISSING_MATCHING_PACKAGE_ERROR_MESSAGE end context 'with existing package' do @@ -251,41 +267,41 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ end context 'with an invalid package name' do - invalid_names = [ - '', - 'My/package', - '../../../my_package', - '%2e%2e%2fmy_package' - ] - - invalid_names.each do |invalid_name| - context "with #{invalid_name}" do - before do - allow(service).to receive(:package_name).and_return(invalid_name) - end + invalid_name_error_msg = 'Validation failed: Name is invalid' + + where(:invalid_name, :error_message) do + '' | described_class::INVALID_METADATA_ERROR_MESSAGE + 'My/package' | invalid_name_error_msg + '../../../my_package' | invalid_name_error_msg + '%2e%2e%2fmy_package' | invalid_name_error_msg + end - it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError + with_them do + before do + allow(service).to receive(:package_name).and_return(invalid_name) end + + it_behaves_like 'raising an', described_class::InvalidMetadataError, with_message: params[:error_message] end end context 'with an invalid package version' do - invalid_versions = [ - '', - '555', - '1./2.3', - '../../../../../1.2.3', - '%2e%2e%2f1.2.3' - ] - - invalid_versions.each do |invalid_version| - context "with #{invalid_version}" do - before do - allow(service).to receive(:package_version).and_return(invalid_version) - end + invalid_version_error_msg = 'Validation failed: Version is invalid' + + where(:invalid_version, :error_message) do + '' | described_class::INVALID_METADATA_ERROR_MESSAGE + '555' | invalid_version_error_msg + '1./2.3' | invalid_version_error_msg + '../../../../../1.2.3' | invalid_version_error_msg + '%2e%2e%2f1.2.3' | invalid_version_error_msg + end - it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError + with_them do + before do + allow(service).to receive(:package_version).and_return(invalid_version) end + + it_behaves_like 'raising an', described_class::InvalidMetadataError, with_message: params[:error_message] end end end diff --git a/spec/services/pages/delete_service_spec.rb b/spec/services/pages/delete_service_spec.rb index 590378af22b..488f29f6b7e 100644 --- a/spec/services/pages/delete_service_spec.rb +++ b/spec/services/pages/delete_service_spec.rb @@ -25,7 +25,7 @@ RSpec.describe Pages::DeleteService, feature_category: :pages do service.execute expect(PagesDomain.find_by_id(domain.id)).to eq(nil) - expect(PagesDomain.find_by_id(unrelated_domain.id)).to be + expect(PagesDomain.find_by_id(unrelated_domain.id)).to be_present end it 'schedules a destruction of pages deployments' do diff --git a/spec/services/pages/migrate_from_legacy_storage_service_spec.rb b/spec/services/pages/migrate_from_legacy_storage_service_spec.rb index 4348ce4a271..48690a035f5 100644 --- a/spec/services/pages/migrate_from_legacy_storage_service_spec.rb +++ b/spec/services/pages/migrate_from_legacy_storage_service_spec.rb @@ -58,7 +58,7 @@ RSpec.describe Pages::MigrateFromLegacyStorageService, feature_category: :pages expect(project.pages_metadatum.reload.pages_deployment).to eq(nil) expect(subject).to eq(migrated: 1, errored: 0) - expect(project.pages_metadatum.reload.pages_deployment).to be + expect(project.pages_metadatum.reload.pages_deployment).to be_present end context 'when deployed already exists for the project' do diff --git a/spec/services/personal_access_tokens/create_service_spec.rb b/spec/services/personal_access_tokens/create_service_spec.rb index d80be5cccce..621211bc883 100644 --- a/spec/services/personal_access_tokens/create_service_spec.rb +++ b/spec/services/personal_access_tokens/create_service_spec.rb @@ -67,6 +67,13 @@ RSpec.describe PersonalAccessTokens::CreateService, feature_category: :system_ac end end + context 'with no expires_at set', :freeze_time do + let(:params) { { name: 'Test token', impersonation: false, scopes: [:no_valid] } } + let(:service) { described_class.new(current_user: user, target_user: user, params: params) } + + it { expect(subject.payload[:personal_access_token].expires_at).to eq PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS.days.from_now.to_date } + end + context 'when invalid scope' do let(:params) { { name: 'Test token', impersonation: false, scopes: [:no_valid], expires_at: Date.today + 1.month } } diff --git a/spec/services/personal_access_tokens/last_used_service_spec.rb b/spec/services/personal_access_tokens/last_used_service_spec.rb index 20eabc20338..77ea5e10379 100644 --- a/spec/services/personal_access_tokens/last_used_service_spec.rb +++ b/spec/services/personal_access_tokens/last_used_service_spec.rb @@ -6,8 +6,8 @@ RSpec.describe PersonalAccessTokens::LastUsedService, feature_category: :system_ describe '#execute' do subject { described_class.new(personal_access_token).execute } - context 'when the personal access token has not been used recently' do - let_it_be(:personal_access_token) { create(:personal_access_token, last_used_at: 1.year.ago) } + context 'when the personal access token was used 10 minutes ago', :freeze_time do + let(:personal_access_token) { create(:personal_access_token, last_used_at: 10.minutes.ago) } it 'updates the last_used_at timestamp' do expect { subject }.to change { personal_access_token.last_used_at } @@ -20,8 +20,8 @@ RSpec.describe PersonalAccessTokens::LastUsedService, feature_category: :system_ end end - context 'when the personal access token has been used recently' do - let_it_be(:personal_access_token) { create(:personal_access_token, last_used_at: 1.minute.ago) } + context 'when the personal access token was used less than 10 minutes ago', :freeze_time do + let(:personal_access_token) { create(:personal_access_token, last_used_at: (10.minutes - 1.second).ago) } it 'does not update the last_used_at timestamp' do expect { subject }.not_to change { personal_access_token.last_used_at } @@ -43,5 +43,49 @@ RSpec.describe PersonalAccessTokens::LastUsedService, feature_category: :system_ expect(subject).to be_nil end end + + context 'when update_personal_access_token_usage_information_every_10_minutes is disabled' do + before do + stub_feature_flags(update_personal_access_token_usage_information_every_10_minutes: false) + end + + context 'when the personal access token was used 1 day ago', :freeze_time do + let(:personal_access_token) { create(:personal_access_token, last_used_at: 1.day.ago) } + + it 'updates the last_used_at timestamp' do + expect { subject }.to change { personal_access_token.last_used_at } + end + + it 'does not run on read-only GitLab instances' do + allow(::Gitlab::Database).to receive(:read_only?).and_return(true) + + expect { subject }.not_to change { personal_access_token.last_used_at } + end + end + + context 'when the personal access token was used less than 1 day ago', :freeze_time do + let(:personal_access_token) { create(:personal_access_token, last_used_at: (1.day - 1.second).ago) } + + it 'does not update the last_used_at timestamp' do + expect { subject }.not_to change { personal_access_token.last_used_at } + end + end + + context 'when the last_used_at timestamp is nil' do + let_it_be(:personal_access_token) { create(:personal_access_token, last_used_at: nil) } + + it 'updates the last_used_at timestamp' do + expect { subject }.to change { personal_access_token.last_used_at } + end + end + + context 'when not a personal access token' do + let_it_be(:personal_access_token) { create(:oauth_access_token) } + + it 'does not execute' do + expect(subject).to be_nil + end + end + end end end diff --git a/spec/services/post_receive_service_spec.rb b/spec/services/post_receive_service_spec.rb index 13bd103003f..20d86f74f86 100644 --- a/spec/services/post_receive_service_spec.rb +++ b/spec/services/post_receive_service_spec.rb @@ -214,11 +214,17 @@ RSpec.describe PostReceiveService, feature_category: :team_planning do end context 'broadcast message banner exists' do - it 'outputs a broadcast message' do - broadcast_message = create(:broadcast_message) + it 'outputs a broadcast message when show_in_cli is true' do + broadcast_message = create(:broadcast_message, show_in_cli: true) expect(subject).to include(build_alert_message(broadcast_message.message)) end + + it 'does not output a broadcast message when show_in_cli is false' do + create(:broadcast_message, show_in_cli: false) + + expect(has_alert_messages?(subject)).to be_falsey + end end context 'broadcast message notification exists' do diff --git a/spec/services/projects/after_rename_service_spec.rb b/spec/services/projects/after_rename_service_spec.rb index 3097d6d1498..411ff5662d4 100644 --- a/spec/services/projects/after_rename_service_spec.rb +++ b/spec/services/projects/after_rename_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::AfterRenameService, feature_category: :projects do +RSpec.describe Projects::AfterRenameService, feature_category: :groups_and_projects do let(:legacy_storage) { Storage::LegacyProject.new(project) } let(:hashed_storage) { Storage::Hashed.new(project) } let!(:path_before_rename) { project.path } diff --git a/spec/services/projects/alerting/notify_service_spec.rb b/spec/services/projects/alerting/notify_service_spec.rb index 8cd9b5d3e00..bbe69e4102f 100644 --- a/spec/services/projects/alerting/notify_service_spec.rb +++ b/spec/services/projects/alerting/notify_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::Alerting::NotifyService, feature_category: :projects do +RSpec.describe Projects::Alerting::NotifyService, feature_category: :groups_and_projects do let_it_be_with_reload(:project) { create(:project) } let(:payload) { ActionController::Parameters.new(payload_raw).permit! } diff --git a/spec/services/projects/all_issues_count_service_spec.rb b/spec/services/projects/all_issues_count_service_spec.rb index e8e08a25c45..0118f0d5e8b 100644 --- a/spec/services/projects/all_issues_count_service_spec.rb +++ b/spec/services/projects/all_issues_count_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::AllIssuesCountService, :use_clean_rails_memory_store_caching, feature_category: :projects do +RSpec.describe Projects::AllIssuesCountService, :use_clean_rails_memory_store_caching, feature_category: :groups_and_projects do let_it_be(:group) { create(:group, :public) } let_it_be(:project) { create(:project, :public, namespace: group) } let_it_be(:banned_user) { create(:user, :banned) } diff --git a/spec/services/projects/all_merge_requests_count_service_spec.rb b/spec/services/projects/all_merge_requests_count_service_spec.rb index ca10fbc00ad..7f4465fd8e7 100644 --- a/spec/services/projects/all_merge_requests_count_service_spec.rb +++ b/spec/services/projects/all_merge_requests_count_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::AllMergeRequestsCountService, :use_clean_rails_memory_store_caching, feature_category: :projects do +RSpec.describe Projects::AllMergeRequestsCountService, :use_clean_rails_memory_store_caching, feature_category: :groups_and_projects do let_it_be(:project) { create(:project) } subject { described_class.new(project) } diff --git a/spec/services/projects/apple_target_platform_detector_service_spec.rb b/spec/services/projects/apple_target_platform_detector_service_spec.rb index 787faaa0f79..74a04da9e68 100644 --- a/spec/services/projects/apple_target_platform_detector_service_spec.rb +++ b/spec/services/projects/apple_target_platform_detector_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::AppleTargetPlatformDetectorService, feature_category: :projects do +RSpec.describe Projects::AppleTargetPlatformDetectorService, feature_category: :groups_and_projects do let_it_be(:project) { build(:project) } subject { described_class.new(project).execute } diff --git a/spec/services/projects/autocomplete_service_spec.rb b/spec/services/projects/autocomplete_service_spec.rb index 9d3075874a2..67e715142f8 100644 --- a/spec/services/projects/autocomplete_service_spec.rb +++ b/spec/services/projects/autocomplete_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::AutocompleteService, feature_category: :projects do +RSpec.describe Projects::AutocompleteService, feature_category: :groups_and_projects do describe '#issues' do describe 'confidential issues' do let(:author) { create(:user) } diff --git a/spec/services/projects/batch_open_issues_count_service_spec.rb b/spec/services/projects/batch_open_issues_count_service_spec.rb index d29115a697f..578c3d066e1 100644 --- a/spec/services/projects/batch_open_issues_count_service_spec.rb +++ b/spec/services/projects/batch_open_issues_count_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::BatchOpenIssuesCountService, feature_category: :projects do +RSpec.describe Projects::BatchOpenIssuesCountService, feature_category: :groups_and_projects do let!(:project_1) { create(:project) } let!(:project_2) { create(:project) } 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 c4e6c7f4a11..ecabaa28119 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 @@ -55,7 +55,7 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService, feature context 'with timeout' do context 'set to a valid value' do before do - allow(Time.zone).to receive(:now).and_return(10, 15, 25) # third call to Time.zone.now will be triggering the timeout + allow(service).to receive(:timeout?).and_return(false, true) stub_delete_reference_requests('A' => 200) end diff --git a/spec/services/projects/container_repository/third_party/cleanup_tags_service_spec.rb b/spec/services/projects/container_repository/third_party/cleanup_tags_service_spec.rb index 836e722eb99..78343490e3a 100644 --- a/spec/services/projects/container_repository/third_party/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/third_party/cleanup_tags_service_spec.rb @@ -325,9 +325,13 @@ RSpec.describe Projects::ContainerRepository::ThirdParty::CleanupTagsService, :c Gitlab::Redis::Cache.with do |redis| expect(redis).to receive(:pipelined).and_call_original - expect_next_instance_of(Redis::PipelinedConnection) do |pipeline| + times = Gitlab::Redis::ClusterUtil.cluster?(redis) ? 2 : 1 + + # Set 2 instances as redis is a MultiStore. + # Redis Cluster uses only 1 pipeline as the keys have hash-tags + expect_next_instances_of(Redis::PipelinedConnection, times) do |pipeline| selected_tags.each do |tag_name, created_at, ex| - expect(pipeline).to receive(:set).with(cache_key(tag_name), rfc3339(created_at), ex: ex) + expect(pipeline).to receive(:set).with(cache_key(tag_name), rfc3339(created_at), ex: ex).and_call_original end end end @@ -372,7 +376,11 @@ RSpec.describe Projects::ContainerRepository::ThirdParty::CleanupTagsService, :c expect(redis).to receive(:mget).and_call_original expect(redis).to receive(:pipelined).and_call_original - expect_next_instance_of(Redis::PipelinedConnection) do |pipeline| + times = Gitlab::Redis::ClusterUtil.cluster?(redis) ? 2 : 1 + + # Set 2 instances as redis is a MultiStore + # Redis Cluster uses only 1 pipeline as the keys have hash-tags + expect_next_instances_of(Redis::PipelinedConnection, times) do |pipeline| expect(pipeline).to receive(:set).and_call_original end end diff --git a/spec/services/projects/count_service_spec.rb b/spec/services/projects/count_service_spec.rb index 71940fa396e..8797f30bed2 100644 --- a/spec/services/projects/count_service_spec.rb +++ b/spec/services/projects/count_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::CountService, feature_category: :projects do +RSpec.describe Projects::CountService, feature_category: :groups_and_projects do let(:project) { build(:project, id: 1) } let(:service) { described_class.new(project) } diff --git a/spec/services/projects/create_from_template_service_spec.rb b/spec/services/projects/create_from_template_service_spec.rb index a3fdb258f75..0d649dee022 100644 --- a/spec/services/projects/create_from_template_service_spec.rb +++ b/spec/services/projects/create_from_template_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::CreateFromTemplateService, feature_category: :projects do +RSpec.describe Projects::CreateFromTemplateService, feature_category: :groups_and_projects do let(:user) { create(:user) } let(:template_name) { 'rails' } let(:project_params) do diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 303a98cb35b..59db0b47a3c 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::CreateService, '#execute', feature_category: :projects do +RSpec.describe Projects::CreateService, '#execute', feature_category: :groups_and_projects do include ExternalAuthorizationServiceHelpers let(:user) { create :user } @@ -1117,4 +1117,45 @@ RSpec.describe Projects::CreateService, '#execute', feature_category: :projects end end end + + context 'when using access_level params' do + def expect_not_disabled_features(project, exclude: []) + ProjectFeature::FEATURES.excluding(exclude) + .excluding(project.project_feature.send(:feature_validation_exclusion)) + .each do |feature| + expect(project.project_feature.public_send(ProjectFeature.access_level_attribute(feature))).not_to eq(Featurable::DISABLED) + end + end + + # repository is tested on its own below because it requires other features to be set as well + # package_registry has different behaviour and is modified from the model based on other attributes + ProjectFeature::FEATURES.excluding(:repository, :package_registry).each do |feature| + it "when using #{feature}", :aggregate_failures do + feature_attribute = ProjectFeature.access_level_attribute(feature) + opts[feature_attribute] = ProjectFeature.str_from_access_level(Featurable::DISABLED) + project = create_project(user, opts) + + expect(project).to be_valid + expect(project.project_feature.public_send(feature_attribute)).to eq(Featurable::DISABLED) + + expect_not_disabled_features(project, exclude: [feature]) + end + end + + it 'when using repository', :aggregate_failures do + # model validation will fail if builds or merge_requests have higher visibility than repository + disabled = ProjectFeature.str_from_access_level(Featurable::DISABLED) + opts[:repository_access_level] = disabled + opts[:builds_access_level] = disabled + opts[:merge_requests_access_level] = disabled + project = create_project(user, opts) + + expect(project).to be_valid + expect(project.project_feature.repository_access_level).to eq(Featurable::DISABLED) + expect(project.project_feature.builds_access_level).to eq(Featurable::DISABLED) + expect(project.project_feature.merge_requests_access_level).to eq(Featurable::DISABLED) + + expect_not_disabled_features(project, exclude: [:repository, :builds, :merge_requests]) + end + end end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 665f930a0a8..7aa6980fb24 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publisher, feature_category: :projects do +RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publisher, feature_category: :groups_and_projects do include ProjectForksHelper include BatchDestroyDependentAssociationsHelper @@ -113,7 +113,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi destroy_project(project, user, {}) expect(project.reload.delete_error).to be_present - expect(project.delete_error).to include(error_message) + expect(project.delete_error).to match(error_message) end end @@ -287,7 +287,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi .to receive(:remove_legacy_registry_tags).and_return(false) end - it_behaves_like 'handles errors thrown during async destroy', "Failed to remove some tags" + it_behaves_like 'handles errors thrown during async destroy', /Failed to remove some tags/ end context 'when `remove_repository` fails' do @@ -296,7 +296,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi .to receive(:remove_repository).and_return(false) end - it_behaves_like 'handles errors thrown during async destroy', "Failed to remove project repository" + it_behaves_like 'handles errors thrown during async destroy', /Failed to remove/ end context 'when `execute` raises expected error' do @@ -305,7 +305,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi .to receive(:destroy!).and_raise(StandardError.new("Other error message")) end - it_behaves_like 'handles errors thrown during async destroy', "Other error message" + it_behaves_like 'handles errors thrown during async destroy', /Other error message/ end context 'when `execute` raises unexpected error' do @@ -456,6 +456,8 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi end context 'repository removal' do + # 1. Project repository + # 2. Wiki repository it 'removal of existing repos' do expect_next_instances_of(Repositories::DestroyService, 2) do |instance| expect(instance).to receive(:execute).and_return(status: :success) @@ -529,7 +531,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi end end - it_behaves_like 'handles errors thrown during async destroy', "Failed to remove webhooks" + it_behaves_like 'handles errors thrown during async destroy', /Failed to remove webhooks/ end end diff --git a/spec/services/projects/detect_repository_languages_service_spec.rb b/spec/services/projects/detect_repository_languages_service_spec.rb index 5759f8128d0..29d5569ba76 100644 --- a/spec/services/projects/detect_repository_languages_service_spec.rb +++ b/spec/services/projects/detect_repository_languages_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::DetectRepositoryLanguagesService, :clean_gitlab_redis_shared_state, feature_category: :projects do +RSpec.describe Projects::DetectRepositoryLanguagesService, :clean_gitlab_redis_shared_state, feature_category: :groups_and_projects do let_it_be(:project, reload: true) { create(:project, :repository) } subject { described_class.new(project) } diff --git a/spec/services/projects/download_service_spec.rb b/spec/services/projects/download_service_spec.rb index 52bdbefe01a..e062ee04bf4 100644 --- a/spec/services/projects/download_service_spec.rb +++ b/spec/services/projects/download_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::DownloadService, feature_category: :projects do +RSpec.describe Projects::DownloadService, feature_category: :groups_and_projects do describe 'File service' do before do @user = create(:user) diff --git a/spec/services/projects/fetch_statistics_increment_service_spec.rb b/spec/services/projects/fetch_statistics_increment_service_spec.rb index 9e24e68fa98..5ad91e142a0 100644 --- a/spec/services/projects/fetch_statistics_increment_service_spec.rb +++ b/spec/services/projects/fetch_statistics_increment_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' module Projects - RSpec.describe FetchStatisticsIncrementService, feature_category: :projects do + RSpec.describe FetchStatisticsIncrementService, feature_category: :groups_and_projects do let(:project) { create(:project) } describe '#execute' do diff --git a/spec/services/projects/group_links/create_service_spec.rb b/spec/services/projects/group_links/create_service_spec.rb index 4f2f480cf1c..ca2902af472 100644 --- a/spec/services/projects/group_links/create_service_spec.rb +++ b/spec/services/projects/group_links/create_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::GroupLinks::CreateService, '#execute', feature_category: :subgroups do +RSpec.describe Projects::GroupLinks::CreateService, '#execute', feature_category: :groups_and_projects do let_it_be(:user) { create :user } let_it_be(:group) { create :group } let_it_be(:project) { create(:project, namespace: create(:namespace, :with_namespace_settings)) } diff --git a/spec/services/projects/group_links/destroy_service_spec.rb b/spec/services/projects/group_links/destroy_service_spec.rb index 76bdd536a0d..103aff8c659 100644 --- a/spec/services/projects/group_links/destroy_service_spec.rb +++ b/spec/services/projects/group_links/destroy_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::GroupLinks::DestroyService, '#execute', feature_category: :subgroups do +RSpec.describe Projects::GroupLinks::DestroyService, '#execute', feature_category: :groups_and_projects do let_it_be(:user) { create :user } let_it_be(:project) { create(:project, :private) } let_it_be(:group) { create(:group) } diff --git a/spec/services/projects/group_links/update_service_spec.rb b/spec/services/projects/group_links/update_service_spec.rb index 4232412cf54..f7607deef04 100644 --- a/spec/services/projects/group_links/update_service_spec.rb +++ b/spec/services/projects/group_links/update_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::GroupLinks::UpdateService, '#execute', feature_category: :subgroups do +RSpec.describe Projects::GroupLinks::UpdateService, '#execute', feature_category: :groups_and_projects do let_it_be(:user) { create :user } let_it_be(:group) { create :group } let_it_be(:project) { create :project } diff --git a/spec/services/projects/hashed_storage/base_attachment_service_spec.rb b/spec/services/projects/hashed_storage/base_attachment_service_spec.rb index 01036fc2d9c..e32747ad907 100644 --- a/spec/services/projects/hashed_storage/base_attachment_service_spec.rb +++ b/spec/services/projects/hashed_storage/base_attachment_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::HashedStorage::BaseAttachmentService, feature_category: :projects do +RSpec.describe Projects::HashedStorage::BaseAttachmentService, feature_category: :groups_and_projects do let(:project) { create(:project, :repository, storage_version: 0, skip_disk_validation: true) } subject(:service) { described_class.new(project: project, old_disk_path: project.full_path, logger: nil) } diff --git a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb index 39263506bca..6a87b2fafb9 100644 --- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::HashedStorage::MigrateAttachmentsService, feature_category: :projects do +RSpec.describe Projects::HashedStorage::MigrateAttachmentsService, feature_category: :groups_and_projects do subject(:service) { described_class.new(project: project, old_disk_path: project.full_path, logger: nil) } let(:project) { create(:project, :repository, storage_version: 1, skip_disk_validation: true) } diff --git a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb index bcc914e72b5..e21d8b6fa83 100644 --- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::HashedStorage::MigrateRepositoryService, feature_category: :projects do +RSpec.describe Projects::HashedStorage::MigrateRepositoryService, feature_category: :groups_and_projects do let(:gitlab_shell) { Gitlab::Shell.new } let(:project) { create(:project, :legacy_storage, :repository, :wiki_repo, :design_repo) } let(:legacy_storage) { Storage::LegacyProject.new(project) } diff --git a/spec/services/projects/hashed_storage/migration_service_spec.rb b/spec/services/projects/hashed_storage/migration_service_spec.rb index 89bc55dbaf6..ffbd5c2500a 100644 --- a/spec/services/projects/hashed_storage/migration_service_spec.rb +++ b/spec/services/projects/hashed_storage/migration_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::HashedStorage::MigrationService, feature_category: :projects do +RSpec.describe Projects::HashedStorage::MigrationService, feature_category: :groups_and_projects do let(:project) { create(:project, :empty_repo, :wiki_repo, :legacy_storage) } let(:logger) { double } let!(:project_attachment) { build(:file_uploader, project: project) } diff --git a/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb b/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb index 95491d63df2..d1a68503fa3 100644 --- a/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb +++ b/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::HashedStorage::RollbackAttachmentsService, feature_category: :projects do +RSpec.describe Projects::HashedStorage::RollbackAttachmentsService, feature_category: :groups_and_projects do subject(:service) { described_class.new(project: project, old_disk_path: project.disk_path, logger: nil) } let(:project) { create(:project, :repository, skip_disk_validation: true) } diff --git a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb index 19f1856e39a..1e5d4ae4d20 100644 --- a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis_shared_state, feature_category: :projects do +RSpec.describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis_shared_state, feature_category: :groups_and_projects do let(:gitlab_shell) { Gitlab::Shell.new } let(:project) { create(:project, :repository, :wiki_repo, :design_repo, storage_version: ::Project::HASHED_STORAGE_FEATURES[:repository]) } let(:legacy_storage) { Storage::LegacyProject.new(project) } diff --git a/spec/services/projects/hashed_storage/rollback_service_spec.rb b/spec/services/projects/hashed_storage/rollback_service_spec.rb index 6d047f856ec..088eb9d2734 100644 --- a/spec/services/projects/hashed_storage/rollback_service_spec.rb +++ b/spec/services/projects/hashed_storage/rollback_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::HashedStorage::RollbackService, feature_category: :projects do +RSpec.describe Projects::HashedStorage::RollbackService, feature_category: :groups_and_projects do let(:project) { create(:project, :empty_repo, :wiki_repo) } let(:logger) { double } let!(:project_attachment) { build(:file_uploader, project: project) } diff --git a/spec/services/projects/lfs_pointers/lfs_import_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_import_service_spec.rb index f1e4db55962..363f871bb9d 100644 --- a/spec/services/projects/lfs_pointers/lfs_import_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_import_service_spec.rb @@ -59,6 +59,20 @@ RSpec.describe Projects::LfsPointers::LfsImportService, feature_category: :sourc expect(result[:message]).to eq error_message end end + + context 'when an GRPC::Core::CallError exception raised' do + it 'returns error' do + error_message = "error message" + expect_next_instance_of(Projects::LfsPointers::LfsObjectDownloadListService) do |instance| + expect(instance).to receive(:each_list_item).and_raise(GRPC::Core::CallError, error_message) + end + + result = subject.execute + + expect(result[:status]).to eq :error + expect(result[:message]).to eq error_message + end + end end context 'when lfs is not enabled for the project' do diff --git a/spec/services/projects/move_access_service_spec.rb b/spec/services/projects/move_access_service_spec.rb index b9244002f6c..a6407ddc849 100644 --- a/spec/services/projects/move_access_service_spec.rb +++ b/spec/services/projects/move_access_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::MoveAccessService, feature_category: :projects do +RSpec.describe Projects::MoveAccessService, feature_category: :groups_and_projects do let(:user) { create(:user) } let(:group) { create(:group) } let(:project_with_access) { create(:project, namespace: user.namespace) } diff --git a/spec/services/projects/move_notification_settings_service_spec.rb b/spec/services/projects/move_notification_settings_service_spec.rb index 5ef6e8a0647..e9b523f6273 100644 --- a/spec/services/projects/move_notification_settings_service_spec.rb +++ b/spec/services/projects/move_notification_settings_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::MoveNotificationSettingsService, feature_category: :projects do +RSpec.describe Projects::MoveNotificationSettingsService, feature_category: :groups_and_projects do let(:user) { create(:user) } let(:project_with_notifications) { create(:project, namespace: user.namespace) } let(:target_project) { create(:project, namespace: user.namespace) } diff --git a/spec/services/projects/move_project_authorizations_service_spec.rb b/spec/services/projects/move_project_authorizations_service_spec.rb index 6cd0b056325..c01a0b2c90e 100644 --- a/spec/services/projects/move_project_authorizations_service_spec.rb +++ b/spec/services/projects/move_project_authorizations_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::MoveProjectAuthorizationsService, feature_category: :projects do +RSpec.describe Projects::MoveProjectAuthorizationsService, feature_category: :groups_and_projects do let!(:user) { create(:user) } let(:project_with_users) { create(:project, namespace: user.namespace) } let(:target_project) { create(:project, namespace: user.namespace) } diff --git a/spec/services/projects/move_project_group_links_service_spec.rb b/spec/services/projects/move_project_group_links_service_spec.rb index cfd4b51b001..6d6a8b402de 100644 --- a/spec/services/projects/move_project_group_links_service_spec.rb +++ b/spec/services/projects/move_project_group_links_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::MoveProjectGroupLinksService, feature_category: :projects do +RSpec.describe Projects::MoveProjectGroupLinksService, feature_category: :groups_and_projects do let!(:user) { create(:user) } let(:project_with_groups) { create(:project, namespace: user.namespace) } let(:target_project) { create(:project, namespace: user.namespace) } diff --git a/spec/services/projects/move_project_members_service_spec.rb b/spec/services/projects/move_project_members_service_spec.rb index 364fb7faaf2..d8330863405 100644 --- a/spec/services/projects/move_project_members_service_spec.rb +++ b/spec/services/projects/move_project_members_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::MoveProjectMembersService, feature_category: :projects do +RSpec.describe Projects::MoveProjectMembersService, feature_category: :groups_and_projects do let!(:user) { create(:user) } let(:project_with_users) { create(:project, namespace: user.namespace) } let(:target_project) { create(:project, namespace: user.namespace) } diff --git a/spec/services/projects/move_users_star_projects_service_spec.rb b/spec/services/projects/move_users_star_projects_service_spec.rb index b99e51d954b..6a01896bd58 100644 --- a/spec/services/projects/move_users_star_projects_service_spec.rb +++ b/spec/services/projects/move_users_star_projects_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::MoveUsersStarProjectsService, feature_category: :projects do +RSpec.describe Projects::MoveUsersStarProjectsService, feature_category: :groups_and_projects do let!(:user) { create(:user) } let!(:project_with_stars) { create(:project, namespace: user.namespace) } let!(:target_project) { create(:project, namespace: user.namespace) } diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb index 7babaf4d0d8..5f9b1a59bf9 100644 --- a/spec/services/projects/operations/update_service_spec.rb +++ b/spec/services/projects/operations/update_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::Operations::UpdateService, feature_category: :projects do +RSpec.describe Projects::Operations::UpdateService, feature_category: :groups_and_projects do let_it_be_with_refind(:project) { create(:project) } let_it_be(:user) { create(:user) } @@ -92,61 +92,6 @@ RSpec.describe Projects::Operations::UpdateService, feature_category: :projects end end - context 'metrics dashboard setting' do - let(:params) do - { - metrics_setting_attributes: { - external_dashboard_url: 'http://gitlab.com', - dashboard_timezone: 'utc' - } - } - end - - context 'without existing metrics dashboard setting' do - it 'creates a setting' do - expect(result[:status]).to eq(:success) - - expect(project.reload.metrics_setting.external_dashboard_url).to eq( - 'http://gitlab.com' - ) - expect(project.metrics_setting.dashboard_timezone).to eq('utc') - end - end - - context 'with existing metrics dashboard setting' do - before do - create(:project_metrics_setting, project: project) - end - - it 'updates the settings' do - expect(result[:status]).to eq(:success) - - expect(project.reload.metrics_setting.external_dashboard_url).to eq( - 'http://gitlab.com' - ) - expect(project.metrics_setting.dashboard_timezone).to eq('utc') - end - end - - context 'with blank external_dashboard_url' do - let(:params) do - { - metrics_setting_attributes: { - external_dashboard_url: '', - dashboard_timezone: 'utc' - } - } - end - - it 'updates dashboard_timezone' do - expect(result[:status]).to eq(:success) - - expect(project.reload.metrics_setting.external_dashboard_url).to be(nil) - expect(project.metrics_setting.dashboard_timezone).to eq('utc') - end - end - end - context 'error tracking' do context 'with existing error tracking setting' do let(:params) do @@ -354,62 +299,6 @@ RSpec.describe Projects::Operations::UpdateService, feature_category: :projects end end - context 'grafana integration' do - let(:params) do - { - grafana_integration_attributes: { - grafana_url: 'http://new.grafana.com', - token: 'VerySecureToken=' - } - } - end - - context 'without existing grafana integration' do - it 'creates an integration' do - expect(result[:status]).to eq(:success) - - expected_attrs = params[:grafana_integration_attributes] - integration = project.reload.grafana_integration - - expect(integration.grafana_url).to eq(expected_attrs[:grafana_url]) - expect(integration.send(:token)).to eq(expected_attrs[:token]) - end - end - - context 'with an existing grafana integration' do - before do - create(:grafana_integration, project: project) - end - - it 'updates the settings' do - expect(result[:status]).to eq(:success) - - expected_attrs = params[:grafana_integration_attributes] - integration = project.reload.grafana_integration - - expect(integration.grafana_url).to eq(expected_attrs[:grafana_url]) - expect(integration.send(:token)).to eq(expected_attrs[:token]) - end - - context 'with all grafana attributes blank in params' do - let(:params) do - { - grafana_integration_attributes: { - grafana_url: '', - token: '' - } - } - end - - it 'destroys the metrics_setting entry in DB' do - expect(result[:status]).to eq(:success) - - expect(project.reload.grafana_integration).to be_nil - end - end - end - end - context 'prometheus integration' do context 'prometheus params were passed into service' do let!(:prometheus_integration) do diff --git a/spec/services/projects/overwrite_project_service_spec.rb b/spec/services/projects/overwrite_project_service_spec.rb index b4faf45a1cb..99be630d6f6 100644 --- a/spec/services/projects/overwrite_project_service_spec.rb +++ b/spec/services/projects/overwrite_project_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::OverwriteProjectService, feature_category: :projects do +RSpec.describe Projects::OverwriteProjectService, feature_category: :groups_and_projects do include ProjectForksHelper let(:user) { create(:user) } diff --git a/spec/services/projects/participants_service_spec.rb b/spec/services/projects/participants_service_spec.rb index bd297343879..2f090577805 100644 --- a/spec/services/projects/participants_service_spec.rb +++ b/spec/services/projects/participants_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::ParticipantsService, feature_category: :projects do +RSpec.describe Projects::ParticipantsService, feature_category: :groups_and_projects do describe '#execute' do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project, :public) } @@ -179,6 +179,18 @@ RSpec.describe Projects::ParticipantsService, feature_category: :projects do end end + context 'when public project maintainer is signed in' do + let(:service) { described_class.new(public_project, public_project_maintainer) } + + it 'returns private group members' do + expect(usernames).to include(private_group_member.username) + end + + it 'returns members of the ancestral groups of the private group' do + expect(usernames).to include(group_ancestor_owner.username) + end + end + context 'when private group owner is signed in' do let(:service) { described_class.new(public_project, private_group_owner) } diff --git a/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/spec/services/projects/prometheus/alerts/notify_service_spec.rb index 0feac6c3e72..cc1f83ddc2b 100644 --- a/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::Prometheus::Alerts::NotifyService, feature_category: :metrics do +RSpec.describe Projects::Prometheus::Alerts::NotifyService, feature_category: :incident_management do include PrometheusHelpers using RSpec::Parameterized::TableSyntax @@ -163,6 +163,24 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService, feature_category: :m raise "invalid result: #{result.inspect}" end end + + context 'with simultaneous manual configuration' do + let_it_be(:integration) { create(:alert_management_prometheus_integration, :legacy, project: project) } + let_it_be(:old_prometheus_integration) { create(:prometheus_integration, project: project) } + let_it_be(:alerting_setting) { create(:project_alerting_setting, project: project, token: integration.token) } + + subject { service.execute(integration.token, integration) } + + it_behaves_like 'processes one firing and one resolved prometheus alerts' + + context 'when HTTP integration is inactive' do + before do + integration.update!(active: false) + end + + it_behaves_like 'alerts service responds with an error and takes no actions', :unauthorized + end + end end context 'incident settings' do @@ -206,12 +224,9 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService, feature_category: :m end context 'process Alert Management alerts' do - let(:process_service) { instance_double(AlertManagement::ProcessPrometheusAlertService) } + let(:integration) { build_stubbed(:alert_management_http_integration, project: project, token: token) } - before do - create(:prometheus_integration, project: project) - create(:project_alerting_setting, project: project, token: token) - end + subject { service.execute(token_input, integration) } context 'with multiple firing alerts and resolving alerts' do let(:payload_raw) do @@ -221,7 +236,7 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService, feature_category: :m it 'processes Prometheus alerts' do expect(AlertManagement::ProcessPrometheusAlertService) .to receive(:new) - .with(project, kind_of(Hash)) + .with(project, kind_of(Hash), integration: integration) .exactly(3).times .and_call_original diff --git a/spec/services/projects/readme_renderer_service_spec.rb b/spec/services/projects/readme_renderer_service_spec.rb index 842d75e82ee..cced9e52227 100644 --- a/spec/services/projects/readme_renderer_service_spec.rb +++ b/spec/services/projects/readme_renderer_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::ReadmeRendererService, '#execute', feature_category: :projects do +RSpec.describe Projects::ReadmeRendererService, '#execute', feature_category: :groups_and_projects do using RSpec::Parameterized::TableSyntax subject(:service) { described_class.new(project, nil, opts) } @@ -52,7 +52,7 @@ RSpec.describe Projects::ReadmeRendererService, '#execute', feature_category: :p context 'with path traversal in mind' do where(:template_name, :exception, :expected_path) do - '../path/traversal/bad' | [Gitlab::Utils::PathTraversalAttackError, 'Invalid path'] | nil + '../path/traversal/bad' | [Gitlab::PathTraversal::PathTraversalAttackError, 'Invalid path'] | nil '/bad/template' | [StandardError, 'path /bad/template.md.tt is not allowed'] | nil 'good/template' | nil | 'good/template.md.tt' end diff --git a/spec/services/projects/record_target_platforms_service_spec.rb b/spec/services/projects/record_target_platforms_service_spec.rb index 17aa7fd7009..7c6907c7a95 100644 --- a/spec/services/projects/record_target_platforms_service_spec.rb +++ b/spec/services/projects/record_target_platforms_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::RecordTargetPlatformsService, '#execute', feature_category: :projects do +RSpec.describe Projects::RecordTargetPlatformsService, '#execute', feature_category: :groups_and_projects do let_it_be(:project) { create(:project) } let(:detector_service) { Projects::AppleTargetPlatformDetectorService } diff --git a/spec/services/projects/slack_application_install_service_spec.rb b/spec/services/projects/slack_application_install_service_spec.rb new file mode 100644 index 00000000000..9502562a7d4 --- /dev/null +++ b/spec/services/projects/slack_application_install_service_spec.rb @@ -0,0 +1,143 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::SlackApplicationInstallService, feature_category: :integrations do + let_it_be(:user) { create(:user) } + let_it_be_with_refind(:project) { create(:project) } + + let(:integration) { project.gitlab_slack_application_integration } + let(:installation) { integration.slack_integration } + + let(:slack_app_id) { 'A12345' } + let(:slack_app_secret) { 'secret' } + let(:oauth_code) { 'code' } + let(:params) { { code: oauth_code } } + let(:exchange_url) { described_class::SLACK_EXCHANGE_TOKEN_URL } + let(:redirect_url) { Gitlab::Routing.url_helpers.slack_auth_project_settings_slack_url(project) } + + subject(:service) { described_class.new(project, user, params) } + + before do + stub_application_setting(slack_app_id: slack_app_id, slack_app_secret: slack_app_secret) + + query = { + client_id: slack_app_id, + client_secret: slack_app_secret, + code: oauth_code, + redirect_uri: redirect_url + } + + stub_request(:get, exchange_url) + .with(query: query) + .to_return(body: response.to_json, headers: { 'Content-Type' => 'application/json' }) + end + + context 'when Slack responds with an error' do + let(:response) do + { + ok: false, + error: 'something is wrong' + } + end + + it 'returns error result' do + result = service.execute + + expect(result).to eq(message: 'Slack: something is wrong', status: :error) + end + end + + context 'when Slack responds with an access token' do + let_it_be(:team_id) { 'T11111' } + let_it_be(:team_name) { 'Team name' } + let_it_be(:user_id) { 'U11111' } + let_it_be(:bot_user_id) { 'U99999' } + let_it_be(:bot_access_token) { 'token-XXXXX' } + + let(:response) do + { + ok: true, + app_id: 'A12345', + authed_user: { id: user_id }, + token_type: 'bot', + access_token: bot_access_token, + bot_user_id: bot_user_id, + team: { id: team_id, name: 'Team name' }, + enterprise: { is_enterprise_install: false }, + scope: 'chat:a,chat:b,chat:c' + } + end + + shared_examples 'success response' do + it 'returns success result and creates all needed records' do + result = service.execute + + expect(result).to eq(status: :success) + expect(integration).to be_present + expect(installation).to be_present + expect(installation).to have_attributes( + integration_id: integration.id, + team_id: team_id, + team_name: team_name, + alias: project.full_path, + user_id: user_id, + bot_user_id: bot_user_id, + bot_access_token: bot_access_token, + authorized_scope_names: contain_exactly('chat:a', 'chat:b', 'chat:c') + ) + end + end + + it_behaves_like 'success response' + + context 'when integration record already exists' do + before do + project.create_gitlab_slack_application_integration! + end + + it_behaves_like 'success response' + + context 'when installation record already exists' do + before do + integration.create_slack_integration!( + team_id: 'old value', + team_name: 'old value', + alias: 'old value', + user_id: 'old value', + bot_user_id: 'old value', + bot_access_token: 'old value' + ) + end + + it_behaves_like 'success response' + end + end + + context 'when the team has other Slack installation records' do + let_it_be_with_reload(:other_installation) { create(:slack_integration, team_id: team_id) } + let_it_be_with_reload(:other_legacy_installation) { create(:slack_integration, :legacy, team_id: team_id) } + let_it_be_with_reload(:legacy_installation_for_other_team) { create(:slack_integration, :legacy) } + + it_behaves_like 'success response' + + it 'updates related legacy records' do + travel_to(1.minute.from_now) do + expected_attributes = { + 'user_id' => user_id, + 'bot_user_id' => bot_user_id, + 'bot_access_token' => bot_access_token, + 'updated_at' => Time.current, + 'authorized_scope_names' => %w[chat:a chat:b chat:c] + } + + service.execute + + expect(other_installation).to have_attributes(expected_attributes) + expect(other_legacy_installation).to have_attributes(expected_attributes) + expect(legacy_installation_for_other_team).not_to have_attributes(expected_attributes) + end + end + end + end +end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 48d5935f22f..46fe7d7bbbe 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::TransferService, feature_category: :projects do +RSpec.describe Projects::TransferService, feature_category: :groups_and_projects do let_it_be(:group) { create(:group) } let_it_be(:user) { create(:user) } let_it_be(:group_integration) { create(:integrations_slack, :group, group: group, webhook: 'http://group.slack.com') } @@ -716,7 +716,7 @@ RSpec.describe Projects::TransferService, feature_category: :projects do end def clear_design_repo_memoization - project.design_management_repository.clear_memoization(:repository) + project&.design_management_repository&.clear_memoization(:repository) project.clear_memoization(:design_repository) end diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index a97369c4b08..a113f3506e1 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -95,7 +95,7 @@ RSpec.describe Projects::UpdatePagesService, feature_category: :pages do deployment = project.pages_deployments.last expect(deployment.size).to eq(file.size) - expect(deployment.file).to be + 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) diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 8f55ee705ab..badbc8b628e 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'spec_helper' -RSpec.describe Projects::UpdateService, feature_category: :projects do +RSpec.describe Projects::UpdateService, feature_category: :groups_and_projects do include ExternalAuthorizationServiceHelpers include ProjectForksHelper diff --git a/spec/services/projects/update_statistics_service_spec.rb b/spec/services/projects/update_statistics_service_spec.rb index f685b86acc0..762378c93ec 100644 --- a/spec/services/projects/update_statistics_service_spec.rb +++ b/spec/services/projects/update_statistics_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::UpdateStatisticsService, feature_category: :projects do +RSpec.describe Projects::UpdateStatisticsService, feature_category: :groups_and_projects do using RSpec::Parameterized::TableSyntax let(:service) { described_class.new(project, nil, statistics: statistics) } diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 966782aca98..bd09dae0a5a 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -2131,6 +2131,46 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning let(:user) { developer } end + context 'unlink command' do + let_it_be(:private_issue) { create(:issue, project: create(:project, :private)) } + let_it_be(:other_issue) { create(:issue, project: project) } + let(:content) { "/unlink #{other_issue.to_reference(issue)}" } + + subject(:unlink_issues) { service.execute(content, issue) } + + shared_examples 'command with failure' do + it 'does not destroy issues relation' do + expect { unlink_issues }.not_to change { IssueLink.count } + end + + it 'return correct execution message' do + expect(unlink_issues[2]).to eq('No linked issue matches the provided parameter.') + end + end + + context 'when command includes linked issue' do + let_it_be(:link1) { create(:issue_link, source: issue, target: other_issue) } + let_it_be(:link2) { create(:issue_link, source: issue, target: private_issue) } + + it 'executes command successfully' do + expect { unlink_issues }.to change { IssueLink.count }.by(-1) + expect(unlink_issues[2]).to eq("Removed link with #{other_issue.to_reference(issue)}.") + expect(issue.notes.last.note).to eq("removed the relation with #{other_issue.to_reference}") + expect(other_issue.notes.last.note).to eq("removed the relation with #{issue.to_reference}") + end + + context 'when user has no access' do + let(:content) { "/unlink #{private_issue.to_reference(issue)}" } + + it_behaves_like 'command with failure' + end + end + + context 'when provided issue is not linked' do + it_behaves_like 'command with failure' + end + end + context 'invite_email command' do let_it_be(:issuable) { issue } @@ -2377,54 +2417,7 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning end end - describe 'type command' do - let_it_be(:project) { create(:project, :private) } - let_it_be(:work_item) { create(:work_item, project: project) } - - let(:command) { '/type Task' } - - context 'when user has sufficient permissions to create new type' do - before do - allow(Ability).to receive(:allowed?).and_call_original - allow(Ability).to receive(:allowed?).with(current_user, :create_task, work_item).and_return(true) - end - - it 'populates :issue_type: and :work_item_type' do - _, updates, message = service.execute(command, work_item) - - expect(message).to eq(_('Type changed successfully.')) - expect(updates).to eq({ issue_type: 'task', work_item_type: WorkItems::Type.default_by_type(:task) }) - end - - it 'returns error with an invalid type' do - _, updates, message = service.execute('/type foo', work_item) - - expect(message).to eq(_("Failed to convert this work item: Provided type is not supported.")) - expect(updates).to eq({}) - end - - it 'returns error with same type' do - _, updates, message = service.execute('/type Issue', work_item) - - expect(message).to eq(_("Failed to convert this work item: Types are the same.")) - expect(updates).to eq({}) - end - end - - context 'when user has insufficient permissions to create new type' do - before do - allow(Ability).to receive(:allowed?).and_call_original - allow(Ability).to receive(:allowed?).with(current_user, :create_task, work_item).and_return(false) - end - - it 'returns error' do - _, updates, message = service.execute(command, work_item) - - expect(message).to eq(_("Failed to convert this work item: You have insufficient permissions.")) - expect(updates).to eq({}) - end - end - end + it_behaves_like 'quick actions that change work item type' end describe '#explain' do @@ -2875,28 +2868,66 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning expect(explanations) .to contain_exactly("Converts work item to Issue. Widgets not supported in new type are removed.") end + end + + describe 'relate and unlink commands' do + let_it_be(:other_issue) { create(:issue, project: project).to_reference(issue) } + let(:relate_content) { "/relate #{other_issue}" } + let(:unlink_content) { "/unlink #{other_issue}" } + + context 'when user has permissions' do + it '/relate command is available' do + _, explanations = service.explain(relate_content, issue) + + expect(explanations).to eq(["Marks this issue as related to #{other_issue}."]) + end + + it '/unlink command is available' do + _, explanations = service.explain(unlink_content, issue) - context 'when feature flag work_items_mvc_2 is disabled' do + expect(explanations).to eq(["Removes link with #{other_issue}."]) + end + end + + context 'when user has insufficient permissions' do before do - stub_feature_flags(work_items_mvc_2: false) + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(current_user, :admin_issue_link, issue).and_return(false) + end + + it '/relate command is not available' do + _, explanations = service.explain(relate_content, issue) + + expect(explanations).to be_empty end - it 'does not have the command available' do - _, explanations = service.explain(command, work_item) + it '/unlink command is not available' do + _, explanations = service.explain(unlink_content, issue) expect(explanations).to be_empty end end end - describe 'relate command' do - let_it_be(:other_issue) { create(:issue, project: project) } - let(:content) { "/relate #{other_issue.to_reference}" } + describe 'promote_to command' do + let(:content) { '/promote_to issue' } - it 'includes explain message' do - _, explanations = service.explain(content, issue) + context 'when work item supports promotion' do + let_it_be(:task) { build(:work_item, :task, project: project) } + + it 'includes the value' do + _, explanations = service.explain(content, task) + expect(explanations).to eq(['Promotes work item to issue.']) + end + end + + context 'when work item does not support promotion' do + let_it_be(:incident) { build(:work_item, :incident, project: project) } - expect(explanations).to eq(["Marks this issue as related to #{other_issue.to_reference}."]) + it 'does not include the value' do + _, explanations = service.explain(content, incident) + expect(explanations).to be_empty + end end end end diff --git a/spec/services/releases/links/params_spec.rb b/spec/services/releases/links/params_spec.rb new file mode 100644 index 00000000000..580bddf4fd9 --- /dev/null +++ b/spec/services/releases/links/params_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Releases::Links::Params, feature_category: :release_orchestration do + subject(:filter) { described_class.new(params) } + + let(:params) { { name: name, url: url, direct_asset_path: direct_asset_path, link_type: link_type, unknown: '?' } } + let(:name) { 'link' } + let(:url) { 'https://example.com' } + let(:direct_asset_path) { '/path' } + let(:link_type) { 'other' } + + describe '#allowed_params' do + subject { filter.allowed_params } + + it 'returns only allowed params' do + is_expected.to eq('name' => name, 'url' => url, 'filepath' => direct_asset_path, 'link_type' => link_type) + end + + context 'when deprecated filepath is used' do + let(:params) { super().merge(direct_asset_path: nil, filepath: 'filepath') } + + it 'uses filepath value' do + is_expected.to eq('name' => name, 'url' => url, 'filepath' => 'filepath', 'link_type' => link_type) + end + end + + context 'when both direct_asset_path and filepath are provided' do + let(:params) { super().merge(filepath: 'filepath') } + + it 'uses direct_asset_path value' do + is_expected.to eq('name' => name, 'url' => url, 'filepath' => direct_asset_path, 'link_type' => link_type) + end + end + end +end diff --git a/spec/services/reset_project_cache_service_spec.rb b/spec/services/reset_project_cache_service_spec.rb index 6ae516a5f07..6540b93bcc5 100644 --- a/spec/services/reset_project_cache_service_spec.rb +++ b/spec/services/reset_project_cache_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ResetProjectCacheService, feature_category: :projects do +RSpec.describe ResetProjectCacheService, feature_category: :groups_and_projects do let(:project) { create(:project) } let(:user) { create(:user) } diff --git a/spec/services/resource_access_tokens/create_service_spec.rb b/spec/services/resource_access_tokens/create_service_spec.rb index 59d582f038a..31e4e008d4f 100644 --- a/spec/services/resource_access_tokens/create_service_spec.rb +++ b/spec/services/resource_access_tokens/create_service_spec.rb @@ -188,51 +188,26 @@ RSpec.describe ResourceAccessTokens::CreateService, feature_category: :system_ac context 'expires_at' do context 'when no expiration value is passed' do - context 'when default_pat_expiration feature flag is true' do - it 'defaults to PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS' do - freeze_time do - response = subject - access_token = response.payload[:access_token] - - expect(access_token.expires_at).to eq( - max_pat_access_token_lifetime.to_date - ) - end - end - - context 'expiry of the project bot member' do - it 'project bot membership does not expire' do - response = subject - access_token = response.payload[:access_token] - project_bot = access_token.user + it 'defaults to PersonalAccessToken::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS' do + freeze_time do + response = subject + access_token = response.payload[:access_token] - expect(resource.members.find_by(user_id: project_bot.id).expires_at).to eq( - max_pat_access_token_lifetime.to_date - ) - end + expect(access_token.expires_at).to eq( + max_pat_access_token_lifetime.to_date + ) end end - context 'when default_pat_expiration feature flag is false' do - before do - stub_feature_flags(default_pat_expiration: false) - end - - it 'uses nil expiration value' do + context 'expiry of the project bot member' do + it 'project bot membership does not expire' do response = subject access_token = response.payload[:access_token] + project_bot = access_token.user - expect(access_token.expires_at).to eq(nil) - end - - context 'expiry of the project bot member' do - it 'project bot membership expires' do - response = subject - access_token = response.payload[:access_token] - project_bot = access_token.user - - expect(resource.members.find_by(user_id: project_bot.id).expires_at).to eq(nil) - end + expect(resource.members.find_by(user_id: project_bot.id).expires_at).to eq( + max_pat_access_token_lifetime.to_date + ) end end end diff --git a/spec/services/search/global_service_spec.rb b/spec/services/search/global_service_spec.rb index 6250d32574f..f77d81851e3 100644 --- a/spec/services/search/global_service_spec.rb +++ b/spec/services/search/global_service_spec.rb @@ -3,13 +3,14 @@ require 'spec_helper' RSpec.describe Search::GlobalService, feature_category: :global_search do - let(:user) { create(:user) } - let(:internal_user) { create(:user) } + let_it_be(:user) { create(:user) } + let_it_be(:internal_user) { create(:user) } - let!(:found_project) { create(:project, :private, name: 'searchable_project') } - let!(:unfound_project) { create(:project, :private, name: 'unfound_project') } - let!(:internal_project) { create(:project, :internal, name: 'searchable_internal_project') } - let!(:public_project) { create(:project, :public, name: 'searchable_public_project') } + let_it_be(:found_project) { create(:project, :private, name: 'searchable_project') } + let_it_be(:unfound_project) { create(:project, :private, name: 'unfound_project') } + let_it_be(:internal_project) { create(:project, :internal, name: 'searchable_internal_project') } + let_it_be(:public_project) { create(:project, :public, name: 'searchable_public_project') } + let_it_be(:archived_project) { create(:project, :public, archived: true, name: 'archived_project') } before do found_project.add_maintainer(user) @@ -44,12 +45,16 @@ RSpec.describe Search::GlobalService, feature_category: :global_search do end it 'does not return archived projects' do - archived_project = create(:project, :public, archived: true, name: 'archived_project') - results = described_class.new(user, search: "archived").execute expect(results.objects('projects')).not_to include(archived_project) end + + it 'returns archived projects if the include_archived option is passed' do + results = described_class.new(user, { include_archived: true, search: "archived" }).execute + + expect(results.objects('projects')).to include(archived_project) + end end end diff --git a/spec/services/search_service_spec.rb b/spec/services/search_service_spec.rb index d11fc377d83..c937a93c6ef 100644 --- a/spec/services/search_service_spec.rb +++ b/spec/services/search_service_spec.rb @@ -485,6 +485,8 @@ RSpec.describe SearchService, feature_category: :global_search do 'issues' | :global_search_issues_tab | true | true 'merge_requests' | :global_search_merge_requests_tab | false | false 'merge_requests' | :global_search_merge_requests_tab | true | true + 'snippet_titles' | :global_search_snippet_titles_tab | false | false + 'snippet_titles' | :global_search_snippet_titles_tab | true | true 'wiki_blobs' | :global_search_wiki_tab | false | false 'wiki_blobs' | :global_search_wiki_tab | true | true 'users' | :global_search_users_tab | false | false @@ -498,5 +500,25 @@ RSpec.describe SearchService, feature_category: :global_search do expect(subject.global_search_enabled_for_scope?).to eq expected end end + + context 'when snippet search is enabled' do + let(:scope) { 'snippet_titles' } + + before do + allow(described_class).to receive(:show_snippets?).and_return(true) + end + + it 'returns false when feature_flag is not enabled' do + stub_feature_flags(global_search_snippet_titles_tab: false) + + expect(subject.global_search_enabled_for_scope?).to eq false + end + + it 'returns true when feature_flag is enabled' do + stub_feature_flags(global_search_snippet_titles_tab: true) + + expect(subject.global_search_enabled_for_scope?).to eq true + end + end end end diff --git a/spec/services/service_desk/custom_email_verifications/create_service_spec.rb b/spec/services/service_desk/custom_email_verifications/create_service_spec.rb new file mode 100644 index 00000000000..fceb6fc78b4 --- /dev/null +++ b/spec/services/service_desk/custom_email_verifications/create_service_spec.rb @@ -0,0 +1,139 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ServiceDesk::CustomEmailVerifications::CreateService, feature_category: :service_desk do + describe '#execute' do + let_it_be_with_reload(:project) { create(:project) } + let_it_be(:user) { create(:user) } + + let!(:credential) { create(:service_desk_custom_email_credential, project: project) } + + let(:message_delivery) { instance_double(ActionMailer::MessageDelivery) } + let(:message) { instance_double(Mail::Message) } + + let(:service) { described_class.new(project: project, current_user: user) } + + before do + allow(message_delivery).to receive(:deliver_later) + allow(Notify).to receive(:service_desk_verification_triggered_email).and_return(message_delivery) + + # We send verification email directly + allow(message).to receive(:deliver) + allow(Notify).to receive(:service_desk_custom_email_verification_email).and_return(message) + end + + shared_examples 'a verification process that exits early' do + it 'aborts verification process and exits early', :aggregate_failures do + # Because we exit early it should not send any verification or notification emails + expect(service).to receive(:setup_and_deliver_verification_email).exactly(0).times + expect(Notify).to receive(:service_desk_verification_triggered_email).exactly(0).times + + response = service.execute + + expect(response).to be_error + end + end + + shared_examples 'a verification process with ramp up error' do |error, error_identifier| + it 'aborts verification process', :aggregate_failures do + allow(message).to receive(:deliver).and_raise(error) + + # Creates one verification email + expect(Notify).to receive(:service_desk_custom_email_verification_email).once + + # Correct amount of notification emails were sent + expect(Notify).to receive(:service_desk_verification_triggered_email).exactly(project.owners.size + 1).times + + # Correct amount of result notification emails were sent + expect(Notify).to receive(:service_desk_verification_result_email).exactly(project.owners.size + 1).times + + response = service.execute + + expect(response).to be_error + expect(response.reason).to eq error_identifier + + expect(settings).not_to be_custom_email_enabled + expect(settings.custom_email_verification.triggered_at).not_to be_nil + expect(settings.custom_email_verification).to have_attributes( + token: nil, + triggerer: user, + error: error_identifier, + state: 'failed' + ) + end + end + + it_behaves_like 'a verification process that exits early' + + context 'when feature flag :service_desk_custom_email is disabled' do + before do + stub_feature_flags(service_desk_custom_email: false) + end + + it_behaves_like 'a verification process that exits early' + end + + context 'when service desk setting exists' do + let(:settings) { create(:service_desk_setting, project: project, custom_email: 'user@example.com') } + let(:service) { described_class.new(project: settings.project, current_user: user) } + + it 'aborts verification process and exits early', :aggregate_failures do + # Because we exit early it should not send any verification or notification emails + expect(service).to receive(:setup_and_deliver_verification_email).exactly(0).times + expect(Notify).to receive(:service_desk_verification_triggered_email).exactly(0).times + + response = service.execute + settings.reload + + expect(response).to be_error + + expect(settings.custom_email_enabled).to be false + # Because service should normally add initial verification object + expect(settings.custom_email_verification).to be nil + end + + context 'when user has maintainer role in project' do + before do + project.add_maintainer(user) + end + + it 'initiates verification process successfully', :aggregate_failures do + # Creates one verification email + expect(Notify).to receive(:service_desk_custom_email_verification_email).once + + # Check whether the correct amount of notification emails were sent + expect(Notify).to receive(:service_desk_verification_triggered_email).exactly(project.owners.size + 1).times + + response = service.execute + + settings.reload + verification = settings.custom_email_verification + + expect(response).to be_success + + expect(settings.custom_email_enabled).to be false + + expect(verification).to be_started + expect(verification.token).not_to be_nil + expect(verification.triggered_at).not_to be_nil + expect(verification).to have_attributes( + triggerer: user, + error: nil + ) + end + + context 'when providing invalid SMTP credentials' do + before do + allow(Notify).to receive(:service_desk_verification_result_email).and_return(message_delivery) + end + + it_behaves_like 'a verification process with ramp up error', SocketError, 'smtp_host_issue' + it_behaves_like 'a verification process with ramp up error', OpenSSL::SSL::SSLError, 'smtp_host_issue' + it_behaves_like 'a verification process with ramp up error', + Net::SMTPAuthenticationError.new('Invalid username or password'), 'invalid_credentials' + end + end + end + end +end diff --git a/spec/services/service_desk/custom_email_verifications/update_service_spec.rb b/spec/services/service_desk/custom_email_verifications/update_service_spec.rb new file mode 100644 index 00000000000..f1e683c0185 --- /dev/null +++ b/spec/services/service_desk/custom_email_verifications/update_service_spec.rb @@ -0,0 +1,151 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ServiceDesk::CustomEmailVerifications::UpdateService, feature_category: :service_desk do + describe '#execute' do + let_it_be_with_reload(:project) { create(:project) } + let_it_be(:user) { create(:user) } + + let!(:credential) { create(:service_desk_custom_email_credential, project: project) } + let(:settings) { create(:service_desk_setting, project: project, custom_email: 'custom-support-email@example.com') } + + let(:mail_object) { nil } + let(:message_delivery) { instance_double(ActionMailer::MessageDelivery) } + let(:service) { described_class.new(project: settings.project, params: { mail: mail_object }) } + + before do + allow(message_delivery).to receive(:deliver_later) + allow(Notify).to receive(:service_desk_verification_result_email).and_return(message_delivery) + end + + shared_examples 'a failing verification process' do |expected_error_identifier| + it 'refuses to verify and sends result emails' do + expect(Notify).to receive(:service_desk_verification_result_email).twice + + response = described_class.new(project: settings.project, params: { mail: mail_object }).execute + + settings.reset + verification.reset + + expect(response).to be_error + expect(settings).not_to be_custom_email_enabled + expect(verification).to be_failed + + expect(response.reason).to eq expected_error_identifier + expect(verification.error).to eq expected_error_identifier + end + end + + shared_examples 'an early exit from the verification process' do |expected_state| + it 'exits early' do + expect(Notify).to receive(:service_desk_verification_result_email).exactly(0).times + + response = service.execute + + settings.reset + verification.reset + + expect(response).to be_error + expect(settings).not_to be_custom_email_enabled + expect(verification.state).to eq expected_state + end + end + + it 'exits early' do + expect(Notify).to receive(:service_desk_verification_result_email).exactly(0).times + + response = service.execute + + settings.reset + + expect(response).to be_error + expect(settings).not_to be_custom_email_enabled + end + + context 'when feature flag :service_desk_custom_email is disabled' do + before do + stub_feature_flags(service_desk_custom_email: false) + end + + it 'exits early' do + expect(Notify).to receive(:service_desk_verification_result_email).exactly(0).times + + response = service.execute + + expect(response).to be_error + end + end + + context 'when verification exists' do + let!(:verification) { create(:service_desk_custom_email_verification, project: project) } + + context 'when we do not have a verification email' do + # Raise if verification started but no email provided + it_behaves_like 'a failing verification process', 'mail_not_received_within_timeframe' + + context 'when already verified' do + before do + verification.mark_as_finished! + end + + it_behaves_like 'an early exit from the verification process', 'finished' + end + + context 'when we already have an error' do + before do + verification.mark_as_failed!(:smtp_host_issue) + end + + it_behaves_like 'an early exit from the verification process', 'failed' + end + end + + context 'when we have a verification email' do + before do + verification.update!(token: 'ZROT4ZZXA-Y6') # token from email fixture + end + + let(:email_raw) { email_fixture('emails/service_desk_custom_email_address_verification.eml') } + let(:mail_object) { Mail::Message.new(email_raw) } + + it 'verifies and sends result emails' do + expect(Notify).to receive(:service_desk_verification_result_email).twice + + response = service.execute + + settings.reset + verification.reset + + expect(response).to be_success + expect(settings).not_to be_custom_email_enabled + expect(verification).to be_finished + end + + context 'and verification tokens do not match' do + before do + verification.update!(token: 'XXXXXXZXA-XX') + end + + it_behaves_like 'a failing verification process', 'incorrect_token' + end + + context 'and from address does not match with custom email' do + before do + settings.update!(custom_email: 'some-other@example.com') + end + + it_behaves_like 'a failing verification process', 'incorrect_from' + end + + context 'and timeframe for receiving the email is over' do + before do + verification.update!(triggered_at: 40.minutes.ago) + end + + it_behaves_like 'a failing verification process', 'mail_not_received_within_timeframe' + end + end + end + end +end diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index 725f1b165a2..59deffe9ccd 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -20,9 +20,8 @@ RSpec.describe Snippets::CreateService, feature_category: :source_code_managemen let(:extra_opts) { {} } let(:creator) { admin } - let(:spam_params) { double } - subject { described_class.new(project: project, current_user: creator, params: opts, spam_params: spam_params).execute } + subject { described_class.new(project: project, current_user: creator, params: opts).execute } let(:snippet) { subject.payload[:snippet] } @@ -303,10 +302,6 @@ RSpec.describe Snippets::CreateService, feature_category: :source_code_managemen end end - before do - stub_spam_services - end - context 'when ProjectSnippet' do let_it_be(:project) { create(:project) } diff --git a/spec/services/snippets/update_service_spec.rb b/spec/services/snippets/update_service_spec.rb index 99bb70a3077..b428897ce27 100644 --- a/spec/services/snippets/update_service_spec.rb +++ b/spec/services/snippets/update_service_spec.rb @@ -21,9 +21,8 @@ RSpec.describe Snippets::UpdateService, feature_category: :source_code_managemen let(:extra_opts) { {} } let(:options) { base_opts.merge(extra_opts) } let(:updater) { user } - let(:spam_params) { double } - let(:service) { Snippets::UpdateService.new(project: project, current_user: updater, params: options, spam_params: spam_params) } + let(:service) { Snippets::UpdateService.new(project: project, current_user: updater, params: options, perform_spam_check: true) } subject { service.execute(snippet) } @@ -724,10 +723,6 @@ RSpec.describe Snippets::UpdateService, feature_category: :source_code_managemen end end - before do - stub_spam_services - end - context 'when Project Snippet' do let_it_be(:project) { create(:project) } diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb index e2cc2ea7ce3..15cb4977b61 100644 --- a/spec/services/spam/spam_action_service_spec.rb +++ b/spec/services/spam/spam_action_service_spec.rb @@ -30,11 +30,15 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d before do issue.spam = false personal_snippet.spam = false + + allow_next_instance_of(described_class) do |service| + allow(service).to receive(:spam_params).and_return(spam_params) + end end describe 'constructor argument validation' do subject do - described_service = described_class.new(spammable: issue, spam_params: spam_params, user: user, action: :create) + described_service = described_class.new(spammable: issue, user: user, action: :create) described_service.execute end @@ -51,6 +55,21 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d expect(issue).not_to be_spam end end + + context 'when user is nil' do + let(:spam_params) { true } + let(:user) { nil } + let(:expected_service_user_not_present_message) do + /Skipped spam check because user was not present/ + end + + it "returns success with a messaage" do + response = subject + + expect(response.message).to match(expected_service_user_not_present_message) + expect(issue).not_to be_spam + end + end end shared_examples 'allows user' do @@ -108,7 +127,7 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d let_it_be(:existing_spam_log) { create(:spam_log, user: user, recaptcha_verified: false) } subject do - described_service = described_class.new(spammable: target, spam_params: spam_params, extra_features: + described_service = described_class.new(spammable: target, extra_features: extra_features, user: user, action: :create) described_service.execute end @@ -116,6 +135,7 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d before do allow(Captcha::CaptchaVerificationService).to receive(:new).with(spam_params: spam_params) { fake_captcha_verification_service } allow(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args).and_return(fake_verdict_service) + allow(fake_verdict_service).to receive(:execute).and_return({}) end context 'when captcha response verification returns true' do @@ -166,6 +186,24 @@ RSpec.describe Spam::SpamActionService, feature_category: :instance_resiliency d target.description = 'Lovely Spam! Wonderful Spam!' end + context 'when captcha is not supported' do + before do + allow(target).to receive(:supports_recaptcha?).and_return(false) + end + + it "does not execute with captcha support" do + expect(Captcha::CaptchaVerificationService).not_to receive(:new) + + subject + end + + it "executes a spam check" do + expect(fake_verdict_service).to receive(:execute) + + subject + end + end + context 'when user is a gitlab bot' do before do allow(user).to receive(:gitlab_bot?).and_return(true) diff --git a/spec/services/spam/spam_verdict_service_spec.rb b/spec/services/spam/spam_verdict_service_spec.rb index 00e320ed56c..6b14cf33041 100644 --- a/spec/services/spam/spam_verdict_service_spec.rb +++ b/spec/services/spam/spam_verdict_service_spec.rb @@ -271,17 +271,6 @@ RSpec.describe Spam::SpamVerdictService, feature_category: :instance_resiliency expect(user.spam_score).to eq(0.0) end end - - context 'user spam score feature is disabled' do - before do - stub_feature_flags(user_spam_scores: false) - end - - it 'returns the verdict and does not update the spam score' do - expect(subject).to eq(ALLOW) - expect(user.spam_score).to eq(0.0) - end - end end context 'when recaptcha is enabled' do diff --git a/spec/services/submodules/update_service_spec.rb b/spec/services/submodules/update_service_spec.rb index aeaf8ec1c7b..f4b8a3db29c 100644 --- a/spec/services/submodules/update_service_spec.rb +++ b/spec/services/submodules/update_service_spec.rb @@ -86,13 +86,15 @@ RSpec.describe Submodules::UpdateService, feature_category: :source_code_managem end end - context 'has traversal path' do - let(:submodule) { '../six' } - - it_behaves_like 'returns error result' do - let(:error_message) { 'Invalid submodule path' } - end - end + # Can be re-enabled when problem from https://gitlab.com/gitlab-org/gitlab/-/issues/413964#note_1421909142 + # is fixed + # context 'has traversal path' do + # let(:submodule) { '../six' } + + # it_behaves_like 'returns error result' do + # let(:error_message) { 'Invalid submodule path' } + # end + # end end context 'commit_sha' do diff --git a/spec/services/system_notes/alert_management_service_spec.rb b/spec/services/system_notes/alert_management_service_spec.rb index 4d40a6a6cfd..1e3be24b05f 100644 --- a/spec/services/system_notes/alert_management_service_spec.rb +++ b/spec/services/system_notes/alert_management_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ::SystemNotes::AlertManagementService, feature_category: :projects do +RSpec.describe ::SystemNotes::AlertManagementService, feature_category: :groups_and_projects do let_it_be(:author) { create(:user) } let_it_be(:project) { create(:project, :repository) } let_it_be(:noteable) { create(:alert_management_alert, :with_incident, :acknowledged, project: project) } diff --git a/spec/services/system_notes/base_service_spec.rb b/spec/services/system_notes/base_service_spec.rb index 6ea4751b613..5c0ecf71d01 100644 --- a/spec/services/system_notes/base_service_spec.rb +++ b/spec/services/system_notes/base_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe SystemNotes::BaseService, feature_category: :projects do +RSpec.describe SystemNotes::BaseService, feature_category: :groups_and_projects do let(:noteable) { double } let(:project) { double } let(:author) { double } diff --git a/spec/services/tasks_to_be_done/base_service_spec.rb b/spec/services/tasks_to_be_done/base_service_spec.rb index 3ca9d140197..32b07cab095 100644 --- a/spec/services/tasks_to_be_done/base_service_spec.rb +++ b/spec/services/tasks_to_be_done/base_service_spec.rb @@ -35,7 +35,7 @@ RSpec.describe TasksToBeDone::BaseService, feature_category: :team_planning do expect(Issues::CreateService) .to receive(:new) - .with(container: project, current_user: current_user, params: params, spam_params: nil) + .with(container: project, current_user: current_user, params: params, perform_spam_check: false) .and_call_original expect { service.execute }.to change(Issue, :count).by(1) diff --git a/spec/services/user_agent_detail_service_spec.rb b/spec/services/user_agent_detail_service_spec.rb new file mode 100644 index 00000000000..3984ec33716 --- /dev/null +++ b/spec/services/user_agent_detail_service_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe UserAgentDetailService, feature_category: :instance_resiliency do + describe '#create', :request_store do + let_it_be(:spammable) { create(:issue) } + + using RSpec::Parameterized::TableSyntax + + where(:perform_spam_check, :spam_params_present, :user_agent, :ip_address, :creates_user_agent_detail) do + true | true | 'UA' | 'IP' | true + true | false | 'UA' | 'IP' | false + false | true | 'UA' | 'IP' | false + true | true | '' | 'IP' | false + true | true | nil | 'IP' | false + true | true | 'UA' | '' | false + true | true | 'UA' | nil | false + end + + with_them do + let(:spam_params) do + instance_double('Spam::SpamParams', user_agent: user_agent, ip_address: ip_address) if spam_params_present + end + + before do + allow(Gitlab::RequestContext.instance).to receive(:spam_params).and_return(spam_params) + end + + subject { described_class.new(spammable: spammable, perform_spam_check: perform_spam_check).create } # rubocop:disable Rails/SaveBang + + it 'creates a user agent detail when expected' do + if creates_user_agent_detail + expect { subject }.to change { UserAgentDetail.count }.by(1) + else + expect(subject).to be_a ServiceResponse + end + end + end + end +end diff --git a/spec/services/users/activate_service_spec.rb b/spec/services/users/activate_service_spec.rb new file mode 100644 index 00000000000..8b8c0dbdd3e --- /dev/null +++ b/spec/services/users/activate_service_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::ActivateService, feature_category: :user_management do + let_it_be(:current_user) { build(:admin) } + + subject(:service) { described_class.new(current_user) } + + describe '#execute' do + let!(:user) { create(:user, :deactivated) } + + subject(:operation) { service.execute(user) } + + context 'when successful', :enable_admin_mode do + it 'returns success status' do + expect(operation[:status]).to eq(:success) + end + + it "changes the user's state" do + expect { operation }.to change { user.state }.to('active') + end + + it 'creates a log entry' do + expect(Gitlab::AppLogger).to receive(:info).with(message: "User activated", user: user.username, + email: user.email, activated_by: current_user.username, ip_address: current_user.current_sign_in_ip.to_s) + + operation + end + end + + context 'when the user is already active', :enable_admin_mode do + let(:user) { create(:user) } + + it 'returns success result' do + aggregate_failures 'success result' do + expect(operation[:status]).to eq(:success) + expect(operation[:message]).to eq('Successfully activated') + end + end + + it "does not change the user's state" do + expect { operation }.not_to change { user.state } + end + end + + context 'when user activation fails', :enable_admin_mode do + before do + allow(user).to receive(:activate).and_return(false) + end + + it 'returns an unprocessable entity error' do + result = service.execute(user) + + expect(result[:status]).to eq(:error) + expect(result[:reason]).to eq(:unprocessable_entity) + end + end + + context 'when user is not an admin' do + let(:non_admin_user) { build(:user) } + let(:service) { described_class.new(non_admin_user) } + + it 'returns permissions error message' do + expect(operation[:status]).to eq(:error) + expect(operation[:message]).to eq("You are not authorized to perform this action") + expect(operation.reason).to eq :forbidden + end + end + end +end diff --git a/spec/services/users/set_namespace_commit_email_service_spec.rb b/spec/services/users/set_namespace_commit_email_service_spec.rb new file mode 100644 index 00000000000..4f64d454ecb --- /dev/null +++ b/spec/services/users/set_namespace_commit_email_service_spec.rb @@ -0,0 +1,195 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::SetNamespaceCommitEmailService, feature_category: :user_profile do + include AfterNextHelpers + + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:email) { create(:email, user: user) } + let_it_be(:existing_achievement) { create(:achievement, namespace: group) } + + let(:namespace) { group } + let(:current_user) { user } + let(:target_user) { user } + let(:email_id) { email.id } + let(:params) { { user: target_user } } + let(:service) { described_class.new(current_user, namespace, email_id, params) } + + before_all do + group.add_reporter(user) + end + + shared_examples 'success' do + it 'creates namespace commit email' do + result = service.execute + + expect(result.payload[:namespace_commit_email]).to be_a(Users::NamespaceCommitEmail) + expect(result.payload[:namespace_commit_email]).to be_persisted + end + end + + describe '#execute' do + context 'when current_user is not provided' do + let(:current_user) { nil } + + it 'returns error message' do + expect(service.execute.message) + .to eq("User doesn't exist or you don't have permission to change namespace commit emails.") + end + end + + context 'when current_user does not have permission to change namespace commit emails' do + let(:target_user) { create(:user) } + + it 'returns error message' do + expect(service.execute.message) + .to eq("User doesn't exist or you don't have permission to change namespace commit emails.") + end + end + + context 'when target_user does not have permission to access the namespace' do + let(:namespace) { create(:group) } + + it 'returns error message' do + expect(service.execute.message).to eq("Namespace doesn't exist or you don't have permission.") + end + end + + context 'when namespace is not provided' do + let(:namespace) { nil } + + it 'returns error message' do + expect(service.execute.message).to eq('Namespace must be provided.') + end + end + + context 'when target user is not current user' do + context 'when current user is an admin' do + let(:current_user) { create(:user, :admin) } + + context 'when admin mode is enabled', :enable_admin_mode do + it 'creates namespace commit email' do + result = service.execute + + expect(result.payload[:namespace_commit_email]).to be_a(Users::NamespaceCommitEmail) + expect(result.payload[:namespace_commit_email]).to be_persisted + end + end + + context 'when admin mode is not enabled' do + it 'returns error message' do + expect(service.execute.message) + .to eq("User doesn't exist or you don't have permission to change namespace commit emails.") + end + end + end + + context 'when current user is not an admin' do + let(:current_user) { create(:user) } + + it 'returns error message' do + expect(service.execute.message) + .to eq("User doesn't exist or you don't have permission to change namespace commit emails.") + end + end + end + + context 'when namespace commit email does not exist' do + context 'when email_id is not provided' do + let(:email_id) { nil } + + it 'returns error message' do + expect(service.execute.message).to eq('Email must be provided.') + end + end + + context 'when model save fails' do + before do + allow_next(::Users::NamespaceCommitEmail).to receive(:save).and_return(false) + end + + it 'returns error message' do + expect(service.execute.message).to eq('Failed to save namespace commit email.') + end + end + + context 'when namepsace is a group' do + it_behaves_like 'success' + end + + context 'when namespace is a user' do + let(:namespace) { current_user.namespace } + + it_behaves_like 'success' + end + + context 'when namespace is a project' do + let_it_be(:project) { create(:project) } + + let(:namespace) { project.project_namespace } + + before do + project.add_reporter(current_user) + end + + it_behaves_like 'success' + end + end + + context 'when namespace commit email already exists' do + let!(:existing_namespace_commit_email) do + create(:namespace_commit_email, + user: target_user, + namespace: namespace, + email: create(:email, user: target_user)) + end + + context 'when email_id is not provided' do + let(:email_id) { nil } + + it 'destroys the namespace commit email' do + result = service.execute + + expect(result.message).to be_nil + expect(result.payload[:namespace_commit_email]).to be_nil + end + end + + context 'and email_id is provided' do + let(:email_id) { create(:email, user: current_user).id } + + it 'updates namespace commit email' do + result = service.execute + + existing_namespace_commit_email.reload + + expect(result.payload[:namespace_commit_email]).to eq(existing_namespace_commit_email) + expect(existing_namespace_commit_email.email_id).to eq(email_id) + end + end + + context 'when model save fails' do + before do + allow_any_instance_of(::Users::NamespaceCommitEmail).to receive(:save).and_return(false) # rubocop:disable RSpec/AnyInstanceOf + end + + it 'returns generic error message' do + expect(service.execute.message).to eq('Failed to save namespace commit email.') + end + + context 'with model errors' do + before do + allow_any_instance_of(::Users::NamespaceCommitEmail).to receive_message_chain(:errors, :empty?).and_return(false) # rubocop:disable RSpec/AnyInstanceOf + allow_any_instance_of(::Users::NamespaceCommitEmail).to receive_message_chain(:errors, :full_messages, :to_sentence).and_return('Model error') # rubocop:disable RSpec/AnyInstanceOf + end + + it 'returns the model error message' do + expect(service.execute.message).to eq('Model error') + end + end + end + end + end +end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index b4250fcf04d..2aa62f932ed 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state, feature_category: :integrations do +RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state, feature_category: :webhooks do include StubRequests let(:ellipsis) { '…' } diff --git a/spec/services/webauthn/destroy_service_spec.rb b/spec/services/webauthn/destroy_service_spec.rb new file mode 100644 index 00000000000..dd04601ccf0 --- /dev/null +++ b/spec/services/webauthn/destroy_service_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Webauthn::DestroyService, feature_category: :system_access do + let(:user) { create(:user, :two_factor_via_webauthn, registrations_count: 1) } + let(:current_user) { user } + + describe '#execute' do + let(:webauthn_id) { user.webauthn_registrations.first.id } + + subject { described_class.new(current_user, user, webauthn_id).execute } + + context 'with only one webauthn method enabled' do + context 'when another user is calling the service' do + context 'for a user without permissions' do + let(:current_user) { create(:user) } + + it 'does not destry the webauthn registration' do + expect { subject }.not_to change { user.webauthn_registrations.count } + end + + it 'does not remove the user backup codes' do + expect { subject }.not_to change { user.otp_backup_codes } + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + end + end + + context 'for an admin' do + it 'destroys the webauthn registration' do + expect { subject }.to change { user.webauthn_registrations.count }.by(-1) + end + + it 'removes the user backup codes' do + subject + + expect(user.otp_backup_codes).to be_nil + end + end + end + + context 'when current user is calling the service' do + context 'when there is also OTP enabled' do + before do + user.otp_required_for_login = true + user.otp_secret = User.generate_otp_secret(32) + user.otp_grace_period_started_at = Time.current + user.generate_otp_backup_codes! + user.save! + end + + it 'removes the webauth registrations' do + expect { subject }.to change { user.webauthn_registrations.count }.by(-1) + end + + it 'does not remove the user backup codes' do + expect { subject }.not_to change { user.otp_backup_codes } + end + end + end + end + + context 'with multiple webauthn methods enabled' do + before do + create(:webauthn_registration, user: user) + end + + it 'destroys the webauthn registration' do + expect { subject }.to change { user.webauthn_registrations.count }.by(-1) + end + + it 'does not remove the user backup codes' do + expect { subject }.not_to change { user.otp_backup_codes } + end + end + end +end diff --git a/spec/services/work_items/widgets/award_emoji_service/update_service_spec.rb b/spec/services/work_items/callbacks/award_emoji_spec.rb index 186e4d56cc4..831604d73b1 100644 --- a/spec/services/work_items/widgets/award_emoji_service/update_service_spec.rb +++ b/spec/services/work_items/callbacks/award_emoji_spec.rb @@ -2,27 +2,26 @@ require 'spec_helper' -RSpec.describe WorkItems::Widgets::AwardEmojiService::UpdateService, feature_category: :team_planning do +RSpec.describe WorkItems::Callbacks::AwardEmoji, feature_category: :team_planning do let_it_be(:reporter) { create(:user) } let_it_be(:unauthorized_user) { create(:user) } let_it_be(:project) { create(:project, :private) } let_it_be(:work_item) { create(:work_item, project: project) } let(:current_user) { reporter } - let(:widget) { work_item.widgets.find { |widget| widget.is_a?(WorkItems::Widgets::AwardEmoji) } } before_all do project.add_reporter(reporter) end - describe '#before_update_in_transaction' do + describe '#before_update' do subject do - described_class.new(widget: widget, current_user: current_user) - .before_update_in_transaction(params: params) + described_class.new(issuable: work_item, current_user: current_user, params: params) + .before_update end shared_examples 'raises a WidgetError' do - it { expect { subject }.to raise_error(described_class::WidgetError, message) } + it { expect { subject }.to raise_error(::WorkItems::Widgets::BaseService::WidgetError, message) } end context 'when awarding an emoji' do diff --git a/spec/services/work_items/create_and_link_service_spec.rb b/spec/services/work_items/create_and_link_service_spec.rb index 00372d460e1..b83492274a3 100644 --- a/spec/services/work_items/create_and_link_service_spec.rb +++ b/spec/services/work_items/create_and_link_service_spec.rb @@ -9,7 +9,6 @@ RSpec.describe WorkItems::CreateAndLinkService, feature_category: :portfolio_man let_it_be(:related_work_item, refind: true) { create(:work_item, project: project) } let_it_be(:invalid_parent) { create(:work_item, :task, project: project) } - let(:spam_params) { double } let(:link_params) { {} } let(:params) do @@ -45,11 +44,7 @@ RSpec.describe WorkItems::CreateAndLinkService, feature_category: :portfolio_man end describe '#execute' do - subject(:service_result) { described_class.new(project: project, current_user: user, params: params, spam_params: spam_params, link_params: link_params).execute } - - before do - stub_spam_services - end + subject(:service_result) { described_class.new(project: project, current_user: user, params: params, link_params: link_params).execute } context 'when work item params are valid' do it { is_expected.to be_success } diff --git a/spec/services/work_items/create_from_task_service_spec.rb b/spec/services/work_items/create_from_task_service_spec.rb index b2f81f1dc54..2ab9209ab05 100644 --- a/spec/services/work_items/create_from_task_service_spec.rb +++ b/spec/services/work_items/create_from_task_service_spec.rb @@ -8,7 +8,6 @@ RSpec.describe WorkItems::CreateFromTaskService, feature_category: :team_plannin let_it_be(:list_work_item, refind: true) { create(:work_item, project: project, description: "- [ ] Item to be converted\n second line\n third line") } let(:work_item_to_update) { list_work_item } - let(:spam_params) { double } let(:link_params) { {} } let(:current_user) { developer } let(:params) do @@ -38,11 +37,7 @@ RSpec.describe WorkItems::CreateFromTaskService, feature_category: :team_plannin end describe '#execute' do - subject(:service_result) { described_class.new(work_item: work_item_to_update, current_user: current_user, work_item_params: params, spam_params: spam_params).execute } - - before do - stub_spam_services - end + subject(:service_result) { described_class.new(work_item: work_item_to_update, current_user: current_user, work_item_params: params).execute } context 'when work item params are valid' do it { is_expected.to be_success } diff --git a/spec/services/work_items/create_service_spec.rb b/spec/services/work_items/create_service_spec.rb index 46e598c3f11..b64d9a29fbf 100644 --- a/spec/services/work_items/create_service_spec.rb +++ b/spec/services/work_items/create_service_spec.rb @@ -30,7 +30,7 @@ RSpec.describe WorkItems::CreateService, feature_category: :team_planning do let_it_be(:user_with_no_access) { create(:user) } let(:widget_params) { {} } - let(:spam_params) { double } + let(:perform_spam_check) { false } let(:current_user) { guest } let(:opts) do { @@ -60,17 +60,13 @@ RSpec.describe WorkItems::CreateService, feature_category: :team_planning do container: container, current_user: current_user, params: opts, - spam_params: spam_params, + perform_spam_check: perform_spam_check, widget_params: widget_params ) end subject(:service_result) { service.execute } - before do - stub_spam_services - end - context 'when user is not allowed to create a work item in the container' do let(:current_user) { user_with_no_access } @@ -151,21 +147,27 @@ RSpec.describe WorkItems::CreateService, feature_category: :team_planning do end context 'checking spam' do - it 'executes SpamActionService' do - expect_next_instance_of( - Spam::SpamActionService, - { - spammable: kind_of(WorkItem), - spam_params: spam_params, - user: an_instance_of(User), - action: :create - } - ) do |instance| - expect(instance).to receive(:execute) + let(:perform_spam_check) { true } + + it 'checks for spam' do + expect_next_instance_of(WorkItem) do |instance| + expect(instance).to receive(:check_for_spam).with(user: current_user, action: :create) end service_result end + + context 'when `perform_spam_check` is set to `false`' do + let(:perform_spam_check) { false } + + it 'does not check for spam' do + expect_next_instance_of(WorkItem) do |instance| + expect(instance).not_to receive(:check_for_spam) + end + + service_result + end + end end it_behaves_like 'work item widgetable service' do @@ -180,7 +182,6 @@ RSpec.describe WorkItems::CreateService, feature_category: :team_planning do container: container, current_user: current_user, params: opts, - spam_params: spam_params, widget_params: widget_params ) end diff --git a/spec/services/work_items/update_service_spec.rb b/spec/services/work_items/update_service_spec.rb index 2cf52ee853a..30c16458353 100644 --- a/spec/services/work_items/update_service_spec.rb +++ b/spec/services/work_items/update_service_spec.rb @@ -9,7 +9,6 @@ RSpec.describe WorkItems::UpdateService, feature_category: :team_planning do let_it_be(:parent) { create(:work_item, project: project) } let_it_be_with_reload(:work_item) { create(:work_item, project: project, assignees: [developer]) } - let(:spam_params) { double } let(:widget_params) { {} } let(:opts) { {} } let(:current_user) { developer } @@ -25,17 +24,12 @@ RSpec.describe WorkItems::UpdateService, feature_category: :team_planning do container: project, current_user: current_user, params: opts, - spam_params: spam_params, widget_params: widget_params ) end subject(:update_work_item) { service.execute(work_item) } - before do - stub_spam_services - end - shared_examples 'update service that triggers graphql dates updated subscription' do it 'triggers graphql subscription issueableDatesUpdated' do expect(GraphqlTriggers).to receive(:issuable_dates_updated).with(work_item).and_call_original @@ -87,6 +81,10 @@ RSpec.describe WorkItems::UpdateService, feature_category: :team_planning do let(:user) { current_user } subject(:service_action) { update_work_item[:status] } end + + it_behaves_like 'update service that triggers GraphQL work_item_updated subscription' do + subject(:execute_service) { update_work_item } + end end context 'when title is not changed' do @@ -113,6 +111,10 @@ RSpec.describe WorkItems::UpdateService, feature_category: :team_planning do update_work_item end + + it_behaves_like 'update service that triggers GraphQL work_item_updated subscription' do + subject(:execute_service) { update_work_item } + end end context 'when decription is changed' do @@ -123,6 +125,10 @@ RSpec.describe WorkItems::UpdateService, feature_category: :team_planning do update_work_item end + + it_behaves_like 'update service that triggers GraphQL work_item_updated subscription' do + subject(:execute_service) { update_work_item } + end end context 'when decription is not changed' do @@ -176,7 +182,6 @@ RSpec.describe WorkItems::UpdateService, feature_category: :team_planning do container: project, current_user: current_user, params: opts, - spam_params: spam_params, widget_params: widget_params ) end @@ -226,6 +231,10 @@ RSpec.describe WorkItems::UpdateService, feature_category: :team_planning do expect(work_item.description).to eq('changed') end + it_behaves_like 'update service that triggers GraphQL work_item_updated subscription' do + subject(:execute_service) { update_work_item } + end + context 'with mentions', :mailer, :sidekiq_might_not_need_inline do shared_examples 'creates the todo and sends email' do |attribute| it 'creates a todo and sends email' do @@ -305,6 +314,10 @@ RSpec.describe WorkItems::UpdateService, feature_category: :team_planning do expect(work_item.work_item_children).to include(child_work_item) end + it_behaves_like 'update service that triggers GraphQL work_item_updated subscription' do + subject(:execute_service) { update_work_item } + end + context 'when child type is invalid' do let_it_be(:child_work_item) { create(:work_item, project: project) } @@ -351,6 +364,10 @@ RSpec.describe WorkItems::UpdateService, feature_category: :team_planning do update_work_item end + + it_behaves_like 'update service that triggers GraphQL work_item_updated subscription' do + subject(:execute_service) { update_work_item } + end end context 'when milestone remains unchanged' do @@ -382,6 +399,10 @@ RSpec.describe WorkItems::UpdateService, feature_category: :team_planning do update_work_item end + it_behaves_like 'update service that triggers GraphQL work_item_updated subscription' do + subject(:execute_service) { update_work_item } + end + it_behaves_like 'broadcasting issuable labels updates' do let(:issuable) { work_item } let(:label_a) { label1 } @@ -392,7 +413,6 @@ RSpec.describe WorkItems::UpdateService, feature_category: :team_planning do container: project, current_user: current_user, params: update_params, - spam_params: spam_params, widget_params: widget_params ).execute(work_item) end |