diff options
Diffstat (limited to 'spec/services')
108 files changed, 3683 insertions, 913 deletions
diff --git a/spec/services/alert_management/process_prometheus_alert_service_spec.rb b/spec/services/alert_management/process_prometheus_alert_service_spec.rb index 8f81c1967d5..288a33b71cd 100644 --- a/spec/services/alert_management/process_prometheus_alert_service_spec.rb +++ b/spec/services/alert_management/process_prometheus_alert_service_spec.rb @@ -68,36 +68,29 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do let!(:alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: fingerprint) } it_behaves_like 'creates an alert management alert' + it_behaves_like 'Alert Notification Service sends notification email' end context 'existing alert is ignored' do let!(:alert) { create(:alert_management_alert, :ignored, project: project, fingerprint: fingerprint) } it_behaves_like 'adds an alert management alert event' + it_behaves_like 'Alert Notification Service sends no notifications' end - context 'two existing alerts, one resolved one open' do - let!(:resolved_alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: fingerprint) } - let!(:alert) { create(:alert_management_alert, project: project, fingerprint: fingerprint) } + context 'existing alert is acknowledged' do + let!(:alert) { create(:alert_management_alert, :acknowledged, project: project, fingerprint: fingerprint) } it_behaves_like 'adds an alert management alert event' + it_behaves_like 'Alert Notification Service sends no notifications' end - context 'when status change did not succeed' do - before do - allow(AlertManagement::Alert).to receive(:for_fingerprint).and_return([alert]) - allow(alert).to receive(:trigger).and_return(false) - end - - it 'writes a warning to the log' do - expect(Gitlab::AppLogger).to receive(:warn).with( - message: 'Unable to update AlertManagement::Alert status to triggered', - project_id: project.id, - alert_id: alert.id - ) + context 'two existing alerts, one resolved one open' do + let!(:resolved_alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: fingerprint) } + let!(:alert) { create(:alert_management_alert, project: project, fingerprint: fingerprint) } - execute - end + it_behaves_like 'adds an alert management alert event' + it_behaves_like 'Alert Notification Service sends notification email' end context 'when auto-creation of issues is disabled' do @@ -109,11 +102,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do context 'when emails are disabled' do let(:send_email) { false } - it 'does not send notification' do - expect(NotificationService).not_to receive(:new) - - expect(subject).to be_success - end + it_behaves_like 'Alert Notification Service sends no notifications' end end @@ -136,11 +125,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do context 'when emails are disabled' do let(:send_email) { false } - it 'does not send notification' do - expect(NotificationService).not_to receive(:new) - - expect(subject).to be_success - end + it_behaves_like 'Alert Notification Service sends no notifications' end end @@ -158,7 +143,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do it 'writes a warning to the log' do expect(Gitlab::AppLogger).to receive(:warn).with( - message: 'Unable to create AlertManagement::Alert', + message: 'Unable to create AlertManagement::Alert from Prometheus', project_id: project.id, alert_errors: { hosts: ['hosts array is over 255 chars'] } ) @@ -235,11 +220,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do context 'when emails are disabled' do let(:send_email) { false } - it 'does not send notification' do - expect(NotificationService).not_to receive(:new) - - expect(subject).to be_success - end + it_behaves_like 'Alert Notification Service sends no notifications' end end diff --git a/spec/services/application_settings/update_service_spec.rb b/spec/services/application_settings/update_service_spec.rb index 46483fede97..1352a595ba4 100644 --- a/spec/services/application_settings/update_service_spec.rb +++ b/spec/services/application_settings/update_service_spec.rb @@ -122,7 +122,7 @@ RSpec.describe ApplicationSettings::UpdateService do it_behaves_like 'invalidates markdown cache', { asset_proxy_enabled: true } it_behaves_like 'invalidates markdown cache', { asset_proxy_url: 'http://test.com' } it_behaves_like 'invalidates markdown cache', { asset_proxy_secret_key: 'another secret' } - it_behaves_like 'invalidates markdown cache', { asset_proxy_whitelist: ['domain.com'] } + it_behaves_like 'invalidates markdown cache', { asset_proxy_allowlist: ['domain.com'] } context 'when also setting the local_markdown_version' do let(:params) { { asset_proxy_enabled: true, local_markdown_version: 12 } } diff --git a/spec/services/authorized_project_update/recalculate_for_user_range_service_spec.rb b/spec/services/authorized_project_update/recalculate_for_user_range_service_spec.rb index a4637b6ba1c..0c944cad40c 100644 --- a/spec/services/authorized_project_update/recalculate_for_user_range_service_spec.rb +++ b/spec/services/authorized_project_update/recalculate_for_user_range_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe AuthorizedProjectUpdate::RecalculateForUserRangeService do it 'calls Users::RefreshAuthorizedProjectsService' do users.each do |user| expect(Users::RefreshAuthorizedProjectsService).to( - receive(:new).with(user).and_call_original) + receive(:new).with(user, source: described_class.name).and_call_original) end range = users.map(&:id).minmax diff --git a/spec/services/boards/lists/create_service_spec.rb b/spec/services/boards/lists/create_service_spec.rb index d639fdbb46a..cac26b3c88d 100644 --- a/spec/services/boards/lists/create_service_spec.rb +++ b/spec/services/boards/lists/create_service_spec.rb @@ -3,99 +3,23 @@ require 'spec_helper' RSpec.describe Boards::Lists::CreateService do - describe '#execute' do - shared_examples 'creating board lists' do - let_it_be(:user) { create(:user) } + context 'when board parent is a project' do + let_it_be(:parent) { create(:project) } + let_it_be(:board) { create(:board, project: parent) } + let_it_be(:label) { create(:label, project: parent, name: 'in-progress') } - before_all do - parent.add_developer(user) - end - - subject(:service) { described_class.new(parent, user, label_id: label.id) } - - context 'when board lists is empty' do - it 'creates a new list at beginning of the list' do - response = service.execute(board) - - expect(response.success?).to eq(true) - expect(response.payload[:list].position).to eq 0 - end - end - - context 'when board lists has the done list' do - it 'creates a new list at beginning of the list' do - response = service.execute(board) - - expect(response.success?).to eq(true) - expect(response.payload[:list].position).to eq 0 - end - end - - context 'when board lists has labels lists' do - it 'creates a new list at end of the lists' do - create(:list, board: board, position: 0) - create(:list, board: board, position: 1) - - response = service.execute(board) - - expect(response.success?).to eq(true) - expect(response.payload[:list].position).to eq 2 - end - end - - context 'when board lists has label and done lists' do - it 'creates a new list at end of the label lists' do - list1 = create(:list, board: board, position: 0) - - list2 = service.execute(board).payload[:list] - - expect(list1.reload.position).to eq 0 - expect(list2.reload.position).to eq 1 - end - end - - context 'when provided label does not belong to the parent' do - it 'returns an error' do - label = create(:label, name: 'in-development') - service = described_class.new(parent, user, label_id: label.id) - - response = service.execute(board) - - expect(response.success?).to eq(false) - expect(response.errors).to include('Label not found') - end - end - - context 'when backlog param is sent' do - it 'creates one and only one backlog list' do - service = described_class.new(parent, user, 'backlog' => true) - list = service.execute(board).payload[:list] - - expect(list.list_type).to eq('backlog') - expect(list.position).to be_nil - expect(list).to be_valid - - another_backlog = service.execute(board).payload[:list] - - expect(another_backlog).to eq list - end - end - end - - context 'when board parent is a project' do - let_it_be(:parent) { create(:project) } - let_it_be(:board) { create(:board, project: parent) } - let_it_be(:label) { create(:label, project: parent, name: 'in-progress') } + it_behaves_like 'board lists create service' + end - it_behaves_like 'creating board lists' - end + context 'when board parent is a group' do + let_it_be(:parent) { create(:group) } + let_it_be(:board) { create(:board, group: parent) } + let_it_be(:label) { create(:group_label, group: parent, name: 'in-progress') } - context 'when board parent is a group' do - let_it_be(:parent) { create(:group) } - let_it_be(:board) { create(:board, group: parent) } - let_it_be(:label) { create(:group_label, group: parent, name: 'in-progress') } + it_behaves_like 'board lists create service' + end - it_behaves_like 'creating board lists' - end + def create_list(params) + create(:list, params.merge(board: board)) end end diff --git a/spec/services/bulk_create_integration_service_spec.rb b/spec/services/bulk_create_integration_service_spec.rb index 674382ee14f..3ac993972c6 100644 --- a/spec/services/bulk_create_integration_service_spec.rb +++ b/spec/services/bulk_create_integration_service_spec.rb @@ -43,46 +43,6 @@ RSpec.describe BulkCreateIntegrationService do end end - shared_examples 'updates project callbacks' do - it 'updates projects#has_external_issue_tracker for issue tracker services' do - described_class.new(integration, batch, association).execute - - expect(project.reload.has_external_issue_tracker).to eq(true) - expect(excluded_project.reload.has_external_issue_tracker).to eq(false) - end - - context 'with an external wiki integration' do - before do - integration.update!(category: 'common', type: 'ExternalWikiService') - end - - it 'updates projects#has_external_wiki for external wiki services' do - described_class.new(integration, batch, association).execute - - expect(project.reload.has_external_wiki).to eq(true) - expect(excluded_project.reload.has_external_wiki).to eq(false) - end - end - end - - shared_examples 'does not update project callbacks' do - it 'does not update projects#has_external_issue_tracker for issue tracker services' do - described_class.new(integration, batch, association).execute - - expect(project.reload.has_external_issue_tracker).to eq(false) - end - - context 'with an inactive external wiki integration' do - let(:integration) { create(:external_wiki_service, :instance, active: false) } - - it 'does not update projects#has_external_wiki for external wiki services' do - described_class.new(integration, batch, association).execute - - expect(project.reload.has_external_wiki).to eq(false) - end - end - end - context 'passing an instance-level integration' do let(:integration) { instance_integration } let(:inherit_from_id) { integration.id } @@ -95,15 +55,6 @@ RSpec.describe BulkCreateIntegrationService do it_behaves_like 'creates integration from batch ids' it_behaves_like 'updates inherit_from_id' - it_behaves_like 'updates project callbacks' - - context 'when integration is not active' do - before do - integration.update!(active: false) - end - - it_behaves_like 'does not update project callbacks' - end end context 'with a group association' do @@ -130,7 +81,6 @@ RSpec.describe BulkCreateIntegrationService do it_behaves_like 'creates integration from batch ids' it_behaves_like 'updates inherit_from_id' - it_behaves_like 'updates project callbacks' end context 'with a group association' do @@ -157,7 +107,6 @@ RSpec.describe BulkCreateIntegrationService do let(:inherit_from_id) { integration.id } it_behaves_like 'creates integration from batch ids' - it_behaves_like 'updates project callbacks' end end end diff --git a/spec/services/captcha/captcha_verification_service_spec.rb b/spec/services/captcha/captcha_verification_service_spec.rb new file mode 100644 index 00000000000..245e06703f5 --- /dev/null +++ b/spec/services/captcha/captcha_verification_service_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Captcha::CaptchaVerificationService do + describe '#execute' do + let(:captcha_response) { nil } + let(:request) { double(:request) } + let(:service) { described_class.new } + + subject { service.execute(captcha_response: captcha_response, request: request) } + + context 'when there is no captcha_response' do + it 'returns false' do + expect(subject).to eq(false) + end + end + + context 'when there is a captcha_response' do + let(:captcha_response) { 'abc123' } + + before do + expect(Gitlab::Recaptcha).to receive(:load_configurations!) + end + + it 'returns false' do + expect(service).to receive(:verify_recaptcha).with(response: captcha_response) { true } + + expect(subject).to eq(true) + end + + it 'has a request method which returns the request' do + subject + + expect(service.send(:request)).to eq(request) + end + end + end +end diff --git a/spec/services/ci/create_job_artifacts_service_spec.rb b/spec/services/ci/create_job_artifacts_service_spec.rb index 29e51a23dea..1efd1d390a2 100644 --- a/spec/services/ci/create_job_artifacts_service_spec.rb +++ b/spec/services/ci/create_job_artifacts_service_spec.rb @@ -27,6 +27,14 @@ RSpec.describe Ci::CreateJobArtifactsService do UploadedFile.new(upload.path, **params) end + def unique_metrics_report_uploaders + Gitlab::UsageDataCounters::HLLRedisCounter.unique_events( + event_names: described_class::METRICS_REPORT_UPLOAD_EVENT_NAME, + start_date: 2.weeks.ago, + end_date: 2.weeks.from_now + ) + end + describe '#execute' do subject { service.execute(artifacts_file, params, metadata_file: metadata_file) } @@ -42,6 +50,12 @@ RSpec.describe Ci::CreateJobArtifactsService do expect(new_artifact.file_sha256).to eq(artifacts_sha256) end + it 'does not track the job user_id' do + subject + + expect(unique_metrics_report_uploaders).to eq(0) + end + context 'when metadata file is also uploaded' do let(:metadata_file) do file_to_upload('spec/fixtures/ci_build_artifacts_metadata.gz', sha256: artifacts_sha256) @@ -174,6 +188,20 @@ RSpec.describe Ci::CreateJobArtifactsService do end end + context 'when artifact_type is metrics' do + before do + allow(job).to receive(:user_id).and_return(123) + end + + let(:params) { { 'artifact_type' => 'metrics', 'artifact_format' => 'gzip' }.with_indifferent_access } + + it 'tracks the job user_id' do + subject + + expect(unique_metrics_report_uploaders).to eq(1) + end + end + context 'when artifact type is cluster_applications' do let(:artifacts_file) do file_to_upload('spec/fixtures/helm/helm_list_v2_prometheus_missing.json.gz', sha256: artifacts_sha256) diff --git a/spec/services/ci/create_pipeline_service/cross_project_pipeline_spec.rb b/spec/services/ci/create_pipeline_service/cross_project_pipeline_spec.rb new file mode 100644 index 00000000000..9cf66dfceb0 --- /dev/null +++ b/spec/services/ci/create_pipeline_service/cross_project_pipeline_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::CreatePipelineService, '#execute' do + let_it_be(:group) { create(:group, name: 'my-organization') } + let(:upstream_project) { create(:project, :repository, name: 'upstream', group: group) } + let(:downstram_project) { create(:project, :repository, name: 'downstream', group: group) } + let(:user) { create(:user) } + + let(:service) do + described_class.new(upstream_project, user, ref: 'master') + end + + before do + upstream_project.add_developer(user) + downstram_project.add_developer(user) + create_gitlab_ci_yml(upstream_project, upstream_config) + create_gitlab_ci_yml(downstram_project, downstream_config) + end + + context 'with resource group', :aggregate_failures do + let(:upstream_config) do + <<~YAML + instrumentation_test: + stage: test + resource_group: iOS + trigger: + project: my-organization/downstream + strategy: depend + YAML + end + + let(:downstream_config) do + <<~YAML + test: + script: echo "Testing..." + YAML + end + + it 'creates bridge job with resource group' do + pipeline = create_pipeline! + + test = pipeline.statuses.find_by(name: 'instrumentation_test') + expect(pipeline).to be_created_successfully + expect(pipeline.triggered_pipelines).not_to be_exist + expect(upstream_project.resource_groups.count).to eq(1) + expect(test).to be_a Ci::Bridge + expect(test).to be_waiting_for_resource + expect(test.resource_group.key).to eq('iOS') + end + + context 'when sidekiq processes the job', :sidekiq_inline do + it 'transitions to pending status and triggers a downstream pipeline' do + pipeline = create_pipeline! + + test = pipeline.statuses.find_by(name: 'instrumentation_test') + expect(test).to be_pending + expect(pipeline.triggered_pipelines.count).to eq(1) + end + + context 'when the resource is occupied by the other bridge' do + before do + resource_group = create(:ci_resource_group, project: upstream_project, key: 'iOS') + resource_group.assign_resource_to(create(:ci_build, project: upstream_project)) + end + + it 'stays waiting for resource' do + pipeline = create_pipeline! + + test = pipeline.statuses.find_by(name: 'instrumentation_test') + expect(test).to be_waiting_for_resource + expect(pipeline.triggered_pipelines.count).to eq(0) + end + end + end + end + + def create_pipeline! + service.execute(:push) + end + + def create_gitlab_ci_yml(project, content) + project.repository.create_file(user, '.gitlab-ci.yml', content, branch_name: 'master', message: 'test') + end +end diff --git a/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb b/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb new file mode 100644 index 00000000000..4cf52223e38 --- /dev/null +++ b/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Ci::CreatePipelineService do + describe '!reference tags' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { project.owner } + + let(:ref) { 'refs/heads/master' } + let(:source) { :push } + let(:service) { described_class.new(project, user, { ref: ref }) } + let(:pipeline) { service.execute(source) } + + before do + stub_ci_pipeline_yaml_file(config) + end + + context 'with valid config' do + let(:config) do + <<~YAML + .job-1: + script: + - echo doing step 1 of job 1 + + .job-2: + before_script: + - ls + script: !reference [.job-1, script] + + job: + before_script: !reference [.job-2, before_script] + script: + - echo doing my first step + - !reference [.job-2, script] + - echo doing my last step + YAML + end + + it 'creates a pipeline' do + expect(pipeline).to be_persisted + expect(pipeline.builds.first.options).to match(a_hash_including({ + 'before_script' => ['ls'], + 'script' => [ + 'echo doing my first step', + 'echo doing step 1 of job 1', + 'echo doing my last step' + ] + })) + end + end + + context 'with invalid config' do + let(:config) do + <<~YAML + job-1: + script: + - echo doing step 1 of job 1 + - !reference [job-3, script] + + job-2: + script: + - echo doing step 1 of job 2 + - !reference [job-3, script] + + job-3: + script: + - echo doing step 1 of job 3 + - !reference [job-1, script] + YAML + end + + it 'creates a pipeline without builds' do + expect(pipeline).to be_persisted + expect(pipeline.builds).to be_empty + expect(pipeline.yaml_errors).to eq("!reference [\"job-3\", \"script\"] is part of a circular chain") + end + end + end +end diff --git a/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb b/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb index 8df9b0c3e60..a3818937113 100644 --- a/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb +++ b/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb @@ -76,6 +76,56 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do } end end + + context 'with resource group' do + let(:config) do + <<~YAML + instrumentation_test: + stage: test + resource_group: iOS + trigger: + include: path/to/child.yml + strategy: depend + YAML + end + + it 'creates bridge job with resource group', :aggregate_failures do + pipeline = create_pipeline! + + test = pipeline.statuses.find_by(name: 'instrumentation_test') + expect(pipeline).to be_created_successfully + expect(pipeline.triggered_pipelines).not_to be_exist + expect(project.resource_groups.count).to eq(1) + expect(test).to be_a Ci::Bridge + expect(test).to be_waiting_for_resource + expect(test.resource_group.key).to eq('iOS') + end + + context 'when sidekiq processes the job', :sidekiq_inline do + it 'transitions to pending status and triggers a downstream pipeline' do + pipeline = create_pipeline! + + test = pipeline.statuses.find_by(name: 'instrumentation_test') + expect(test).to be_pending + expect(pipeline.triggered_pipelines.count).to eq(1) + end + + context 'when the resource is occupied by the other bridge' do + before do + resource_group = create(:ci_resource_group, project: project, key: 'iOS') + resource_group.assign_resource_to(create(:ci_build, project: project)) + end + + it 'stays waiting for resource' do + pipeline = create_pipeline! + + test = pipeline.statuses.find_by(name: 'instrumentation_test') + expect(test).to be_waiting_for_resource + expect(pipeline.triggered_pipelines.count).to eq(0) + end + end + end + end end describe 'child pipeline triggers' do diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index ac6c4c188e4..04ecac6a85a 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -145,20 +145,6 @@ RSpec.describe Ci::CreatePipelineService do expect(find_job('job-2').options.dig(:allow_failure_criteria)).to be_nil expect(find_job('job-3').options.dig(:allow_failure_criteria, :exit_codes)).to eq([42]) end - - context 'with ci_allow_failure_with_exit_codes disabled' do - before do - stub_feature_flags(ci_allow_failure_with_exit_codes: false) - end - - it 'does not persist allow_failure_criteria' do - expect(pipeline).to be_persisted - - expect(find_job('job-1').options.key?(:allow_failure_criteria)).to be_falsey - expect(find_job('job-2').options.key?(:allow_failure_criteria)).to be_falsey - expect(find_job('job-3').options.key?(:allow_failure_criteria)).to be_falsey - end - end end context 'if:' do diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index e1f1bdc41a1..1005985b3e4 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -102,7 +102,6 @@ RSpec.describe Ci::CreatePipelineService do describe 'recording a conversion event' do it 'schedules a record conversion event worker' do expect(Experiments::RecordConversionEventWorker).to receive(:perform_async).with(:ci_syntax_templates, user.id) - expect(Experiments::RecordConversionEventWorker).to receive(:perform_async).with(:pipelines_empty_state, user.id) pipeline end @@ -538,7 +537,7 @@ RSpec.describe Ci::CreatePipelineService do it 'pull it from Auto-DevOps' do pipeline = execute_service expect(pipeline).to be_auto_devops_source - expect(pipeline.builds.map(&:name)).to match_array(%w[build code_quality eslint-sast secret_detection_default_branch test]) + expect(pipeline.builds.map(&:name)).to match_array(%w[brakeman-sast build code_quality eslint-sast secret_detection_default_branch test]) end end @@ -952,9 +951,9 @@ RSpec.describe Ci::CreatePipelineService do expect(result).to be_persisted expect(deploy_job.resource_group.key).to eq(resource_group_key) expect(project.resource_groups.count).to eq(1) - expect(resource_group.builds.count).to eq(1) + expect(resource_group.processables.count).to eq(1) expect(resource_group.resources.count).to eq(1) - expect(resource_group.resources.first.build).to eq(nil) + expect(resource_group.resources.first.processable).to eq(nil) end context 'when resource group key includes predefined variables' do diff --git a/spec/services/ci/daily_build_group_report_result_service_spec.rb b/spec/services/ci/daily_build_group_report_result_service_spec.rb index e54f10cc4f4..e58a5de26a1 100644 --- a/spec/services/ci/daily_build_group_report_result_service_spec.rb +++ b/spec/services/ci/daily_build_group_report_result_service_spec.rb @@ -3,10 +3,12 @@ require 'spec_helper' RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do - let!(:pipeline) { create(:ci_pipeline, created_at: '2020-02-06 00:01:10') } - let!(:rspec_job) { create(:ci_build, pipeline: pipeline, name: '3/3 rspec', coverage: 80) } - let!(:karma_job) { create(:ci_build, pipeline: pipeline, name: '2/2 karma', coverage: 90) } - let!(:extra_job) { create(:ci_build, pipeline: pipeline, name: 'extra', coverage: nil) } + let_it_be(:group) { create(:group, :private) } + let_it_be(:pipeline) { create(:ci_pipeline, project: create(:project, group: group), created_at: '2020-02-06 00:01:10') } + let_it_be(:rspec_job) { create(:ci_build, pipeline: pipeline, name: 'rspec 3/3', coverage: 80) } + let_it_be(:karma_job) { create(:ci_build, pipeline: pipeline, name: 'karma 2/2', coverage: 90) } + let_it_be(:extra_job) { create(:ci_build, pipeline: pipeline, name: 'extra', coverage: nil) } + let(:coverages) { Ci::DailyBuildGroupReportResult.all } it 'creates daily code coverage record for each job in the pipeline that has coverage value' do @@ -19,7 +21,8 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do ref_path: pipeline.source_ref_path, group_name: rspec_job.group_name, data: { 'coverage' => rspec_job.coverage }, - date: pipeline.created_at.to_date + date: pipeline.created_at.to_date, + group_id: group.id ) end @@ -30,7 +33,8 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do ref_path: pipeline.source_ref_path, group_name: karma_job.group_name, data: { 'coverage' => karma_job.coverage }, - date: pipeline.created_at.to_date + date: pipeline.created_at.to_date, + group_id: group.id ) end @@ -38,8 +42,8 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do end context 'when there are multiple builds with the same group name that report coverage' do - let!(:test_job_1) { create(:ci_build, pipeline: pipeline, name: '1/2 test', coverage: 70) } - let!(:test_job_2) { create(:ci_build, pipeline: pipeline, name: '2/2 test', coverage: 80) } + let!(:test_job_1) { create(:ci_build, pipeline: pipeline, name: 'test 1/2', coverage: 70) } + let!(:test_job_2) { create(:ci_build, pipeline: pipeline, name: 'test 2/2', coverage: 80) } it 'creates daily code coverage record with the average as the value' do described_class.new.execute(pipeline) @@ -67,8 +71,8 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do ) end - let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, name: '4/4 rspec', coverage: 84) } - let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, name: '3/3 karma', coverage: 92) } + let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, name: 'rspec 4/4', coverage: 84) } + let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, name: 'karma 3/3', coverage: 92) } before do # Create the existing daily code coverage records @@ -107,8 +111,8 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do ) end - let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, name: '4/4 rspec', coverage: 84) } - let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, name: '3/3 karma', coverage: 92) } + let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, name: 'rspec 4/4', coverage: 84) } + let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, name: 'karma 3/3', coverage: 92) } before do # Create the existing daily code coverage records diff --git a/spec/services/ci/generate_codequality_mr_diff_report_service_spec.rb b/spec/services/ci/generate_codequality_mr_diff_report_service_spec.rb new file mode 100644 index 00000000000..5d747a09f2a --- /dev/null +++ b/spec/services/ci/generate_codequality_mr_diff_report_service_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::GenerateCodequalityMrDiffReportService do + let(:service) { described_class.new(project) } + let(:project) { create(:project, :repository) } + + describe '#execute' do + subject { service.execute(base_pipeline, head_pipeline) } + + context 'when head pipeline has codequality mr diff report' do + let!(:merge_request) { create(:merge_request, :with_codequality_mr_diff_reports, source_project: project) } + let!(:service) { described_class.new(project, nil, id: merge_request.id) } + let!(:head_pipeline) { merge_request.head_pipeline } + let!(:base_pipeline) { nil } + + it 'returns status and data', :aggregate_failures do + expect_any_instance_of(Ci::PipelineArtifact) do |instance| + expect(instance).to receive(:present) + expect(instance).to receive(:for_files).with(merge_request.new_paths).and_call_original + end + + expect(subject[:status]).to eq(:parsed) + expect(subject[:data]).to eq(files: {}) + end + end + + context 'when head pipeline does not have a codequality mr diff report' do + let!(:merge_request) { create(:merge_request, source_project: project) } + let!(:service) { described_class.new(project, nil, id: merge_request.id) } + let!(:head_pipeline) { merge_request.head_pipeline } + let!(:base_pipeline) { nil } + + it 'returns status and error message' do + expect(subject[:status]).to eq(:error) + expect(subject[:status_reason]).to include('An error occurred while fetching codequality mr diff reports.') + end + end + + context 'when head pipeline has codequality mr diff report and no merge request associated' do + let!(:head_pipeline) { create(:ci_pipeline, :with_codequality_mr_diff_report, project: project) } + let!(:base_pipeline) { nil } + + it 'returns status and error message' do + expect(subject[:status]).to eq(:error) + expect(subject[:status_reason]).to include('An error occurred while fetching codequality mr diff reports.') + end + end + end +end diff --git a/spec/services/ci/pipeline_artifacts/create_code_quality_mr_diff_report_service_spec.rb b/spec/services/ci/pipeline_artifacts/create_code_quality_mr_diff_report_service_spec.rb new file mode 100644 index 00000000000..0c48f15d726 --- /dev/null +++ b/spec/services/ci/pipeline_artifacts/create_code_quality_mr_diff_report_service_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::PipelineArtifacts::CreateCodeQualityMrDiffReportService do + describe '#execute' do + subject(:pipeline_artifact) { described_class.new.execute(pipeline) } + + context 'when pipeline has codequality reports' do + let(:project) { create(:project, :repository) } + + describe 'pipeline completed status' do + using RSpec::Parameterized::TableSyntax + + where(:status, :result) do + :success | 1 + :failed | 1 + :canceled | 1 + :skipped | 1 + end + + with_them do + let(:pipeline) { create(:ci_pipeline, :with_codequality_reports, status: status, project: project) } + + it 'creates a pipeline artifact' do + expect { pipeline_artifact }.to change(Ci::PipelineArtifact, :count).by(result) + end + + it 'persists the default file name' do + expect(pipeline_artifact.file.filename).to eq('code_quality_mr_diff.json') + end + + it 'sets expire_at to 1 week' do + freeze_time do + expect(pipeline_artifact.expire_at).to eq(1.week.from_now) + end + end + end + end + + context 'when pipeline artifact has already been created' do + let(:pipeline) { create(:ci_pipeline, :with_codequality_reports, project: project) } + + it 'does not persist the same artifact twice' do + 2.times { described_class.new.execute(pipeline) } + + expect(Ci::PipelineArtifact.count).to eq(1) + end + end + end + + context 'when pipeline is not completed and codequality report does not exist' do + let(:pipeline) { create(:ci_pipeline, :running) } + + it 'does not persist data' do + pipeline_artifact + + expect(Ci::PipelineArtifact.count).to eq(0) + end + end + end +end diff --git a/spec/services/ci/pipeline_trigger_service_spec.rb b/spec/services/ci/pipeline_trigger_service_spec.rb index 0cc66e67b91..89d3da89011 100644 --- a/spec/services/ci/pipeline_trigger_service_spec.rb +++ b/spec/services/ci/pipeline_trigger_service_spec.rb @@ -45,6 +45,27 @@ RSpec.describe Ci::PipelineTriggerService do expect(result[:status]).to eq(:success) end + it 'stores the payload as a variable' do + expect { result }.to change { Ci::PipelineVariable.count }.by(1) + + var = result[:pipeline].variables.first + + expect(var.key).to eq('TRIGGER_PAYLOAD') + expect(var.value).to eq('{"ref":"master","variables":null}') + expect(var.variable_type).to eq('file') + end + + context 'when FF ci_trigger_payload_into_pipeline is disabled' do + before do + stub_feature_flags(ci_trigger_payload_into_pipeline: false) + end + + it 'does not store the payload as a variable' do + expect { result }.not_to change { Ci::PipelineVariable.count } + expect(result[:pipeline].variables).to be_empty + end + end + context 'when commit message has [ci skip]' do before do allow_next(Ci::Pipeline).to receive(:git_commit_message) { '[ci skip]' } @@ -60,8 +81,8 @@ RSpec.describe Ci::PipelineTriggerService do let(:params) { { token: trigger.token, ref: 'master', variables: variables } } let(:variables) { { 'AAA' => 'AAA123' } } - it 'has a variable' do - expect { result }.to change { Ci::PipelineVariable.count }.by(1) + it 'has variables' do + expect { result }.to change { Ci::PipelineVariable.count }.by(2) .and change { Ci::TriggerRequest.count }.by(1) expect(result[:pipeline].variables.map { |v| { v.key => v.value } }.first).to eq(variables) expect(result[:pipeline].trigger_requests.last.variables).to be_nil @@ -155,8 +176,8 @@ RSpec.describe Ci::PipelineTriggerService do let(:params) { { token: job.token, ref: 'master', variables: variables } } let(:variables) { { 'AAA' => 'AAA123' } } - it 'has a variable' do - expect { result }.to change { Ci::PipelineVariable.count }.by(1) + it 'has variables' do + expect { result }.to change { Ci::PipelineVariable.count }.by(2) .and change { Ci::Sources::Pipeline.count }.by(1) expect(result[:pipeline].variables.map { |v| { v.key => v.value } }.first).to eq(variables) expect(job.sourced_pipelines.last.pipeline_id).to eq(result[:pipeline].id) diff --git a/spec/services/ci/process_build_service_spec.rb b/spec/services/ci/process_build_service_spec.rb index 6d2af81a6e8..42a92504839 100644 --- a/spec/services/ci/process_build_service_spec.rb +++ b/spec/services/ci/process_build_service_spec.rb @@ -146,9 +146,11 @@ RSpec.describe Ci::ProcessBuildService, '#execute' do end end - context 'when FF skip_dag_manual_and_delayed_jobs is disabled' do + context 'when FF skip_dag_manual_and_delayed_jobs is disabled on the project' do + let_it_be(:other_project) { create(:project) } + before do - stub_feature_flags(skip_dag_manual_and_delayed_jobs: false) + stub_feature_flags(skip_dag_manual_and_delayed_jobs: other_project) end where(:build_when, :current_status, :after_status) do diff --git a/spec/services/ci/process_pipeline_service_spec.rb b/spec/services/ci/process_pipeline_service_spec.rb index a7889f0644d..d316c9a262b 100644 --- a/spec/services/ci/process_pipeline_service_spec.rb +++ b/spec/services/ci/process_pipeline_service_spec.rb @@ -50,6 +50,35 @@ RSpec.describe Ci::ProcessPipelineService do expect(all_builds.retried).to contain_exactly(build_retried) end + context 'counter ci_legacy_update_jobs_as_retried_total' do + let(:counter) { double(increment: true) } + + before do + allow(Gitlab::Metrics).to receive(:counter).and_call_original + allow(Gitlab::Metrics).to receive(:counter) + .with(:ci_legacy_update_jobs_as_retried_total, anything) + .and_return(counter) + end + + it 'increments the counter' do + expect(counter).to receive(:increment) + + subject.execute + end + + context 'when the previous build has already retried column true' do + before do + build_retried.update_columns(retried: true) + end + + it 'does not increment the counter' do + expect(counter).not_to receive(:increment) + + subject.execute + end + end + end + def create_build(name, **opts) create(:ci_build, :created, pipeline: pipeline, name: name, **opts) end diff --git a/spec/services/ci/prometheus_metrics/observe_histograms_service_spec.rb b/spec/services/ci/prometheus_metrics/observe_histograms_service_spec.rb new file mode 100644 index 00000000000..2eef852b0f4 --- /dev/null +++ b/spec/services/ci/prometheus_metrics/observe_histograms_service_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::PrometheusMetrics::ObserveHistogramsService do + let_it_be(:project) { create(:project) } + let(:params) { {} } + + subject(:execute) { described_class.new(project, params).execute } + + before do + Gitlab::Metrics.reset_registry! + end + + context 'with empty data' do + it 'does not raise errors' do + is_expected.to be_success + end + end + + context 'observes metrics successfully' do + let(:params) do + { + histograms: [ + { name: 'pipeline_graph_link_calculation_duration_seconds', value: '1' }, + { name: 'pipeline_graph_links_per_job_ratio', value: '0.9' } + ] + } + end + + it 'increments the metrics' do + execute + + expect(histogram_data).to match(a_hash_including({ 0.8 => 0.0, 1 => 1.0, 2 => 1.0 })) + + expect(histogram_data(:pipeline_graph_links_per_job_ratio)) + .to match(a_hash_including({ 0.8 => 0.0, 0.9 => 1.0, 1 => 1.0 })) + end + + it 'returns an empty body and status code' do + is_expected.to be_success + expect(subject.http_status).to eq(:created) + expect(subject.payload).to eq({}) + end + end + + context 'with unknown histograms' do + let(:params) do + { histograms: [{ name: 'chunky_bacon', value: '4' }] } + end + + it 'raises ActiveRecord::RecordNotFound error' do + expect { subject }.to raise_error ActiveRecord::RecordNotFound + end + end + + context 'with feature flag disabled' do + before do + stub_feature_flags(ci_accept_frontend_prometheus_metrics: false) + end + + let(:params) do + { + histograms: [ + { name: 'pipeline_graph_link_calculation_duration_seconds', value: '4' } + ] + } + end + + it 'does not register the metrics' do + execute + + expect(histogram_data).to be_nil + end + + it 'returns an empty body and status code' do + is_expected.to be_success + expect(subject.http_status).to eq(:accepted) + expect(subject.payload).to eq({}) + end + end + + def histogram_data(name = :pipeline_graph_link_calculation_duration_seconds) + Gitlab::Metrics.registry.get(name)&.get({}) + end +end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 0cdc8d2c870..88770c8095b 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -455,7 +455,7 @@ module Ci end before do - stub_feature_flags(ci_disable_validates_dependencies: false) + stub_feature_flags(ci_validate_build_dependencies_override: false) end let!(:pre_stage_job) { create(:ci_build, :success, pipeline: pipeline, name: 'test', stage_idx: 0) } @@ -470,15 +470,31 @@ module Ci context 'when validates for dependencies is enabled' do before do - stub_feature_flags(ci_disable_validates_dependencies: false) + stub_feature_flags(ci_validate_build_dependencies_override: false) end it_behaves_like 'validation is active' + + context 'when the main feature flag is enabled for a specific project' do + before do + stub_feature_flags(ci_validate_build_dependencies: pipeline.project) + end + + it_behaves_like 'validation is active' + end + + context 'when the main feature flag is enabled for a different project' do + before do + stub_feature_flags(ci_validate_build_dependencies: create(:project)) + end + + it_behaves_like 'validation is not active' + end end context 'when validates for dependencies is disabled' do before do - stub_feature_flags(ci_disable_validates_dependencies: true) + stub_feature_flags(ci_validate_build_dependencies_override: true) end it_behaves_like 'validation is not active' diff --git a/spec/services/container_expiration_policies/cleanup_service_spec.rb b/spec/services/container_expiration_policies/cleanup_service_spec.rb index 34f69d24141..746e3464427 100644 --- a/spec/services/container_expiration_policies/cleanup_service_spec.rb +++ b/spec/services/container_expiration_policies/cleanup_service_spec.rb @@ -61,7 +61,8 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do original_size: 1000, before_truncate_size: 800, after_truncate_size: 200, - before_delete_size: 100 + before_delete_size: 100, + deleted_size: 100 } end @@ -77,7 +78,8 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do cleanup_tags_service_original_size: 1000, cleanup_tags_service_before_truncate_size: 800, cleanup_tags_service_after_truncate_size: 200, - cleanup_tags_service_before_delete_size: 100 + cleanup_tags_service_before_delete_size: 100, + cleanup_tags_service_deleted_size: 100 ) expect(ContainerRepository.waiting_for_cleanup.count).to eq(1) expect(repository.reload.cleanup_unfinished?).to be_truthy @@ -97,5 +99,21 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do expect(response.success?).to eq(false) end end + + context 'with a network error' do + before do + expect(Projects::ContainerRepository::CleanupTagsService) + .to receive(:new).and_raise(Faraday::TimeoutError) + end + + it 'raises an error' do + expect { subject }.to raise_error(Faraday::TimeoutError) + + expect(ContainerRepository.waiting_for_cleanup.count).to eq(1) + expect(repository.reload.cleanup_unfinished?).to be_truthy + expect(repository.expiration_policy_started_at).not_to eq(nil) + expect(repository.expiration_policy_completed_at).to eq(nil) + end + end end end diff --git a/spec/services/deployments/create_service_spec.rb b/spec/services/deployments/create_service_spec.rb index 2d157c9d114..0bb5949ddb1 100644 --- a/spec/services/deployments/create_service_spec.rb +++ b/spec/services/deployments/create_service_spec.rb @@ -41,6 +41,27 @@ RSpec.describe Deployments::CreateService do expect(service.execute).to be_persisted end + + context 'when the last deployment has the same parameters' do + let(:params) do + { + sha: 'b83d6e391c22777fca1ed3012fce84f633d7fed0', + ref: 'master', + tag: false, + status: 'success' + } + end + + it 'does not create a new deployment' do + described_class.new(environment, user, params).execute + + expect(Deployments::UpdateEnvironmentWorker).not_to receive(:perform_async) + expect(Deployments::LinkMergeRequestWorker).not_to receive(:perform_async) + expect(Deployments::ExecuteHooksWorker).not_to receive(:perform_async) + + described_class.new(environment.reload, user, params).execute + end + end end describe '#deployment_attributes' do diff --git a/spec/services/design_management/move_designs_service_spec.rb b/spec/services/design_management/move_designs_service_spec.rb index a43f0a2f805..c8abce77325 100644 --- a/spec/services/design_management/move_designs_service_spec.rb +++ b/spec/services/design_management/move_designs_service_spec.rb @@ -76,18 +76,6 @@ RSpec.describe DesignManagement::MoveDesignsService do end end - context 'the designs are not adjacent' do - let(:current_design) { designs.first } - let(:previous_design) { designs.second } - let(:next_design) { designs.third } - - it 'raises not_adjacent' do - create(:design, issue: issue, relative_position: next_design.relative_position - 1) - - expect(subject).to be_error.and(have_attributes(message: :not_adjacent)) - end - end - context 'moving a design with neighbours' do let(:current_design) { designs.first } let(:previous_design) { designs.second } diff --git a/spec/services/discussions/resolve_service_spec.rb b/spec/services/discussions/resolve_service_spec.rb index 42c4ef52741..2e30455eb0a 100644 --- a/spec/services/discussions/resolve_service_spec.rb +++ b/spec/services/discussions/resolve_service_spec.rb @@ -24,6 +24,13 @@ RSpec.describe Discussions::ResolveService do expect(discussion).to be_resolved end + it 'tracks thread resolve usage data' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_resolve_thread_action).with(user: user) + + service.execute + end + it 'executes the notification service' do expect_next_instance_of(MergeRequests::ResolvedDiscussionNotificationService) do |instance| expect(instance).to receive(:execute).with(discussion.noteable) @@ -101,6 +108,13 @@ RSpec.describe Discussions::ResolveService do service.execute end + it 'does not track thread resolve usage data' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .not_to receive(:track_resolve_thread_action).with(user: user) + + service.execute + end + it 'does not schedule an auto-merge' do expect(AutoMergeProcessWorker).not_to receive(:perform_async) diff --git a/spec/services/discussions/unresolve_service_spec.rb b/spec/services/discussions/unresolve_service_spec.rb new file mode 100644 index 00000000000..6298a00a474 --- /dev/null +++ b/spec/services/discussions/unresolve_service_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe Discussions::UnresolveService do + describe "#execute" do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user, developer_projects: [project]) } + let_it_be(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds, source_project: project) } + let(:discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion } + + let(:service) { described_class.new(discussion, user) } + + before do + project.add_developer(user) + discussion.resolve!(user) + end + + it "unresolves the discussion" do + service.execute + + expect(discussion).not_to be_resolved + end + + it "counts the unresolve event" do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_unresolve_thread_action).with(user: user) + + service.execute + end + end +end diff --git a/spec/services/feature_flags/create_service_spec.rb b/spec/services/feature_flags/create_service_spec.rb index e115d8098c9..128fab114fe 100644 --- a/spec/services/feature_flags/create_service_spec.rb +++ b/spec/services/feature_flags/create_service_spec.rb @@ -66,18 +66,6 @@ RSpec.describe FeatureFlags::CreateService do subject end - context 'the feature flag is disabled' do - before do - stub_feature_flags(jira_sync_feature_flags: false) - end - - it 'does not sync the feature flag to Jira' do - expect(::JiraConnect::SyncFeatureFlagsWorker).not_to receive(:perform_async) - - subject - end - end - it 'creates audit event' do expected_message = 'Created feature flag <strong>feature_flag</strong> '\ 'with description <strong>"description"</strong>. '\ diff --git a/spec/services/feature_flags/update_service_spec.rb b/spec/services/feature_flags/update_service_spec.rb index 8c4055ddd9e..9639cf3081d 100644 --- a/spec/services/feature_flags/update_service_spec.rb +++ b/spec/services/feature_flags/update_service_spec.rb @@ -26,18 +26,6 @@ RSpec.describe FeatureFlags::UpdateService do expect(subject[:status]).to eq(:success) end - context 'the feature flag is disabled' do - before do - stub_feature_flags(jira_sync_feature_flags: false) - end - - it 'does not sync the feature flag to Jira' do - expect(::JiraConnect::SyncFeatureFlagsWorker).not_to receive(:perform_async) - - subject - end - end - it 'syncs the feature flag to Jira' do expect(::JiraConnect::SyncFeatureFlagsWorker).to receive(:perform_async).with(Integer, Integer) diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb index a5290f0be68..52df21897b9 100644 --- a/spec/services/git/branch_hooks_service_spec.rb +++ b/spec/services/git/branch_hooks_service_spec.rb @@ -11,7 +11,8 @@ RSpec.describe Git::BranchHooksService do let(:branch) { project.default_branch } let(:ref) { "refs/heads/#{branch}" } - let(:commit) { project.commit(sample_commit.id) } + let(:commit_id) { sample_commit.id } + let(:commit) { project.commit(commit_id) } let(:oldrev) { commit.parent_id } let(:newrev) { commit.id } @@ -93,12 +94,12 @@ RSpec.describe Git::BranchHooksService do describe 'Push Event' do let(:event) { Event.pushed_action.first } - before do - service.execute - end + subject(:execute_service) { service.execute } context "with an existing branch" do it 'generates a push event with one commit' do + execute_service + expect(event).to be_an_instance_of(PushEvent) expect(event.project).to eq(project) expect(event).to be_pushed_action @@ -109,12 +110,87 @@ RSpec.describe Git::BranchHooksService do expect(event.push_event_payload.ref).to eq('master') expect(event.push_event_payload.commit_count).to eq(1) end + + context 'with changing CI config' do + before do + allow_next_instance_of(Gitlab::Git::Diff) do |diff| + allow(diff).to receive(:new_path).and_return('.gitlab-ci.yml') + end + + allow(Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event) + end + + let!(:commit_author) { create(:user, email: sample_commit.author_email) } + + let(:tracking_params) do + ['o_pipeline_authoring_unique_users_committing_ciconfigfile', values: commit_author.id] + end + + it 'tracks the event' do + execute_service + + expect(Gitlab::UsageDataCounters::HLLRedisCounter) + .to have_received(:track_event).with(*tracking_params) + end + + context 'when the FF usage_data_unique_users_committing_ciconfigfile is disabled' do + before do + stub_feature_flags(usage_data_unique_users_committing_ciconfigfile: false) + end + + it 'does not track the event' do + execute_service + + expect(Gitlab::UsageDataCounters::HLLRedisCounter) + .not_to have_received(:track_event).with(*tracking_params) + end + end + + context 'when usage ping is disabled' do + before do + stub_application_setting(usage_ping_enabled: false) + end + + it 'does not track the event' do + execute_service + + expect(Gitlab::UsageDataCounters::HLLRedisCounter) + .not_to have_received(:track_event).with(*tracking_params) + end + end + + context 'when the branch is not the main branch' do + let(:branch) { 'feature' } + + it 'does not track the event' do + execute_service + + expect(Gitlab::UsageDataCounters::HLLRedisCounter) + .not_to have_received(:track_event).with(*tracking_params) + end + end + + context 'when the CI config is a different path' do + before do + project.ci_config_path = 'config/ci.yml' + end + + it 'does not track the event' do + execute_service + + expect(Gitlab::UsageDataCounters::HLLRedisCounter) + .not_to have_received(:track_event).with(*tracking_params) + end + end + end end context "with a new branch" do let(:oldrev) { Gitlab::Git::BLANK_SHA } it 'generates a push event with more than one commit' do + execute_service + expect(event).to be_an_instance_of(PushEvent) expect(event.project).to eq(project) expect(event).to be_pushed_action @@ -131,6 +207,8 @@ RSpec.describe Git::BranchHooksService do let(:newrev) { Gitlab::Git::BLANK_SHA } it 'generates a push event with no commits' do + execute_service + expect(event).to be_an_instance_of(PushEvent) expect(event.project).to eq(project) expect(event).to be_pushed_action @@ -150,7 +228,6 @@ RSpec.describe Git::BranchHooksService do ) end - let(:commit) { project.repository.commit(commit_id) } let(:blank_sha) { Gitlab::Git::BLANK_SHA } def clears_cache(extended: []) @@ -431,11 +508,7 @@ RSpec.describe Git::BranchHooksService do end describe 'Metrics dashboard sync' do - context 'with feature flag enabled' do - before do - Feature.enable(:metrics_dashboards_sync) - end - + shared_examples 'trigger dashboard sync' do it 'imports metrics to database' do expect(Metrics::Dashboard::SyncDashboardsWorker).to receive(:perform_async) @@ -443,12 +516,95 @@ RSpec.describe Git::BranchHooksService do end end - context 'with feature flag disabled' do - it 'imports metrics to database' do - expect(Metrics::Dashboard::SyncDashboardsWorker).to receive(:perform_async) + 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.multi_action( + 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/wiki_push_service_spec.rb b/spec/services/git/wiki_push_service_spec.rb index cd38f2e97fb..df9a48d7b1c 100644 --- a/spec/services/git/wiki_push_service_spec.rb +++ b/spec/services/git/wiki_push_service_spec.rb @@ -257,6 +257,46 @@ RSpec.describe Git::WikiPushService, services: true do end end + describe '#perform_housekeeping', :clean_gitlab_redis_shared_state do + let(:housekeeping) { Repositories::HousekeepingService.new(wiki) } + + subject { create_service(current_sha).execute } + + before do + allow(Repositories::HousekeepingService).to receive(:new).and_return(housekeeping) + end + + it 'does not perform housekeeping when not needed' do + expect(housekeeping).not_to receive(:execute) + + subject + end + + context 'when housekeeping is needed' do + before do + allow(housekeeping).to receive(:needed?).and_return(true) + end + + it 'performs housekeeping' do + expect(housekeeping).to receive(:execute) + + subject + end + + it 'does not raise an exception' do + allow(housekeeping).to receive(:try_obtain_lease).and_return(false) + + expect { subject }.not_to raise_error + end + end + + it 'increments the push counter' do + expect(housekeeping).to receive(:increment!) + + subject + end + end + # In order to construct the correct GitPostReceive object that represents the # changes we are applying, we need to describe the changes between old-ref and # new-ref. Old ref (the base sha) we have to capture before we perform any diff --git a/spec/services/groups/import_export/export_service_spec.rb b/spec/services/groups/import_export/export_service_spec.rb index 690bcb94556..d6ce40f413b 100644 --- a/spec/services/groups/import_export/export_service_spec.rb +++ b/spec/services/groups/import_export/export_service_spec.rb @@ -71,6 +71,16 @@ RSpec.describe Groups::ImportExport::ExportService do service.execute end + it 'compresses and removes tmp files' do + expect(group.import_export_upload).to be_nil + expect(Gitlab::ImportExport::Saver).to receive(:new).and_call_original + + service.execute + + expect(Dir.exist?(shared.archive_path)).to eq false + expect(File.exist?(group.import_export_upload.export_file.path)).to eq true + end + it 'notifies the user' do expect_next_instance_of(NotificationService) do |instance| expect(instance).to receive(:group_was_exported) @@ -134,7 +144,7 @@ RSpec.describe Groups::ImportExport::ExportService do expect { service.execute }.to raise_error(Gitlab::ImportExport::Error) expect(group.import_export_upload).to be_nil - expect(Dir.exist?(shared.base_path)).to eq(false) + expect(Dir.exist?(shared.archive_path)).to eq(false) end it 'notifies the user about failed group export' do @@ -159,7 +169,7 @@ RSpec.describe Groups::ImportExport::ExportService do expect { service.execute }.to raise_error(Gitlab::ImportExport::Error) expect(group.import_export_upload).to be_nil - expect(Dir.exist?(shared.base_path)).to eq(false) + expect(Dir.exist?(shared.archive_path)).to eq(false) end it 'notifies logger' do diff --git a/spec/services/groups/open_issues_count_service_spec.rb b/spec/services/groups/open_issues_count_service_spec.rb new file mode 100644 index 00000000000..8bbb1c90c6b --- /dev/null +++ b/spec/services/groups/open_issues_count_service_spec.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_caching do + let_it_be(:group) { create(:group, :public)} + let_it_be(:project) { create(:project, :public, namespace: group) } + let_it_be(:user) { create(:user) } + let_it_be(:issue) { create(:issue, :opened, project: project) } + let_it_be(:confidential) { create(:issue, :opened, confidential: true, project: project) } + let_it_be(:closed) { create(:issue, :closed, project: project) } + + subject { described_class.new(group, user) } + + describe '#relation_for_count' do + before do + allow(IssuesFinder).to receive(:new).and_call_original + end + + it 'uses the IssuesFinder to scope issues' do + expect(IssuesFinder) + .to receive(:new) + .with(user, group_id: group.id, state: 'opened', non_archived: true, include_subgroups: true, public_only: true) + + subject.count + end + end + + describe '#count' do + context 'when user is nil' do + it 'does not include confidential issues in the issue count' do + expect(described_class.new(group).count).to eq(1) + end + end + + context 'when user is provided' do + context 'when user can read confidential issues' do + before do + group.add_reporter(user) + end + + it 'returns the right count with confidential issues' do + expect(subject.count).to eq(2) + end + end + + context 'when user cannot read confidential issues' do + before do + group.add_guest(user) + end + + it 'does not include confidential issues' do + expect(subject.count).to eq(1) + end + end + + context 'with different cache values' do + let(:public_count_key) { subject.cache_key(described_class::PUBLIC_COUNT_KEY) } + let(:under_threshold) { described_class::CACHED_COUNT_THRESHOLD - 1 } + let(:over_threshold) { described_class::CACHED_COUNT_THRESHOLD + 1 } + + context 'when cache is empty' do + before do + Rails.cache.delete(public_count_key) + end + + it 'refreshes cache if value over threshold' do + allow(subject).to receive(:uncached_count).and_return(over_threshold) + + expect(subject.count).to eq(over_threshold) + expect(Rails.cache.read(public_count_key)).to eq(over_threshold) + end + + it 'does not refresh cache if value under threshold' do + allow(subject).to receive(:uncached_count).and_return(under_threshold) + + expect(subject.count).to eq(under_threshold) + expect(Rails.cache.read(public_count_key)).to be_nil + end + end + + context 'when cached count is under the threshold value' do + before do + Rails.cache.write(public_count_key, under_threshold) + end + + it 'does not refresh cache' do + expect(Rails.cache).not_to receive(:write) + expect(subject.count).to eq(under_threshold) + end + end + + context 'when cached count is over the threshold value' do + before do + Rails.cache.write(public_count_key, over_threshold) + end + + it 'does not refresh cache' do + expect(Rails.cache).not_to receive(:write) + expect(subject.count).to eq(over_threshold) + end + end + end + end + end +end diff --git a/spec/services/integrations/test/project_service_spec.rb b/spec/services/integrations/test/project_service_spec.rb index dd603765d59..052b25b0f10 100644 --- a/spec/services/integrations/test/project_service_spec.rb +++ b/spec/services/integrations/test/project_service_spec.rb @@ -3,11 +3,12 @@ require 'spec_helper' RSpec.describe Integrations::Test::ProjectService do - let(:user) { double('user') } + include AfterNextHelpers describe '#execute' do - let(:project) { create(:project) } + let_it_be(:project) { create(:project) } let(:integration) { create(:slack_service, project: project) } + let(:user) { project.owner } let(:event) { nil } let(:sample_data) { { data: 'sample' } } let(:success_result) { { success: true, result: {} } } @@ -70,16 +71,17 @@ RSpec.describe Integrations::Test::ProjectService do end it 'executes integration' do - allow(project).to receive(:notes).and_return([Note.new]) + create(:note, project: project) + allow(Gitlab::DataBuilder::Note).to receive(:build).and_return(sample_data) + allow_next(NotesFinder).to receive(:execute).and_return(Note.all) expect(integration).to receive(:test).with(sample_data).and_return(success_result) expect(subject).to eq(success_result) end end - context 'issue' do - let(:event) { 'issue' } + shared_examples_for 'a test of an integration that operates on issues' do let(:issue) { build(:issue) } it 'returns error message if not enough data' do @@ -90,32 +92,28 @@ RSpec.describe Integrations::Test::ProjectService do it 'executes integration' do allow(project).to receive(:issues).and_return([issue]) allow(issue).to receive(:to_hook_data).and_return(sample_data) + allow_next(IssuesFinder).to receive(:execute).and_return([issue]) expect(integration).to receive(:test).with(sample_data).and_return(success_result) expect(subject).to eq(success_result) end end - context 'confidential_issue' do - let(:event) { 'confidential_issue' } - let(:issue) { build(:issue) } + context 'issue' do + let(:event) { 'issue' } - it 'returns error message if not enough data' do - expect(integration).not_to receive(:test) - expect(subject).to include({ status: :error, message: 'Ensure the project has issues.' }) - end + it_behaves_like 'a test of an integration that operates on issues' + end - it 'executes integration' do - allow(project).to receive(:issues).and_return([issue]) - allow(issue).to receive(:to_hook_data).and_return(sample_data) + context 'confidential_issue' do + let(:event) { 'confidential_issue' } - expect(integration).to receive(:test).with(sample_data).and_return(success_result) - expect(subject).to eq(success_result) - end + it_behaves_like 'a test of an integration that operates on issues' end context 'merge_request' do let(:event) { 'merge_request' } + let(:merge_request) { build(:merge_request) } it 'returns error message if not enough data' do expect(integration).not_to receive(:test) @@ -123,16 +121,17 @@ RSpec.describe Integrations::Test::ProjectService do end it 'executes integration' do - create(:merge_request, source_project: project) - allow_any_instance_of(MergeRequest).to receive(:to_hook_data).and_return(sample_data) + allow(merge_request).to receive(:to_hook_data).and_return(sample_data) + allow_next(MergeRequestsFinder).to receive(:execute).and_return([merge_request]) expect(integration).to receive(:test).with(sample_data).and_return(success_result) - expect(subject).to eq(success_result) + expect(subject).to include(success_result) end end context 'deployment' do - let(:project) { create(:project, :test_repo) } + let_it_be(:project) { create(:project, :test_repo) } + let(:deployment) { build(:deployment) } let(:event) { 'deployment' } it 'returns error message if not enough data' do @@ -141,8 +140,8 @@ RSpec.describe Integrations::Test::ProjectService do end it 'executes integration' do - create(:deployment, project: project) allow(Gitlab::DataBuilder::Deployment).to receive(:build).and_return(sample_data) + allow_next(DeploymentsFinder).to receive(:execute).and_return([deployment]) expect(integration).to receive(:test).with(sample_data).and_return(success_result) expect(subject).to eq(success_result) @@ -151,6 +150,7 @@ RSpec.describe Integrations::Test::ProjectService do context 'pipeline' do let(:event) { 'pipeline' } + let(:pipeline) { build(:ci_pipeline) } it 'returns error message if not enough data' do expect(integration).not_to receive(:test) @@ -158,8 +158,8 @@ RSpec.describe Integrations::Test::ProjectService do end it 'executes integration' do - create(:ci_empty_pipeline, project: project) allow(Gitlab::DataBuilder::Pipeline).to receive(:build).and_return(sample_data) + allow_next(Ci::PipelinesFinder).to receive(:execute).and_return([pipeline]) expect(integration).to receive(:test).with(sample_data).and_return(success_result) expect(subject).to eq(success_result) @@ -167,7 +167,7 @@ RSpec.describe Integrations::Test::ProjectService do end context 'wiki_page' do - let(:project) { create(:project, :wiki_repo) } + let_it_be(:project) { create(:project, :wiki_repo) } let(:event) { 'wiki_page' } it 'returns error message if wiki disabled' do diff --git a/spec/services/issue_rebalancing_service_spec.rb b/spec/services/issue_rebalancing_service_spec.rb index 94f594c8083..7b3d4213b24 100644 --- a/spec/services/issue_rebalancing_service_spec.rb +++ b/spec/services/issue_rebalancing_service_spec.rb @@ -32,70 +32,88 @@ RSpec.describe IssueRebalancingService do project.reload.issues.reorder(relative_position: :asc).to_a end - it 'rebalances a set of issues with clumps at the end and start' do - all_issues = start_clump + unclumped + end_clump.reverse - service = described_class.new(project.issues.first) + shared_examples 'IssueRebalancingService shared examples' do + it 'rebalances a set of issues with clumps at the end and start' do + all_issues = start_clump + unclumped + end_clump.reverse + service = described_class.new(project.issues.first) - expect { service.execute }.not_to change { issues_in_position_order.map(&:id) } + expect { service.execute }.not_to change { issues_in_position_order.map(&:id) } - all_issues.each(&:reset) + all_issues.each(&:reset) - gaps = all_issues.take(all_issues.count - 1).zip(all_issues.drop(1)).map do |a, b| - b.relative_position - a.relative_position + gaps = all_issues.take(all_issues.count - 1).zip(all_issues.drop(1)).map do |a, b| + b.relative_position - a.relative_position + end + + expect(gaps).to all(be > RelativePositioning::MIN_GAP) + expect(all_issues.first.relative_position).to be > (RelativePositioning::MIN_POSITION * 0.9999) + expect(all_issues.last.relative_position).to be < (RelativePositioning::MAX_POSITION * 0.9999) end - expect(gaps).to all(be > RelativePositioning::MIN_GAP) - expect(all_issues.first.relative_position).to be > (RelativePositioning::MIN_POSITION * 0.9999) - expect(all_issues.last.relative_position).to be < (RelativePositioning::MAX_POSITION * 0.9999) - end + it 'is idempotent' do + service = described_class.new(project.issues.first) - it 'is idempotent' do - service = described_class.new(project.issues.first) + expect do + service.execute + service.execute + end.not_to change { issues_in_position_order.map(&:id) } + end - expect do - service.execute - service.execute - end.not_to change { issues_in_position_order.map(&:id) } - end + it 'does nothing if the feature flag is disabled' do + stub_feature_flags(rebalance_issues: false) + issue = project.issues.first + issue.project + issue.project.group + old_pos = issue.relative_position - it 'does nothing if the feature flag is disabled' do - stub_feature_flags(rebalance_issues: false) - issue = project.issues.first - issue.project - issue.project.group - old_pos = issue.relative_position + service = described_class.new(issue) - service = described_class.new(issue) + expect { service.execute }.not_to exceed_query_limit(0) + expect(old_pos).to eq(issue.reload.relative_position) + end - expect { service.execute }.not_to exceed_query_limit(0) - expect(old_pos).to eq(issue.reload.relative_position) - end + it 'acts if the flag is enabled for the project' do + issue = create(:issue, project: project, author: user, relative_position: max_pos) + stub_feature_flags(rebalance_issues: issue.project) - it 'acts if the flag is enabled for the project' do - issue = create(:issue, project: project, author: user, relative_position: max_pos) - stub_feature_flags(rebalance_issues: issue.project) + service = described_class.new(issue) - service = described_class.new(issue) + expect { service.execute }.to change { issue.reload.relative_position } + end - expect { service.execute }.to change { issue.reload.relative_position } - end + it 'acts if the flag is enabled for the group' do + issue = create(:issue, project: project, author: user, relative_position: max_pos) + project.update!(group: create(:group)) + stub_feature_flags(rebalance_issues: issue.project.group) - it 'acts if the flag is enabled for the group' do - issue = create(:issue, project: project, author: user, relative_position: max_pos) - project.update!(group: create(:group)) - stub_feature_flags(rebalance_issues: issue.project.group) + service = described_class.new(issue) - service = described_class.new(issue) + expect { service.execute }.to change { issue.reload.relative_position } + end + + it 'aborts if there are too many issues' do + issue = project.issues.first + base = double(count: 10_001) - expect { service.execute }.to change { issue.reload.relative_position } + allow(Issue).to receive(:relative_positioning_query_base).with(issue).and_return(base) + + expect { described_class.new(issue).execute }.to raise_error(described_class::TooManyIssues) + end end - it 'aborts if there are too many issues' do - issue = project.issues.first - base = double(count: 10_001) + context 'when issue_rebalancing_optimization feature flag is on' do + before do + stub_feature_flags(issue_rebalancing_optimization: true) + end + + it_behaves_like 'IssueRebalancingService shared examples' + end - allow(Issue).to receive(:relative_positioning_query_base).with(issue).and_return(base) + context 'when issue_rebalancing_optimization feature flag is on' do + before do + stub_feature_flags(issue_rebalancing_optimization: false) + end - expect { described_class.new(issue).execute }.to raise_error(described_class::TooManyIssues) + it_behaves_like 'IssueRebalancingService shared examples' end end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index dc545f57d23..3cf45143594 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -84,6 +84,7 @@ RSpec.describe Issues::CloseService do let!(:external_issue_tracker) { create(:jira_service, project: project) } it 'closes the issue on the external issue tracker' do + project.reload expect(project.external_issue_tracker).to receive(:close_issue) described_class.new(project, user).close_issue(external_issue) @@ -94,6 +95,7 @@ RSpec.describe Issues::CloseService do let!(:external_issue_tracker) { create(:jira_service, project: project, active: false) } it 'does not close the issue on the external issue tracker' do + project.reload expect(project.external_issue_tracker).not_to receive(:close_issue) described_class.new(project, user).close_issue(external_issue) @@ -104,6 +106,7 @@ RSpec.describe Issues::CloseService do let!(:external_issue_tracker) { create(:bugzilla_service, project: project) } it 'does not close the issue on the external issue tracker' do + project.reload expect(project.external_issue_tracker).not_to receive(:close_issue) described_class.new(project, user).close_issue(external_issue) diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index cc6a49fc4cf..e42e9722297 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -452,162 +452,50 @@ RSpec.describe Issues::CreateService do end context 'checking spam' do - include_context 'includes Spam constants' + let(:request) { double(:request) } + let(:api) { true } + let(:captcha_response) { 'abc123' } + let(:spam_log_id) { 1 } - let(:title) { 'Legit issue' } - let(:description) { 'please fix' } - let(:opts) do + let(:params) do { - title: title, - description: description, - request: double(:request, env: {}) + title: 'Spam issue', + request: request, + api: api, + captcha_response: captcha_response, + spam_log_id: spam_log_id } end - subject { described_class.new(project, user, opts) } - - before do - stub_feature_flags(allow_possible_spam: false) + subject do + described_class.new(project, user, params) end - context 'when reCAPTCHA was verified' do - let(:log_user) { user } - let(:spam_logs) { create_list(:spam_log, 2, user: log_user, title: title) } - let(:target_spam_log) { spam_logs.last } - - before do - opts[:recaptcha_verified] = true - opts[:spam_log_id] = target_spam_log.id - - expect(Spam::SpamVerdictService).not_to receive(:new) - end - - it 'does not mark an issue as spam' do - expect(issue).not_to be_spam - end - - it 'creates a valid issue' do - expect(issue).to be_valid - end - - it 'does not assign a spam_log to the issue' do - expect(issue.spam_log).to be_nil - end - - it 'marks related spam_log as recaptcha_verified' do - expect { issue }.to change { target_spam_log.reload.recaptcha_verified }.from(false).to(true) - end - - context 'when spam log does not belong to a user' do - let(:log_user) { create(:user) } - - it 'does not mark spam_log as recaptcha_verified' do - expect { issue }.not_to change { target_spam_log.reload.recaptcha_verified } - end + before do + allow_next_instance_of(UserAgentDetailService) do |instance| + allow(instance).to receive(:create) end end - context 'when reCAPTCHA was not verified' do - before do - expect_next_instance_of(Spam::SpamActionService) do |spam_service| - expect(spam_service).to receive_messages(check_for_spam?: true) - end - end - - context 'when SpamVerdictService requires reCAPTCHA' do - before do - expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service| - expect(verdict_service).to receive(:execute).and_return(CONDITIONAL_ALLOW) - end - end - - it 'does not mark the issue as spam' do - expect(issue).not_to be_spam - end - - it 'marks the issue as needing reCAPTCHA' do - expect(issue.needs_recaptcha?).to be_truthy - end - - it 'invalidates the issue' do - expect(issue).to be_invalid - end - - it 'creates a new spam_log' do - expect { issue } - .to have_spam_log(title: title, description: description, user_id: user.id, noteable_type: 'Issue') - end - end - - context 'when SpamVerdictService disallows creation' do - before do - expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service| - expect(verdict_service).to receive(:execute).and_return(DISALLOW) - end - end - - context 'when allow_possible_spam feature flag is false' do - it 'marks the issue as spam' do - expect(issue).to be_spam - end - - it 'does not mark the issue as needing reCAPTCHA' do - expect(issue.needs_recaptcha?).to be_falsey - end - - it 'invalidates the issue' do - expect(issue).to be_invalid - end - - it 'creates a new spam_log' do - expect { issue } - .to have_spam_log(title: title, description: description, user_id: user.id, noteable_type: 'Issue') - end - end - - context 'when allow_possible_spam feature flag is true' do - before do - stub_feature_flags(allow_possible_spam: true) - end - - it 'does not mark the issue as spam' do - expect(issue).not_to be_spam - end - - it 'does not mark the issue as needing reCAPTCHA' do - expect(issue.needs_recaptcha?).to be_falsey - end - - it 'creates a valid issue' do - expect(issue).to be_valid - end - - it 'creates a new spam_log' do - expect { issue } - .to have_spam_log(title: title, description: description, user_id: user.id, noteable_type: 'Issue') - end - end + it 'executes SpamActionService' do + spam_params = Spam::SpamParams.new( + api: api, + captcha_response: captcha_response, + spam_log_id: spam_log_id + ) + expect_next_instance_of( + Spam::SpamActionService, + { + spammable: an_instance_of(Issue), + request: request, + user: user, + action: :create + } + ) do |instance| + expect(instance).to receive(:execute).with(spam_params: spam_params) end - context 'when the SpamVerdictService allows creation' do - before do - expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service| - expect(verdict_service).to receive(:execute).and_return(ALLOW) - end - end - - it 'does not mark an issue as spam' do - expect(issue).not_to be_spam - end - - it 'creates a valid issue' do - expect(issue).to be_valid - end - - it 'does not assign a spam_log to an issue' do - expect(issue.spam_log).to be_nil - end - end + subject.execute end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 06a6a52bc41..fd42a84e405 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -711,7 +711,7 @@ RSpec.describe Issues::UpdateService, :mailer do } service = described_class.new(project, user, params) - expect(service).not_to receive(:spam_check) + expect(Spam::SpamActionService).not_to receive(:new) service.execute(issue) end diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb index 2d4457f3f62..a1b1397d444 100644 --- a/spec/services/members/update_service_spec.rb +++ b/spec/services/members/update_service_spec.rb @@ -13,9 +13,11 @@ RSpec.describe Members::UpdateService do { access_level: Gitlab::Access::MAINTAINER } end + subject { described_class.new(current_user, params).execute(member, permission: permission) } + shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do it 'raises Gitlab::Access::AccessDeniedError' do - expect { described_class.new(current_user, params).execute(member, permission: permission) } + expect { subject } .to raise_error(Gitlab::Access::AccessDeniedError) end end @@ -24,18 +26,24 @@ RSpec.describe Members::UpdateService do it 'updates the member' do expect(TodosDestroyer::EntityLeaveWorker).not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name) - updated_member = described_class.new(current_user, params).execute(member, permission: permission) + updated_member = subject.fetch(:member) expect(updated_member).to be_valid expect(updated_member.access_level).to eq(Gitlab::Access::MAINTAINER) end + it 'returns success status' do + result = subject.fetch(:status) + + expect(result).to eq(:success) + end + context 'when member is downgraded to guest' do shared_examples 'schedules to delete confidential todos' do it do expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once - updated_member = described_class.new(current_user, params).execute(member, permission: permission) + updated_member = subject.fetch(:member) expect(updated_member).to be_valid expect(updated_member.access_level).to eq(Gitlab::Access::GUEST) @@ -62,6 +70,16 @@ RSpec.describe Members::UpdateService do expect { described_class.new(current_user, params) }.to raise_error(ArgumentError, 'invalid value for Integer(): "invalid"') end end + + context 'when member is not valid' do + let(:params) { { expires_at: 2.days.ago } } + + it 'returns error status' do + result = subject + + expect(result[:status]).to eq(:error) + end + end end before do diff --git a/spec/services/merge_requests/after_create_service_spec.rb b/spec/services/merge_requests/after_create_service_spec.rb index 9ae310d8cee..f21feb70bc5 100644 --- a/spec/services/merge_requests/after_create_service_spec.rb +++ b/spec/services/merge_requests/after_create_service_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' RSpec.describe MergeRequests::AfterCreateService do - include AfterNextHelpers - let_it_be(:merge_request) { create(:merge_request) } subject(:after_create_service) do @@ -66,15 +64,8 @@ RSpec.describe MergeRequests::AfterCreateService do execute_service end - it 'registers an onboarding progress action' do - OnboardingProgress.onboard(merge_request.target_project.namespace) - - expect_next(OnboardingProgressService, merge_request.target_project.namespace) - .to receive(:execute).with(action: :merge_request_created).and_call_original - - execute_service - - expect(OnboardingProgress.completed?(merge_request.target_project.namespace, :merge_request_created)).to be(true) + it_behaves_like 'records an onboarding progress action', :merge_request_created do + let(:namespace) { merge_request.target_project.namespace } end end end diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb index 124501f17d5..df9a98c5540 100644 --- a/spec/services/merge_requests/approval_service_spec.rb +++ b/spec/services/merge_requests/approval_service_spec.rb @@ -31,6 +31,13 @@ RSpec.describe MergeRequests::ApprovalService do expect(todo.reload).to be_pending end + + it 'does not track merge request approve action' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .not_to receive(:track_approve_mr_action).with(user: user) + + service.execute(merge_request) + end end context 'with valid approval' do @@ -59,6 +66,13 @@ RSpec.describe MergeRequests::ApprovalService do service.execute(merge_request) end end + + it 'tracks merge request approve action' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_approve_mr_action).with(user: user) + + service.execute(merge_request) + end end context 'user cannot update the merge request' do diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index f83b8d98ce8..22b3456708f 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -252,6 +252,7 @@ RSpec.describe MergeRequests::BuildService do issue.update!(iid: 123) else create(:"#{issue_tracker}_service", project: project) + project.reload end end @@ -351,6 +352,7 @@ RSpec.describe MergeRequests::BuildService do issue.update!(iid: 123) else create(:"#{issue_tracker}_service", project: project) + project.reload end end diff --git a/spec/services/merge_requests/create_from_issue_service_spec.rb b/spec/services/merge_requests/create_from_issue_service_spec.rb index be8c41bc4a1..6528edfc8b7 100644 --- a/spec/services/merge_requests/create_from_issue_service_spec.rb +++ b/spec/services/merge_requests/create_from_issue_service_spec.rb @@ -52,6 +52,14 @@ RSpec.describe MergeRequests::CreateFromIssueService do service.execute end + it 'tracks the mr creation when the mr is valid' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_mr_create_from_issue) + .with(user: user) + + service.execute + end + it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created', :sidekiq_might_not_need_inline do expect_next_instance_of(MergeRequest) do |instance| expect(instance).to receive(:valid?).at_least(:once).and_return(false) @@ -62,6 +70,17 @@ RSpec.describe MergeRequests::CreateFromIssueService do service.execute end + it 'does not track the mr creation when the Mr is invalid' do + expect_next_instance_of(MergeRequest) do |instance| + expect(instance).to receive(:valid?).at_least(:once).and_return(false) + end + + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .not_to receive(:track_mr_create_from_issue) + + service.execute + end + it 'creates a merge request', :sidekiq_might_not_need_inline do expect { service.execute }.to change(target_project.merge_requests, :count).by(1) end diff --git a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb index cdaacaf5fca..d2070a466b1 100644 --- a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb +++ b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb @@ -12,6 +12,8 @@ RSpec.describe MergeRequests::DeleteNonLatestDiffsService, :clean_gitlab_redis_s stub_const("#{described_class.name}::BATCH_SIZE", 2) 3.times { merge_request.create_merge_request_diff } + merge_request.create_merge_head_diff + merge_request.reset end it 'schedules non-latest merge request diffs removal' do diff --git a/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb b/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb new file mode 100644 index 00000000000..1075f6f9034 --- /dev/null +++ b/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::MarkReviewerReviewedService do + let(:current_user) { create(:user) } + let(:merge_request) { create(:merge_request, reviewers: [current_user]) } + let(:reviewer) { merge_request.merge_request_reviewers.find_by(user_id: current_user.id) } + let(:project) { merge_request.project } + let(:service) { described_class.new(project, current_user) } + let(:result) { service.execute(merge_request) } + + before do + project.add_developer(current_user) + end + + describe '#execute' do + describe 'invalid permissions' do + let(:service) { described_class.new(project, create(:user)) } + + it 'returns an error' do + expect(result[:status]).to eq :error + end + end + + describe 'reviewer does not exist' do + let(:service) { described_class.new(project, create(:user)) } + + it 'returns an error' do + expect(result[:status]).to eq :error + end + end + + describe 'reviewer exists' do + it 'returns success' do + expect(result[:status]).to eq :success + end + + it 'updates reviewers state' do + expect(result[:status]).to eq :success + expect(reviewer.state).to eq 'reviewed' + end + end + end +end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index dd37d87e3f5..611f12c8146 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -161,7 +161,7 @@ RSpec.describe MergeRequests::MergeService do commit = double('commit', safe_message: "Fixes #{jira_issue.to_reference}") allow(merge_request).to receive(:commits).and_return([commit]) - expect_any_instance_of(JiraService).to receive(:close_issue).with(merge_request, jira_issue).once + expect_any_instance_of(JiraService).to receive(:close_issue).with(merge_request, jira_issue, user).once service.execute(merge_request) end @@ -310,12 +310,12 @@ RSpec.describe MergeRequests::MergeService do it 'logs and saves error if there is an exception' do error_message = 'error message' - allow(service).to receive(:repository).and_raise('error message') + allow(service).to receive(:repository).and_raise(error_message) allow(service).to receive(:execute_hooks) service.execute(merge_request) - expect(merge_request.merge_error).to include('Something went wrong during merge') + expect(merge_request.merge_error).to eq(described_class::GENERIC_ERROR_MESSAGE) expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end @@ -343,9 +343,7 @@ RSpec.describe MergeRequests::MergeService do expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end - it 'logs and saves error if there is a merge conflict' do - error_message = 'Conflicts detected during merge' - + it 'logs and saves error if commit is not created' do allow_any_instance_of(Repository).to receive(:merge).and_return(false) allow(service).to receive(:execute_hooks) @@ -353,8 +351,8 @@ RSpec.describe MergeRequests::MergeService do expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil - expect(merge_request.merge_error).to include(error_message) - expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + expect(merge_request.merge_error).to include(described_class::GENERIC_ERROR_MESSAGE) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(described_class::GENERIC_ERROR_MESSAGE)) end context 'when squashing is required' do diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb index 17bfa9d7368..e0baf5af8b4 100644 --- a/spec/services/merge_requests/mergeability_check_service_spec.rb +++ b/spec/services/merge_requests/mergeability_check_service_spec.rb @@ -33,6 +33,14 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar expect(merge_request.merge_status).to eq('can_be_merged') end + it 'reloads merge head diff' do + expect_next_instance_of(MergeRequests::ReloadMergeHeadDiffService) do |service| + expect(service).to receive(:execute) + end + + subject + end + it 'update diff discussion positions' do expect_next_instance_of(Discussions::CaptureDiffNotePositionsService) do |service| expect(service).to receive(:execute) @@ -142,7 +150,11 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar end it 'resets one merge request upon execution' do - expect_any_instance_of(MergeRequest).to receive(:reset).once + expect_next_instance_of(MergeRequests::ReloadMergeHeadDiffService) do |svc| + expect(svc).to receive(:execute).and_return(status: :success) + end + + expect_any_instance_of(MergeRequest).to receive(:reset).once.and_call_original execute_within_threads(amount: 2) end @@ -266,6 +278,14 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar it_behaves_like 'unmergeable merge request' + it 'reloads merge head diff' do + expect_next_instance_of(MergeRequests::ReloadMergeHeadDiffService) do |service| + expect(service).to receive(:execute) + end + + subject + end + it 'returns ServiceResponse.error' do result = subject @@ -329,6 +349,12 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar subject end + + it 'does not reload merge head diff' do + expect(MergeRequests::ReloadMergeHeadDiffService).not_to receive(:new) + + subject + end end end diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index 6523b5a158c..71329905558 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -3,9 +3,11 @@ require 'spec_helper' RSpec.describe MergeRequests::PostMergeService do - let(:user) { create(:user) } - let(:merge_request) { create(:merge_request, assignees: [user]) } - let(:project) { merge_request.project } + include ProjectForksHelper + + let_it_be(:user) { create(:user) } + let_it_be(:merge_request, reload: true) { create(:merge_request, assignees: [user]) } + let_it_be(:project) { merge_request.project } subject { described_class.new(project, user).execute(merge_request) } @@ -128,5 +130,139 @@ RSpec.describe MergeRequests::PostMergeService do expect(deploy_job.reload.canceled?).to be false end end + + context 'for a merge request chain' do + before do + ::MergeRequests::UpdateService + .new(project, user, force_remove_source_branch: '1') + .execute(merge_request) + end + + context 'when there is another MR' do + let!(:another_merge_request) do + create(:merge_request, + source_project: source_project, + source_branch: 'my-awesome-feature', + target_project: merge_request.source_project, + target_branch: merge_request.source_branch + ) + end + + shared_examples 'does not retarget merge request' do + it 'another merge request is unchanged' do + expect { subject }.not_to change { another_merge_request.reload.target_branch } + .from(merge_request.source_branch) + end + end + + shared_examples 'retargets merge request' do + it 'another merge request is retargeted' do + expect(SystemNoteService) + .to receive(:change_branch).once + .with(another_merge_request, another_merge_request.project, user, + 'target', 'delete', + merge_request.source_branch, merge_request.target_branch) + + expect { subject }.to change { another_merge_request.reload.target_branch } + .from(merge_request.source_branch) + .to(merge_request.target_branch) + end + + context 'when FF retarget_merge_requests is disabled' do + before do + stub_feature_flags(retarget_merge_requests: false) + end + + include_examples 'does not retarget merge request' + end + + context 'when source branch is to be kept' do + before do + ::MergeRequests::UpdateService + .new(project, user, force_remove_source_branch: false) + .execute(merge_request) + end + + include_examples 'does not retarget merge request' + end + end + + context 'in the same project' do + let(:source_project) { project } + + it_behaves_like 'retargets merge request' + + context 'and is closed' do + before do + another_merge_request.close + end + + it_behaves_like 'does not retarget merge request' + end + + context 'and is merged' do + before do + another_merge_request.mark_as_merged + end + + it_behaves_like 'does not retarget merge request' + end + end + + context 'in forked project' do + let!(:source_project) { fork_project(project) } + + context 'when user has access to source project' do + before do + source_project.add_developer(user) + end + + it_behaves_like 'retargets merge request' + end + + context 'when user does not have access to source project' do + it_behaves_like 'does not retarget merge request' + end + end + + context 'and current and another MR is from a fork' do + let(:project) { create(:project) } + let(:source_project) { fork_project(project) } + + let(:merge_request) do + create(:merge_request, + source_project: source_project, + target_project: project + ) + end + + before do + source_project.add_developer(user) + end + + it_behaves_like 'does not retarget merge request' + end + end + + context 'when many merge requests are to be retargeted' do + let!(:many_merge_requests) do + create_list(:merge_request, 10, :unique_branches, + source_project: merge_request.source_project, + target_project: merge_request.source_project, + target_branch: merge_request.source_branch + ) + end + + it 'retargets only 4 of them' do + subject + + expect(many_merge_requests.each(&:reload).pluck(:target_branch).tally) + .to eq( + merge_request.source_branch => 6, + merge_request.target_branch => 4 + ) + end + end + end end end diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index 85bcf4562b1..c2769d4fa88 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -21,6 +21,7 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } let(:deleted_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 #{Gitlab::Git::BLANK_SHA} refs/heads/#{source_branch}" } let(:default_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{project.default_branch}" } + let(:error_mr_required) { "A merge_request.create push option is required to create a merge request for branch #{source_branch}" } shared_examples_for 'a service that can create a merge request' do subject(:last_mr) { MergeRequest.last } @@ -176,11 +177,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -197,11 +196,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -263,11 +260,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -308,11 +303,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -329,11 +322,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -374,11 +365,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -395,11 +384,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -440,11 +427,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -461,11 +446,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -506,11 +489,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -527,11 +508,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 3ccf02fcdfb..747ecbf4fa4 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -633,31 +633,37 @@ RSpec.describe MergeRequests::RefreshService do end context 'merge request metrics' do - let(:issue) { create :issue, project: @project } - let(:commit_author) { create :user } + let(:user) { create(:user) } + let(:project) { create(:project, :repository) } + let(:issue) { create(:issue, project: project) } let(:commit) { project.commit } before do - project.add_developer(commit_author) project.add_developer(user) allow(commit).to receive_messages( safe_message: "Closes #{issue.to_reference}", references: [issue], - author_name: commit_author.name, - author_email: commit_author.email, + author_name: user.name, + author_email: user.email, committed_date: Time.current ) - - allow_any_instance_of(MergeRequest).to receive(:commits).and_return(CommitCollection.new(@project, [commit], 'feature')) end context 'when the merge request is sourced from the same project' do it 'creates a `MergeRequestsClosingIssues` record for each issue closed by a commit' do - merge_request = create(:merge_request, target_branch: 'master', source_branch: 'feature', source_project: @project) - refresh_service = service.new(@project, @user) + allow_any_instance_of(MergeRequest).to receive(:commits).and_return( + CommitCollection.new(project, [commit], 'close-by-commit') + ) + + merge_request = create(:merge_request, + target_branch: 'master', + source_branch: 'close-by-commit', + source_project: project) + + refresh_service = service.new(project, user) allow(refresh_service).to receive(:execute_hooks) - refresh_service.execute(@oldrev, @newrev, 'refs/heads/feature') + refresh_service.execute(@oldrev, @newrev, 'refs/heads/close-by-commit') issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id) expect(issue_ids).to eq([issue.id]) @@ -666,16 +672,21 @@ RSpec.describe MergeRequests::RefreshService do context 'when the merge request is sourced from a different project' do it 'creates a `MergeRequestsClosingIssues` record for each issue closed by a commit' do - forked_project = fork_project(@project, @user, repository: true) + forked_project = fork_project(project, user, repository: true) + + allow_any_instance_of(MergeRequest).to receive(:commits).and_return( + CommitCollection.new(forked_project, [commit], 'close-by-commit') + ) merge_request = create(:merge_request, target_branch: 'master', - source_branch: 'feature', - target_project: @project, + target_project: project, + source_branch: 'close-by-commit', source_project: forked_project) - refresh_service = service.new(@project, @user) + + refresh_service = service.new(forked_project, user) allow(refresh_service).to receive(:execute_hooks) - refresh_service.execute(@oldrev, @newrev, 'refs/heads/feature') + refresh_service.execute(@oldrev, @newrev, 'refs/heads/close-by-commit') issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id) expect(issue_ids).to eq([issue.id]) diff --git a/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb b/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb new file mode 100644 index 00000000000..3152a4e3861 --- /dev/null +++ b/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::ReloadMergeHeadDiffService do + let(:merge_request) { create(:merge_request) } + + subject { described_class.new(merge_request).execute } + + describe '#execute' do + before do + MergeRequests::MergeToRefService + .new(merge_request.project, merge_request.author) + .execute(merge_request) + end + + it 'creates a merge head diff' do + expect(subject[:status]).to eq(:success) + expect(merge_request.reload.merge_head_diff).to be_present + end + + context 'when merge ref head is not present' do + before do + allow(merge_request).to receive(:merge_ref_head).and_return(nil) + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + end + end + + context 'when failed to create merge head diff' do + before do + allow(merge_request).to receive(:create_merge_head_diff!).and_raise('fail') + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + end + end + + context 'when there is existing merge head diff' do + let!(:existing_merge_head_diff) { create(:merge_request_diff, :merge_head, merge_request: merge_request) } + + it 'recreates merge head diff' do + expect(subject[:status]).to eq(:success) + expect(merge_request.reload.merge_head_diff).not_to eq(existing_merge_head_diff) + end + end + + context 'when default_merge_ref_for_diffs feature flag is disabled' do + before do + stub_feature_flags(default_merge_ref_for_diffs: false) + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + end + end + end +end diff --git a/spec/services/merge_requests/remove_approval_service_spec.rb b/spec/services/merge_requests/remove_approval_service_spec.rb index 40da928e832..4ef2da290e1 100644 --- a/spec/services/merge_requests/remove_approval_service_spec.rb +++ b/spec/services/merge_requests/remove_approval_service_spec.rb @@ -32,6 +32,13 @@ RSpec.describe MergeRequests::RemoveApprovalService do execute! end + + it 'tracks merge request unapprove action' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_unapprove_mr_action).with(user: user) + + execute! + end end context 'with a user who has not approved' do @@ -41,6 +48,13 @@ RSpec.describe MergeRequests::RemoveApprovalService do execute! end + + it 'does not track merge request unapprove action' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .not_to receive(:track_unapprove_mr_action).with(user: user) + + execute! + end end end end diff --git a/spec/services/merge_requests/request_review_service_spec.rb b/spec/services/merge_requests/request_review_service_spec.rb new file mode 100644 index 00000000000..5cb4120852a --- /dev/null +++ b/spec/services/merge_requests/request_review_service_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::RequestReviewService do + let(:current_user) { create(:user) } + let(:user) { create(:user) } + let(:merge_request) { create(:merge_request, reviewers: [user]) } + let(:reviewer) { merge_request.find_reviewer(user) } + let(:project) { merge_request.project } + let(:service) { described_class.new(project, current_user) } + let(:result) { service.execute(merge_request, user) } + let(:todo_service) { spy('todo service') } + let(:notification_service) { spy('notification service') } + + before do + allow(NotificationService).to receive(:new) { notification_service } + allow(service).to receive(:todo_service).and_return(todo_service) + allow(service).to receive(:notification_service).and_return(notification_service) + + reviewer.update!(state: MergeRequestReviewer.states[:reviewed]) + + project.add_developer(current_user) + project.add_developer(user) + end + + describe '#execute' do + describe 'invalid permissions' do + let(:service) { described_class.new(project, create(:user)) } + + it 'returns an error' do + expect(result[:status]).to eq :error + end + end + + describe 'reviewer does not exist' do + let(:result) { service.execute(merge_request, create(:user)) } + + it 'returns an error' do + expect(result[:status]).to eq :error + end + end + + describe 'reviewer exists' do + it 'returns success' do + expect(result[:status]).to eq :success + end + + it 'updates reviewers state' do + service.execute(merge_request, user) + reviewer.reload + + expect(reviewer.state).to eq 'unreviewed' + end + + it 'sends email to reviewer' do + expect(notification_service).to receive_message_chain(:async, :review_requested_of_merge_request).with(merge_request, current_user, user) + + service.execute(merge_request, user) + end + + it 'creates a new todo for the reviewer' do + expect(todo_service).to receive(:create_request_review_todo).with(merge_request, current_user, user) + + service.execute(merge_request, user) + end + end + end +end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 9eb82dcd0ad..edb95840604 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -87,6 +87,38 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(@merge_request.discussion_locked).to be_truthy end + context 'usage counters' do + let(:merge_request2) { create(:merge_request) } + let(:draft_merge_request) { create(:merge_request, :draft_merge_request)} + + it 'update as expected' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_title_edit_action).once.with(user: user) + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_description_edit_action).once.with(user: user) + + MergeRequests::UpdateService.new(project, user, opts).execute(merge_request2) + end + + it 'tracks Draft/WIP marking' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_marked_as_draft_action).once.with(user: user) + + opts[:title] = "WIP: #{opts[:title]}" + + MergeRequests::UpdateService.new(project, user, opts).execute(merge_request2) + end + + it 'tracks Draft/WIP un-marking' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_unmarked_as_draft_action).once.with(user: user) + + opts[:title] = "Non-draft/wip title string" + + MergeRequests::UpdateService.new(project, user, opts).execute(draft_merge_request) + end + end + context 'updating milestone' do RSpec.shared_examples 'updates milestone' do it 'sets milestone' do @@ -142,29 +174,19 @@ RSpec.describe MergeRequests::UpdateService, :mailer do context 'with reviewers' do let(:opts) { { reviewer_ids: [user2.id] } } - context 'when merge_request_reviewers feature is disabled' do - before(:context) do - stub_feature_flags(merge_request_reviewers: false) - end - - it 'does not create a system note about merge_request review request' do - note = find_note('review requested from') + it 'creates system note about merge_request review request' do + note = find_note('requested review from') - expect(note).to be_nil - end + expect(note).not_to be_nil + expect(note.note).to include "requested review from #{user2.to_reference}" end - context 'when merge_request_reviewers feature is enabled' do - before(:context) do - stub_feature_flags(merge_request_reviewers: true) - end - - it 'creates system note about merge_request review request' do - note = find_note('requested review from') + it 'updates the tracking' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_users_review_requested) + .with(users: [user]) - expect(note).not_to be_nil - expect(note.note).to include "requested review from #{user2.to_reference}" - end + update_merge_request(reviewer_ids: [user.id]) end end @@ -794,6 +816,14 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(merge_request.assignee_ids).to eq([user.id]) end + it 'updates the tracking when user ids are valid' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_users_assigned_to_mr) + .with(users: [user]) + + update_merge_request(assignee_ids: [user.id]) + end + it 'does not update assignee_id when user cannot read issue' do non_member = create(:user) original_assignees = merge_request.assignees @@ -883,6 +913,33 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end end + context 'updating `target_branch`' do + let(:merge_request) do + create(:merge_request, + source_project: project, + source_branch: 'mr-b', + target_branch: 'mr-a') + end + + it 'updates to master' do + expect(SystemNoteService).to receive(:change_branch).with( + merge_request, project, user, 'target', 'update', 'mr-a', 'master' + ) + + expect { update_merge_request(target_branch: 'master') } + .to change { merge_request.reload.target_branch }.from('mr-a').to('master') + end + + it 'updates to master because of branch deletion' do + expect(SystemNoteService).to receive(:change_branch).with( + merge_request, project, user, 'target', 'delete', 'mr-a', 'master' + ) + + expect { update_merge_request(target_branch: 'master', target_branch_was_deleted: true) } + .to change { merge_request.reload.target_branch }.from('mr-a').to('master') + end + end + it_behaves_like 'issuable record that supports quick actions' do let(:existing_merge_request) { create(:merge_request, source_project: project) } let(:issuable) { described_class.new(project, user, params).execute(existing_merge_request) } diff --git a/spec/services/namespaces/in_product_marketing_emails_service_spec.rb b/spec/services/namespaces/in_product_marketing_emails_service_spec.rb new file mode 100644 index 00000000000..7346a5b95ae --- /dev/null +++ b/spec/services/namespaces/in_product_marketing_emails_service_spec.rb @@ -0,0 +1,159 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do + subject(:execute_service) { described_class.new(track, interval).execute } + + let(:track) { :create } + let(:interval) { 1 } + + let(:previous_action_completed_at) { 2.days.ago.middle_of_day } + let(:current_action_completed_at) { nil } + let(:experiment_enabled) { true } + let(:user_can_perform_current_track_action) { true } + let(:actions_completed) { { created_at: previous_action_completed_at, git_write_at: current_action_completed_at } } + + let_it_be(:group) { create(:group) } + let_it_be(:user) { create(:user, email_opted_in: true) } + + before do + create(:onboarding_progress, namespace: group, **actions_completed) + group.add_developer(user) + stub_experiment_for_subject(in_product_marketing_emails: experiment_enabled) + allow(Ability).to receive(:allowed?).with(user, anything, anything).and_return(user_can_perform_current_track_action) + allow(Notify).to receive(:in_product_marketing_email).and_return(double(deliver_later: nil)) + end + + RSpec::Matchers.define :send_in_product_marketing_email do |*args| + match do + expect(Notify).to have_received(:in_product_marketing_email).with(*args).once + end + + match_when_negated do + expect(Notify).not_to have_received(:in_product_marketing_email) + end + end + + context 'for each track and series with the right conditions' do + using RSpec::Parameterized::TableSyntax + + where(:track, :interval, :actions_completed) do + :create | 1 | { created_at: 2.days.ago.middle_of_day } + :create | 5 | { created_at: 6.days.ago.middle_of_day } + :create | 10 | { created_at: 11.days.ago.middle_of_day } + :verify | 1 | { created_at: 2.days.ago.middle_of_day, git_write_at: 2.days.ago.middle_of_day } + :verify | 5 | { created_at: 6.days.ago.middle_of_day, git_write_at: 6.days.ago.middle_of_day } + :verify | 10 | { created_at: 11.days.ago.middle_of_day, git_write_at: 11.days.ago.middle_of_day } + :trial | 1 | { created_at: 2.days.ago.middle_of_day, git_write_at: 2.days.ago.middle_of_day, pipeline_created_at: 2.days.ago.middle_of_day } + :trial | 5 | { created_at: 6.days.ago.middle_of_day, git_write_at: 6.days.ago.middle_of_day, pipeline_created_at: 6.days.ago.middle_of_day } + :trial | 10 | { created_at: 11.days.ago.middle_of_day, git_write_at: 11.days.ago.middle_of_day, pipeline_created_at: 11.days.ago.middle_of_day } + :team | 1 | { created_at: 2.days.ago.middle_of_day, git_write_at: 2.days.ago.middle_of_day, pipeline_created_at: 2.days.ago.middle_of_day, trial_started_at: 2.days.ago.middle_of_day } + :team | 5 | { created_at: 6.days.ago.middle_of_day, git_write_at: 6.days.ago.middle_of_day, pipeline_created_at: 6.days.ago.middle_of_day, trial_started_at: 6.days.ago.middle_of_day } + :team | 10 | { created_at: 11.days.ago.middle_of_day, git_write_at: 11.days.ago.middle_of_day, pipeline_created_at: 11.days.ago.middle_of_day, trial_started_at: 11.days.ago.middle_of_day } + end + + with_them do + it { is_expected.to send_in_product_marketing_email(user.id, group.id, track, described_class::INTERVAL_DAYS.index(interval)) } + end + end + + context 'when initialized with a different track' do + let(:track) { :verify } + + it { is_expected.not_to send_in_product_marketing_email } + + context 'when the previous track actions have been completed' do + let(:current_action_completed_at) { 2.days.ago.middle_of_day } + + it { is_expected.to send_in_product_marketing_email(user.id, group.id, :verify, 0) } + end + end + + context 'when initialized with a different interval' do + let(:interval) { 5 } + + it { is_expected.not_to send_in_product_marketing_email } + + context 'when the previous track action was completed within the intervals range' do + let(:previous_action_completed_at) { 6.days.ago.middle_of_day } + + it { is_expected.to send_in_product_marketing_email(user.id, group.id, :create, 1) } + end + end + + describe 'experimentation' do + context 'when the experiment is enabled' do + it 'adds the group as an experiment subject in the experimental group' do + expect(Experiment).to receive(:add_group) + .with(:in_product_marketing_emails, variant: :experimental, group: group) + + execute_service + end + end + + context 'when the experiment is disabled' do + let(:experiment_enabled) { false } + + it 'adds the group as an experiment subject in the control group' do + expect(Experiment).to receive(:add_group) + .with(:in_product_marketing_emails, variant: :control, group: group) + + execute_service + end + + it { is_expected.not_to send_in_product_marketing_email } + end + end + + context 'when the previous track action is not yet completed' do + let(:previous_action_completed_at) { nil } + + it { is_expected.not_to send_in_product_marketing_email } + end + + context 'when the previous track action is completed outside the intervals range' do + let(:previous_action_completed_at) { 3.days.ago } + + it { is_expected.not_to send_in_product_marketing_email } + end + + context 'when the current track action is completed' do + let(:current_action_completed_at) { Time.current } + + it { is_expected.not_to send_in_product_marketing_email } + end + + context "when the user cannot perform the current track's action" do + let(:user_can_perform_current_track_action) { false } + + it { is_expected.not_to send_in_product_marketing_email } + end + + context 'when the user has not opted into marketing emails' do + let(:user) { create(:user, email_opted_in: false) } + + it { is_expected.not_to send_in_product_marketing_email } + end + + context 'when the user has already received a marketing email as part of another group' do + before do + other_group = create(:group) + other_group.add_developer(user) + create(:onboarding_progress, namespace: other_group, created_at: previous_action_completed_at, git_write_at: current_action_completed_at) + end + + # For any group Notify is called exactly once + it { is_expected.to send_in_product_marketing_email(user.id, anything, :create, 0) } + end + + context 'when invoked with a non existing track' do + let(:track) { :foo } + + before do + stub_const("#{described_class}::TRACKS", { foo: :git_write }) + end + + it { expect { subject }.to raise_error(NotImplementedError, 'No ability defined for track foo') } + end +end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 20f06619e02..f59749f0b63 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -459,6 +459,26 @@ RSpec.describe Notes::CreateService do .and change { existing_note.updated_at } end + context 'failure in when_saved' do + let(:service) { described_class.new(project, user, reply_opts) } + + it 'converts existing note to DiscussionNote' do + expect do + existing_note + + allow(service).to receive(:when_saved).and_raise(ActiveRecord::StatementInvalid) + + travel_to(Time.current + 1.minute) do + service.execute + rescue ActiveRecord::StatementInvalid + end + + existing_note.reload + end.to change { existing_note.type }.from(nil).to('DiscussionNote') + .and change { existing_note.updated_at } + end + end + it 'returns a DiscussionNote with its parent discussion refreshed correctly' do discussion_notes = subject.discussion.notes diff --git a/spec/services/notification_recipients/build_service_spec.rb b/spec/services/notification_recipients/build_service_spec.rb index cc08f9fceff..ff54d6ccd2f 100644 --- a/spec/services/notification_recipients/build_service_spec.rb +++ b/spec/services/notification_recipients/build_service_spec.rb @@ -110,4 +110,28 @@ RSpec.describe NotificationRecipients::BuildService do end end end + + describe '#build_requested_review_recipients' do + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + + before do + merge_request.reviewers.push(assignee) + end + + shared_examples 'no N+1 queries' do + it 'avoids N+1 queries', :request_store do + create_user + + service.build_requested_review_recipients(note) + + control_count = ActiveRecord::QueryRecorder.new do + service.build_requested_review_recipients(note) + end + + create_user + + expect { service.build_requested_review_recipients(note) }.not_to exceed_query_limit(control_count) + end + end + end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 85234077b1f..b67c37ba02d 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -2177,6 +2177,46 @@ RSpec.describe NotificationService, :mailer do let(:notification_trigger) { notification.merge_when_pipeline_succeeds(merge_request, @u_disabled) } end end + + describe '#review_requested_of_merge_request' do + let(:merge_request) { create(:merge_request, author: author, source_project: project, reviewers: [reviewer]) } + + let_it_be(:current_user) { create(:user) } + let_it_be(:reviewer) { create(:user) } + + it 'sends email to reviewer', :aggregate_failures do + notification.review_requested_of_merge_request(merge_request, current_user, reviewer) + + merge_request.reviewers.each { |reviewer| should_email(reviewer) } + should_not_email(merge_request.author) + should_not_email(@u_watcher) + should_not_email(@u_participant_mentioned) + should_not_email(@subscriber) + should_not_email(@watcher_and_subscriber) + should_not_email(@u_guest_watcher) + should_not_email(@u_guest_custom) + should_not_email(@u_custom_global) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + should_not_email(@u_lazy_participant) + end + + it 'adds "review requested" reason for new reviewer' do + notification.review_requested_of_merge_request(merge_request, current_user, [reviewer]) + + merge_request.reviewers.each do |reviewer| + email = find_email_for(reviewer) + + expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::REVIEW_REQUESTED) + end + end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.review_requested_of_merge_request(merge_request, current_user, reviewer) } + end + end end describe 'Projects', :deliver_mails_inline do diff --git a/spec/services/packages/composer/create_package_service_spec.rb b/spec/services/packages/composer/create_package_service_spec.rb index d10356cfda7..4f1a46e7e45 100644 --- a/spec/services/packages/composer/create_package_service_spec.rb +++ b/spec/services/packages/composer/create_package_service_spec.rb @@ -43,6 +43,7 @@ RSpec.describe Packages::Composer::CreatePackageService do end it_behaves_like 'assigns build to package' + it_behaves_like 'assigns status to package' end context 'with a tag' do @@ -66,6 +67,7 @@ RSpec.describe Packages::Composer::CreatePackageService do end it_behaves_like 'assigns build to package' + it_behaves_like 'assigns status to package' end end diff --git a/spec/services/packages/conan/create_package_service_spec.rb b/spec/services/packages/conan/create_package_service_spec.rb index ca783475503..6f644f5ef95 100644 --- a/spec/services/packages/conan/create_package_service_spec.rb +++ b/spec/services/packages/conan/create_package_service_spec.rb @@ -31,6 +31,7 @@ RSpec.describe Packages::Conan::CreatePackageService do it_behaves_like 'assigns the package creator' it_behaves_like 'assigns build to package' + it_behaves_like 'assigns status to package' end context 'invalid params' do diff --git a/spec/services/packages/debian/create_distribution_service_spec.rb b/spec/services/packages/debian/create_distribution_service_spec.rb new file mode 100644 index 00000000000..87cf1070075 --- /dev/null +++ b/spec/services/packages/debian/create_distribution_service_spec.rb @@ -0,0 +1,122 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Debian::CreateDistributionService do + RSpec.shared_examples 'Create Debian Distribution' do |expected_message, expected_components, expected_architectures| + it 'returns ServiceResponse', :aggregate_failures do + if expected_message.nil? + expect { response } + .to change { container.debian_distributions.klass.all.count } + .from(0).to(1) + .and change { container.debian_distributions.count } + .from(0).to(1) + .and change { container.debian_distributions.first&.components&.count } + .from(nil).to(expected_components.count) + .and change { container.debian_distributions.first&.architectures&.count } + .from(nil).to(expected_architectures.count) + .and not_change { Packages::Debian::ProjectComponentFile.count } + .and not_change { Packages::Debian::GroupComponentFile.count } + else + expect { response } + .to not_change { container.debian_distributions.klass.all.count } + .and not_change { container.debian_distributions.count } + .and not_change { Packages::Debian::ProjectComponent.count } + .and not_change { Packages::Debian::GroupComponent.count } + .and not_change { Packages::Debian::ProjectArchitecture.count } + .and not_change { Packages::Debian::GroupArchitecture.count } + .and not_change { Packages::Debian::ProjectComponentFile.count } + .and not_change { Packages::Debian::GroupComponentFile.count } + end + + expect(response).to be_a(ServiceResponse) + expect(response.success?).to eq(expected_message.nil?) + expect(response.error?).to eq(!expected_message.nil?) + expect(response.message).to eq(expected_message) + + distribution = response.payload[:distribution] + expect(distribution.persisted?).to eq(expected_message.nil?) + expect(distribution.container).to eq(container) + expect(distribution.creator).to eq(user) + params.each_pair do |name, value| + expect(distribution.send(name)).to eq(value) + end + + expect(distribution.components.map(&:name)).to contain_exactly(*expected_components) + expect(distribution.architectures.map(&:name)).to contain_exactly(*expected_architectures) + end + end + + shared_examples 'Debian Create Distribution Service' do + context 'with only the codename param' do + let(:params) { { codename: 'my-codename' } } + + it_behaves_like 'Create Debian Distribution', nil, %w[main], %w[all amd64] + end + + context 'with codename, components and architectures' do + let(:params) do + { + codename: 'my-codename', + components: %w[contrib non-free], + architectures: %w[arm64] + } + end + + it_behaves_like 'Create Debian Distribution', nil, %w[contrib non-free], %w[all arm64] + end + + context 'with invalid suite' do + let(:params) do + { + codename: 'my-codename', + suite: 'erroné' + } + end + + it_behaves_like 'Create Debian Distribution', 'Suite is invalid', %w[], %w[] + end + + context 'with invalid component name' do + let(:params) do + { + codename: 'my-codename', + components: %w[before erroné after], + architectures: %w[arm64] + } + end + + it_behaves_like 'Create Debian Distribution', 'Component Name is invalid', %w[before erroné], %w[] + end + + context 'with invalid architecture name' do + let(:params) do + { + codename: 'my-codename', + components: %w[contrib non-free], + architectures: %w[before erroné after'] + } + end + + it_behaves_like 'Create Debian Distribution', 'Architecture Name is invalid', %w[contrib non-free], %w[before erroné] + end + end + + let_it_be(:user) { create(:user) } + + subject { described_class.new(container, user, params) } + + let(:response) { subject.execute } + + context 'within a projet' do + let_it_be(:container) { create(:project) } + + it_behaves_like 'Debian Create Distribution Service' + end + + context 'within a group' do + let_it_be(:container) { create(:group) } + + it_behaves_like 'Debian Create Distribution Service' + end +end diff --git a/spec/services/packages/debian/destroy_distribution_service_spec.rb b/spec/services/packages/debian/destroy_distribution_service_spec.rb new file mode 100644 index 00000000000..e4c43884bb4 --- /dev/null +++ b/spec/services/packages/debian/destroy_distribution_service_spec.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Debian::DestroyDistributionService do + RSpec.shared_examples 'Destroy Debian Distribution' do |expected_message| + it 'returns ServiceResponse', :aggregate_failures do + if expected_message.nil? + expect { response } + .to change { container.debian_distributions.klass.all.count } + .from(1).to(0) + .and change { container.debian_distributions.count } + .from(1).to(0) + .and change { component1.class.all.count } + .from(2).to(0) + .and change { architecture1.class.all.count } + .from(3).to(0) + .and change { component_file1.class.all.count } + .from(4).to(0) + else + expect { response } + .to not_change { container.debian_distributions.klass.all.count } + .and not_change { container.debian_distributions.count } + .and not_change { component1.class.all.count } + .and not_change { architecture1.class.all.count } + .and not_change { component_file1.class.all.count } + end + + expect(response).to be_a(ServiceResponse) + expect(response.success?).to eq(expected_message.nil?) + expect(response.error?).to eq(!expected_message.nil?) + expect(response.message).to eq(expected_message) + + if expected_message.nil? + expect(response.payload).to eq({}) + else + expect(response.payload).to eq(distribution: distribution) + end + end + end + + RSpec.shared_examples 'Debian Destroy Distribution Service' do |container_type, can_freeze| + context "with a Debian #{container_type} distribution" do + let_it_be(:container, freeze: can_freeze) { create(container_type) } # rubocop:disable Rails/SaveBang + let_it_be(:distribution, freeze: can_freeze) { create("debian_#{container_type}_distribution", container: container) } + let_it_be(:component1, freeze: can_freeze) { create("debian_#{container_type}_component", distribution: distribution, name: 'component1') } + let_it_be(:component2, freeze: can_freeze) { create("debian_#{container_type}_component", distribution: distribution, name: 'component2') } + let_it_be(:architecture0, freeze: true) { create("debian_#{container_type}_architecture", distribution: distribution, name: 'all') } + let_it_be(:architecture1, freeze: can_freeze) { create("debian_#{container_type}_architecture", distribution: distribution, name: 'architecture1') } + let_it_be(:architecture2, freeze: can_freeze) { create("debian_#{container_type}_architecture", distribution: distribution, name: 'architecture2') } + let_it_be(:component_file1, freeze: can_freeze) { create("debian_#{container_type}_component_file", :source, component: component1) } + let_it_be(:component_file2, freeze: can_freeze) { create("debian_#{container_type}_component_file", component: component1, architecture: architecture1) } + let_it_be(:component_file3, freeze: can_freeze) { create("debian_#{container_type}_component_file", :source, component: component2) } + let_it_be(:component_file4, freeze: can_freeze) { create("debian_#{container_type}_component_file", component: component2, architecture: architecture2) } + + subject { described_class.new(distribution) } + + let(:response) { subject.execute } + + context 'with a distribution' do + it_behaves_like 'Destroy Debian Distribution' + end + + context 'when destroy fails' do + let(:distribution) { create("debian_#{container_type}_distribution", container: container) } + + before do + expect(distribution).to receive(:destroy).and_return(false) + end + + it_behaves_like 'Destroy Debian Distribution', "Unable to destroy Debian #{container_type} distribution" + end + end + end + + it_behaves_like 'Debian Destroy Distribution Service', :project, true + it_behaves_like 'Debian Destroy Distribution Service', :group, false +end diff --git a/spec/services/packages/debian/update_distribution_service_spec.rb b/spec/services/packages/debian/update_distribution_service_spec.rb new file mode 100644 index 00000000000..852fc713e34 --- /dev/null +++ b/spec/services/packages/debian/update_distribution_service_spec.rb @@ -0,0 +1,159 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Debian::UpdateDistributionService do + RSpec.shared_examples 'Update Debian Distribution' do |expected_message, expected_components, expected_architectures, component_file_delta = 0| + it 'returns ServiceResponse', :aggregate_failures do + expect(distribution).to receive(:update).with(simple_params).and_call_original if expected_message.nil? + + if component_file_delta.zero? + expect { response } + .to not_change { container.debian_distributions.klass.all.count } + .and not_change { container.debian_distributions.count } + .and not_change { component1.class.all.count } + .and not_change { architecture1.class.all.count } + .and not_change { component_file1.class.all.count } + else + expect { response } + .to not_change { container.debian_distributions.klass.all.count } + .and not_change { container.debian_distributions.count } + .and not_change { component1.class.all.count } + .and not_change { architecture1.class.all.count } + .and change { component_file1.class.all.count } + .from(4).to(4 + component_file_delta) + end + + expect(response).to be_a(ServiceResponse) + expect(response.success?).to eq(expected_message.nil?) + expect(response.error?).to eq(!expected_message.nil?) + expect(response.message).to eq(expected_message) + + expect(response.payload).to eq(distribution: distribution) + + distribution.reload + distribution.components.reload + distribution.architectures.reload + + if expected_message.nil? + simple_params.each_pair do |name, value| + expect(distribution.send(name)).to eq(value) + end + else + original_params.each_pair do |name, value| + expect(distribution.send(name)).to eq(value) + end + end + + expect(distribution.components.map(&:name)).to contain_exactly(*expected_components) + expect(distribution.architectures.map(&:name)).to contain_exactly(*expected_architectures) + end + end + + RSpec.shared_examples 'Debian Update Distribution Service' do |container_type, can_freeze| + context "with a Debian #{container_type} distribution" do + let_it_be(:container, freeze: can_freeze) { create(container_type) } # rubocop:disable Rails/SaveBang + let_it_be(:distribution, reload: true) { create("debian_#{container_type}_distribution", container: container) } + let_it_be(:component1) { create("debian_#{container_type}_component", distribution: distribution, name: 'component1') } + let_it_be(:component2) { create("debian_#{container_type}_component", distribution: distribution, name: 'component2') } + let_it_be(:architecture0) { create("debian_#{container_type}_architecture", distribution: distribution, name: 'all') } + let_it_be(:architecture1) { create("debian_#{container_type}_architecture", distribution: distribution, name: 'architecture1') } + let_it_be(:architecture2) { create("debian_#{container_type}_architecture", distribution: distribution, name: 'architecture2') } + let_it_be(:component_file1) { create("debian_#{container_type}_component_file", :source, component: component1) } + let_it_be(:component_file2) { create("debian_#{container_type}_component_file", component: component1, architecture: architecture1) } + let_it_be(:component_file3) { create("debian_#{container_type}_component_file", :source, component: component2) } + let_it_be(:component_file4) { create("debian_#{container_type}_component_file", component: component2, architecture: architecture2) } + + let(:original_params) do + { + suite: nil, + origin: nil, + label: nil, + version: nil, + description: nil, + valid_time_duration_seconds: nil, + automatic: true, + automatic_upgrades: false + } + end + + let(:params) { {} } + let(:simple_params) { params.except(:components, :architectures) } + + subject { described_class.new(distribution, params) } + + let(:response) { subject.execute } + + context 'with valid simple params' do + let(:params) do + { + suite: 'my-suite', + origin: 'my-origin', + label: 'my-label', + version: '42.0', + description: 'my-description', + valid_time_duration_seconds: 7.days, + automatic: false, + automatic_upgrades: true + } + end + + it_behaves_like 'Update Debian Distribution', nil, %w[component1 component2], %w[all architecture1 architecture2] + end + + context 'with invalid simple params' do + let(:params) do + { + suite: 'suite erronée', + origin: 'origin erronée', + label: 'label erronée', + version: 'version erronée', + description: 'description erronée', + valid_time_duration_seconds: 1.hour + } + end + + it_behaves_like 'Update Debian Distribution', 'Suite is invalid, Origin is invalid, Label is invalid, Version is invalid, and Valid time duration seconds must be greater than or equal to 86400', %w[component1 component2], %w[all architecture1 architecture2] + end + + context 'with valid components and architectures' do + let(:params) do + { + suite: 'my-suite', + components: %w[component2 component3], + architectures: %w[architecture2 architecture3] + } + end + + it_behaves_like 'Update Debian Distribution', nil, %w[component2 component3], %w[all architecture2 architecture3], -2 + end + + context 'with invalid components' do + let(:params) do + { + suite: 'my-suite', + components: %w[component2 erroné], + architectures: %w[architecture2 architecture3] + } + end + + it_behaves_like 'Update Debian Distribution', 'Component Name is invalid', %w[component1 component2], %w[all architecture1 architecture2] + end + + context 'with invalid architectures' do + let(:params) do + { + suite: 'my-suite', + components: %w[component2 component3], + architectures: %w[architecture2 erroné] + } + end + + it_behaves_like 'Update Debian Distribution', 'Architecture Name is invalid', %w[component1 component2], %w[all architecture1 architecture2] + end + end + end + + it_behaves_like 'Debian Update Distribution Service', :project, true + it_behaves_like 'Debian Update Distribution Service', :group, false +end diff --git a/spec/services/packages/generic/create_package_file_service_spec.rb b/spec/services/packages/generic/create_package_file_service_spec.rb index 816e728c342..10c54369f26 100644 --- a/spec/services/packages/generic/create_package_file_service_spec.rb +++ b/spec/services/packages/generic/create_package_file_service_spec.rb @@ -13,6 +13,8 @@ RSpec.describe Packages::Generic::CreatePackageFileService do let(:temp_file) { Tempfile.new("test") } let(:file) { UploadedFile.new(temp_file.path, sha256: sha256) } let(:package) { create(:generic_package, project: project) } + let(:package_service) { double } + let(:params) do { package_name: 'mypackage', @@ -23,31 +25,34 @@ RSpec.describe Packages::Generic::CreatePackageFileService do } end + let(:package_params) do + { + name: params[:package_name], + version: params[:package_version], + build: params[:build], + status: nil + } + end + subject { described_class.new(project, user, params).execute } before do FileUtils.touch(temp_file) + expect(::Packages::Generic::FindOrCreatePackageService).to receive(:new).with(project, user, package_params).and_return(package_service) + expect(package_service).to receive(:execute).and_return(package) end after do FileUtils.rm_f(temp_file) end - it 'creates package file' do - package_service = double - package_params = { - name: params[:package_name], - version: params[:package_version], - build: params[:build] - } - expect(::Packages::Generic::FindOrCreatePackageService).to receive(:new).with(project, user, package_params).and_return(package_service) - expect(package_service).to receive(:execute).and_return(package) - + it 'creates package file', :aggregate_failures do expect { subject }.to change { package.package_files.count }.by(1) .and change { Packages::PackageFileBuildInfo.count }.by(1) package_file = package.package_files.last aggregate_failures do + expect(package_file.package.status).to eq('default') expect(package_file.package).to eq(package) expect(package_file.file_name).to eq('myfile.tar.gz.1') expect(package_file.size).to eq(file.size) @@ -55,6 +60,21 @@ RSpec.describe Packages::Generic::CreatePackageFileService do end end + context 'with a status' do + let(:params) { super().merge(status: 'hidden') } + let(:package_params) { super().merge(status: 'hidden') } + + it 'updates an existing packages status' do + expect { subject }.to change { package.package_files.count }.by(1) + .and change { Packages::PackageFileBuildInfo.count }.by(1) + + package_file = package.package_files.last + aggregate_failures do + expect(package_file.package.status).to eq('hidden') + end + end + end + it_behaves_like 'assigns build to package file' end end diff --git a/spec/services/packages/maven/find_or_create_package_service_spec.rb b/spec/services/packages/maven/find_or_create_package_service_spec.rb index 82dffeefcde..2543ab0c669 100644 --- a/spec/services/packages/maven/find_or_create_package_service_spec.rb +++ b/spec/services/packages/maven/find_or_create_package_service_spec.rb @@ -36,10 +36,11 @@ RSpec.describe Packages::Maven::FindOrCreatePackageService do expect(pkg.version).to eq(version) end - context 'with a build' do + context 'with optional attributes' do subject { service.execute.payload[:package] } it_behaves_like 'assigns build to package' + it_behaves_like 'assigns status to package' end end @@ -111,6 +112,13 @@ RSpec.describe Packages::Maven::FindOrCreatePackageService do expect(subject.errors).to include('Duplicate package is not allowed') end + context 'when uploading to the versionless package which contains metadata about all versions' do + let(:version) { nil } + let(:param_path) { path } + + it_behaves_like 'reuse existing package' + end + context 'when uploading different non-duplicate files to the same package' do before do package_file = existing_package.package_files.find_by(file_name: 'my-app-1.0-20180724.124855-1.jar') diff --git a/spec/services/packages/npm/create_package_service_spec.rb b/spec/services/packages/npm/create_package_service_spec.rb index 6db3777cde8..10fce6c1651 100644 --- a/spec/services/packages/npm/create_package_service_spec.rb +++ b/spec/services/packages/npm/create_package_service_spec.rb @@ -53,6 +53,7 @@ RSpec.describe Packages::Npm::CreatePackageService do let(:params) { super().merge(build: job) } it_behaves_like 'assigns build to package' + it_behaves_like 'assigns status to package' it 'creates a package file build info' do expect { subject }.to change { Packages::PackageFileBuildInfo.count }.by(1) diff --git a/spec/services/packages/nuget/create_package_service_spec.rb b/spec/services/packages/nuget/create_package_service_spec.rb index 5289ad40d61..e338ac36fc3 100644 --- a/spec/services/packages/nuget/create_package_service_spec.rb +++ b/spec/services/packages/nuget/create_package_service_spec.rb @@ -32,5 +32,6 @@ RSpec.describe Packages::Nuget::CreatePackageService do it_behaves_like 'assigns the package creator' it_behaves_like 'assigns build to package' + it_behaves_like 'assigns status to package' end end diff --git a/spec/services/packages/pypi/create_package_service_spec.rb b/spec/services/packages/pypi/create_package_service_spec.rb index 28a727c4a09..a932cf73eb7 100644 --- a/spec/services/packages/pypi/create_package_service_spec.rb +++ b/spec/services/packages/pypi/create_package_service_spec.rb @@ -52,6 +52,7 @@ RSpec.describe Packages::Pypi::CreatePackageService do end it_behaves_like 'assigns build to package' + it_behaves_like 'assigns status to package' context 'with an existing package' do before do diff --git a/spec/services/pages/delete_services_spec.rb b/spec/services/pages/delete_services_spec.rb index 440549020a2..f1edf93b0c1 100644 --- a/spec/services/pages/delete_services_spec.rb +++ b/spec/services/pages/delete_services_spec.rb @@ -3,35 +3,74 @@ require 'spec_helper' RSpec.describe Pages::DeleteService do - shared_examples 'remove pages' do - let_it_be(:project) { create(:project, path: "my.project")} - let_it_be(:admin) { create(:admin) } - let_it_be(:domain) { create(:pages_domain, project: project) } - let_it_be(:service) { described_class.new(project, admin)} + let_it_be(:admin) { create(:admin) } - it 'deletes published pages' do - expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true - expect(PagesWorker).to receive(:perform_in).with(5.minutes, :remove, project.namespace.full_path, anything) + let(:project) { create(:project, path: "my.project")} + let!(:domain) { create(:pages_domain, project: project) } + let(:service) { described_class.new(project, admin)} - Sidekiq::Testing.inline! { service.execute } + before do + project.mark_pages_as_deployed + end + + it 'deletes published pages', :sidekiq_inline do + expect(project.pages_deployed?).to be(true) + + expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true + expect(PagesWorker).to receive(:perform_in).with(5.minutes, :remove, project.namespace.full_path, anything) + + service.execute + + expect(project.pages_deployed?).to be(false) + end + + it "doesn't remove anything from the legacy storage if updates on it are disabled", :sidekiq_inline do + stub_feature_flags(pages_update_legacy_storage: false) + + expect(project.pages_deployed?).to be(true) + + expect(PagesWorker).not_to receive(:perform_in) + + service.execute - expect(project.reload.pages_metadatum.deployed?).to be(false) - end + expect(project.pages_deployed?).to be(false) + end + + it 'deletes all domains', :sidekiq_inline do + expect(project.pages_domains.count).to eq(1) + + service.execute + + expect(project.reload.pages_domains.count).to eq(0) + end - it 'deletes all domains' do - expect(project.pages_domains.count).to be 1 + it 'schedules a destruction of pages deployments' do + expect(DestroyPagesDeploymentsWorker).to( + receive(:perform_async).with(project.id) + ) - Sidekiq::Testing.inline! { service.execute } + service.execute + end + + it 'removes pages deployments', :sidekiq_inline do + create(:pages_deployment, project: project) - expect(project.reload.pages_domains.count).to be 0 - end + expect do + service.execute + end.to change { PagesDeployment.count }.by(-1) end - context 'with feature flag enabled' do - before do - expect(PagesRemoveWorker).to receive(:perform_async).and_call_original - end + it 'marks pages as not deployed, deletes domains and schedules worker to remove pages from disk' do + expect(project.pages_deployed?).to eq(true) + expect(project.pages_domains.count).to eq(1) + + service.execute + + expect(project.pages_deployed?).to eq(false) + expect(project.pages_domains.count).to eq(0) + + expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true - it_behaves_like 'remove pages' + Sidekiq::Worker.drain_all end end diff --git a/spec/services/pages/migrate_from_legacy_storage_service_spec.rb b/spec/services/pages/migrate_from_legacy_storage_service_spec.rb new file mode 100644 index 00000000000..4ec57044912 --- /dev/null +++ b/spec/services/pages/migrate_from_legacy_storage_service_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Pages::MigrateFromLegacyStorageService do + let(:service) { described_class.new(Rails.logger, migration_threads: 3, batch_size: 10, ignore_invalid_entries: false) } + + it 'does not try to migrate pages if pages are not deployed' do + expect(::Pages::MigrateLegacyStorageToDeploymentService).not_to receive(:new) + + expect(service.execute).to eq(migrated: 0, errored: 0) + end + + it 'uses multiple threads' do + projects = create_list(:project, 20) + projects.each do |project| + project.mark_pages_as_deployed + + FileUtils.mkdir_p File.join(project.pages_path, "public") + File.open(File.join(project.pages_path, "public/index.html"), "w") do |f| + f.write("Hello!") + end + end + + service = described_class.new(Rails.logger, migration_threads: 3, batch_size: 2, ignore_invalid_entries: false) + + threads = Concurrent::Set.new + + expect(service).to receive(:migrate_project).exactly(20).times.and_wrap_original do |m, *args| + threads.add(Thread.current) + + # sleep to be 100% certain that once thread can't consume all the queue + # it works without it, but I want to avoid making this test flaky + sleep(0.01) + + m.call(*args) + end + + expect(service.execute).to eq(migrated: 20, errored: 0) + expect(threads.length).to eq(3) + end + + context 'when pages are marked as deployed' do + let(:project) { create(:project) } + + before do + project.mark_pages_as_deployed + end + + context 'when pages directory does not exist' do + it 'tries to migrate the project, but does not crash' do + expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false) do |service| + expect(service).to receive(:execute).and_call_original + end + + expect(service.execute).to eq(migrated: 0, errored: 1) + end + end + + context 'when pages directory exists on disk' do + before do + FileUtils.mkdir_p File.join(project.pages_path, "public") + File.open(File.join(project.pages_path, "public/index.html"), "w") do |f| + f.write("Hello!") + end + end + + it 'migrates pages projects without deployments' do + expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false) do |service| + expect(service).to receive(:execute).and_call_original + end + + expect do + expect(service.execute).to eq(migrated: 1, errored: 0) + end.to change { project.pages_metadatum.reload.pages_deployment }.from(nil) + end + + context 'when deployed already exists for the project' do + before do + deployment = create(:pages_deployment, project: project) + project.set_first_pages_deployment!(deployment) + end + + it 'does not try to migrate project' do + expect(::Pages::MigrateLegacyStorageToDeploymentService).not_to receive(:new) + + expect(service.execute).to eq(migrated: 0, errored: 0) + end + end + end + end +end diff --git a/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb b/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb index 29023621413..d95303c3e85 100644 --- a/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb +++ b/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb @@ -6,6 +6,14 @@ RSpec.describe Pages::MigrateLegacyStorageToDeploymentService do let(:project) { create(:project, :repository) } let(:service) { described_class.new(project) } + it 'calls ::Pages::ZipDirectoryService' do + expect_next_instance_of(::Pages::ZipDirectoryService, project.pages_path, ignore_invalid_entries: true) do |zip_service| + expect(zip_service).to receive(:execute).and_call_original + end + + expect(described_class.new(project, ignore_invalid_entries: true).execute[:status]).to eq(:error) + end + it 'marks pages as not deployed if public directory is absent' do project.mark_pages_as_deployed diff --git a/spec/services/pages/zip_directory_service_spec.rb b/spec/services/pages/zip_directory_service_spec.rb index dcab6b2dada..9de68dd62bb 100644 --- a/spec/services/pages/zip_directory_service_spec.rb +++ b/spec/services/pages/zip_directory_service_spec.rb @@ -10,8 +10,14 @@ RSpec.describe Pages::ZipDirectoryService do end end + let(:ignore_invalid_entries) { false } + + let(:service) do + described_class.new(@work_dir, ignore_invalid_entries: ignore_invalid_entries) + end + let(:result) do - described_class.new(@work_dir).execute + service.execute end let(:status) { result[:status] } @@ -20,6 +26,8 @@ RSpec.describe Pages::ZipDirectoryService do let(:entries_count) { result[:entries_count] } it 'returns error if project pages dir does not exist' do + expect(Gitlab::ErrorTracking).not_to receive(:track_exception) + expect( described_class.new("/tmp/not/existing/dir").execute ).to eq(status: :error, message: "Can not find valid public dir in /tmp/not/existing/dir") @@ -132,32 +140,69 @@ RSpec.describe Pages::ZipDirectoryService do end end - it 'ignores the symlink pointing outside of public directory' do - create_file("target.html", "hello") - create_link("public/link.html", "../target.html") + shared_examples "raises or ignores file" do |raised_exception, file| + it 'raises error' do + expect do + result + end.to raise_error(raised_exception) + end - with_zip_file do |zip_file| - expect { zip_file.get_entry("public/link.html") }.to raise_error(Errno::ENOENT) + context 'when errors are ignored' do + let(:ignore_invalid_entries) { true } + + it 'does not create entry' do + with_zip_file do |zip_file| + expect { zip_file.get_entry(file) }.to raise_error(Errno::ENOENT) + end + end end end - it 'ignores the symlink if target is absent' do - create_link("public/link.html", "./target.html") + context 'when symlink points outside of public directory' do + before do + create_file("target.html", "hello") + create_link("public/link.html", "../target.html") + end - with_zip_file do |zip_file| - expect { zip_file.get_entry("public/link.html") }.to raise_error(Errno::ENOENT) + include_examples "raises or ignores file", described_class::InvalidEntryError, "public/link.html" + end + + context 'when target of the symlink is absent' do + before do + create_link("public/link.html", "./target.html") end + + include_examples "raises or ignores file", Errno::ENOENT, "public/link.html" end - it 'ignores symlink if is absolute and points to outside of directory' do - target = File.join(@work_dir, "target") - FileUtils.touch(target) + context 'when targets itself' do + before do + create_link("public/link.html", "./link.html") + end - create_link("public/link.html", target) + include_examples "raises or ignores file", Errno::ELOOP, "public/link.html" + end - with_zip_file do |zip_file| - expect { zip_file.get_entry("public/link.html") }.to raise_error(Errno::ENOENT) + context 'when symlink is absolute and points to outside of directory' do + before do + target = File.join(@work_dir, "target") + FileUtils.touch(target) + + create_link("public/link.html", target) end + + include_examples "raises or ignores file", described_class::InvalidEntryError, "public/link.html" + end + + context 'when entry has unknown ftype' do + before do + file = create_file("public/index.html", "hello") + + allow(File).to receive(:lstat).and_call_original + expect(File).to receive(:lstat).with(file) { double("lstat", ftype: "unknown") } + end + + include_examples "raises or ignores file", described_class::InvalidEntryError, "public/index.html" end it "includes raw symlink if it's target is a valid directory" do @@ -204,9 +249,13 @@ RSpec.describe Pages::ZipDirectoryService do end def create_file(name, content) - File.open(File.join(@work_dir, name), "w") do |f| + file_path = File.join(@work_dir, name) + + File.open(file_path, "w") do |f| f.write(content) end + + file_path end def create_dir(dir) diff --git a/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb b/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb index 4d489d7fe4b..79654c9b190 100644 --- a/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb +++ b/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb @@ -135,7 +135,7 @@ RSpec.describe PagesDomains::ObtainLetsEncryptCertificateService do cert.add_extension ef.create_extension("authorityKeyIdentifier", "keyid:always,issuer:always") - cert.sign key, OpenSSL::Digest::SHA1.new + cert.sign key, OpenSSL::Digest.new('SHA1') cert.to_pem end diff --git a/spec/services/post_receive_service_spec.rb b/spec/services/post_receive_service_spec.rb index 6e2cd7edf04..033194972c7 100644 --- a/spec/services/post_receive_service_spec.rb +++ b/spec/services/post_receive_service_spec.rb @@ -4,7 +4,6 @@ require 'spec_helper' RSpec.describe PostReceiveService do include Gitlab::Routing - include AfterNextHelpers let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project, :repository, :wiki_repo, namespace: user.namespace) } @@ -47,11 +46,7 @@ RSpec.describe PostReceiveService do expect(subject).to be_empty end - it 'does not record an onboarding progress action' do - expect_next(OnboardingProgressService).not_to receive(:execute) - - subject - end + it_behaves_like 'does not record an onboarding progress action' end context 'when repository is nil' do @@ -88,11 +83,8 @@ RSpec.describe PostReceiveService do expect(response.reference_counter_decreased).to be(true) end - it 'records an onboarding progress action' do - expect_next(OnboardingProgressService, project.namespace) - .to receive(:execute).with(action: :git_write) - - subject + it_behaves_like 'records an onboarding progress action', :git_write do + let(:namespace) { project.namespace } end end diff --git a/spec/services/projects/alerting/notify_service_spec.rb b/spec/services/projects/alerting/notify_service_spec.rb index 4b7b7b0b200..4e366fce0d9 100644 --- a/spec/services/projects/alerting/notify_service_spec.rb +++ b/spec/services/projects/alerting/notify_service_spec.rb @@ -36,7 +36,7 @@ RSpec.describe Projects::Alerting::NotifyService do subject { service.execute(token, nil) } - shared_examples 'notifcations are handled correctly' do + shared_examples 'notifications are handled correctly' do context 'with valid token' do let(:token) { integration.token } let(:incident_management_setting) { double(send_email?: email_enabled, create_issue?: issue_enabled, auto_close_incident?: auto_close_enabled) } @@ -85,6 +85,15 @@ RSpec.describe Projects::Alerting::NotifyService do it_behaves_like 'creates an alert management alert' it_behaves_like 'assigns the alert properties' + it 'passes the integration to alert processing' do + expect(Gitlab::AlertManagement::Payload) + .to receive(:parse) + .with(project, payload.to_h, integration: integration) + .and_call_original + + subject + end + it 'creates a system note corresponding to alert creation' do expect { subject }.to change(Note, :count).by(1) expect(Note.last.note).to include(payload_raw.fetch(:monitoring_tool)) @@ -259,7 +268,7 @@ RSpec.describe Projects::Alerting::NotifyService do subject { service.execute(token, integration) } - it_behaves_like 'notifcations are handled correctly' do + it_behaves_like 'notifications are handled correctly' do let(:source) { integration.name } end diff --git a/spec/services/projects/branches_by_mode_service_spec.rb b/spec/services/projects/branches_by_mode_service_spec.rb new file mode 100644 index 00000000000..9199c3e0b3a --- /dev/null +++ b/spec/services/projects/branches_by_mode_service_spec.rb @@ -0,0 +1,136 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::BranchesByModeService do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :repository) } + + let(:finder) { described_class.new(project, params) } + let(:params) { { mode: 'all' } } + + subject { finder.execute } + + describe '#execute' do + context 'page is passed' do + let(:params) { { page: 4, mode: 'all', offset: 3 } } + + it 'uses offset pagination' do + expect(finder).to receive(:fetch_branches_via_offset_pagination).and_call_original + + branches, prev_page, next_page = subject + + expect(branches.size).to eq(10) + expect(next_page).to be_nil + expect(prev_page).to eq("/#{project.full_path}/-/branches/all?offset=2&page=3") + end + + context 'but the page does not contain any branches' do + let(:params) { { page: 10, mode: 'all' } } + + it 'uses offset pagination' do + expect(finder).to receive(:fetch_branches_via_offset_pagination).and_call_original + + branches, prev_page, next_page = subject + + expect(branches).to eq([]) + expect(next_page).to be_nil + expect(prev_page).to be_nil + end + end + end + + context 'search is passed' do + let(:params) { { search: 'feature' } } + + it 'uses offset pagination' do + expect(finder).to receive(:fetch_branches_via_offset_pagination).and_call_original + + branches, prev_page, next_page = subject + + expect(branches.map(&:name)).to match_array(%w(feature feature_conflict)) + expect(next_page).to be_nil + expect(prev_page).to be_nil + end + end + + context 'branch_list_keyset_pagination is disabled' do + it 'uses offset pagination' do + stub_feature_flags(branch_list_keyset_pagination: false) + + expect(finder).to receive(:fetch_branches_via_offset_pagination).and_call_original + + branches, prev_page, next_page = subject + + expect(branches.size).to eq(20) + expect(next_page).to eq("/#{project.full_path}/-/branches/all?offset=1&page_token=conflict-resolvable") + expect(prev_page).to be_nil + end + end + + context 'uses gitaly pagination' do + before do + expect(finder).to receive(:fetch_branches_via_gitaly_pagination).and_call_original + end + + it 'returns branches for the first page' do + branches, prev_page, next_page = subject + + expect(branches.size).to eq(20) + expect(next_page).to eq("/#{project.full_path}/-/branches/all?offset=1&page_token=conflict-resolvable") + expect(prev_page).to be_nil + end + + context 'when second page is requested' do + let(:params) { { page_token: 'conflict-resolvable', mode: 'all', sort: 'name_asc', offset: 1 } } + + it 'returns branches for the first page' do + branches, prev_page, next_page = subject + + expect(branches.size).to eq(20) + expect(next_page).to eq("/#{project.full_path}/-/branches/all?offset=2&page_token=improve%2Fawesome&sort=name_asc") + expect(prev_page).to eq("/#{project.full_path}/-/branches/all?offset=0&page=1&sort=name_asc") + end + end + + context 'when last page is requested' do + let(:params) { { page_token: 'signed-commits', mode: 'all', sort: 'name_asc', offset: 4 } } + + it 'returns branches after the specified branch' do + branches, prev_page, next_page = subject + + expect(branches.size).to eq(14) + expect(next_page).to be_nil + expect(prev_page).to eq("/#{project.full_path}/-/branches/all?offset=3&page=4&sort=name_asc") + end + end + end + + context 'filter by mode' do + let(:stale) { double(state: 'stale') } + let(:active) { double(state: 'active') } + + before do + allow_next_instance_of(BranchesFinder) do |instance| + allow(instance).to receive(:execute).and_return([stale, active]) + end + end + + context 'stale' do + let(:params) { { mode: 'stale' } } + + it 'returns stale branches' do + is_expected.to eq([[stale], nil, nil]) + end + end + + context 'active' do + let(:params) { { mode: 'active' } } + + it 'returns active branches' do + is_expected.to eq([[active], nil, nil]) + end + end + end + end +end diff --git a/spec/services/projects/cleanup_service_spec.rb b/spec/services/projects/cleanup_service_spec.rb index 6fd29813d98..f2c052d9397 100644 --- a/spec/services/projects/cleanup_service_spec.rb +++ b/spec/services/projects/cleanup_service_spec.rb @@ -88,7 +88,7 @@ RSpec.describe Projects::CleanupService do end it 'runs garbage collection on the repository' do - expect_next_instance_of(GitGarbageCollectWorker) do |worker| + expect_next_instance_of(Projects::GitGarbageCollectWorker) do |worker| expect(worker).to receive(:perform).with(project.id, :prune, "project_cleanup:gc:#{project.id}") end diff --git a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb index 17c2f0f6c17..eed22416868 100644 --- a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb @@ -284,7 +284,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do deleted: nil ) - expect(result).to eq(service_response.compact) + expect(result).to eq(service_response) end end @@ -369,6 +369,6 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do before_truncate_size: before_truncate_size, after_truncate_size: after_truncate_size, before_delete_size: before_delete_size - }.compact + }.compact.merge(deleted_size: deleted&.size) end end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 6c0e6654622..f7da6f75141 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -40,6 +40,48 @@ RSpec.describe Projects::CreateService, '#execute' do end end + describe 'setting name and path' do + subject(:project) { create_project(user, opts) } + + context 'when both are set' do + let(:opts) { { name: 'one', path: 'two' } } + + it 'keeps them as specified' do + expect(project.name).to eq('one') + expect(project.path).to eq('two') + end + end + + context 'when path is set' do + let(:opts) { { path: 'one.two_three-four' } } + + it 'sets name == path' do + expect(project.path).to eq('one.two_three-four') + expect(project.name).to eq(project.path) + end + end + + context 'when name is a valid path' do + let(:opts) { { name: 'one.two_three-four' } } + + it 'sets path == name' do + expect(project.name).to eq('one.two_three-four') + expect(project.path).to eq(project.name) + end + end + + context 'when name is not a valid path' do + let(:opts) { { name: 'one.two_three-four and five' } } + + # TODO: Retained for backwards compatibility. Remove in API v5. + # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52725 + it 'parameterizes the name' do + expect(project.name).to eq('one.two_three-four and five') + expect(project.path).to eq('one-two_three-four-and-five') + end + end + end + context 'user namespace' do it do project = create_project(user, opts) @@ -419,7 +461,7 @@ RSpec.describe Projects::CreateService, '#execute' do context 'when another repository already exists on disk' do let(:opts) do { - name: 'Existing', + name: 'existing', namespace_id: user.namespace.id } end diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index a11f16573f5..df02f8ea15d 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -323,6 +323,50 @@ RSpec.describe Projects::ForkService do end end end + + describe 'fork with optional attributes' do + let(:public_project) { create(:project, :public) } + + it 'sets optional attributes to specified values' do + forked_project = fork_project( + public_project, + nil, + namespace: public_project.namespace, + path: 'forked', + name: 'My Fork', + description: 'Description', + visibility: 'internal', + using_service: true + ) + + expect(forked_project.path).to eq('forked') + expect(forked_project.name).to eq('My Fork') + expect(forked_project.description).to eq('Description') + expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL) + end + + it 'sets visibility level to private if an unknown visibility is requested' do + forked_project = fork_project(public_project, nil, using_service: true, visibility: 'unknown') + + expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE) + end + + it 'sets visibility level to project visibility level if requested visibility is greater' do + private_project = create(:project, :private) + + forked_project = fork_project(private_project, nil, using_service: true, visibility: 'public') + + expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE) + end + + it 'sets visibility level to target namespace visibility level if requested visibility is greater' do + private_group = create(:group, :private) + + forked_project = fork_project(public_project, nil, namespace: private_group, using_service: true, visibility: 'public') + + expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE) + end + end end context 'when a project is already forked' do diff --git a/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/spec/services/projects/prometheus/alerts/notify_service_spec.rb index 8ae47ec266c..e196220eabe 100644 --- a/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Projects::Prometheus::Alerts::NotifyService do include PrometheusHelpers + using RSpec::Parameterized::TableSyntax let_it_be(:project, reload: true) { create(:project) } @@ -61,8 +62,6 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do end context 'with project specific cluster' do - using RSpec::Parameterized::TableSyntax - where(:cluster_enabled, :status, :configured_token, :token_input, :result) do true | :installed | token | token | :success true | :installed | nil | nil | :success @@ -104,8 +103,6 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do end context 'with manual prometheus installation' do - using RSpec::Parameterized::TableSyntax - where(:alerting_setting, :configured_token, :token_input, :result) do true | token | token | :success true | token | 'x' | :failure @@ -139,8 +136,6 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do end context 'with HTTP integration' do - using RSpec::Parameterized::TableSyntax - where(:active, :token, :result) do :active | :valid | :success :active | :invalid | :failure diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index a6730c5de52..6bf2876f640 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -16,7 +16,7 @@ RSpec.describe Projects::UpdatePagesService do subject { described_class.new(project, build) } before do - project.remove_pages + project.legacy_remove_pages end context '::TMP_EXTRACT_PATH' do @@ -55,6 +55,15 @@ RSpec.describe Projects::UpdatePagesService do end end + it "doesn't deploy to legacy storage if it's disabled" do + stub_feature_flags(pages_update_legacy_storage: false) + + expect(execute).to eq(:success) + expect(project.pages_deployed?).to be_truthy + + expect(File.exist?(File.join(project.pages_path, 'public', 'index.html'))).to eq(false) + end + it 'creates pages_deployment and saves it in the metadata' do expect do expect(execute).to eq(:success) diff --git a/spec/services/projects/update_repository_storage_service_spec.rb b/spec/services/projects/update_repository_storage_service_spec.rb index ef8f166cc3f..828667fdfc2 100644 --- a/spec/services/projects/update_repository_storage_service_spec.rb +++ b/spec/services/projects/update_repository_storage_service_spec.rb @@ -59,13 +59,18 @@ RSpec.describe Projects::UpdateRepositoryStorageService do end context 'when the filesystems are the same' do - let(:destination) { project.repository_storage } + before do + expect(Gitlab::GitalyClient).to receive(:filesystem_id).twice.and_return(SecureRandom.uuid) + end - it 'bails out and does nothing' do + it 'updates the database without trying to move the repostory', :aggregate_failures do result = subject.execute + project.reload - expect(result).to be_error - expect(result.message).to match(/SameFilesystemError/) + expect(result).to be_success + expect(project).not_to be_repository_read_only + expect(project.repository_storage).to eq('test_second_storage') + expect(project.project_repository.shard_name).to eq('test_second_storage') end end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 21e294418a1..1a102b125f6 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -879,139 +879,123 @@ RSpec.describe QuickActions::InterpretService do let(:issuable) { issue } end - context 'when the merge_request_reviewers flag is enabled' do - describe 'assign_reviewer command' do - let(:content) { "/assign_reviewer @#{developer.username}" } - let(:issuable) { merge_request } + describe 'assign_reviewer command' do + let(:content) { "/assign_reviewer @#{developer.username}" } + let(:issuable) { merge_request } - context 'with one user' do - it_behaves_like 'assign_reviewer command' - end + context 'with one user' do + it_behaves_like 'assign_reviewer command' + end - context 'with an issue instead of a merge request' do - let(:issuable) { issue } + context 'with an issue instead of a merge request' do + let(:issuable) { issue } - it_behaves_like 'empty command' - end + it_behaves_like 'empty command' + end - # CE does not have multiple reviewers - context 'assign command with multiple assignees' do - before do - project.add_developer(developer2) - end + # CE does not have multiple reviewers + context 'assign command with multiple assignees' do + before do + project.add_developer(developer2) + end - # There's no guarantee that the reference extractor will preserve - # the order of the mentioned users since this is dependent on the - # order in which rows are returned. We just ensure that at least - # one of the mentioned users is assigned. - context 'assigns to one of the two users' do - let(:content) { "/assign_reviewer @#{developer.username} @#{developer2.username}" } + # There's no guarantee that the reference extractor will preserve + # the order of the mentioned users since this is dependent on the + # order in which rows are returned. We just ensure that at least + # one of the mentioned users is assigned. + context 'assigns to one of the two users' do + let(:content) { "/assign_reviewer @#{developer.username} @#{developer2.username}" } - it 'assigns to a single reviewer' do - _, updates, message = service.execute(content, issuable) + it 'assigns to a single reviewer' do + _, updates, message = service.execute(content, issuable) - expect(updates[:reviewer_ids].count).to eq(1) - reviewer = updates[:reviewer_ids].first - expect([developer.id, developer2.id]).to include(reviewer) + expect(updates[:reviewer_ids].count).to eq(1) + reviewer = updates[:reviewer_ids].first + expect([developer.id, developer2.id]).to include(reviewer) - user = reviewer == developer.id ? developer : developer2 + user = reviewer == developer.id ? developer : developer2 - expect(message).to match("Assigned #{user.to_reference} as reviewer.") - end + expect(message).to match("Assigned #{user.to_reference} as reviewer.") end end + end - context 'with "me" alias' do - let(:content) { '/assign_reviewer me' } + context 'with "me" alias' do + let(:content) { '/assign_reviewer me' } - it_behaves_like 'assign_reviewer command' - end + it_behaves_like 'assign_reviewer command' + end - context 'with an alias and whitespace' do - let(:content) { '/assign_reviewer me ' } + context 'with an alias and whitespace' do + let(:content) { '/assign_reviewer me ' } - it_behaves_like 'assign_reviewer command' - end + it_behaves_like 'assign_reviewer command' + end - context 'with an incorrect user' do - let(:content) { '/assign_reviewer @abcd1234' } + context 'with an incorrect user' do + let(:content) { '/assign_reviewer @abcd1234' } - it_behaves_like 'empty command', "Failed to assign a reviewer because no user was found." - end + it_behaves_like 'empty command', "Failed to assign a reviewer because no user was found." + end - context 'with the "reviewer" alias' do - let(:content) { "/reviewer @#{developer.username}" } + context 'with the "reviewer" alias' do + let(:content) { "/reviewer @#{developer.username}" } - it_behaves_like 'assign_reviewer command' - end + it_behaves_like 'assign_reviewer command' + end - context 'with no user' do - let(:content) { '/assign_reviewer' } + context 'with the "request_review" alias' do + let(:content) { "/request_review @#{developer.username}" } - it_behaves_like 'empty command', "Failed to assign a reviewer because no user was found." - end + it_behaves_like 'assign_reviewer command' + end - context 'includes only the user reference with extra text' do - let(:content) { "/assign_reviewer @#{developer.username} do it!" } + context 'with no user' do + let(:content) { '/assign_reviewer' } - it_behaves_like 'assign_reviewer command' - end + it_behaves_like 'empty command', "Failed to assign a reviewer because no user was found." end - describe 'unassign_reviewer command' do - # CE does not have multiple reviewers, so basically anything - # after /unassign_reviewer (including whitespace) will remove - # all the current reviewers. - let(:issuable) { create(:merge_request, reviewers: [developer]) } - let(:content) { "/unassign_reviewer @#{developer.username}" } + context 'includes only the user reference with extra text' do + let(:content) { "/assign_reviewer @#{developer.username} do it!" } - context 'with one user' do - it_behaves_like 'unassign_reviewer command' - end - - context 'with an issue instead of a merge request' do - let(:issuable) { issue } - - it_behaves_like 'empty command' - end + it_behaves_like 'assign_reviewer command' + end + end - context 'with anything after the command' do - let(:content) { '/unassign_reviewer supercalifragilisticexpialidocious' } + describe 'unassign_reviewer command' do + # CE does not have multiple reviewers, so basically anything + # after /unassign_reviewer (including whitespace) will remove + # all the current reviewers. + let(:issuable) { create(:merge_request, reviewers: [developer]) } + let(:content) { "/unassign_reviewer @#{developer.username}" } - it_behaves_like 'unassign_reviewer command' - end + context 'with one user' do + it_behaves_like 'unassign_reviewer command' + end - context 'with the "remove_reviewer" alias' do - let(:content) { "/remove_reviewer @#{developer.username}" } + context 'with an issue instead of a merge request' do + let(:issuable) { issue } - it_behaves_like 'unassign_reviewer command' - end + it_behaves_like 'empty command' + end - context 'with no user' do - let(:content) { '/unassign_reviewer' } + context 'with anything after the command' do + let(:content) { '/unassign_reviewer supercalifragilisticexpialidocious' } - it_behaves_like 'unassign_reviewer command' - end + it_behaves_like 'unassign_reviewer command' end - end - context 'when the merge_request_reviewers flag is disabled' do - before do - stub_feature_flags(merge_request_reviewers: false) - end + context 'with the "remove_reviewer" alias' do + let(:content) { "/remove_reviewer @#{developer.username}" } - describe 'assign_reviewer command' do - it_behaves_like 'empty command' do - let(:content) { "/assign_reviewer @#{developer.username}" } - let(:issuable) { merge_request } - end + it_behaves_like 'unassign_reviewer command' end - describe 'unassign_reviewer command' do - it_behaves_like 'empty command' do - let(:content) { "/unassign_reviewer @#{developer.username}" } - let(:issuable) { merge_request } - end + context 'with no user' do + let(:content) { '/unassign_reviewer' } + + it_behaves_like 'unassign_reviewer command' end end @@ -1787,6 +1771,24 @@ RSpec.describe QuickActions::InterpretService do expect(text).to eq(" - list\n\ntest") end + it 'tracks MAU for commands' do + content = "/shrug test\n/assign me\n/milestone %4" + + expect(Gitlab::UsageDataCounters::QuickActionActivityUniqueCounter) + .to receive(:track_unique_action) + .with('shrug', args: 'test', user: developer) + + expect(Gitlab::UsageDataCounters::QuickActionActivityUniqueCounter) + .to receive(:track_unique_action) + .with('assign', args: 'me', user: developer) + + expect(Gitlab::UsageDataCounters::QuickActionActivityUniqueCounter) + .to receive(:track_unique_action) + .with('milestone', args: '%4', user: developer) + + service.execute(content, issue) + end + context '/create_merge_request command' do let(:branch_name) { '1-feature' } let(:content) { "/create_merge_request #{branch_name}" } diff --git a/spec/services/repositories/changelog_service_spec.rb b/spec/services/repositories/changelog_service_spec.rb new file mode 100644 index 00000000000..a545b0f070a --- /dev/null +++ b/spec/services/repositories/changelog_service_spec.rb @@ -0,0 +1,130 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Repositories::ChangelogService do + describe '#execute' do + it 'generates and commits a changelog section' do + project = create(:project, :empty_repo) + creator = project.creator + author1 = create(:user) + author2 = create(:user) + + project.add_maintainer(author1) + project.add_maintainer(author2) + + mr1 = create(:merge_request, :merged, target_project: project) + mr2 = create(:merge_request, :merged, target_project: project) + + # The range of commits ignores the first commit, but includes the last + # commit. To ensure both the commits below are included, we must create an + # extra commit. + # + # In the real world, the start commit of the range will be the last commit + # of the previous release, so ignoring that is expected and desired. + sha1 = create_commit( + project, + creator, + commit_message: 'Initial commit', + actions: [{ action: 'create', content: 'test', file_path: 'README.md' }] + ) + + sha2 = create_commit( + project, + author1, + commit_message: "Title 1\n\nChangelog: feature", + actions: [{ action: 'create', content: 'foo', file_path: 'a.txt' }] + ) + + sha3 = create_commit( + project, + author2, + commit_message: "Title 2\n\nChangelog: feature", + actions: [{ action: 'create', content: 'bar', file_path: 'b.txt' }] + ) + + commit1 = project.commit(sha2) + commit2 = project.commit(sha3) + + allow(MergeRequestDiffCommit) + .to receive(:oldest_merge_request_id_per_commit) + .with(project.id, [commit2.id, commit1.id]) + .and_return([ + { sha: sha2, merge_request_id: mr1.id }, + { sha: sha3, merge_request_id: mr2.id } + ]) + + recorder = ActiveRecord::QueryRecorder.new do + described_class + .new(project, creator, version: '1.0.0', from: sha1, to: sha3) + .execute + end + + changelog = project.repository.blob_at('master', 'CHANGELOG.md')&.data + + expect(recorder.count).to eq(10) + expect(changelog).to include('Title 1', 'Title 2') + end + end + + describe '#start_of_commit_range' do + let(:project) { build_stubbed(:project) } + let(:user) { build_stubbed(:user) } + + context 'when the "from" argument is specified' do + it 'returns the value of the argument' do + service = described_class + .new(project, user, version: '1.0.0', from: 'foo', to: 'bar') + + expect(service.start_of_commit_range).to eq('foo') + end + end + + context 'when the "from" argument is unspecified' do + it 'returns the tag commit of the previous version' do + service = described_class + .new(project, user, version: '1.0.0', to: 'bar') + + finder_spy = instance_spy(Repositories::PreviousTagFinder) + tag = double(:tag, target_commit: double(:commit, id: '123')) + + allow(Repositories::PreviousTagFinder) + .to receive(:new) + .with(project) + .and_return(finder_spy) + + allow(finder_spy) + .to receive(:execute) + .with('1.0.0') + .and_return(tag) + + expect(service.start_of_commit_range).to eq('123') + end + + it 'raises an error when no tag is found' do + service = described_class + .new(project, user, version: '1.0.0', to: 'bar') + + finder_spy = instance_spy(Repositories::PreviousTagFinder) + + allow(Repositories::PreviousTagFinder) + .to receive(:new) + .with(project) + .and_return(finder_spy) + + allow(finder_spy) + .to receive(:execute) + .with('1.0.0') + .and_return(nil) + + expect { service.start_of_commit_range } + .to raise_error(Gitlab::Changelog::Error) + end + end + end + + def create_commit(project, user, params) + params = { start_branch: 'master', branch_name: 'master' }.merge(params) + Files::MultiService.new(project, user, params).execute.fetch(:result) + end +end diff --git a/spec/services/resource_access_tokens/create_service_spec.rb b/spec/services/resource_access_tokens/create_service_spec.rb index 5cfa1ae93e6..517ed086713 100644 --- a/spec/services/resource_access_tokens/create_service_spec.rb +++ b/spec/services/resource_access_tokens/create_service_spec.rb @@ -195,6 +195,14 @@ RSpec.describe ResourceAccessTokens::CreateService do end end end + + it 'logs the event' do + allow(Gitlab::AppLogger).to receive(:info) + + response = subject + + expect(Gitlab::AppLogger).to have_received(:info).with(/PROJECT ACCESS TOKEN CREATION: created_by: #{user.username}, project_id: #{resource.id}, token_user: #{response.payload[:access_token].user.name}, token_id: \d+/) + end end context 'when resource is a project' do @@ -208,7 +216,7 @@ RSpec.describe ResourceAccessTokens::CreateService do response = subject expect(response.error?).to be true - expect(response.errors).to include("User does not have permission to create #{resource_type} Access Token") + expect(response.errors).to include("User does not have permission to create #{resource_type} access token") end end diff --git a/spec/services/resource_access_tokens/revoke_service_spec.rb b/spec/services/resource_access_tokens/revoke_service_spec.rb index af29ee2a721..99adb4bb7a0 100644 --- a/spec/services/resource_access_tokens/revoke_service_spec.rb +++ b/spec/services/resource_access_tokens/revoke_service_spec.rb @@ -40,6 +40,14 @@ RSpec.describe ResourceAccessTokens::RevokeService do expect(User.exists?(resource_bot.id)).to be_falsy end + + it 'logs the event' do + allow(Gitlab::AppLogger).to receive(:info) + + subject + + expect(Gitlab::AppLogger).to have_received(:info).with("PROJECT ACCESS TOKEN REVOCATION: revoked_by: #{user.username}, project_id: #{resource.id}, token_user: #{resource_bot.name}, token_id: #{access_token.id}") + end end shared_examples 'rollback revoke steps' do diff --git a/spec/services/resource_events/change_milestone_service_spec.rb b/spec/services/resource_events/change_milestone_service_spec.rb index a2131c5c1b0..ed234376381 100644 --- a/spec/services/resource_events/change_milestone_service_spec.rb +++ b/spec/services/resource_events/change_milestone_service_spec.rb @@ -6,8 +6,8 @@ RSpec.describe ResourceEvents::ChangeMilestoneService do let_it_be(:timebox) { create(:milestone) } let(:created_at_time) { Time.utc(2019, 12, 30) } - let(:add_timebox_args) { { created_at: created_at_time, old_milestone: nil } } - let(:remove_timebox_args) { { created_at: created_at_time, old_milestone: timebox } } + let(:add_timebox_args) { { old_milestone: nil } } + let(:remove_timebox_args) { { old_milestone: timebox } } [:issue, :merge_request].each do |issuable| it_behaves_like 'timebox(milestone or iteration) resource events creator', ResourceMilestoneEvent do diff --git a/spec/services/search/global_service_spec.rb b/spec/services/search/global_service_spec.rb index 7b914a4d3d6..e8716ef4d90 100644 --- a/spec/services/search/global_service_spec.rb +++ b/spec/services/search/global_service_spec.rb @@ -56,14 +56,20 @@ RSpec.describe Search::GlobalService do context 'issues' do let(:scope) { 'issues' } - context 'sort by created_at' do - let!(:project) { create(:project, :public) } + context 'sorting' do + let_it_be(:project) { create(:project, :public) } + let!(:old_result) { create(:issue, project: project, title: 'sorted old', created_at: 1.month.ago) } let!(:new_result) { create(:issue, project: project, title: 'sorted recent', created_at: 1.day.ago) } let!(:very_old_result) { create(:issue, project: project, title: 'sorted very old', created_at: 1.year.ago) } + let!(:old_updated) { create(:issue, project: project, title: 'updated old', updated_at: 1.month.ago) } + let!(:new_updated) { create(:issue, project: project, title: 'updated recent', updated_at: 1.day.ago) } + let!(:very_old_updated) { create(:issue, project: project, title: 'updated very old', updated_at: 1.year.ago) } + include_examples 'search results sorted' do - let(:results) { described_class.new(nil, search: 'sorted', sort: sort).execute } + let(:results_created) { described_class.new(nil, search: 'sorted', sort: sort).execute } + let(:results_updated) { described_class.new(nil, search: 'updated', sort: sort).execute } end end end @@ -71,14 +77,20 @@ RSpec.describe Search::GlobalService do context 'merge_request' do let(:scope) { 'merge_requests' } - context 'sort by created_at' do + context 'sorting' do let!(:project) { create(:project, :public) } + let!(:old_result) { create(:merge_request, :opened, source_project: project, source_branch: 'old-1', title: 'sorted old', created_at: 1.month.ago) } let!(:new_result) { create(:merge_request, :opened, source_project: project, source_branch: 'new-1', title: 'sorted recent', created_at: 1.day.ago) } let!(:very_old_result) { create(:merge_request, :opened, source_project: project, source_branch: 'very-old-1', title: 'sorted very old', created_at: 1.year.ago) } + let!(:old_updated) { create(:merge_request, :opened, source_project: project, source_branch: 'updated-old-1', title: 'updated old', updated_at: 1.month.ago) } + let!(:new_updated) { create(:merge_request, :opened, source_project: project, source_branch: 'updated-new-1', title: 'updated recent', updated_at: 1.day.ago) } + let!(:very_old_updated) { create(:merge_request, :opened, source_project: project, source_branch: 'updated-very-old-1', title: 'updated very old', updated_at: 1.year.ago) } + include_examples 'search results sorted' do - let(:results) { described_class.new(nil, search: 'sorted', sort: sort).execute } + let(:results_created) { described_class.new(nil, search: 'sorted', sort: sort).execute } + let(:results_updated) { described_class.new(nil, search: 'updated', sort: sort).execute } end end end diff --git a/spec/services/search/group_service_spec.rb b/spec/services/search/group_service_spec.rb index 2bfe714f393..7beeec98b23 100644 --- a/spec/services/search/group_service_spec.rb +++ b/spec/services/search/group_service_spec.rb @@ -44,15 +44,21 @@ RSpec.describe Search::GroupService do context 'issues' do let(:scope) { 'issues' } - context 'sort by created_at' do - let!(:group) { create(:group) } - let!(:project) { create(:project, :public, group: group) } + context 'sorting' do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :public, group: group) } + let!(:old_result) { create(:issue, project: project, title: 'sorted old', created_at: 1.month.ago) } let!(:new_result) { create(:issue, project: project, title: 'sorted recent', created_at: 1.day.ago) } let!(:very_old_result) { create(:issue, project: project, title: 'sorted very old', created_at: 1.year.ago) } + let!(:old_updated) { create(:issue, project: project, title: 'updated old', updated_at: 1.month.ago) } + let!(:new_updated) { create(:issue, project: project, title: 'updated recent', updated_at: 1.day.ago) } + let!(:very_old_updated) { create(:issue, project: project, title: 'updated very old', updated_at: 1.year.ago) } + include_examples 'search results sorted' do - let(:results) { described_class.new(nil, group, search: 'sorted', sort: sort).execute } + let(:results_created) { described_class.new(nil, group, search: 'sorted', sort: sort).execute } + let(:results_updated) { described_class.new(nil, group, search: 'updated', sort: sort).execute } end end end @@ -60,15 +66,21 @@ RSpec.describe Search::GroupService do context 'merge requests' do let(:scope) { 'merge_requests' } - context 'sort by created_at' do + context 'sorting' do let!(:group) { create(:group) } let!(:project) { create(:project, :public, group: group) } + let!(:old_result) { create(:merge_request, :opened, source_project: project, source_branch: 'old-1', title: 'sorted old', created_at: 1.month.ago) } let!(:new_result) { create(:merge_request, :opened, source_project: project, source_branch: 'new-1', title: 'sorted recent', created_at: 1.day.ago) } let!(:very_old_result) { create(:merge_request, :opened, source_project: project, source_branch: 'very-old-1', title: 'sorted very old', created_at: 1.year.ago) } + let!(:old_updated) { create(:merge_request, :opened, source_project: project, source_branch: 'updated-old-1', title: 'updated old', updated_at: 1.month.ago) } + let!(:new_updated) { create(:merge_request, :opened, source_project: project, source_branch: 'updated-new-1', title: 'updated recent', updated_at: 1.day.ago) } + let!(:very_old_updated) { create(:merge_request, :opened, source_project: project, source_branch: 'updated-very-old-1', title: 'updated very old', updated_at: 1.year.ago) } + include_examples 'search results sorted' do - let(:results) { described_class.new(nil, group, search: 'sorted', sort: sort).execute } + let(:results_created) { described_class.new(nil, group, search: 'sorted', sort: sort).execute } + let(:results_updated) { described_class.new(nil, group, search: 'updated', sort: sort).execute } end end end diff --git a/spec/services/security/ci_configuration/sast_create_service_spec.rb b/spec/services/security/ci_configuration/sast_create_service_spec.rb new file mode 100644 index 00000000000..ff7ab614e08 --- /dev/null +++ b/spec/services/security/ci_configuration/sast_create_service_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::CiConfiguration::SastCreateService, :snowplow do + describe '#execute' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let(:params) { {} } + + subject(:result) { described_class.new(project, user, params).execute } + + context 'user does not belong to project' do + it 'returns an error status' do + expect(result[:status]).to eq(:error) + expect(result[:success_path]).to be_nil + end + + it 'does not track a snowplow event' do + subject + + expect_no_snowplow_event + end + end + + context 'user belongs to project' do + before do + project.add_developer(user) + end + + it 'does track the snowplow event' do + subject + + expect_snowplow_event( + category: 'Security::CiConfiguration::SastCreateService', + action: 'create', + label: 'false' + ) + end + + it 'raises exception if the user does not have permission to create a new branch' do + allow(project).to receive(:repository).and_raise(Gitlab::Git::PreReceiveError, "You are not allowed to create protected branches on this project.") + + expect { subject }.to raise_error(Gitlab::Git::PreReceiveError) + end + + context 'with no parameters' do + it 'returns the path to create a new merge request' do + expect(result[:status]).to eq(:success) + expect(result[:success_path]).to match(/#{Gitlab::Routing.url_helpers.project_new_merge_request_url(project, {})}(.*)description(.*)source_branch/) + end + end + + context 'with parameters' do + let(:params) do + { 'stage' => 'security', + 'SEARCH_MAX_DEPTH' => 1, + 'SECURE_ANALYZERS_PREFIX' => 'new_registry', + 'SAST_EXCLUDED_PATHS' => 'spec,docs' } + end + + it 'returns the path to create a new merge request' do + expect(result[:status]).to eq(:success) + expect(result[:success_path]).to match(/#{Gitlab::Routing.url_helpers.project_new_merge_request_url(project, {})}(.*)description(.*)source_branch/) + end + end + end + end +end diff --git a/spec/services/security/ci_configuration/sast_parser_service_spec.rb b/spec/services/security/ci_configuration/sast_parser_service_spec.rb new file mode 100644 index 00000000000..21490f993c7 --- /dev/null +++ b/spec/services/security/ci_configuration/sast_parser_service_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::CiConfiguration::SastParserService do + describe '#configuration' do + include_context 'read ci configuration for sast enabled project' + + let(:configuration) { described_class.new(project).configuration } + let(:secure_analyzers_prefix) { configuration['global'][0] } + let(:sast_excluded_paths) { configuration['global'][1] } + let(:sast_analyzer_image_tag) { configuration['global'][2] } + let(:sast_pipeline_stage) { configuration['pipeline'][0] } + let(:sast_search_max_depth) { configuration['pipeline'][1] } + let(:brakeman) { configuration['analyzers'][0] } + let(:bandit) { configuration['analyzers'][1] } + let(:sast_brakeman_level) { brakeman['variables'][0] } + + it 'parses the configuration for SAST' do + expect(secure_analyzers_prefix['default_value']).to eql('registry.gitlab.com/gitlab-org/security-products/analyzers') + expect(sast_excluded_paths['default_value']).to eql('spec, test, tests, tmp') + expect(sast_analyzer_image_tag['default_value']).to eql('2') + expect(sast_pipeline_stage['default_value']).to eql('test') + expect(sast_search_max_depth['default_value']).to eql('4') + expect(brakeman['enabled']).to be(true) + expect(sast_brakeman_level['default_value']).to eql('1') + end + + context 'while populating current values of the entities' do + context 'when .gitlab-ci.yml is present' do + it 'populates the current values from the file' do + allow(project.repository).to receive(:blob_data_at).and_return(gitlab_ci_yml_content) + expect(secure_analyzers_prefix['value']).to eql('registry.gitlab.com/gitlab-org/security-products/analyzers2') + expect(sast_excluded_paths['value']).to eql('spec, executables') + expect(sast_analyzer_image_tag['value']).to eql('2') + expect(sast_pipeline_stage['value']).to eql('our_custom_security_stage') + expect(sast_search_max_depth['value']).to eql('8') + expect(brakeman['enabled']).to be(false) + expect(bandit['enabled']).to be(true) + expect(sast_brakeman_level['value']).to eql('2') + end + + context 'SAST_DEFAULT_ANALYZERS is set' do + it 'enables analyzers correctly' do + allow(project.repository).to receive(:blob_data_at).and_return(gitlab_ci_yml_default_analyzers_content) + + expect(brakeman['enabled']).to be(false) + expect(bandit['enabled']).to be(true) + end + end + + context 'SAST_EXCLUDED_ANALYZERS is set' do + it 'enables analyzers correctly' do + allow(project.repository).to receive(:blob_data_at).and_return(gitlab_ci_yml_excluded_analyzers_content) + + expect(brakeman['enabled']).to be(false) + expect(bandit['enabled']).to be(true) + end + end + end + + context 'when .gitlab-ci.yml is absent' do + it 'populates the current values with the default values' do + allow(project.repository).to receive(:blob_data_at).and_return(nil) + expect(secure_analyzers_prefix['value']).to eql('registry.gitlab.com/gitlab-org/security-products/analyzers') + expect(sast_excluded_paths['value']).to eql('spec, test, tests, tmp') + expect(sast_analyzer_image_tag['value']).to eql('2') + expect(sast_pipeline_stage['value']).to eql('test') + expect(sast_search_max_depth['value']).to eql('4') + expect(brakeman['enabled']).to be(true) + expect(sast_brakeman_level['value']).to eql('1') + end + end + end + end +end diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index 96807fd629f..32a09e1afc8 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Snippets::CreateService do describe '#execute' do let_it_be(:user) { create(:user) } let_it_be(:admin) { create(:user, :admin) } + let(:action) { :create } let(:opts) { base_opts.merge(extra_opts) } let(:base_opts) do { @@ -309,7 +310,7 @@ RSpec.describe Snippets::CreateService do it_behaves_like 'a service that creates a snippet' it_behaves_like 'public visibility level restrictions apply' - it_behaves_like 'snippets spam check is performed' + it_behaves_like 'checking spam' it_behaves_like 'snippet create data is tracked' it_behaves_like 'an error service response when save fails' it_behaves_like 'creates repository and files' @@ -337,7 +338,7 @@ RSpec.describe Snippets::CreateService do it_behaves_like 'a service that creates a snippet' it_behaves_like 'public visibility level restrictions apply' - it_behaves_like 'snippets spam check is performed' + it_behaves_like 'checking spam' it_behaves_like 'snippet create data is tracked' it_behaves_like 'an error service response when save fails' it_behaves_like 'creates repository and files' diff --git a/spec/services/snippets/update_repository_storage_service_spec.rb b/spec/services/snippets/update_repository_storage_service_spec.rb index b2bcd620d76..6ba09a9dca9 100644 --- a/spec/services/snippets/update_repository_storage_service_spec.rb +++ b/spec/services/snippets/update_repository_storage_service_spec.rb @@ -54,13 +54,18 @@ RSpec.describe Snippets::UpdateRepositoryStorageService do end context 'when the filesystems are the same' do - let(:destination) { snippet.repository_storage } + before do + expect(Gitlab::GitalyClient).to receive(:filesystem_id).twice.and_return(SecureRandom.uuid) + end - it 'bails out and does nothing' do + it 'updates the database without trying to move the repostory', :aggregate_failures do result = subject.execute + snippet.reload - expect(result).to be_error - expect(result.message).to match(/SameFilesystemError/) + expect(result).to be_success + expect(snippet).not_to be_repository_read_only + expect(snippet.repository_storage).to eq(destination) + expect(snippet.snippet_repository.shard_name).to eq(destination) end end diff --git a/spec/services/snippets/update_service_spec.rb b/spec/services/snippets/update_service_spec.rb index a2341dc71b2..e737c00ae67 100644 --- a/spec/services/snippets/update_service_spec.rb +++ b/spec/services/snippets/update_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Snippets::UpdateService do describe '#execute', :aggregate_failures do let_it_be(:user) { create(:user) } let_it_be(:admin) { create :user, admin: true } + let(:action) { :update } let(:visibility_level) { Gitlab::VisibilityLevel::PRIVATE } let(:base_opts) do { @@ -738,11 +739,7 @@ RSpec.describe Snippets::UpdateService do it_behaves_like 'only file_name is present' it_behaves_like 'only content is present' it_behaves_like 'invalid params error response' - it_behaves_like 'snippets spam check is performed' do - before do - subject - end - end + it_behaves_like 'checking spam' context 'when snippet does not have a repository' do let!(:snippet) { create(:project_snippet, author: user, project: project) } @@ -766,11 +763,7 @@ RSpec.describe Snippets::UpdateService do it_behaves_like 'only file_name is present' it_behaves_like 'only content is present' it_behaves_like 'invalid params error response' - it_behaves_like 'snippets spam check is performed' do - before do - subject - end - end + it_behaves_like 'checking spam' context 'when snippet does not have a repository' do let!(:snippet) { create(:personal_snippet, author: user, project: project) } diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb index 8edd9406bce..371923f1518 100644 --- a/spec/services/spam/spam_action_service_spec.rb +++ b/spec/services/spam/spam_action_service_spec.rb @@ -24,41 +24,16 @@ RSpec.describe Spam::SpamActionService do issue.spam = false end - describe '#initialize' do - subject { described_class.new(spammable: issue, request: request, user: user) } - - context 'when the request is nil' do - let(:request) { nil } - - it 'assembles the options with information from the spammable' do - aggregate_failures do - expect(subject.options[:ip_address]).to eq(issue.ip_address) - expect(subject.options[:user_agent]).to eq(issue.user_agent) - expect(subject.options.key?(:referrer)).to be_falsey - end - end - end - - context 'when the request is present' do - let(:request) { double(:request, env: env) } - - it 'assembles the options with information from the spammable' do - aggregate_failures do - expect(subject.options[:ip_address]).to eq(fake_ip) - expect(subject.options[:user_agent]).to eq(fake_user_agent) - expect(subject.options[:referrer]).to eq(fake_referrer) - end - end - end - end - shared_examples 'only checks for spam if a request is provided' do context 'when request is missing' do - subject { described_class.new(spammable: issue, request: nil, user: user) } + let(:request) { nil } it "doesn't check as spam" do - subject + expect(fake_verdict_service).not_to receive(:execute) + + response = subject + expect(response.message).to match(/request was not present/) expect(issue).not_to be_spam end end @@ -66,34 +41,88 @@ RSpec.describe Spam::SpamActionService do context 'when request exists' do it 'creates a spam log' do expect { subject } - .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue') + .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue') end end end + shared_examples 'creates a spam log' do + it do + expect { subject }.to change { SpamLog.count }.by(1) + + new_spam_log = SpamLog.last + expect(new_spam_log.user_id).to eq(user.id) + expect(new_spam_log.title).to eq(issue.title) + expect(new_spam_log.description).to eq(issue.description) + expect(new_spam_log.source_ip).to eq(fake_ip) + expect(new_spam_log.user_agent).to eq(fake_user_agent) + expect(new_spam_log.noteable_type).to eq('Issue') + expect(new_spam_log.via_api).to eq(false) + end + end + describe '#execute' do let(:request) { double(:request, env: env) } + let(:fake_captcha_verification_service) { double(:captcha_verification_service) } let(:fake_verdict_service) { double(:spam_verdict_service) } let(:allowlisted) { false } + let(:api) { nil } + let(:captcha_response) { 'abc123' } + let(:spam_log_id) { existing_spam_log.id } + let(:spam_params) do + Spam::SpamActionService.filter_spam_params!( + api: api, + captcha_response: captcha_response, + spam_log_id: spam_log_id + ) + end + + let(:verdict_service_opts) do + { + ip_address: fake_ip, + user_agent: fake_user_agent, + referrer: fake_referrer + } + end + + let(:verdict_service_args) do + { + target: issue, + user: user, + request: request, + options: verdict_service_opts, + context: { + action: :create, + target_type: 'Issue' + } + } + end let_it_be(:existing_spam_log) { create(:spam_log, user: user, recaptcha_verified: false) } subject do - described_service = described_class.new(spammable: issue, request: request, user: user) + described_service = described_class.new(spammable: issue, request: request, user: user, action: :create) allow(described_service).to receive(:allowlisted?).and_return(allowlisted) - described_service.execute(api: nil, recaptcha_verified: recaptcha_verified, spam_log_id: existing_spam_log.id) + described_service.execute(spam_params: spam_params) end before do - allow(Spam::SpamVerdictService).to receive(:new).and_return(fake_verdict_service) + allow(Captcha::CaptchaVerificationService).to receive(:new) { fake_captcha_verification_service } + allow(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args).and_return(fake_verdict_service) end - context 'when reCAPTCHA was already verified' do - let(:recaptcha_verified) { true } + context 'when captcha response verification returns true' do + before do + expect(fake_captcha_verification_service) + .to receive(:execute).with(captcha_response: captcha_response, request: request) { true } + end it "doesn't check with the SpamVerdictService" do aggregate_failures do - expect(SpamLog).to receive(:verify_recaptcha!) + expect(SpamLog).to receive(:verify_recaptcha!).with( + user_id: user.id, + id: spam_log_id + ) expect(fake_verdict_service).not_to receive(:execute) end @@ -105,8 +134,11 @@ RSpec.describe Spam::SpamActionService do end end - context 'when reCAPTCHA was not verified' do - let(:recaptcha_verified) { false } + context 'when captcha response verification returns false' do + before do + expect(fake_captcha_verification_service) + .to receive(:execute).with(captcha_response: captcha_response, request: request) { false } + end context 'when spammable attributes have not changed' do before do @@ -120,6 +152,10 @@ RSpec.describe Spam::SpamActionService do end context 'when spammable attributes have changed' do + let(:expected_service_check_response_message) do + /check Issue spammable model for any errors or captcha requirement/ + end + before do issue.description = 'SPAM!' end @@ -130,7 +166,9 @@ RSpec.describe Spam::SpamActionService do it 'does not perform spam check' do expect(Spam::SpamVerdictService).not_to receive(:new) - subject + response = subject + + expect(response.message).to match(/user was allowlisted/) end end @@ -147,8 +185,9 @@ RSpec.describe Spam::SpamActionService do it_behaves_like 'only checks for spam if a request is provided' it 'marks as spam' do - subject + response = subject + expect(response.message).to match(expected_service_check_response_message) expect(issue).to be_spam end end @@ -157,8 +196,9 @@ RSpec.describe Spam::SpamActionService do it_behaves_like 'only checks for spam if a request is provided' it 'does not mark as spam' do - subject + response = subject + expect(response.message).to match(expected_service_check_response_message) expect(issue).not_to be_spam end end @@ -176,15 +216,19 @@ RSpec.describe Spam::SpamActionService do it_behaves_like 'only checks for spam if a request is provided' + it_behaves_like 'creates a spam log' + it 'does not mark as spam' do - subject + response = subject + expect(response.message).to match(expected_service_check_response_message) expect(issue).not_to be_spam end it 'marks as needing reCAPTCHA' do - subject + response = subject + expect(response.message).to match(expected_service_check_response_message) expect(issue.needs_recaptcha?).to be_truthy end end @@ -192,9 +236,12 @@ RSpec.describe Spam::SpamActionService do context 'when allow_possible_spam feature flag is true' do it_behaves_like 'only checks for spam if a request is provided' + it_behaves_like 'creates a spam log' + it 'does not mark as needing reCAPTCHA' do - subject + response = subject + expect(response.message).to match(expected_service_check_response_message) expect(issue.needs_recaptcha).to be_falsey end end @@ -209,6 +256,51 @@ RSpec.describe Spam::SpamActionService do expect { subject } .not_to change { SpamLog.count } end + + it 'clears spam flags' do + expect(issue).to receive(:clear_spam_flags!) + + subject + end + end + + context 'spam verdict service options' do + before do + allow(fake_verdict_service).to receive(:execute) { ALLOW } + end + + context 'when the request is nil' do + let(:request) { nil } + let(:issue_ip_address) { '1.2.3.4' } + let(:issue_user_agent) { 'lynx' } + let(:verdict_service_opts) do + { + ip_address: issue_ip_address, + user_agent: issue_user_agent + } + end + + before do + allow(issue).to receive(:ip_address) { issue_ip_address } + allow(issue).to receive(:user_agent) { issue_user_agent } + end + + it 'assembles the options with information from the spammable' do + # TODO: This code untestable, because we do not perform a verification if there is not a + # request. See corresponding comment in code + # expect(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args) + + subject + end + end + + context 'when the request is present' do + it 'assembles the options with information from the request' do + expect(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args) + + subject + end + end end end end diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb index 3e7594bd30f..77d0e892725 100644 --- a/spec/services/suggestions/apply_service_spec.rb +++ b/spec/services/suggestions/apply_service_spec.rb @@ -32,8 +32,8 @@ RSpec.describe Suggestions::ApplyService do create(:suggestion, :content_from_repo, suggestion_args) end - def apply(suggestions) - result = apply_service.new(user, *suggestions).execute + def apply(suggestions, custom_message = nil) + result = apply_service.new(user, *suggestions, message: custom_message).execute suggestions.map { |suggestion| suggestion.reload } @@ -74,6 +74,14 @@ RSpec.describe Suggestions::ApplyService do expect(commit.author_name).to eq(user.name) end + it 'tracks apply suggestion event' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_apply_suggestion_action) + .with(user: user) + + apply(suggestions) + end + context 'when a custom suggestion commit message' do before do project.update!(suggestion_commit_message: message) @@ -103,6 +111,16 @@ RSpec.describe Suggestions::ApplyService do end end end + + context 'with a user suggested commit message' do + let(:message) { "i'm a custom commit message!" } + + it "uses the user's commit message" do + apply(suggestions, message) + + expect(project.repository.commit.message).to(eq(message)) + end + end end subject(:apply_service) { described_class } @@ -570,56 +588,84 @@ RSpec.describe Suggestions::ApplyService do project.add_maintainer(user) end + shared_examples_for 'service not tracking apply suggestion event' do + it 'does not track apply suggestion event' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .not_to receive(:track_apply_suggestion_action) + + result + end + end + context 'diff file was not found' do - it 'returns error message' do - expect(suggestion.note).to receive(:latest_diff_file) { nil } + let(:result) { apply_service.new(user, suggestion).execute } - result = apply_service.new(user, suggestion).execute + before do + expect(suggestion.note).to receive(:latest_diff_file) { nil } + end + it 'returns error message' do expect(result).to eq(message: 'A file was not found.', status: :error) end + + it_behaves_like 'service not tracking apply suggestion event' end context 'when not all suggestions belong to the same branch' do - it 'renders error message' do - merge_request2 = create(:merge_request, - :conflict, - source_project: project, - target_project: project) - - position2 = Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", - new_path: "files/ruby/popen.rb", - old_line: nil, - new_line: 15, - diff_refs: merge_request2 - .diff_refs) + let(:merge_request2) do + create( + :merge_request, + :conflict, + source_project: project, + target_project: project + ) + end - diff_note2 = create(:diff_note_on_merge_request, - noteable: merge_request2, - position: position2, - project: project) + let(:position2) do + Gitlab::Diff::Position.new( + old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 15, + diff_refs: merge_request2.diff_refs + ) + end - other_branch_suggestion = create(:suggestion, note: diff_note2) + let(:diff_note2) do + create( + :diff_note_on_merge_request, + noteable: merge_request2, + position: position2, + project: project + ) + end - result = apply_service.new(user, suggestion, other_branch_suggestion).execute + let(:other_branch_suggestion) { create(:suggestion, note: diff_note2) } + let(:result) { apply_service.new(user, suggestion, other_branch_suggestion).execute } + it 'renders error message' do expect(result).to eq(message: 'Suggestions must all be on the same branch.', status: :error) end + + it_behaves_like 'service not tracking apply suggestion event' end context 'suggestion is not appliable' do let(:inapplicable_reason) { "Can't apply this suggestion." } + let(:result) { apply_service.new(user, suggestion).execute } - it 'returns error message' do + before do expect(suggestion).to receive(:appliable?).and_return(false) expect(suggestion).to receive(:inapplicable_reason).and_return(inapplicable_reason) + end - result = apply_service.new(user, suggestion).execute - + it 'returns error message' do expect(result).to eq(message: inapplicable_reason, status: :error) end + + it_behaves_like 'service not tracking apply suggestion event' end context 'lines of suggestions overlap' do @@ -632,12 +678,14 @@ RSpec.describe Suggestions::ApplyService do create_suggestion(to_content: "I Overlap!") end - it 'returns error message' do - result = apply_service.new(user, suggestion, overlapping_suggestion).execute + let(:result) { apply_service.new(user, suggestion, overlapping_suggestion).execute } + it 'returns error message' do expect(result).to eq(message: 'Suggestions are not applicable as their lines cannot overlap.', status: :error) end + + it_behaves_like 'service not tracking apply suggestion event' end end end diff --git a/spec/services/suggestions/create_service_spec.rb b/spec/services/suggestions/create_service_spec.rb index 80823364fe8..5148d6756fc 100644 --- a/spec/services/suggestions/create_service_spec.rb +++ b/spec/services/suggestions/create_service_spec.rb @@ -53,6 +53,15 @@ RSpec.describe Suggestions::CreateService do subject { described_class.new(note) } + shared_examples_for 'service not tracking add suggestion event' do + it 'does not track add suggestion event' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .not_to receive(:track_add_suggestion_action) + + subject.execute + end + end + describe '#execute' do context 'should not try to parse suggestions' do context 'when not a diff note for merge requests' do @@ -66,6 +75,8 @@ RSpec.describe Suggestions::CreateService do subject.execute end + + it_behaves_like 'service not tracking add suggestion event' end context 'when diff note is not for text' do @@ -76,17 +87,21 @@ RSpec.describe Suggestions::CreateService do note: markdown) end - it 'does not try to parse suggestions' do + before do allow(note).to receive(:on_text?) { false } + end + it 'does not try to parse suggestions' do expect(Gitlab::Diff::SuggestionsParser).not_to receive(:parse) subject.execute end + + it_behaves_like 'service not tracking add suggestion event' end end - context 'should not create suggestions' do + context 'when diff file is not found' do let(:note) do create(:diff_note_on_merge_request, project: project_with_repo, noteable: merge_request, @@ -94,13 +109,17 @@ RSpec.describe Suggestions::CreateService do note: markdown) end - it 'creates no suggestion when diff file is not found' do + before do expect_next_instance_of(DiffNote) do |diff_note| expect(diff_note).to receive(:latest_diff_file).once { nil } end + end + it 'creates no suggestion' do expect { subject.execute }.not_to change(Suggestion, :count) end + + it_behaves_like 'service not tracking add suggestion event' end context 'should create suggestions' do @@ -137,6 +156,14 @@ RSpec.describe Suggestions::CreateService do end end + it 'tracks add suggestion event' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_add_suggestion_action) + .with(user: note.author) + + subject.execute + end + context 'outdated position note' do let!(:outdated_diff) { merge_request.merge_request_diff } let!(:latest_diff) { merge_request.create_merge_request_diff } diff --git a/spec/services/system_hooks_service_spec.rb b/spec/services/system_hooks_service_spec.rb index 2020c67f465..1ec5237370f 100644 --- a/spec/services/system_hooks_service_spec.rb +++ b/spec/services/system_hooks_service_spec.rb @@ -45,15 +45,13 @@ RSpec.describe SystemHooksService do it do expect(event_data(group, :create)).to include( - :event_name, :name, :created_at, :updated_at, :path, :group_id, - :owner_name, :owner_email + :event_name, :name, :created_at, :updated_at, :path, :group_id ) end it do expect(event_data(group, :destroy)).to include( - :event_name, :name, :created_at, :updated_at, :path, :group_id, - :owner_name, :owner_email + :event_name, :name, :created_at, :updated_at, :path, :group_id ) end @@ -156,9 +154,6 @@ RSpec.describe SystemHooksService do it { expect(event_name(project_member, :update)).to eq "user_update_for_team" } it { expect(event_name(key, :create)).to eq 'key_create' } it { expect(event_name(key, :destroy)).to eq 'key_destroy' } - it { expect(event_name(group, :create)).to eq 'group_create' } - it { expect(event_name(group, :destroy)).to eq 'group_destroy' } - it { expect(event_name(group, :rename)).to eq 'group_rename' } end def event_data(*args) diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 9c35f9e3817..df4880dfa13 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -213,15 +213,16 @@ RSpec.describe SystemNoteService do describe '.change_branch' do it 'calls MergeRequestsService' do - old_branch = double - new_branch = double - branch_type = double + old_branch = double('old_branch') + new_branch = double('new_branch') + branch_type = double('branch_type') + event_type = double('event_type') expect_next_instance_of(::SystemNotes::MergeRequestsService) do |service| - expect(service).to receive(:change_branch).with(branch_type, old_branch, new_branch) + expect(service).to receive(:change_branch).with(branch_type, event_type, old_branch, new_branch) end - described_class.change_branch(noteable, project, author, branch_type, old_branch, new_branch) + described_class.change_branch(noteable, project, author, branch_type, event_type, old_branch, new_branch) end end diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index 96afca2f2cb..ae18bc23c17 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -729,12 +729,14 @@ RSpec.describe ::SystemNotes::IssuablesService do it 'is false with issue tracker supporting referencing' do create(:jira_service, project: project) + project.reload expect(service.cross_reference_disallowed?(noteable)).to be_falsey end it 'is true with issue tracker not supporting referencing' do create(:bugzilla_service, project: project) + project.reload expect(service.cross_reference_disallowed?(noteable)).to be_truthy end diff --git a/spec/services/system_notes/merge_requests_service_spec.rb b/spec/services/system_notes/merge_requests_service_spec.rb index 50d16231e8f..2131f3d3bdf 100644 --- a/spec/services/system_notes/merge_requests_service_spec.rb +++ b/spec/services/system_notes/merge_requests_service_spec.rb @@ -167,18 +167,38 @@ RSpec.describe ::SystemNotes::MergeRequestsService do end describe '.change_branch' do - subject { service.change_branch('target', old_branch, new_branch) } - let(:old_branch) { 'old_branch'} let(:new_branch) { 'new_branch'} it_behaves_like 'a system note' do let(:action) { 'branch' } + + subject { service.change_branch('target', 'update', old_branch, new_branch) } end context 'when target branch name changed' do - it 'sets the note text' do - expect(subject.note).to eq "changed target branch from `#{old_branch}` to `#{new_branch}`" + context 'on update' do + subject { service.change_branch('target', 'update', old_branch, new_branch) } + + it 'sets the note text' do + expect(subject.note).to eq "changed target branch from `#{old_branch}` to `#{new_branch}`" + end + end + + context 'on delete' do + subject { service.change_branch('target', 'delete', old_branch, new_branch) } + + it 'sets the note text' do + expect(subject.note).to eq "changed automatically target branch to `#{new_branch}` because `#{old_branch}` was deleted" + end + end + + context 'for invalid event_type' do + subject { service.change_branch('target', 'invalid', old_branch, new_branch) } + + it 'raises exception' do + expect { subject }.to raise_error /invalid value for event_type/ + end end end end diff --git a/spec/services/test_hooks/project_service_spec.rb b/spec/services/test_hooks/project_service_spec.rb index 7470bdff527..a87e612e378 100644 --- a/spec/services/test_hooks/project_service_spec.rb +++ b/spec/services/test_hooks/project_service_spec.rb @@ -3,11 +3,13 @@ require 'spec_helper' RSpec.describe TestHooks::ProjectService do + include AfterNextHelpers + let(:current_user) { create(:user) } describe '#execute' do - let(:project) { create(:project, :repository) } - let(:hook) { create(:project_hook, project: project) } + let_it_be(:project) { create(:project, :repository) } + let(:hook) { create(:project_hook, project: project) } let(:trigger) { 'not_implemented_events' } let(:service) { described_class.new(hook, current_user, trigger) } let(:sample_data) { { data: 'sample' } } @@ -61,17 +63,17 @@ RSpec.describe TestHooks::ProjectService do end it 'executes hook' do - allow(project).to receive(:notes).and_return([Note.new]) + create(:note, project: project) + allow(Gitlab::DataBuilder::Note).to receive(:build).and_return(sample_data) + allow_next(NotesFinder).to receive(:execute).and_return(Note.all) expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) expect(service.execute).to include(success_result) end end - context 'issues_events' do - let(:trigger) { 'issues_events' } - let(:trigger_key) { :issue_hooks } + shared_examples_for 'a test webhook that operates on issues' do let(:issue) { build(:issue) } it 'returns error message if not enough data' do @@ -80,36 +82,32 @@ RSpec.describe TestHooks::ProjectService do end it 'executes hook' do - allow(project).to receive(:issues).and_return([issue]) allow(issue).to receive(:to_hook_data).and_return(sample_data) + allow_next(IssuesFinder).to receive(:execute).and_return([issue]) expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) expect(service.execute).to include(success_result) end end + context 'issues_events' do + let(:trigger) { 'issues_events' } + let(:trigger_key) { :issue_hooks } + + it_behaves_like 'a test webhook that operates on issues' + end + context 'confidential_issues_events' do let(:trigger) { 'confidential_issues_events' } let(:trigger_key) { :confidential_issue_hooks } - let(:issue) { build(:issue) } - it 'returns error message if not enough data' do - expect(hook).not_to receive(:execute) - expect(service.execute).to include({ status: :error, message: 'Ensure the project has issues.' }) - end - - it 'executes hook' do - allow(project).to receive(:issues).and_return([issue]) - allow(issue).to receive(:to_hook_data).and_return(sample_data) - - expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) - expect(service.execute).to include(success_result) - end + it_behaves_like 'a test webhook that operates on issues' end context 'merge_requests_events' do let(:trigger) { 'merge_requests_events' } let(:trigger_key) { :merge_request_hooks } + let(:merge_request) { build(:merge_request) } it 'returns error message if not enough data' do expect(hook).not_to receive(:execute) @@ -117,8 +115,8 @@ RSpec.describe TestHooks::ProjectService do end it 'executes hook' do - create(:merge_request, source_project: project) - allow_any_instance_of(MergeRequest).to receive(:to_hook_data).and_return(sample_data) + allow(merge_request).to receive(:to_hook_data).and_return(sample_data) + allow_next(MergeRequestsFinder).to receive(:execute).and_return([merge_request]) expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) expect(service.execute).to include(success_result) @@ -128,6 +126,7 @@ RSpec.describe TestHooks::ProjectService do context 'job_events' do let(:trigger) { 'job_events' } let(:trigger_key) { :job_hooks } + let(:ci_job) { build(:ci_build) } it 'returns error message if not enough data' do expect(hook).not_to receive(:execute) @@ -135,8 +134,8 @@ RSpec.describe TestHooks::ProjectService do end it 'executes hook' do - create(:ci_build, project: project) allow(Gitlab::DataBuilder::Build).to receive(:build).and_return(sample_data) + allow_next(Ci::JobsFinder).to receive(:execute).and_return([ci_job]) expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) expect(service.execute).to include(success_result) @@ -146,6 +145,7 @@ RSpec.describe TestHooks::ProjectService do context 'pipeline_events' do let(:trigger) { 'pipeline_events' } let(:trigger_key) { :pipeline_hooks } + let(:pipeline) { build(:ci_empty_pipeline) } it 'returns error message if not enough data' do expect(hook).not_to receive(:execute) @@ -153,8 +153,8 @@ RSpec.describe TestHooks::ProjectService do end it 'executes hook' do - create(:ci_empty_pipeline, project: project) allow(Gitlab::DataBuilder::Pipeline).to receive(:build).and_return(sample_data) + allow_next(Ci::PipelinesFinder).to receive(:execute).and_return([pipeline]) expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) expect(service.execute).to include(success_result) @@ -162,7 +162,7 @@ RSpec.describe TestHooks::ProjectService do end context 'wiki_page_events' do - let(:project) { create(:project, :wiki_repo) } + let_it_be(:project) { create(:project, :wiki_repo) } let(:trigger) { 'wiki_page_events' } let(:trigger_key) { :wiki_page_hooks } @@ -190,6 +190,7 @@ RSpec.describe TestHooks::ProjectService do context 'releases_events' do let(:trigger) { 'releases_events' } let(:trigger_key) { :release_hooks } + let(:release) { build(:release) } it 'returns error message if not enough data' do expect(hook).not_to receive(:execute) @@ -197,8 +198,8 @@ RSpec.describe TestHooks::ProjectService do end it 'executes hook' do - allow(project).to receive(:releases).and_return([Release.new]) - allow_any_instance_of(Release).to receive(:to_hook_data).and_return(sample_data) + allow(release).to receive(:to_hook_data).and_return(sample_data) + allow_next(ReleasesFinder).to receive(:execute).and_return([release]) expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) expect(service.execute).to include(success_result) diff --git a/spec/services/test_hooks/system_service_spec.rb b/spec/services/test_hooks/system_service_spec.rb index 34dd2173b09..e500a1057ab 100644 --- a/spec/services/test_hooks/system_service_spec.rb +++ b/spec/services/test_hooks/system_service_spec.rb @@ -3,12 +3,12 @@ require 'spec_helper' RSpec.describe TestHooks::SystemService do - let(:current_user) { create(:user) } + include AfterNextHelpers describe '#execute' do - let(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository) } let(:hook) { create(:system_hook) } - let(:service) { described_class.new(hook, current_user, trigger) } + let(:service) { described_class.new(hook, project.owner, trigger) } let(:success_result) { { status: :success, http_status: 200, message: 'ok' } } before do @@ -63,6 +63,9 @@ RSpec.describe TestHooks::SystemService do context 'merge_requests_events' do let(:trigger) { 'merge_requests_events' } + let(:trigger_key) { :merge_request_hooks } + let(:merge_request) { build(:merge_request) } + let(:sample_data) { { data: 'sample' } } it 'returns error message if the user does not have any repository with a merge request' do expect(hook).not_to receive(:execute) @@ -70,12 +73,8 @@ RSpec.describe TestHooks::SystemService do end it 'executes hook' do - trigger_key = :merge_request_hooks - sample_data = { data: 'sample' } - create(:project_member, user: current_user, project: project) - create(:merge_request, source_project: project) - allow_any_instance_of(MergeRequest).to receive(:to_hook_data).and_return(sample_data) - + expect(MergeRequest).to receive(:of_projects).and_return([merge_request]) + expect(merge_request).to receive(:to_hook_data).and_return(sample_data) expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) expect(service.execute).to include(success_result) end diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 83d233a8112..743dc080b06 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -1193,6 +1193,17 @@ RSpec.describe TodoService do end end + describe '#create_request_review_todo' do + let(:target) { create(:merge_request, author: author, source_project: project) } + let(:reviewer) { create(:user) } + + it 'creates a todo for reviewer' do + service.create_request_review_todo(target, author, reviewer) + + should_create_todo(user: reviewer, target: target, action: Todo::REVIEW_REQUESTED) + end + end + def should_create_todo(attributes = {}) attributes.reverse_merge!( project: project, diff --git a/spec/services/users/approve_service_spec.rb b/spec/services/users/approve_service_spec.rb index 55b2c83f4a8..9999e674c7d 100644 --- a/spec/services/users/approve_service_spec.rb +++ b/spec/services/users/approve_service_spec.rb @@ -33,7 +33,7 @@ RSpec.describe Users::ApproveService do it 'returns error result' do expect(subject[:status]).to eq(:error) expect(subject[:message]) - .to match(/The user you are trying to approve is not pending an approval/) + .to match(/The user you are trying to approve is not pending approval/) end end @@ -61,6 +61,14 @@ RSpec.describe Users::ApproveService do expect(user.reload).to be_active end + it 'logs approval in application logs' do + allow(Gitlab::AppLogger).to receive(:info) + + subject + + expect(Gitlab::AppLogger).to have_received(:info).with(message: "User instance access request approved", user: "#{user.username}", email: "#{user.email}", approved_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") + end + it 'emails the user on approval' do expect(DeviseMailer).to receive(:user_admin_approval).with(user).and_call_original expect { subject }.to have_enqueued_mail(DeviseMailer, :user_admin_approval) @@ -82,6 +90,20 @@ RSpec.describe Users::ApproveService do .not_to have_enqueued_mail(DeviseMailer, :confirmation_instructions) end end + + context 'audit events' do + context 'when not licensed' do + before do + stub_licensed_features( + admin_audit_log: false + ) + end + + it 'does not log any audit event' do + expect { subject }.not_to change(AuditEvent, :count) + end + end + end end context 'pending invitations' do diff --git a/spec/services/users/batch_status_cleaner_service_spec.rb b/spec/services/users/batch_status_cleaner_service_spec.rb new file mode 100644 index 00000000000..46a004542d8 --- /dev/null +++ b/spec/services/users/batch_status_cleaner_service_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::BatchStatusCleanerService do + let_it_be(:user_status_1) { create(:user_status, emoji: 'coffee', message: 'msg1', clear_status_at: 1.year.ago) } + let_it_be(:user_status_2) { create(:user_status, emoji: 'coffee', message: 'msg1', clear_status_at: 1.year.from_now) } + let_it_be(:user_status_3) { create(:user_status, emoji: 'coffee', message: 'msg1', clear_status_at: 2.years.ago) } + let_it_be(:user_status_4) { create(:user_status, emoji: 'coffee', message: 'msg1') } + + subject(:result) { described_class.execute } + + it 'cleans up scheduled user statuses' do + expect(result[:deleted_rows]).to eq(2) + + deleted_statuses = UserStatus.where(user_id: [user_status_1.user_id, user_status_3.user_id]) + expect(deleted_statuses).to be_empty + end + + it 'does not affect rows with future clear_status_at' do + expect { result }.not_to change { user_status_2.reload } + end + + it 'does not affect rows without clear_status_at' do + expect { result }.not_to change { user_status_4.reload } + end + + describe 'batch_size' do + it 'clears status in batches' do + result = described_class.execute(batch_size: 1) + + expect(result[:deleted_rows]).to eq(1) + + result = described_class.execute(batch_size: 1) + + expect(result[:deleted_rows]).to eq(1) + + result = described_class.execute(batch_size: 1) + + expect(result[:deleted_rows]).to eq(0) + end + end +end diff --git a/spec/services/users/build_service_spec.rb b/spec/services/users/build_service_spec.rb index 446741221b3..b2a7d349ce6 100644 --- a/spec/services/users/build_service_spec.rb +++ b/spec/services/users/build_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Users::BuildService do + using RSpec::Parameterized::TableSyntax + describe '#execute' do let(:params) { build_stubbed(:user).slice(:first_name, :last_name, :username, :email, :password) } @@ -72,8 +74,6 @@ RSpec.describe Users::BuildService do end context 'with "user_default_external" application setting' do - using RSpec::Parameterized::TableSyntax - where(:user_default_external, :external, :email, :user_default_internal_regex, :result) do true | nil | 'fl@example.com' | nil | true true | true | 'fl@example.com' | nil | true @@ -192,8 +192,6 @@ RSpec.describe Users::BuildService do end context 'with "user_default_external" application setting' do - using RSpec::Parameterized::TableSyntax - where(:user_default_external, :external, :email, :user_default_internal_regex, :result) do true | nil | 'fl@example.com' | nil | true true | true | 'fl@example.com' | nil | true diff --git a/spec/services/users/refresh_authorized_projects_service_spec.rb b/spec/services/users/refresh_authorized_projects_service_spec.rb index 9404668e3c5..cc01b22f9d2 100644 --- a/spec/services/users/refresh_authorized_projects_service_spec.rb +++ b/spec/services/users/refresh_authorized_projects_service_spec.rb @@ -143,6 +143,21 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do expect(authorizations[0].project_id).to eq(project.id) expect(authorizations[0].access_level).to eq(Gitlab::Access::MAINTAINER) end + + it 'logs the details of the refresh' do + source = :foo + service = described_class.new(user, source: source) + user.project_authorizations.delete_all + + expect(Gitlab::AppJsonLogger).to( + receive(:info) + .with(event: 'authorized_projects_refresh', + 'authorized_projects_refresh.source': source, + 'authorized_projects_refresh.rows_deleted': 0, + 'authorized_projects_refresh.rows_added': 1)) + + service.update_authorizations([], [[user.id, project.id, Gitlab::Access::MAINTAINER]]) + end end describe '#fresh_access_levels_per_project' do diff --git a/spec/services/users/reject_service_spec.rb b/spec/services/users/reject_service_spec.rb index 07863d1a290..b9aaff5cde5 100644 --- a/spec/services/users/reject_service_spec.rb +++ b/spec/services/users/reject_service_spec.rb @@ -48,6 +48,26 @@ RSpec.describe Users::RejectService do subject end + + it 'logs rejection in application logs' do + allow(Gitlab::AppLogger).to receive(:info) + + subject + + expect(Gitlab::AppLogger).to have_received(:info).with(message: "User instance access request rejected", user: "#{user.username}", email: "#{user.email}", rejected_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") + end + end + end + + context 'audit events' do + context 'when not licensed' do + before do + stub_licensed_features(admin_audit_log: false) + end + + it 'does not log any audit event' do + expect { subject }.not_to change(AuditEvent, :count) + end end end end |