diff options
Diffstat (limited to 'spec/services')
102 files changed, 2967 insertions, 1205 deletions
diff --git a/spec/services/admin/set_feature_flag_service_spec.rb b/spec/services/admin/set_feature_flag_service_spec.rb new file mode 100644 index 00000000000..6fa806644c9 --- /dev/null +++ b/spec/services/admin/set_feature_flag_service_spec.rb @@ -0,0 +1,300 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Admin::SetFeatureFlagService do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:group) { create(:group) } + + let(:feature_name) { known_feature_flag.name } + let(:service) { described_class.new(feature_flag_name: feature_name, params: params) } + + # Find any `development` feature flag name + let(:known_feature_flag) do + Feature::Definition.definitions + .values.find(&:development?) + end + + describe '#execute' do + before do + Feature.reset + Flipper.unregister_groups + Flipper.register(:perf_team) do |actor| + actor.respond_to?(:admin) && actor.admin? + end + end + + subject { service.execute } + + context 'when enabling the feature flag' do + let(:params) { { value: 'true' } } + + it 'enables the feature flag' do + expect(Feature).to receive(:enable).with(feature_name) + expect(subject).to be_success + + feature_flag = subject.payload[:feature_flag] + expect(feature_flag.name).to eq(feature_name) + end + + it 'logs the event' do + expect(Feature.logger).to receive(:info).once + + subject + end + + context 'when enabling for a user actor' do + let(:params) { { value: 'true', user: user.username } } + + it 'enables the feature flag' do + expect(Feature).to receive(:enable).with(feature_name, user) + expect(subject).to be_success + end + + context 'when user does not exist' do + let(:params) { { value: 'true', user: 'unknown-user' } } + + it 'does nothing' do + expect(Feature).not_to receive(:enable) + expect(subject).to be_error + expect(subject.reason).to eq(:actor_not_found) + end + end + end + + context 'when enabling for a feature group' do + let(:params) { { value: 'true', feature_group: 'perf_team' } } + let(:feature_group) { Feature.group('perf_team') } + + it 'enables the feature flag' do + expect(Feature).to receive(:enable).with(feature_name, feature_group) + expect(subject).to be_success + end + end + + context 'when enabling for a project' do + let(:params) { { value: 'true', project: project.full_path } } + + it 'enables the feature flag' do + expect(Feature).to receive(:enable).with(feature_name, project) + expect(subject).to be_success + end + end + + context 'when enabling for a group' do + let(:params) { { value: 'true', group: group.full_path } } + + it 'enables the feature flag' do + expect(Feature).to receive(:enable).with(feature_name, group) + expect(subject).to be_success + end + + context 'when group does not exist' do + let(:params) { { value: 'true', group: 'unknown-group' } } + + it 'returns an error' do + expect(Feature).not_to receive(:disable) + expect(subject).to be_error + expect(subject.reason).to eq(:actor_not_found) + end + end + end + + context 'when enabling for a user namespace' do + let(:namespace) { user.namespace } + let(:params) { { value: 'true', namespace: namespace.full_path } } + + it 'enables the feature flag' do + expect(Feature).to receive(:enable).with(feature_name, namespace) + expect(subject).to be_success + end + + context 'when namespace does not exist' do + let(:params) { { value: 'true', namespace: 'unknown-namespace' } } + + it 'returns an error' do + expect(Feature).not_to receive(:disable) + expect(subject).to be_error + expect(subject.reason).to eq(:actor_not_found) + end + end + end + + context 'when enabling for a group namespace' do + let(:params) { { value: 'true', namespace: group.full_path } } + + it 'enables the feature flag' do + expect(Feature).to receive(:enable).with(feature_name, group) + expect(subject).to be_success + end + end + + context 'when enabling for a user actor and a feature group' do + let(:params) { { value: 'true', user: user.username, feature_group: 'perf_team' } } + let(:feature_group) { Feature.group('perf_team') } + + it 'enables the feature flag' do + expect(Feature).to receive(:enable).with(feature_name, user) + expect(Feature).to receive(:enable).with(feature_name, feature_group) + expect(subject).to be_success + end + end + + context 'when enabling given a percentage of time' do + let(:params) { { value: '50' } } + + it 'enables the feature flag' do + expect(Feature).to receive(:enable_percentage_of_time).with(feature_name, 50) + expect(subject).to be_success + end + + context 'when value is a float' do + let(:params) { { value: '0.01' } } + + it 'enables the feature flag' do + expect(Feature).to receive(:enable_percentage_of_time).with(feature_name, 0.01) + expect(subject).to be_success + end + end + end + + context 'when enabling given a percentage of actors' do + let(:params) { { value: '50', key: 'percentage_of_actors' } } + + it 'enables the feature flag' do + expect(Feature).to receive(:enable_percentage_of_actors).with(feature_name, 50) + expect(subject).to be_success + end + + context 'when value is a float' do + let(:params) { { value: '0.01', key: 'percentage_of_actors' } } + + it 'enables the feature flag' do + expect(Feature).to receive(:enable_percentage_of_actors).with(feature_name, 0.01) + expect(subject).to be_success + end + end + end + end + + context 'when disabling the feature flag' do + before do + Feature.enable(feature_name) + end + + let(:params) { { value: 'false' } } + + it 'disables the feature flag' do + expect(Feature).to receive(:disable).with(feature_name) + expect(subject).to be_success + + feature_flag = subject.payload[:feature_flag] + expect(feature_flag.name).to eq(feature_name) + end + + it 'logs the event' do + expect(Feature.logger).to receive(:info).once + + subject + end + + context 'when disabling for a user actor' do + let(:params) { { value: 'false', user: user.username } } + + it 'disables the feature flag' do + expect(Feature).to receive(:disable).with(feature_name, user) + expect(subject).to be_success + end + + context 'when user does not exist' do + let(:params) { { value: 'false', user: 'unknown-user' } } + + it 'returns an error' do + expect(Feature).not_to receive(:disable) + expect(subject).to be_error + expect(subject.reason).to eq(:actor_not_found) + end + end + end + + context 'when disabling for a feature group' do + let(:params) { { value: 'false', feature_group: 'perf_team' } } + let(:feature_group) { Feature.group('perf_team') } + + it 'disables the feature flag' do + expect(Feature).to receive(:disable).with(feature_name, feature_group) + expect(subject).to be_success + end + end + + context 'when disabling for a project' do + let(:params) { { value: 'false', project: project.full_path } } + + it 'disables the feature flag' do + expect(Feature).to receive(:disable).with(feature_name, project) + expect(subject).to be_success + end + end + + context 'when disabling for a group' do + let(:params) { { value: 'false', group: group.full_path } } + + it 'disables the feature flag' do + expect(Feature).to receive(:disable).with(feature_name, group) + expect(subject).to be_success + end + + context 'when group does not exist' do + let(:params) { { value: 'false', group: 'unknown-group' } } + + it 'returns an error' do + expect(Feature).not_to receive(:disable) + expect(subject).to be_error + expect(subject.reason).to eq(:actor_not_found) + end + end + end + + context 'when disabling for a user namespace' do + let(:namespace) { user.namespace } + let(:params) { { value: 'false', namespace: namespace.full_path } } + + it 'disables the feature flag' do + expect(Feature).to receive(:disable).with(feature_name, namespace) + expect(subject).to be_success + end + + context 'when namespace does not exist' do + let(:params) { { value: 'false', namespace: 'unknown-namespace' } } + + it 'returns an error' do + expect(Feature).not_to receive(:disable) + expect(subject).to be_error + expect(subject.reason).to eq(:actor_not_found) + end + end + end + + context 'when disabling for a group namespace' do + let(:params) { { value: 'false', namespace: group.full_path } } + + it 'disables the feature flag' do + expect(Feature).to receive(:disable).with(feature_name, group) + expect(subject).to be_success + end + end + + context 'when disabling for a user actor and a feature group' do + let(:params) { { value: 'false', user: user.username, feature_group: 'perf_team' } } + let(:feature_group) { Feature.group('perf_team') } + + it 'disables the feature flag' do + expect(Feature).to receive(:disable).with(feature_name, user) + expect(Feature).to receive(:disable).with(feature_name, feature_group) + expect(subject).to be_success + end + end + end + end +end diff --git a/spec/services/alert_management/create_alert_issue_service_spec.rb b/spec/services/alert_management/create_alert_issue_service_spec.rb index 083e5b8c6f1..7255a722d26 100644 --- a/spec/services/alert_management/create_alert_issue_service_spec.rb +++ b/spec/services/alert_management/create_alert_issue_service_spec.rb @@ -81,7 +81,7 @@ RSpec.describe AlertManagement::CreateAlertIssueService do it 'checks permissions' do execute - expect(user).to have_received(:can?).with(:create_issue, project) + expect(user).to have_received(:can?).with(:create_issue, project).exactly(2).times end context 'with alert severity' do @@ -161,7 +161,7 @@ RSpec.describe AlertManagement::CreateAlertIssueService do it 'has an unsuccessful status' do expect(execute).to be_error - expect(execute.message).to eq("Title can't be blank") + expect(execute.errors).to contain_exactly("Title can't be blank") end end @@ -170,7 +170,7 @@ RSpec.describe AlertManagement::CreateAlertIssueService do it 'responds with error' do expect(execute).to be_error - expect(execute.message).to eq('Hosts hosts array is over 255 chars') + expect(execute.errors).to contain_exactly('Hosts hosts array is over 255 chars') end end diff --git a/spec/services/award_emojis/copy_service_spec.rb b/spec/services/award_emojis/copy_service_spec.rb index e85c548968e..abb9c65e25d 100644 --- a/spec/services/award_emojis/copy_service_spec.rb +++ b/spec/services/award_emojis/copy_service_spec.rb @@ -4,10 +4,12 @@ require 'spec_helper' RSpec.describe AwardEmojis::CopyService do let_it_be(:from_awardable) do - create(:issue, award_emoji: [ - build(:award_emoji, name: 'thumbsup'), - build(:award_emoji, name: 'thumbsdown') - ]) + create( + :issue, + award_emoji: [ + build(:award_emoji, name: 'thumbsup'), + build(:award_emoji, name: 'thumbsdown') + ]) end describe '#initialize' do diff --git a/spec/services/boards/issues/create_service_spec.rb b/spec/services/boards/issues/create_service_spec.rb index 9a6b48c13bf..c4f1eb093dc 100644 --- a/spec/services/boards/issues/create_service_spec.rb +++ b/spec/services/boards/issues/create_service_spec.rb @@ -29,9 +29,10 @@ RSpec.describe Boards::Issues::CreateService do end it 'adds the label of the list to the issue' do - issue = service.execute + result = service.execute - expect(issue.labels).to eq [label] + expect(result).to be_success + expect(result[:issue].labels).to contain_exactly(label) end end end diff --git a/spec/services/boards/lists/generate_service_spec.rb b/spec/services/boards/lists/generate_service_spec.rb deleted file mode 100644 index 9597c8e0f54..00000000000 --- a/spec/services/boards/lists/generate_service_spec.rb +++ /dev/null @@ -1,45 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Boards::Lists::GenerateService do - describe '#execute' do - let(:project) { create(:project) } - let(:board) { create(:board, project: project) } - let(:user) { create(:user) } - - subject(:service) { described_class.new(project, user) } - - before do - project.add_developer(user) - end - - context 'when board lists is empty' do - it 'creates the default lists' do - expect { service.execute(board) }.to change(board.lists, :count).by(2) - end - end - - context 'when board lists is not empty' do - it 'does not creates the default lists' do - create(:list, board: board) - - expect { service.execute(board) }.not_to change(board.lists, :count) - end - end - - context 'when project labels does not contains any list label' do - it 'creates labels' do - expect { service.execute(board) }.to change(project.labels, :count).by(2) - end - end - - context 'when project labels contains some of list label' do - it 'creates the missing labels' do - create(:label, project: project, name: 'Doing') - - expect { service.execute(board) }.to change(project.labels, :count).by(1) - end - end - end -end diff --git a/spec/services/boards/lists/list_service_spec.rb b/spec/services/boards/lists/list_service_spec.rb index 0c8a8dc7329..2d41de42581 100644 --- a/spec/services/boards/lists/list_service_spec.rb +++ b/spec/services/boards/lists/list_service_spec.rb @@ -3,13 +3,40 @@ require 'spec_helper' RSpec.describe Boards::Lists::ListService do - let(:user) { create(:user) } + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + + RSpec.shared_examples 'FOSS lists only' do + context 'when board contains a non FOSS list' do + # This scenario may happen when there used to be an EE license and user downgraded + let!(:backlog_list) { create_backlog_list(board) } + let_it_be(:milestone) { create(:milestone, group: group) } + let_it_be(:assignee_list) do + list = build(:list, board: board, user_id: user.id, list_type: List.list_types[:assignee], position: 0) + list.save!(validate: false) + list + end + + let_it_be(:milestone_list) do + list = build(:list, board: board, milestone_id: milestone.id, list_type: List.list_types[:milestone], position: 1) # rubocop:disable Layout/LineLength + list.save!(validate: false) + list + end + + it "returns only FOSS board's lists" do + # just making sure these non FOSS lists actually exist on the board + expect(board.lists.with_types([List.list_types[:assignee], List.list_types[:milestone]]).count).to eq 2 + # check that the FOSS lists are not returned from the service + expect(service.execute(board)).to match_array [backlog_list, list, board.lists.closed.first] + end + end + end describe '#execute' do let(:service) { described_class.new(parent, user) } context 'when board parent is a project' do - let_it_be(:project) { create(:project) } + let_it_be(:project) { create(:project, group: group) } let_it_be_with_reload(:board) { create(:board, project: project) } let_it_be(:label) { create(:label, project: project) } let_it_be(:list) { create(:list, board: board, label: label) } @@ -18,10 +45,10 @@ RSpec.describe Boards::Lists::ListService do let(:parent) { project } it_behaves_like 'lists list service' + it_behaves_like 'FOSS lists only' end context 'when board parent is a group' do - let_it_be(:group) { create(:group) } let_it_be_with_reload(:board) { create(:board, group: group) } let_it_be(:label) { create(:group_label, group: group) } let_it_be(:list) { create(:list, board: board, label: label) } @@ -30,6 +57,7 @@ RSpec.describe Boards::Lists::ListService do let(:parent) { group } it_behaves_like 'lists list service' + it_behaves_like 'FOSS lists only' end def create_backlog_list(board) diff --git a/spec/services/bulk_imports/create_pipeline_trackers_service_spec.rb b/spec/services/bulk_imports/create_pipeline_trackers_service_spec.rb index d7b00ba04ab..0de962328c5 100644 --- a/spec/services/bulk_imports/create_pipeline_trackers_service_spec.rb +++ b/spec/services/bulk_imports/create_pipeline_trackers_service_spec.rb @@ -75,7 +75,9 @@ RSpec.describe BulkImports::CreatePipelineTrackersService do expect_next_instance_of(Gitlab::Import::Logger) do |logger| expect(logger).to receive(:info).with({ message: 'Pipeline skipped as source instance version not compatible with pipeline', - entity_id: entity.id, + bulk_import_entity_id: entity.id, + bulk_import_id: entity.bulk_import_id, + importer: 'gitlab_migration', pipeline_name: 'PipelineClass4', minimum_source_version: '15.1.0', maximum_source_version: nil, @@ -84,7 +86,9 @@ RSpec.describe BulkImports::CreatePipelineTrackersService do expect(logger).to receive(:info).with({ message: 'Pipeline skipped as source instance version not compatible with pipeline', - entity_id: entity.id, + bulk_import_entity_id: entity.id, + bulk_import_id: entity.bulk_import_id, + importer: 'gitlab_migration', pipeline_name: 'PipelineClass5', minimum_source_version: '16.0.0', maximum_source_version: nil, diff --git a/spec/services/bulk_imports/create_service_spec.rb b/spec/services/bulk_imports/create_service_spec.rb index 4b655dd5d6d..bf174f5d5a2 100644 --- a/spec/services/bulk_imports/create_service_spec.rb +++ b/spec/services/bulk_imports/create_service_spec.rb @@ -50,6 +50,11 @@ RSpec.describe BulkImports::CreateService do expect(last_bulk_import.user).to eq(user) expect(last_bulk_import.source_version).to eq(source_version.to_s) expect(last_bulk_import.user).to eq(user) + expect_snowplow_event( + category: 'BulkImports::CreateService', + action: 'create', + label: 'bulk_import_group' + ) end it 'creates bulk import entities' do diff --git a/spec/services/bulk_imports/repository_bundle_export_service_spec.rb b/spec/services/bulk_imports/repository_bundle_export_service_spec.rb index a7d98a7474a..f0d63de1ab9 100644 --- a/spec/services/bulk_imports/repository_bundle_export_service_spec.rb +++ b/spec/services/bulk_imports/repository_bundle_export_service_spec.rb @@ -17,6 +17,7 @@ RSpec.describe BulkImports::RepositoryBundleExportService do context 'when repository exists' do it 'bundles repository to disk' do allow(repository).to receive(:exists?).and_return(true) + allow(repository).to receive(:empty?).and_return(false) expect(repository).to receive(:bundle_to_disk).with(File.join(export_path, "#{export_filename}.bundle")) service.execute @@ -31,6 +32,15 @@ RSpec.describe BulkImports::RepositoryBundleExportService do service.execute end end + + context 'when repository is empty' do + it 'does not bundle repository to disk' do + allow(repository).to receive(:empty?).and_return(true) + expect(repository).not_to receive(:bundle_to_disk) + + service.execute + end + end end include_examples 'repository export' do diff --git a/spec/services/bulk_imports/uploads_export_service_spec.rb b/spec/services/bulk_imports/uploads_export_service_spec.rb index 39bcacfdc5e..ad6e005485c 100644 --- a/spec/services/bulk_imports/uploads_export_service_spec.rb +++ b/spec/services/bulk_imports/uploads_export_service_spec.rb @@ -3,9 +3,11 @@ require 'spec_helper' RSpec.describe BulkImports::UploadsExportService do - let_it_be(:project) { create(:project, avatar: fixture_file_upload('spec/fixtures/rails_sample.png', 'image/png')) } - let_it_be(:upload) { create(:upload, :with_file, :issuable_upload, uploader: FileUploader, model: project) } let_it_be(:export_path) { Dir.mktmpdir } + let_it_be(:project) { create(:project, avatar: fixture_file_upload('spec/fixtures/rails_sample.png', 'image/png')) } + + let!(:upload) { create(:upload, :with_file, :issuable_upload, uploader: FileUploader, model: project) } + let(:exported_filepath) { File.join(export_path, upload.secret, upload.retrieve_uploader.filename) } subject(:service) { described_class.new(project, export_path) } @@ -15,10 +17,60 @@ RSpec.describe BulkImports::UploadsExportService do describe '#execute' do it 'exports project uploads and avatar' do - subject.execute + service.execute + + expect(File).to exist(File.join(export_path, 'avatar', 'rails_sample.png')) + expect(File).to exist(exported_filepath) + end + + context 'when upload has underlying file missing' do + context 'with an upload missing its file' do + it 'does not cause errors' do + File.delete(upload.absolute_path) + + expect { service.execute }.not_to raise_error + + expect(File).not_to exist(exported_filepath) + end + end + + context 'when upload is in object storage' do + before do + stub_uploads_object_storage(FileUploader) + end + + shared_examples 'export with invalid upload' do + it 'ignores problematic upload and logs exception' do + allow(service).to receive(:download_or_copy_upload).and_raise(exception) + + expect(Gitlab::ErrorTracking) + .to receive(:log_exception) + .with( + instance_of(exception), { + portable_id: project.id, + portable_class: 'Project', + upload_id: upload.id + } + ) + + service.execute + + expect(File).not_to exist(exported_filepath) + end + end + + context 'when filename is too long' do + let(:exception) { Errno::ENAMETOOLONG } + + include_examples 'export with invalid upload' + end + + context 'when network exception occurs' do + let(:exception) { Net::OpenTimeout } - expect(File.exist?(File.join(export_path, 'avatar', 'rails_sample.png'))).to eq(true) - expect(File.exist?(File.join(export_path, upload.secret, upload.retrieve_uploader.filename))).to eq(true) + include_examples 'export with invalid upload' + end + end end end end diff --git a/spec/services/bulk_update_integration_service_spec.rb b/spec/services/bulk_update_integration_service_spec.rb index 7c5bd1db565..24a868b524d 100644 --- a/spec/services/bulk_update_integration_service_spec.rb +++ b/spec/services/bulk_update_integration_service_spec.rb @@ -11,8 +11,8 @@ RSpec.describe BulkUpdateIntegrationService do let(:excluded_attributes) do %w[ - id project_id group_id inherit_from_id instance template - created_at updated_at encrypted_properties encrypted_properties_iv + id project_id group_id inherit_from_id instance template + created_at updated_at encrypted_properties encrypted_properties_iv ] end diff --git a/spec/services/ci/compare_test_reports_service_spec.rb b/spec/services/ci/compare_test_reports_service_spec.rb index 01d58b2095f..6d3df0f5383 100644 --- a/spec/services/ci/compare_test_reports_service_spec.rb +++ b/spec/services/ci/compare_test_reports_service_spec.rb @@ -72,10 +72,11 @@ RSpec.describe Ci::CompareTestReportsService do it 'loads recent failures on limited test cases to avoid building up a huge DB query', :aggregate_failures do expect(comparison[:data]).to match_schema('entities/test_reports_comparer') - expect(recent_failures_per_test_case).to eq([ - { 'count' => 1, 'base_branch' => 'master' }, - { 'count' => 1, 'base_branch' => 'master' } - ]) + expect(recent_failures_per_test_case).to eq( + [ + { 'count' => 1, 'base_branch' => 'master' }, + { 'count' => 1, 'base_branch' => 'master' } + ]) expect(new_failures.count).to eq(2) end end diff --git a/spec/services/ci/create_pipeline_service/include_spec.rb b/spec/services/ci/create_pipeline_service/include_spec.rb index 67d8530525a..3764663fd74 100644 --- a/spec/services/ci/create_pipeline_service/include_spec.rb +++ b/spec/services/ci/create_pipeline_service/include_spec.rb @@ -126,51 +126,5 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes it_behaves_like 'not including the file' end end - - context 'with ci_increase_includes_to_250 enabled on root project' do - let_it_be(:included_project) do - create(:project, :repository).tap { |p| p.add_developer(user) } - end - - before do - stub_const('::Gitlab::Ci::Config::External::Context::MAX_INCLUDES', 0) - stub_const('::Gitlab::Ci::Config::External::Context::TRIAL_MAX_INCLUDES', 3) - - stub_feature_flags(ci_increase_includes_to_250: false) - stub_feature_flags(ci_increase_includes_to_250: project) - - allow(Project) - .to receive(:find_by_full_path) - .with(included_project.full_path) - .and_return(included_project) - - allow(included_project.repository) - .to receive(:blob_data_at).with(included_project.commit.id, '.gitlab-ci.yml') - .and_return(local_config) - - allow(included_project.repository) - .to receive(:blob_data_at).with(included_project.commit.id, file_location) - .and_return(File.read(Rails.root.join(file_location))) - end - - let(:config) do - <<~EOY - include: - - project: #{included_project.full_path} - file: .gitlab-ci.yml - EOY - end - - let(:local_config) do - <<~EOY - include: #{file_location} - - job: - script: exit 0 - EOY - end - - it_behaves_like 'including the file' - end end end diff --git a/spec/services/ci/create_pipeline_service/limit_active_jobs_spec.rb b/spec/services/ci/create_pipeline_service/limit_active_jobs_spec.rb new file mode 100644 index 00000000000..003d109a27c --- /dev/null +++ b/spec/services/ci/create_pipeline_service/limit_active_jobs_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectness do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { project.first_owner } + let_it_be(:existing_pipeline) { create(:ci_pipeline, project: project) } + + let(:service) { described_class.new(project, user, ref: 'refs/heads/master') } + + subject(:pipeline) { service.execute(:push).payload } + + before do + create_list(:ci_build, 8, pipeline: existing_pipeline) + create_list(:ci_bridge, 1, pipeline: existing_pipeline) + + stub_ci_pipeline_yaml_file(<<~YAML) + job1: + script: echo + job3: + trigger: + project: org/my-project + job4: + script: echo + only: [tags] + YAML + end + + context 'when project has exceeded the active jobs limit' do + before do + project.namespace.actual_limits.update!(ci_active_jobs: 10) + end + + it 'fails the pipeline before populating it' do + expect(pipeline).to be_failed + expect(pipeline).to be_job_activity_limit_exceeded + + expect(pipeline.errors.full_messages) + .to include("Project exceeded the allowed number of jobs in active pipelines. Retry later.") + expect(pipeline.statuses).to be_empty + end + end + + context 'when project has not exceeded the active jobs limit' do + before do + project.namespace.actual_limits.update!(ci_active_jobs: 20) + end + + it 'creates the pipeline successfully' do + expect(pipeline).to be_created + end + end +end diff --git a/spec/services/ci/create_pipeline_service/logger_spec.rb b/spec/services/ci/create_pipeline_service/logger_spec.rb index 2be23802757..3045f8e92b1 100644 --- a/spec/services/ci/create_pipeline_service/logger_spec.rb +++ b/spec/services/ci/create_pipeline_service/logger_spec.rb @@ -20,6 +20,7 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes { 'count' => a_kind_of(Numeric), 'avg' => a_kind_of(Numeric), + 'sum' => a_kind_of(Numeric), 'max' => a_kind_of(Numeric), 'min' => a_kind_of(Numeric) } diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index fc57ca66d3a..c737b8cc329 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -540,19 +540,10 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes let(:compare_to) { 'invalid-branch' } it 'returns an error' do - expect(pipeline.errors.full_messages).to eq([ - 'Failed to parse rule for job1: rules:changes:compare_to is not a valid ref' - ]) - end - - context 'when the FF ci_rules_changes_compare is not enabled' do - before do - stub_feature_flags(ci_rules_changes_compare: false) - end - - it 'ignores compare_to and changes is always true' do - expect(build_names).to contain_exactly('job1', 'job2') - end + expect(pipeline.errors.full_messages).to eq( + [ + 'Failed to parse rule for job1: rules:changes:compare_to is not a valid ref' + ]) end end @@ -563,16 +554,6 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes it 'creates job1 and job2' do expect(build_names).to contain_exactly('job1', 'job2') end - - context 'when the FF ci_rules_changes_compare is not enabled' do - before do - stub_feature_flags(ci_rules_changes_compare: false) - end - - it 'ignores compare_to and changes is always true' do - expect(build_names).to contain_exactly('job1', 'job2') - end - end end context 'when the rule does not match' do @@ -581,16 +562,6 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes it 'does not create job1' do expect(build_names).to contain_exactly('job2') end - - context 'when the FF ci_rules_changes_compare is not enabled' do - before do - stub_feature_flags(ci_rules_changes_compare: false) - end - - it 'ignores compare_to and changes is always true' do - expect(build_names).to contain_exactly('job1', 'job2') - end - end end end end @@ -616,17 +587,6 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes expect(pipeline).to be_created_successfully expect(build_names).to contain_exactly('job1') end - - context 'when the FF ci_rules_changes_compare is not enabled' do - before do - stub_feature_flags(ci_rules_changes_compare: false) - end - - it 'ignores compare_to and changes is always true' do - expect(pipeline).to be_created_successfully - expect(build_names).to contain_exactly('job1') - end - end end context 'when the rule does not match' do diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index c2e80316d26..458692ba1c0 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -293,7 +293,7 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes pipeline_on_previous_commit .builds .joins(:metadata) - .pluck(:name, 'ci_builds_metadata.interruptible') + .pluck(:name, "#{Ci::BuildMetadata.quoted_table_name}.interruptible") expect(interruptible_status).to contain_exactly( ['build_1_1', true], @@ -423,7 +423,7 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes expect(response.message).to eq('Missing CI config file') expect(response.payload).not_to be_persisted expect(Ci::Pipeline.count).to eq(0) - expect(Namespaces::OnboardingPipelineCreatedWorker).not_to receive(:perform_async) + expect(Onboarding::PipelineCreatedWorker).not_to receive(:perform_async) end shared_examples 'a failed pipeline' do @@ -1547,7 +1547,7 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes end it 'schedules a namespace onboarding create action worker' do - expect(Namespaces::OnboardingPipelineCreatedWorker) + expect(Onboarding::PipelineCreatedWorker) .to receive(:perform_async).with(project.namespace_id) pipeline diff --git a/spec/services/ci/find_exposed_artifacts_service_spec.rb b/spec/services/ci/find_exposed_artifacts_service_spec.rb index 32d96471f16..6e11c153a75 100644 --- a/spec/services/ci/find_exposed_artifacts_service_spec.rb +++ b/spec/services/ci/find_exposed_artifacts_service_spec.rb @@ -157,20 +157,21 @@ RSpec.describe Ci::FindExposedArtifactsService do subject { described_class.new(project, user).for_pipeline(pipeline, limit: 2) } it 'returns first 2 results' do - expect(subject).to eq([ - { - text: 'artifact 1', - url: file_project_job_artifacts_path(project, job1, 'ci_artifacts.txt'), - job_name: job1.name, - job_path: project_job_path(project, job1) - }, - { - text: 'artifact 2', - url: browse_project_job_artifacts_path(project, job2), - job_name: job2.name, - job_path: project_job_path(project, job2) - } - ]) + expect(subject).to eq( + [ + { + text: 'artifact 1', + url: file_project_job_artifacts_path(project, job1, 'ci_artifacts.txt'), + job_name: job1.name, + job_path: project_job_path(project, job1) + }, + { + text: 'artifact 2', + url: browse_project_job_artifacts_path(project, job2), + job_name: job2.name, + job_path: project_job_path(project, job2) + } + ]) end end @@ -199,20 +200,21 @@ RSpec.describe Ci::FindExposedArtifactsService do subject { described_class.new(project, user).for_pipeline(pipeline, limit: 2) } it 'returns the correct path for cross-project MRs' do - expect(subject).to eq([ - { - text: 'file artifact', - url: file_project_job_artifacts_path(foreign_project, job_show, 'ci_artifacts.txt'), - job_name: job_show.name, - job_path: project_job_path(foreign_project, job_show) - }, - { - text: 'directory artifact', - url: browse_project_job_artifacts_path(foreign_project, job_browse), - job_name: job_browse.name, - job_path: project_job_path(foreign_project, job_browse) - } - ]) + expect(subject).to eq( + [ + { + text: 'file artifact', + url: file_project_job_artifacts_path(foreign_project, job_show, 'ci_artifacts.txt'), + job_name: job_show.name, + job_path: project_job_path(foreign_project, job_show) + }, + { + text: 'directory artifact', + url: browse_project_job_artifacts_path(foreign_project, job_browse), + job_name: job_browse.name, + job_path: project_job_path(foreign_project, job_browse) + } + ]) end end end diff --git a/spec/services/ci/generate_kubeconfig_service_spec.rb b/spec/services/ci/generate_kubeconfig_service_spec.rb index e3088ca6ea7..bfde39780dd 100644 --- a/spec/services/ci/generate_kubeconfig_service_spec.rb +++ b/spec/services/ci/generate_kubeconfig_service_spec.rb @@ -9,6 +9,8 @@ RSpec.describe Ci::GenerateKubeconfigService do let(:pipeline) { build.pipeline } let(:agent1) { create(:cluster_agent, project: project) } let(:agent2) { create(:cluster_agent) } + let(:authorization1) { create(:agent_project_authorization, agent: agent1) } + let(:authorization2) { create(:agent_project_authorization, agent: agent2) } let(:template) { instance_double(Gitlab::Kubernetes::Kubeconfig::Template) } @@ -16,7 +18,7 @@ RSpec.describe Ci::GenerateKubeconfigService do before do expect(Gitlab::Kubernetes::Kubeconfig::Template).to receive(:new).and_return(template) - expect(pipeline).to receive(:authorized_cluster_agents).and_return([agent1, agent2]) + expect(pipeline).to receive(:cluster_agent_authorizations).and_return([authorization1, authorization2]) end it 'adds a cluster, and a user and context for each available agent' do @@ -36,11 +38,13 @@ RSpec.describe Ci::GenerateKubeconfigService do expect(template).to receive(:add_context).with( name: "#{project.full_path}:#{agent1.name}", + namespace: 'production', cluster: 'gitlab', user: "agent:#{agent1.id}" ) expect(template).to receive(:add_context).with( name: "#{agent2.project.full_path}:#{agent2.name}", + namespace: 'production', cluster: 'gitlab', user: "agent:#{agent2.id}" ) diff --git a/spec/services/ci/job_artifacts/create_service_spec.rb b/spec/services/ci/job_artifacts/create_service_spec.rb index a2259f9813b..030ba84951e 100644 --- a/spec/services/ci/job_artifacts/create_service_spec.rb +++ b/spec/services/ci/job_artifacts/create_service_spec.rb @@ -182,7 +182,8 @@ RSpec.describe Ci::JobArtifacts::CreateService do end context 'with job partitioning' do - let(:job) { create(:ci_build, project: project, partition_id: 123) } + let(:pipeline) { create(:ci_pipeline, project: project, partition_id: 123) } + let(:job) { create(:ci_build, pipeline: pipeline) } it 'sets partition_id on artifacts' do expect { subject }.to change { Ci::JobArtifact.count } diff --git a/spec/services/ci/job_artifacts/delete_service_spec.rb b/spec/services/ci/job_artifacts/delete_service_spec.rb index 62a755eb44a..78e8be48255 100644 --- a/spec/services/ci/job_artifacts/delete_service_spec.rb +++ b/spec/services/ci/job_artifacts/delete_service_spec.rb @@ -14,6 +14,7 @@ RSpec.describe Ci::JobArtifacts::DeleteService do result = service.execute expect(result).to be_success + expect(result[:destroyed_artifacts_count]).to be(2) end it 'deletes erasable artifacts' do @@ -24,7 +25,7 @@ RSpec.describe Ci::JobArtifacts::DeleteService do expect { service.execute }.not_to change { build.has_trace? }.from(true) end - context 'when project is undergoing statistics refresh' do + context 'when project is undergoing stats refresh' do before do allow(build.project).to receive(:refreshing_build_artifacts_size?).and_return(true) end @@ -36,6 +37,30 @@ RSpec.describe Ci::JobArtifacts::DeleteService do service.execute end + + it 'returns an error response with the correct message and reason' do + result = service.execute + + expect(result).to be_error + expect(result[:message]).to be('Action temporarily disabled. ' \ + 'The project this job belongs to is undergoing stats refresh.') + expect(result[:reason]).to be(:project_stats_refresh) + end + end + + context 'when an error response is received from DestroyBatchService' do + before do + allow_next_instance_of(Ci::JobArtifacts::DestroyBatchService) do |service| + allow(service).to receive(:execute).and_return({ status: :error, message: 'something went wrong' }) + end + end + + it 'returns an error response with the correct message' do + result = service.execute + + expect(result).to be_error + expect(result[:message]).to be('something went wrong') + end end end end diff --git a/spec/services/ci/job_token_scope/add_project_service_spec.rb b/spec/services/ci/job_token_scope/add_project_service_spec.rb index bb6df4268dd..bf7df3a5595 100644 --- a/spec/services/ci/job_token_scope/add_project_service_spec.rb +++ b/spec/services/ci/job_token_scope/add_project_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Ci::JobTokenScope::AddProjectService do let(:service) { described_class.new(project, current_user) } - let_it_be(:project) { create(:project, ci_job_token_scope_enabled: true).tap(&:save!) } + let_it_be(:project) { create(:project, ci_outbound_job_token_scope_enabled: true).tap(&:save!) } let_it_be(:target_project) { create(:project) } let_it_be(:current_user) { create(:user) } diff --git a/spec/services/ci/job_token_scope/remove_project_service_spec.rb b/spec/services/ci/job_token_scope/remove_project_service_spec.rb index 155e60ac48e..c3f9081cbd8 100644 --- a/spec/services/ci/job_token_scope/remove_project_service_spec.rb +++ b/spec/services/ci/job_token_scope/remove_project_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Ci::JobTokenScope::RemoveProjectService do let(:service) { described_class.new(project, current_user) } - let_it_be(:project) { create(:project, ci_job_token_scope_enabled: true).tap(&:save!) } + let_it_be(:project) { create(:project, ci_outbound_job_token_scope_enabled: true).tap(&:save!) } let_it_be(:target_project) { create(:project) } let_it_be(:current_user) { create(:user) } diff --git a/spec/services/ci/pipeline_artifacts/coverage_report_service_spec.rb b/spec/services/ci/pipeline_artifacts/coverage_report_service_spec.rb index 6d4dcf28108..c4558bddc85 100644 --- a/spec/services/ci/pipeline_artifacts/coverage_report_service_spec.rb +++ b/spec/services/ci/pipeline_artifacts/coverage_report_service_spec.rb @@ -35,6 +35,7 @@ RSpec.describe Ci::PipelineArtifacts::CoverageReportService do end it 'logs relevant information' do + allow(Gitlab::AppLogger).to receive(:info).and_call_original expect(Gitlab::AppLogger).to receive(:info).with({ project_id: project.id, pipeline_id: pipeline.id, @@ -52,28 +53,12 @@ RSpec.describe Ci::PipelineArtifacts::CoverageReportService do it_behaves_like 'creating or updating a pipeline coverage report' - context 'when ci_update_unlocked_pipeline_artifacts feature flag is enabled' do - it "artifact has pipeline's locked status" do - subject - - artifact = Ci::PipelineArtifact.first - - expect(artifact.locked).to eq(pipeline.locked) - end - end + it "artifact has pipeline's locked status" do + subject - context 'when ci_update_unlocked_pipeline_artifacts is disabled' do - before do - stub_feature_flags(ci_update_unlocked_pipeline_artifacts: false) - end - - it 'artifact has unknown locked status' do - subject + artifact = Ci::PipelineArtifact.first - artifact = Ci::PipelineArtifact.first - - expect(artifact.locked).to eq('unknown') - end + expect(artifact.locked).to eq(pipeline.locked) 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 index 75233248113..5d854b61f14 100644 --- 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 @@ -51,28 +51,12 @@ RSpec.describe ::Ci::PipelineArtifacts::CreateCodeQualityMrDiffReportService do end end - context 'when ci_update_unlocked_pipeline_artifacts feature flag is enabled' do - it "artifact has pipeline's locked status" do - subject - - artifact = Ci::PipelineArtifact.first - - expect(artifact.locked).to eq(head_pipeline.locked) - end - end - - context 'when ci_update_unlocked_pipeline_artifacts is disabled' do - before do - stub_feature_flags(ci_update_unlocked_pipeline_artifacts: false) - end - - it 'artifact has unknown locked status' do - subject + it "artifact has pipeline's locked status" do + subject - artifact = Ci::PipelineArtifact.first + artifact = Ci::PipelineArtifact.first - expect(artifact.locked).to eq('unknown') - end + expect(artifact.locked).to eq(head_pipeline.locked) end it 'does not persist the same artifact twice' do diff --git a/spec/services/ci/pipeline_artifacts/destroy_all_expired_service_spec.rb b/spec/services/ci/pipeline_artifacts/destroy_all_expired_service_spec.rb index eb664043567..47e8766c215 100644 --- a/spec/services/ci/pipeline_artifacts/destroy_all_expired_service_spec.rb +++ b/spec/services/ci/pipeline_artifacts/destroy_all_expired_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::PipelineArtifacts::DestroyAllExpiredService do +RSpec.describe Ci::PipelineArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_shared_state do let(:service) { described_class.new } describe '.execute' do @@ -85,6 +85,36 @@ RSpec.describe Ci::PipelineArtifacts::DestroyAllExpiredService do is_expected.to eq(0) end end + + context 'with unlocked pipeline artifacts' do + let_it_be(:not_expired_artifact) { create(:ci_pipeline_artifact, :artifact_unlocked, expire_at: 2.days.from_now) } + + before do + create_list(:ci_pipeline_artifact, 2, :artifact_unlocked, expire_at: 1.week.ago) + allow(service).to receive(:legacy_destroy_pipeline_artifacts) + end + + it 'destroys all expired artifacts' do + expect { subject }.to change { Ci::PipelineArtifact.count }.by(-2) + expect(not_expired_artifact.reload).to be_present + end + + context 'when the loop limit is reached' do + before do + stub_const('::Ci::PipelineArtifacts::DestroyAllExpiredService::LOOP_LIMIT', 1) + stub_const('::Ci::PipelineArtifacts::DestroyAllExpiredService::BATCH_SIZE', 1) + end + + it 'destroys one artifact' do + expect { subject }.to change { Ci::PipelineArtifact.count }.by(-1) + expect(not_expired_artifact.reload).to be_present + end + + it 'reports the number of destroyed artifacts' do + is_expected.to eq(1) + end + end + end end describe '.destroy_artifacts_batch' do diff --git a/spec/services/ci/runners/register_runner_service_spec.rb b/spec/services/ci/runners/register_runner_service_spec.rb index 6d7b39de21e..2d1b109072f 100644 --- a/spec/services/ci/runners/register_runner_service_spec.rb +++ b/spec/services/ci/runners/register_runner_service_spec.rb @@ -9,7 +9,6 @@ RSpec.describe ::Ci::Runners::RegisterRunnerService, '#execute' do let(:runner) { execute.payload[:runner] } before do - stub_feature_flags(runner_registration_control: false) stub_application_setting(runners_registration_token: registration_token) stub_application_setting(valid_runner_registrars: ApplicationSetting::VALID_RUNNER_REGISTRAR_TYPES) end @@ -166,25 +165,9 @@ RSpec.describe ::Ci::Runners::RegisterRunnerService, '#execute' do stub_application_setting(valid_runner_registrars: ['group']) end - context 'when feature flag is enabled' do - before do - stub_feature_flags(runner_registration_control: true) - end - - it 'returns 403 error' do - expect(execute).to be_error - expect(execute.http_status).to eq :forbidden - end - end - - context 'when feature flag is disabled' do - it 'registers the runner' do - expect(execute).to be_success - - expect(runner).to be_an_instance_of(::Ci::Runner) - expect(runner.errors).to be_empty - expect(runner.active).to be true - end + it 'returns 403 error' do + expect(execute).to be_error + expect(execute.http_status).to eq :forbidden end end end @@ -244,24 +227,8 @@ RSpec.describe ::Ci::Runners::RegisterRunnerService, '#execute' do stub_application_setting(valid_runner_registrars: ['project']) end - context 'when feature flag is enabled' do - before do - stub_feature_flags(runner_registration_control: true) - end - - it 'returns error response' do - is_expected.to be_error - end - end - - context 'when feature flag is disabled' do - it 'registers the runner' do - expect(execute).to be_success - - expect(runner).to be_an_instance_of(::Ci::Runner) - expect(runner.errors).to be_empty - expect(runner.active).to be true - end + it 'returns error response' do + is_expected.to be_error end end end diff --git a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb index 0d2e237c87b..1f44612947b 100644 --- a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb +++ b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb @@ -47,7 +47,11 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute' do it 'reassigns associated projects and returns success response' do expect(execute).to be_success - expect(runner.reload.projects.ids).to eq([owner_project.id] + project_ids) + + runner.reload + + expect(runner.owner_project).to eq(owner_project) + expect(runner.projects.ids).to match_array([owner_project.id] + project_ids) end end @@ -56,7 +60,11 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute' do it 'reassigns associated projects and returns success response' do expect(execute).to be_success - expect(runner.reload.projects.ids).to eq([owner_project.id] + project_ids) + + runner.reload + + expect(runner.owner_project).to eq(owner_project) + expect(runner.projects.ids).to match_array([owner_project.id] + project_ids) end end end diff --git a/spec/services/ci/unlock_artifacts_service_spec.rb b/spec/services/ci/unlock_artifacts_service_spec.rb index 776019f03f8..f21afc7fe9e 100644 --- a/spec/services/ci/unlock_artifacts_service_spec.rb +++ b/spec/services/ci/unlock_artifacts_service_spec.rb @@ -5,15 +5,11 @@ require 'spec_helper' RSpec.describe Ci::UnlockArtifactsService do using RSpec::Parameterized::TableSyntax - where(:tag, :ci_update_unlocked_job_artifacts, :ci_update_unlocked_pipeline_artifacts) do - false | false | false - false | true | false - true | false | false - true | true | false - false | false | true - false | true | true - true | false | true - true | true | true + where(:tag, :ci_update_unlocked_job_artifacts) do + false | false + false | true + true | false + true | true end with_them do @@ -35,8 +31,7 @@ RSpec.describe Ci::UnlockArtifactsService do before do stub_const("#{described_class}::BATCH_SIZE", 1) - stub_feature_flags(ci_update_unlocked_job_artifacts: ci_update_unlocked_job_artifacts, - ci_update_unlocked_pipeline_artifacts: ci_update_unlocked_pipeline_artifacts) + stub_feature_flags(ci_update_unlocked_job_artifacts: ci_update_unlocked_job_artifacts) end describe '#execute' do @@ -80,7 +75,7 @@ RSpec.describe Ci::UnlockArtifactsService do end it 'unlocks pipeline artifact records' do - if ci_update_unlocked_job_artifacts && ci_update_unlocked_pipeline_artifacts + if ci_update_unlocked_job_artifacts expect { execute }.to change { ::Ci::PipelineArtifact.artifact_unlocked.count }.from(0).to(1) else expect { execute }.not_to change { ::Ci::PipelineArtifact.artifact_unlocked.count } @@ -122,7 +117,7 @@ RSpec.describe Ci::UnlockArtifactsService do end it 'unlocks pipeline artifact records' do - if ci_update_unlocked_job_artifacts && ci_update_unlocked_pipeline_artifacts + if ci_update_unlocked_job_artifacts expect { execute }.to change { ::Ci::PipelineArtifact.artifact_unlocked.count }.from(0).to(1) else expect { execute }.not_to change { ::Ci::PipelineArtifact.artifact_unlocked.count } diff --git a/spec/services/clusters/applications/destroy_service_spec.rb b/spec/services/clusters/applications/destroy_service_spec.rb deleted file mode 100644 index 7306256e68e..00000000000 --- a/spec/services/clusters/applications/destroy_service_spec.rb +++ /dev/null @@ -1,63 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Clusters::Applications::DestroyService, '#execute' do - let(:cluster) { create(:cluster, :project, :provided_by_gcp) } - let(:user) { create(:user) } - let(:params) { { application: 'prometheus' } } - let(:service) { described_class.new(cluster, user, params) } - let(:test_request) { double } - let(:worker_class) { Clusters::Applications::UninstallWorker } - - subject { service.execute(test_request) } - - before do - allow(worker_class).to receive(:perform_async) - end - - context 'application is not installed' do - it 'raises Clusters::Applications::BaseService::InvalidApplicationError' do - expect(worker_class).not_to receive(:perform_async) - - expect { subject } - .to raise_exception { Clusters::Applications::BaseService::InvalidApplicationError } - .and not_change { Clusters::Applications::Prometheus.count } - .and not_change { Clusters::Applications::Prometheus.with_status(:scheduled).count } - end - end - - context 'application is installed' do - context 'application is schedulable' do - let!(:application) do - create(:clusters_applications_prometheus, :installed, cluster: cluster) - end - - it 'makes application scheduled!' do - subject - - expect(application.reload).to be_scheduled - end - - it 'schedules UninstallWorker' do - expect(worker_class).to receive(:perform_async).with(application.name, application.id) - - subject - end - end - - context 'application is not schedulable' do - let!(:application) do - create(:clusters_applications_prometheus, :updating, cluster: cluster) - end - - it 'raises StateMachines::InvalidTransition' do - expect(worker_class).not_to receive(:perform_async) - - expect { subject } - .to raise_exception { StateMachines::InvalidTransition } - .and not_change { Clusters::Applications::Prometheus.with_status(:scheduled).count } - end - end - end -end diff --git a/spec/services/clusters/applications/uninstall_service_spec.rb b/spec/services/clusters/applications/uninstall_service_spec.rb deleted file mode 100644 index bfe38ba670d..00000000000 --- a/spec/services/clusters/applications/uninstall_service_spec.rb +++ /dev/null @@ -1,77 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Clusters::Applications::UninstallService, '#execute' do - let(:application) { create(:clusters_applications_prometheus, :scheduled) } - let(:service) { described_class.new(application) } - let(:helm_client) { instance_double(Gitlab::Kubernetes::Helm::API) } - let(:worker_class) { Clusters::Applications::WaitForUninstallAppWorker } - - before do - allow(service).to receive(:helm_api).and_return(helm_client) - end - - context 'when there are no errors' do - before do - expect(helm_client).to receive(:uninstall).with(kind_of(Gitlab::Kubernetes::Helm::V3::DeleteCommand)) - allow(worker_class).to receive(:perform_in).and_return(nil) - end - - it 'make the application to be uninstalling' do - expect(application.cluster).not_to be_nil - service.execute - - expect(application).to be_uninstalling - end - - it 'schedule async installation status check' do - expect(worker_class).to receive(:perform_in).once - - service.execute - end - end - - context 'when k8s cluster communication fails' do - let(:error) { Kubeclient::HttpError.new(500, 'system failure', nil) } - - before do - expect(helm_client).to receive(:uninstall).with(kind_of(Gitlab::Kubernetes::Helm::V3::DeleteCommand)).and_raise(error) - end - - include_examples 'logs kubernetes errors' do - let(:error_name) { 'Kubeclient::HttpError' } - let(:error_message) { 'system failure' } - let(:error_code) { 500 } - end - - it 'make the application errored' do - service.execute - - expect(application).to be_uninstall_errored - expect(application.status_reason).to match('Kubernetes error: 500') - end - end - - context 'a non kubernetes error happens' do - let(:application) { create(:clusters_applications_prometheus, :scheduled) } - let(:error) { StandardError.new('something bad happened') } - - before do - expect(helm_client).to receive(:uninstall).with(kind_of(Gitlab::Kubernetes::Helm::V3::DeleteCommand)).and_raise(error) - end - - include_examples 'logs kubernetes errors' do - let(:error_name) { 'StandardError' } - let(:error_message) { 'something bad happened' } - let(:error_code) { nil } - end - - it 'make the application errored' do - service.execute - - expect(application).to be_uninstall_errored - expect(application.status_reason).to eq('Failed to uninstall.') - end - end -end diff --git a/spec/services/design_management/move_designs_service_spec.rb b/spec/services/design_management/move_designs_service_spec.rb index c8abce77325..519378a8dd4 100644 --- a/spec/services/design_management/move_designs_service_spec.rb +++ b/spec/services/design_management/move_designs_service_spec.rb @@ -88,23 +88,24 @@ RSpec.describe DesignManagement::MoveDesignsService do expect(subject).to be_success - expect(issue.designs.ordered).to eq([ - # Existing designs which already had a relative_position set. - # These should stay at the beginning, in the same order. - other_design1, - other_design2, - - # The designs we're passing into the service. - # These should be placed between the existing designs, in the correct order. - previous_design, - current_design, - next_design, - - # Existing designs which didn't have a relative_position set. - # These should be placed at the end, in the order of their IDs. - other_design3, - other_design4 - ]) + expect(issue.designs.ordered).to eq( + [ + # Existing designs which already had a relative_position set. + # These should stay at the beginning, in the same order. + other_design1, + other_design2, + + # The designs we're passing into the service. + # These should be placed between the existing designs, in the correct order. + previous_design, + current_design, + next_design, + + # Existing designs which didn't have a relative_position set. + # These should be placed at the end, in the order of their IDs. + other_design3, + other_design4 + ]) end end end diff --git a/spec/services/git/tag_hooks_service_spec.rb b/spec/services/git/tag_hooks_service_spec.rb index 2d50c64d63c..01a0d2e8600 100644 --- a/spec/services/git/tag_hooks_service_spec.rb +++ b/spec/services/git/tag_hooks_service_spec.rb @@ -104,12 +104,12 @@ RSpec.describe Git::TagHooksService, :service do id: commit.id, message: commit.safe_message, url: [ - Gitlab.config.gitlab.url, - project.namespace.to_param, - project.to_param, - '-', - 'commit', - commit.id + Gitlab.config.gitlab.url, + project.namespace.to_param, + project.to_param, + '-', + 'commit', + commit.id ].join('/') ) end diff --git a/spec/services/google_cloud/enable_cloudsql_service_spec.rb b/spec/services/google_cloud/enable_cloudsql_service_spec.rb index f267f6d3bc2..aa6d2402d7c 100644 --- a/spec/services/google_cloud/enable_cloudsql_service_spec.rb +++ b/spec/services/google_cloud/enable_cloudsql_service_spec.rb @@ -4,15 +4,29 @@ require 'spec_helper' RSpec.describe GoogleCloud::EnableCloudsqlService do let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be(:params) do + { + google_oauth2_token: 'mock-token', + gcp_project_id: 'mock-gcp-project-id', + environment_name: 'main' + } + end - subject(:result) { described_class.new(project).execute } + subject(:result) { described_class.new(project, user, params).execute } context 'when a project does not have any GCP_PROJECT_IDs configured' do - it 'returns error' do - message = 'No GCP projects found. Configure a service account or GCP_PROJECT_ID CI variable.' + it 'creates GCP_PROJECT_ID project var' do + expect_next_instance_of(GoogleApi::CloudPlatform::Client) do |instance| + expect(instance).to receive(:enable_cloud_sql_admin).with('mock-gcp-project-id') + expect(instance).to receive(:enable_compute).with('mock-gcp-project-id') + expect(instance).to receive(:enable_service_networking).with('mock-gcp-project-id') + end - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq(message) + expect(result[:status]).to eq(:success) + expect(project.variables.count).to eq(1) + expect(project.variables.first.key).to eq('GCP_PROJECT_ID') + expect(project.variables.first.value).to eq('mock-gcp-project-id') end end @@ -30,6 +44,9 @@ RSpec.describe GoogleCloud::EnableCloudsqlService do it 'enables cloudsql, compute and service networking Google APIs', :aggregate_failures do expect_next_instance_of(GoogleApi::CloudPlatform::Client) do |instance| + expect(instance).to receive(:enable_cloud_sql_admin).with('mock-gcp-project-id') + expect(instance).to receive(:enable_compute).with('mock-gcp-project-id') + expect(instance).to receive(:enable_service_networking).with('mock-gcp-project-id') expect(instance).to receive(:enable_cloud_sql_admin).with('prj-prod') expect(instance).to receive(:enable_compute).with('prj-prod') expect(instance).to receive(:enable_service_networking).with('prj-prod') @@ -44,6 +61,9 @@ RSpec.describe GoogleCloud::EnableCloudsqlService do context 'when Google APIs raise an error' do it 'returns error result' do allow_next_instance_of(GoogleApi::CloudPlatform::Client) do |instance| + allow(instance).to receive(:enable_cloud_sql_admin).with('mock-gcp-project-id') + allow(instance).to receive(:enable_compute).with('mock-gcp-project-id') + allow(instance).to receive(:enable_service_networking).with('mock-gcp-project-id') allow(instance).to receive(:enable_cloud_sql_admin).with('prj-prod') allow(instance).to receive(:enable_compute).with('prj-prod') allow(instance).to receive(:enable_service_networking).with('prj-prod') diff --git a/spec/services/google_cloud/setup_cloudsql_instance_service_spec.rb b/spec/services/google_cloud/setup_cloudsql_instance_service_spec.rb index e0a622bfa4a..0a0f05ab4be 100644 --- a/spec/services/google_cloud/setup_cloudsql_instance_service_spec.rb +++ b/spec/services/google_cloud/setup_cloudsql_instance_service_spec.rb @@ -8,17 +8,19 @@ RSpec.describe GoogleCloud::SetupCloudsqlInstanceService do let(:list_databases_empty) { Google::Apis::SqladminV1beta4::ListDatabasesResponse.new(items: []) } let(:list_users_empty) { Google::Apis::SqladminV1beta4::ListUsersResponse.new(items: []) } let(:list_databases) do - Google::Apis::SqladminV1beta4::ListDatabasesResponse.new(items: [ - Google::Apis::SqladminV1beta4::Database.new(name: 'postgres'), - Google::Apis::SqladminV1beta4::Database.new(name: 'main_db') - ]) + Google::Apis::SqladminV1beta4::ListDatabasesResponse.new( + items: [ + Google::Apis::SqladminV1beta4::Database.new(name: 'postgres'), + Google::Apis::SqladminV1beta4::Database.new(name: 'main_db') + ]) end let(:list_users) do - Google::Apis::SqladminV1beta4::ListUsersResponse.new(items: [ - Google::Apis::SqladminV1beta4::User.new(name: 'postgres'), - Google::Apis::SqladminV1beta4::User.new(name: 'main_user') - ]) + Google::Apis::SqladminV1beta4::ListUsersResponse.new( + items: [ + Google::Apis::SqladminV1beta4::User.new(name: 'postgres'), + Google::Apis::SqladminV1beta4::User.new(name: 'main_user') + ]) end context 'when unauthorized user triggers worker' do diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index 9288793cc7a..36e868fa5f1 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -117,12 +117,6 @@ RSpec.describe Groups::DestroyService do Sidekiq::Testing.fake! { destroy_group(group, user, true) } end - after do - # Clean up stale directories - TestEnv.rm_storage_dir(project.repository_storage, group.path) - TestEnv.rm_storage_dir(project.repository_storage, remove_path) - end - it 'verifies original paths and projects still exist' do expect(TestEnv.storage_dir_exists?(project.repository_storage, group.path)).to be_truthy expect(TestEnv.storage_dir_exists?(project.repository_storage, remove_path)).to be_falsey diff --git a/spec/services/groups/import_export/import_service_spec.rb b/spec/services/groups/import_export/import_service_spec.rb index a4dfec4723a..66b50704939 100644 --- a/spec/services/groups/import_export/import_service_spec.rb +++ b/spec/services/groups/import_export/import_service_spec.rb @@ -86,6 +86,16 @@ RSpec.describe Groups::ImportExport::ImportService do service.execute end + + it 'tracks the event' do + service.execute + + expect_snowplow_event( + category: 'Groups::ImportExport::ImportService', + action: 'create', + label: 'import_group_from_file' + ) + end end context 'with a ndjson file' do @@ -105,12 +115,11 @@ RSpec.describe Groups::ImportExport::ImportService do context 'when importing a ndjson export' do let(:user) { create(:user) } let(:group) { create(:group) } - let(:service) { described_class.new(group: group, user: user) } let(:import_file) { fixture_file_upload('spec/fixtures/group_export.tar.gz') } let(:import_logger) { instance_double(Gitlab::Import::Logger) } - subject { service.execute } + subject(:service) { described_class.new(group: group, user: user) } before do ImportExportUpload.create!(group: group, import_file: import_file) @@ -128,11 +137,21 @@ RSpec.describe Groups::ImportExport::ImportService do end it 'imports group structure successfully' do - expect(subject).to be_truthy + expect(service.execute).to be_truthy + end + + it 'tracks the event' do + service.execute + + expect_snowplow_event( + category: 'Groups::ImportExport::ImportService', + action: 'create', + label: 'import_group_from_file' + ) end it 'removes import file' do - subject + service.execute expect(group.import_export_upload.import_file.file).to be_nil end @@ -141,7 +160,7 @@ RSpec.describe Groups::ImportExport::ImportService do shared = Gitlab::ImportExport::Shared.new(group) allow(Gitlab::ImportExport::Shared).to receive(:new).and_return(shared) - subject + service.execute expect(FileUtils).to have_received(:rm_rf).with(shared.base_path) expect(Dir.exist?(shared.base_path)).to eq(false) @@ -154,7 +173,7 @@ RSpec.describe Groups::ImportExport::ImportService do message: 'Group Import/Export: Import succeeded' ).once - subject + service.execute end end @@ -166,7 +185,7 @@ RSpec.describe Groups::ImportExport::ImportService do message: a_string_including('Errors occurred') ) - expect { subject }.to raise_error(Gitlab::ImportExport::Error) + expect { service.execute }.to raise_error(Gitlab::ImportExport::Error) end it 'tracks the error' do @@ -177,7 +196,7 @@ RSpec.describe Groups::ImportExport::ImportService do expect(param.message).to include 'does not have required permissions for' end - expect { subject }.to raise_error(Gitlab::ImportExport::Error) + expect { service.execute }.to raise_error(Gitlab::ImportExport::Error) end end @@ -191,7 +210,7 @@ RSpec.describe Groups::ImportExport::ImportService do message: a_string_including('Errors occurred') ).once - expect { subject }.to raise_error(Gitlab::ImportExport::Error) + expect { service.execute }.to raise_error(Gitlab::ImportExport::Error) end end @@ -203,7 +222,7 @@ RSpec.describe Groups::ImportExport::ImportService do end it 'successfully imports the group' do - expect(subject).to be_truthy + expect(service.execute).to be_truthy end it 'logs the import success' do @@ -215,7 +234,7 @@ RSpec.describe Groups::ImportExport::ImportService do message: 'Group Import/Export: Import succeeded' ) - subject + service.execute end end end @@ -223,12 +242,11 @@ RSpec.describe Groups::ImportExport::ImportService do context 'when importing a json export' do let(:user) { create(:user) } let(:group) { create(:group) } - let(:service) { described_class.new(group: group, user: user) } let(:import_file) { fixture_file_upload('spec/fixtures/legacy_group_export.tar.gz') } let(:import_logger) { instance_double(Gitlab::Import::Logger) } - subject { service.execute } + subject(:service) { described_class.new(group: group, user: user) } before do ImportExportUpload.create!(group: group, import_file: import_file) @@ -246,11 +264,21 @@ RSpec.describe Groups::ImportExport::ImportService do end it 'imports group structure successfully' do - expect(subject).to be_truthy + expect(service.execute).to be_truthy + end + + it 'tracks the event' do + service.execute + + expect_snowplow_event( + category: 'Groups::ImportExport::ImportService', + action: 'create', + label: 'import_group_from_file' + ) end it 'removes import file' do - subject + service.execute expect(group.import_export_upload.import_file.file).to be_nil end @@ -259,7 +287,7 @@ RSpec.describe Groups::ImportExport::ImportService do shared = Gitlab::ImportExport::Shared.new(group) allow(Gitlab::ImportExport::Shared).to receive(:new).and_return(shared) - subject + service.execute expect(FileUtils).to have_received(:rm_rf).with(shared.base_path) expect(Dir.exist?(shared.base_path)).to eq(false) @@ -272,7 +300,7 @@ RSpec.describe Groups::ImportExport::ImportService do message: 'Group Import/Export: Import succeeded' ).once - subject + service.execute end end @@ -284,7 +312,7 @@ RSpec.describe Groups::ImportExport::ImportService do message: a_string_including('Errors occurred') ) - expect { subject }.to raise_error(Gitlab::ImportExport::Error) + expect { service.execute }.to raise_error(Gitlab::ImportExport::Error) end it 'tracks the error' do @@ -295,7 +323,7 @@ RSpec.describe Groups::ImportExport::ImportService do expect(param.message).to include 'does not have required permissions for' end - expect { subject }.to raise_error(Gitlab::ImportExport::Error) + expect { service.execute }.to raise_error(Gitlab::ImportExport::Error) end end @@ -309,7 +337,7 @@ RSpec.describe Groups::ImportExport::ImportService do message: a_string_including('Errors occurred') ).once - expect { subject }.to raise_error(Gitlab::ImportExport::Error) + expect { service.execute }.to raise_error(Gitlab::ImportExport::Error) end end @@ -321,7 +349,7 @@ RSpec.describe Groups::ImportExport::ImportService do end it 'successfully imports the group' do - expect(subject).to be_truthy + expect(service.execute).to be_truthy end it 'logs the import success' do @@ -333,7 +361,7 @@ RSpec.describe Groups::ImportExport::ImportService do message: 'Group Import/Export: Import succeeded' ) - subject + service.execute end end end diff --git a/spec/services/import/github/cancel_project_import_service_spec.rb b/spec/services/import/github/cancel_project_import_service_spec.rb new file mode 100644 index 00000000000..77b8771ee65 --- /dev/null +++ b/spec/services/import/github/cancel_project_import_service_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Import::Github::CancelProjectImportService do + subject(:import_cancel) { described_class.new(project, project.owner) } + + let_it_be(:user) { create(:user) } + let_it_be_with_reload(:project) { create(:project, :import_started, import_type: 'github', import_url: 'https://fake.url') } + + describe '.execute' do + context 'when user is an owner' do + context 'when import is in progress' do + it 'update import state to be canceled' do + expect(import_cancel.execute).to eq({ status: :success, project: project }) + end + end + + context 'when import is finished' do + let(:expected_result) do + { + status: :error, + http_status: :bad_request, + message: 'The import cannot be canceled because it is finished' + } + end + + before do + project.import_state.finish! + end + + it 'returns error' do + expect(import_cancel.execute).to eq(expected_result) + end + end + end + + context 'when user is not allowed to read project' do + it 'returns 404' do + expect(described_class.new(project, user).execute) + .to eq({ status: :error, http_status: :not_found, message: 'Not Found' }) + end + end + + context 'when user is not allowed to cancel project' do + before do + project.add_developer(user) + end + + it 'returns 403' do + expect(described_class.new(project, user).execute) + .to eq({ status: :error, http_status: :forbidden, message: 'Unauthorized access' }) + end + end + end +end diff --git a/spec/services/import/github_service_spec.rb b/spec/services/import/github_service_spec.rb index 67a2c237e43..38d84009f08 100644 --- a/spec/services/import/github_service_spec.rb +++ b/spec/services/import/github_service_spec.rb @@ -6,7 +6,16 @@ RSpec.describe Import::GithubService do let_it_be(:user) { create(:user) } let_it_be(:token) { 'complex-token' } let_it_be(:access_params) { { github_access_token: 'github-complex-token' } } - let_it_be(:params) { { repo_id: 123, new_name: 'new_repo', target_namespace: 'root' } } + let(:settings) { instance_double(Gitlab::GithubImport::Settings) } + let(:optional_stages) { nil } + let(:params) do + { + repo_id: 123, + new_name: 'new_repo', + target_namespace: 'root', + optional_stages: optional_stages + } + end subject(:github_importer) { described_class.new(client, user, params) } @@ -16,6 +25,12 @@ RSpec.describe Import::GithubService do shared_examples 'handles errors' do |klass| let(:client) { klass.new(token) } + let(:project_double) { instance_double(Project, persisted?: true) } + + before do + allow(Gitlab::GithubImport::Settings).to receive(:new).with(project_double).and_return(settings) + allow(settings).to receive(:write).with(optional_stages) + end context 'do not raise an exception on input error' do let(:exception) { Octokit::ClientError.new(status: 404, body: 'Not Found') } @@ -62,13 +77,14 @@ RSpec.describe Import::GithubService do expect(client).to receive(:repository).and_return(repository_double) allow_next_instance_of(Gitlab::LegacyGithubImport::ProjectCreator) do |creator| - allow(creator).to receive(:execute).and_return(double(persisted?: true)) + allow(creator).to receive(:execute).and_return(project_double) end end context 'when there is no repository size limit defined' do it 'skips the check and succeeds' do expect(subject.execute(access_params, :github)).to include(status: :success) + expect(settings).to have_received(:write).with(nil) end end @@ -81,6 +97,7 @@ RSpec.describe Import::GithubService do it 'succeeds when the repository is smaller than the limit' do expect(subject.execute(access_params, :github)).to include(status: :success) + expect(settings).to have_received(:write).with(nil) end it 'returns error when the repository is larger than the limit' do @@ -100,6 +117,7 @@ RSpec.describe Import::GithubService do context 'when application size limit is defined' do it 'succeeds when the repository is smaller than the limit' do expect(subject.execute(access_params, :github)).to include(status: :success) + expect(settings).to have_received(:write).with(nil) end it 'returns error when the repository is larger than the limit' do @@ -109,6 +127,22 @@ RSpec.describe Import::GithubService do end end end + + context 'when optional stages params present' do + let(:optional_stages) do + { + single_endpoint_issue_events_import: true, + single_endpoint_notes_import: 'false', + attachments_import: false + } + end + + it 'saves optional stages choice to import_data' do + subject.execute(access_params, :github) + + expect(settings).to have_received(:write).with(optional_stages) + end + end end context 'when import source is disabled' do @@ -170,7 +204,7 @@ RSpec.describe Import::GithubService do include_examples 'handles errors', Gitlab::GithubImport::Client end - context 'when remove_legacy_github_client feature flag is enabled' do + context 'when remove_legacy_github_client feature flag is disabled' do before do stub_feature_flags(remove_legacy_github_client: false) end diff --git a/spec/services/import/gitlab_projects/create_project_service_spec.rb b/spec/services/import/gitlab_projects/create_project_service_spec.rb index 0da897448b8..59c384bad3c 100644 --- a/spec/services/import/gitlab_projects/create_project_service_spec.rb +++ b/spec/services/import/gitlab_projects/create_project_service_spec.rb @@ -139,10 +139,11 @@ RSpec.describe ::Import::GitlabProjects::CreateProjectService, :aggregate_failur expect(response.http_status).to eq(:bad_request) expect(response.message) .to eq(%{Project namespace path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-', end in '.git' or end in '.atom'}) - expect(response.payload).to eq(other_errors: [ - %{Path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-', end in '.git' or end in '.atom'}, - %{Path must not start or end with a special character and must not contain consecutive special characters.} - ]) + expect(response.payload).to eq( + other_errors: [ + %{Path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-', end in '.git' or end in '.atom'}, + %{Path must not start or end with a special character and must not contain consecutive special characters.} + ]) end end end diff --git a/spec/services/incident_management/incidents/create_service_spec.rb b/spec/services/incident_management/incidents/create_service_spec.rb index ac44bc4608c..851b21e1227 100644 --- a/spec/services/incident_management/incidents/create_service_spec.rb +++ b/spec/services/incident_management/incidents/create_service_spec.rb @@ -77,7 +77,7 @@ RSpec.describe IncidentManagement::Incidents::CreateService do it 'responds with errors' do expect(create_incident).to be_error - expect(create_incident.message).to eq("Title can't be blank") + expect(create_incident.errors).to contain_exactly("Title can't be blank") end it 'result payload contains an Issue object' do @@ -98,7 +98,7 @@ RSpec.describe IncidentManagement::Incidents::CreateService do it 'responds with errors' do expect(create_incident).to be_error - expect(create_incident.message).to eq('Hosts hosts array is over 255 chars') + expect(create_incident.errors).to contain_exactly('Hosts hosts array is over 255 chars') end end end diff --git a/spec/services/incident_management/issuable_escalation_statuses/prepare_update_service_spec.rb b/spec/services/incident_management/issuable_escalation_statuses/prepare_update_service_spec.rb index 761cc5c92ea..e8208c410d5 100644 --- a/spec/services/incident_management/issuable_escalation_statuses/prepare_update_service_spec.rb +++ b/spec/services/incident_management/issuable_escalation_statuses/prepare_update_service_spec.rb @@ -2,7 +2,8 @@ require 'spec_helper' -RSpec.describe IncidentManagement::IssuableEscalationStatuses::PrepareUpdateService do +RSpec.describe IncidentManagement::IssuableEscalationStatuses::PrepareUpdateService, factory_default: :keep do + let_it_be(:project) { create_default(:project) } let_it_be(:escalation_status) { create(:incident_management_issuable_escalation_status, :triggered) } let_it_be(:user_with_permissions) { create(:user) } @@ -10,7 +11,7 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::PrepareUpdateServ let(:issue) { escalation_status.issue } let(:status) { :acknowledged } let(:params) { { status: status } } - let(:service) { IncidentManagement::IssuableEscalationStatuses::PrepareUpdateService.new(issue, current_user, params) } + let(:service) { described_class.new(issue, current_user, params) } subject(:result) { service.execute } @@ -71,9 +72,17 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::PrepareUpdateServ end end - context 'when called without params' do + context 'when called nil params' do let(:params) { nil } + it 'raises an exception' do + expect { result }.to raise_error NoMethodError + end + end + + context 'when called without params' do + let(:params) { {} } + it_behaves_like 'successful response', {} end diff --git a/spec/services/incident_management/pager_duty/create_incident_issue_service_spec.rb b/spec/services/incident_management/pager_duty/create_incident_issue_service_spec.rb index fb536df5d17..572b1a20166 100644 --- a/spec/services/incident_management/pager_duty/create_incident_issue_service_spec.rb +++ b/spec/services/incident_management/pager_duty/create_incident_issue_service_spec.rb @@ -63,7 +63,7 @@ RSpec.describe IncidentManagement::PagerDuty::CreateIncidentIssueService do it 'responds with error' do expect(execute).to be_error - expect(execute.message).to eq("Title can't be blank") + expect(execute.errors).to contain_exactly("Title can't be blank") end end end diff --git a/spec/services/incident_management/timeline_events/create_service_spec.rb b/spec/services/incident_management/timeline_events/create_service_spec.rb index b999403e168..a7f448c825f 100644 --- a/spec/services/incident_management/timeline_events/create_service_spec.rb +++ b/spec/services/incident_management/timeline_events/create_service_spec.rb @@ -71,7 +71,7 @@ RSpec.describe IncidentManagement::TimelineEvents::CreateService do context 'when error occurs during creation' do let(:args) { {} } - it_behaves_like 'error response', "Occurred at can't be blank, Note can't be blank, and Note html can't be blank" + it_behaves_like 'error response', "Occurred at can't be blank and Timeline text can't be blank" end context 'with default action' do @@ -84,50 +84,6 @@ RSpec.describe IncidentManagement::TimelineEvents::CreateService do expect(result.action).to eq(IncidentManagement::TimelineEvents::DEFAULT_ACTION) end - end - - context 'with non_default action' do - it_behaves_like 'success response' - - it 'matches the action from arguments', :aggregate_failures do - result = execute.payload[:timeline_event] - - expect(result.action).to eq(args[:action]) - end - end - - context 'with editable param' do - let(:args) do - { - note: 'note', - occurred_at: Time.current, - action: 'new comment', - promoted_from_note: comment, - editable: editable - } - end - - context 'when editable is true' do - let(:editable) { true } - - it_behaves_like 'success response' - end - - context 'when editable is false' do - let(:editable) { false } - - it_behaves_like 'success response' - end - end - - it 'successfully creates a database record', :aggregate_failures do - expect { execute }.to change { ::IncidentManagement::TimelineEvent.count }.by(1) - end - - context 'when incident_timeline feature flag is enabled' do - before do - stub_feature_flags(incident_timeline: project) - end it 'creates a system note' do expect { execute }.to change { incident.notes.reload.count }.by(1) @@ -168,14 +124,42 @@ RSpec.describe IncidentManagement::TimelineEvents::CreateService do end end - context 'when incident_timeline feature flag is disabled' do - before do - stub_feature_flags(incident_timeline: false) + context 'with non_default action' do + it_behaves_like 'success response' + + it 'matches the action from arguments', :aggregate_failures do + result = execute.payload[:timeline_event] + + expect(result.action).to eq(args[:action]) end + end - it 'does not create a system note' do - expect { execute }.not_to change { incident.notes.reload.count } + context 'with editable param' do + let(:args) do + { + note: 'note', + occurred_at: Time.current, + action: 'new comment', + promoted_from_note: comment, + editable: editable + } + end + + context 'when editable is true' do + let(:editable) { true } + + it_behaves_like 'success response' end + + context 'when editable is false' do + let(:editable) { false } + + it_behaves_like 'success response' + end + end + + it 'successfully creates a database record', :aggregate_failures do + expect { execute }.to change { ::IncidentManagement::TimelineEvent.count }.by(1) end end diff --git a/spec/services/incident_management/timeline_events/destroy_service_spec.rb b/spec/services/incident_management/timeline_events/destroy_service_spec.rb index 09026f87116..e1b258960ae 100644 --- a/spec/services/incident_management/timeline_events/destroy_service_spec.rb +++ b/spec/services/incident_management/timeline_events/destroy_service_spec.rb @@ -48,10 +48,10 @@ RSpec.describe IncidentManagement::TimelineEvents::DestroyService do timeline_event.errors.add(:note, 'cannot be removed') end - it_behaves_like 'error response', 'Note cannot be removed' + it_behaves_like 'error response', 'Timeline text cannot be removed' end - context 'success response' do + context 'with success response' do it 'successfully returns the timeline event', :aggregate_failures do expect(execute).to be_success @@ -60,27 +60,11 @@ RSpec.describe IncidentManagement::TimelineEvents::DestroyService do expect(result.id).to eq(timeline_event.id) end - it_behaves_like 'an incident management tracked event', :incident_management_timeline_event_deleted - end - - context 'when incident_timeline feature flag is enabled' do - before do - stub_feature_flags(incident_timeline: project) - end - it 'creates a system note' do expect { execute }.to change { incident.notes.reload.count }.by(1) end - end - - context 'when incident_timeline feature flag is disabled' do - before do - stub_feature_flags(incident_timeline: false) - end - it 'does not create a system note' do - expect { execute }.not_to change { incident.notes.reload.count } - end + it_behaves_like 'an incident management tracked event', :incident_management_timeline_event_deleted end end end diff --git a/spec/services/incident_management/timeline_events/update_service_spec.rb b/spec/services/incident_management/timeline_events/update_service_spec.rb index f612c72e2a8..5d8518cf2ef 100644 --- a/spec/services/incident_management/timeline_events/update_service_spec.rb +++ b/spec/services/incident_management/timeline_events/update_service_spec.rb @@ -12,10 +12,6 @@ RSpec.describe IncidentManagement::TimelineEvents::UpdateService do let(:params) { { note: 'Updated note', occurred_at: occurred_at } } let(:current_user) { user } - before do - stub_feature_flags(incident_timeline: project) - end - describe '#execute' do shared_examples 'successful response' do it 'responds with success', :aggregate_failures do @@ -70,16 +66,6 @@ RSpec.describe IncidentManagement::TimelineEvents::UpdateService do it_behaves_like 'passing the correct was_changed value', :occurred_at_and_note - context 'when incident_timeline feature flag is disabled' do - before do - stub_feature_flags(incident_timeline: false) - end - - it 'does not add a system note' do - expect { execute }.not_to change { incident.notes } - end - end - context 'when note is nil' do let(:params) { { occurred_at: occurred_at } } @@ -98,7 +84,7 @@ RSpec.describe IncidentManagement::TimelineEvents::UpdateService do context 'when note is blank' do let(:params) { { note: '', occurred_at: occurred_at } } - it_behaves_like 'error response', "Note can't be blank" + it_behaves_like 'error response', "Timeline text can't be blank" end context 'when occurred_at is nil' do diff --git a/spec/services/issuable/process_assignees_spec.rb b/spec/services/issuable/process_assignees_spec.rb index 45d57a1772a..9e909b68172 100644 --- a/spec/services/issuable/process_assignees_spec.rb +++ b/spec/services/issuable/process_assignees_spec.rb @@ -12,7 +12,7 @@ RSpec.describe Issuable::ProcessAssignees do extra_assignee_ids: %w(2 5 12)) result = process.execute - expect(result.sort).to eq(%w(5 7 9).sort) + expect(result).to contain_exactly(5, 7, 9) end it 'combines other ids when assignee_ids is nil' do @@ -23,7 +23,7 @@ RSpec.describe Issuable::ProcessAssignees do extra_assignee_ids: %w(2 5 12)) result = process.execute - expect(result.sort).to eq(%w(1 2 3 5 11 12).sort) + expect(result).to contain_exactly(1, 2, 3, 5, 11, 12) end it 'combines other ids when both add_assignee_ids and remove_assignee_ids are not empty' do @@ -34,7 +34,7 @@ RSpec.describe Issuable::ProcessAssignees do extra_assignee_ids: %w(2 5 12)) result = process.execute - expect(result.sort).to eq(%w(1 2 3 5 6 12).sort) + expect(result).to contain_exactly(1, 2, 3, 5, 6, 12) end it 'combines other ids when remove_assignee_ids is not empty' do @@ -45,7 +45,7 @@ RSpec.describe Issuable::ProcessAssignees do extra_assignee_ids: %w(2 5 12)) result = process.execute - expect(result.sort).to eq(%w(1 2 3 5 12).sort) + expect(result).to contain_exactly(1, 2, 3, 5, 12) end it 'combines other ids when add_assignee_ids is not empty' do @@ -56,7 +56,7 @@ RSpec.describe Issuable::ProcessAssignees do extra_assignee_ids: %w(2 5 12)) result = process.execute - expect(result.sort).to eq(%w(1 2 4 3 5 6 11 12).sort) + expect(result).to contain_exactly(1, 2, 4, 3, 5, 6, 11, 12) end it 'combines ids when existing_assignee_ids and extra_assignee_ids are omitted' do @@ -65,7 +65,18 @@ RSpec.describe Issuable::ProcessAssignees do remove_assignee_ids: %w(4 7 11)) result = process.execute - expect(result.sort).to eq(%w(2 6).sort) + expect(result.sort).to eq([2, 6].sort) + end + + it 'handles mixed string and integer arrays' do + process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9), + add_assignee_ids: [2, 4, 6], + remove_assignee_ids: %w(4 7 11), + existing_assignee_ids: [1, 3, 11], + extra_assignee_ids: %w(2 5 12)) + result = process.execute + + expect(result).to contain_exactly(1, 2, 3, 5, 6, 12) end end end diff --git a/spec/services/issues/clone_service_spec.rb b/spec/services/issues/clone_service_spec.rb index 435488b7f66..67f32b85350 100644 --- a/spec/services/issues/clone_service_spec.rb +++ b/spec/services/issues/clone_service_spec.rb @@ -36,6 +36,21 @@ RSpec.describe Issues::CloneService do context 'issue movable' do include_context 'user can clone issue' + context 'when issue creation fails' do + before do + allow_next_instance_of(Issues::CreateService) do |create_service| + allow(create_service).to receive(:execute).and_return(ServiceResponse.error(message: 'some error')) + end + end + + it 'raises a clone error' do + expect { clone_service.execute(old_issue, new_project) }.to raise_error( + Issues::CloneService::CloneError, + 'some error' + ) + end + end + context 'generic issue' do let!(:new_issue) { clone_service.execute(old_issue, new_project, with_notes: with_notes) } diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 3d52dc07c4f..5fe4c693451 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Issues::CreateService do include AfterNextHelpers let_it_be(:group) { create(:group, :crm_enabled) } - let_it_be_with_reload(:project) { create(:project, group: group) } + let_it_be_with_reload(:project) { create(:project, :public, group: group) } let_it_be(:user) { create(:user) } let(:spam_params) { double } @@ -23,12 +23,27 @@ RSpec.describe Issues::CreateService do let_it_be(:assignee) { create(:user) } let_it_be(:milestone) { create(:milestone, project: project) } - let(:issue) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute } + let(:result) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute } + let(:issue) { result[:issue] } before do stub_spam_services end + context 'when params are invalid' do + let(:opts) { { title: '' } } + + before_all do + project.add_guest(assignee) + end + + it 'returns an error service response' do + expect(result).to be_error + expect(result.errors).to include("Title can't be blank") + expect(issue).not_to be_persisted + end + end + context 'when params are valid' do let_it_be(:labels) { create_pair(:label, project: project) } @@ -60,6 +75,30 @@ RSpec.describe Issues::CreateService do end end + describe 'authorization' do + let_it_be(:project) { create(:project, :private, group: group).tap { |project| project.add_guest(user) } } + + let(:opts) { { title: 'private issue', description: 'please fix' } } + + context 'when the user is authorized' do + it 'allows the user to create an issue' do + expect(result).to be_success + expect(issue).to be_persisted + end + end + + context 'when the user is not authorized' do + let(:user) { create(:user) } + + it 'does not allow the user to create an issue' do + expect(result).to be_error + expect(result.errors).to contain_exactly('Operation not allowed') + expect(result.http_status).to eq(403) + expect(issue).to be_nil + end + end + end + it 'works if base work item types were not created yet' do WorkItems::Type.delete_all @@ -71,6 +110,7 @@ RSpec.describe Issues::CreateService do it 'creates the issue with the given params' do expect(Issuable::CommonSystemNotesService).to receive_message_chain(:new, :execute) + expect(result).to be_success expect(issue).to be_persisted expect(issue).to be_a(::Issue) expect(issue.title).to eq('Awesome issue') @@ -89,12 +129,13 @@ RSpec.describe Issues::CreateService do end context 'when a build_service is provided' do - let(:issue) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params, build_service: build_service).execute } + let(:result) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params, build_service: build_service).execute } let(:issue_from_builder) { WorkItem.new(project: project, title: 'Issue from builder') } let(:build_service) { double(:build_service, execute: issue_from_builder) } it 'uses the provided service to build the issue' do + expect(result).to be_success expect(issue).to be_persisted expect(issue).to be_a(WorkItem) end @@ -119,6 +160,7 @@ RSpec.describe Issues::CreateService do end it 'sets the correct relative position' do + expect(result).to be_success expect(issue).to be_persisted expect(issue.relative_position).to be_present expect(issue.relative_position).to be_between(issue_before.relative_position, issue_after.relative_position) @@ -196,8 +238,10 @@ RSpec.describe Issues::CreateService do let_it_be(:non_member) { create(:user) } it 'filters out params that cannot be set without the :set_issue_metadata permission' do - issue = described_class.new(project: project, current_user: non_member, params: opts, spam_params: spam_params).execute + result = described_class.new(project: project, current_user: non_member, params: opts, spam_params: spam_params).execute + issue = result[:issue] + expect(result).to be_success expect(issue).to be_persisted expect(issue.title).to eq('Awesome issue') expect(issue.description).to eq('please fix') @@ -208,8 +252,10 @@ RSpec.describe Issues::CreateService do end it 'can create confidential issues' do - issue = described_class.new(project: project, current_user: non_member, params: { confidential: true }, spam_params: spam_params).execute + result = described_class.new(project: project, current_user: non_member, params: opts.merge(confidential: true), spam_params: spam_params).execute + issue = result[:issue] + expect(result).to be_success expect(issue.confidential).to be_truthy end end @@ -298,7 +344,7 @@ RSpec.describe Issues::CreateService do let(:opts) do { title: 'Title', description: 'Description', - assignees: [assignee] } + assignee_ids: [assignee.id] } end it 'invalidates open issues counter for assignees when issue is assigned' do @@ -389,7 +435,7 @@ RSpec.describe Issues::CreateService do end it 'schedules a namespace onboarding create action worker' do - expect(Namespaces::OnboardingIssueCreatedWorker).to receive(:perform_async).with(project.namespace.id) + expect(Onboarding::IssueCreatedWorker).to receive(:perform_async).with(project.namespace.id) issue end @@ -404,16 +450,20 @@ RSpec.describe Issues::CreateService do it 'removes assignee when user id is invalid' do opts = { title: 'Title', description: 'Description', assignee_ids: [-1] } - issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute + result = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute + issue = result[:issue] + expect(result).to be_success expect(issue.assignees).to be_empty end it 'removes assignee when user id is 0' do opts = { title: 'Title', description: 'Description', assignee_ids: [0] } - issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute + result = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute + issue = result[:issue] + expect(result).to be_success expect(issue.assignees).to be_empty end @@ -421,8 +471,10 @@ RSpec.describe Issues::CreateService do project.add_maintainer(assignee) opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } - issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute + result = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute + issue = result[:issue] + expect(result).to be_success expect(issue.assignees).to eq([assignee]) end @@ -439,8 +491,10 @@ RSpec.describe Issues::CreateService do project.update!(visibility_level: level) opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } - issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute + result = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute + issue = result[:issue] + expect(result).to be_success expect(issue.assignees).to be_empty end end @@ -449,7 +503,7 @@ RSpec.describe Issues::CreateService do end it_behaves_like 'issuable record that supports quick actions' do - let(:issuable) { described_class.new(project: project, current_user: user, params: params, spam_params: spam_params).execute } + let(:issuable) { described_class.new(project: project, current_user: user, params: params, spam_params: spam_params).execute[:issue] } end context 'Quick actions' do @@ -472,6 +526,7 @@ RSpec.describe Issues::CreateService do end it 'assigns, sets milestone, and sets contact to issuable from command' do + expect(result).to be_success expect(issue).to be_persisted expect(issue.assignees).to eq([assignee]) expect(issue.milestone).to eq(milestone) @@ -493,6 +548,8 @@ RSpec.describe Issues::CreateService do context 'with permission' do it 'assigns contact to issue' do group.add_reporter(user) + + expect(result).to be_success expect(issue).to be_persisted expect(issue.issue_customer_relations_contacts.last.contact).to eq(contact) end @@ -501,6 +558,8 @@ RSpec.describe Issues::CreateService do context 'without permission' do it 'does not assign contact to issue' do group.add_guest(user) + + expect(result).to be_success expect(issue).to be_persisted expect(issue.issue_customer_relations_contacts).to be_empty end @@ -535,6 +594,7 @@ RSpec.describe Issues::CreateService do end it 'can apply labels' do + expect(result).to be_success expect(issue).to be_persisted expect(issue.labels).to eq([label]) end @@ -569,25 +629,32 @@ RSpec.describe Issues::CreateService do end it 'sets default title and description values if not provided' do - issue = described_class.new( + result = described_class.new( project: project, current_user: user, params: opts, spam_params: spam_params ).execute + issue = result[:issue] + expect(result).to be_success expect(issue).to be_persisted expect(issue.title).to eq("Follow-up from \"#{merge_request.title}\"") expect(issue.description).to include("The following discussion from #{merge_request.to_reference} should be addressed") end it 'takes params from the request over the default values' do - issue = described_class.new(project: project, current_user: user, - params: opts.merge( - description: 'Custom issue description', - title: 'My new issue' - ), - spam_params: spam_params).execute + result = described_class.new( + project: project, + current_user: user, + params: opts.merge( + description: 'Custom issue description', + title: 'My new issue' + ), + spam_params: spam_params + ).execute + issue = result[:issue] + expect(result).to be_success expect(issue).to be_persisted expect(issue.description).to eq('Custom issue description') expect(issue.title).to eq('My new issue') @@ -613,25 +680,32 @@ RSpec.describe Issues::CreateService do end it 'sets default title and description values if not provided' do - issue = described_class.new( + result = described_class.new( project: project, current_user: user, params: opts, spam_params: spam_params ).execute + issue = result[:issue] + expect(result).to be_success expect(issue).to be_persisted expect(issue.title).to eq("Follow-up from \"#{merge_request.title}\"") expect(issue.description).to include("The following discussion from #{merge_request.to_reference} should be addressed") end it 'takes params from the request over the default values' do - issue = described_class.new(project: project, current_user: user, - params: opts.merge( - description: 'Custom issue description', - title: 'My new issue' - ), - spam_params: spam_params).execute + result = described_class.new( + project: project, + current_user: user, + params: opts.merge( + description: 'Custom issue description', + title: 'My new issue' + ), + spam_params: spam_params + ).execute + issue = result[:issue] + expect(result).to be_success expect(issue).to be_persisted expect(issue.description).to eq('Custom issue description') expect(issue.title).to eq('My new issue') @@ -648,6 +722,7 @@ RSpec.describe Issues::CreateService do it 'ignores related issue if not accessible' do expect { issue }.not_to change { IssueLink.count } + expect(result).to be_success expect(issue).to be_persisted end @@ -658,6 +733,7 @@ RSpec.describe Issues::CreateService do it 'adds a link to the issue' do expect { issue }.to change { IssueLink.count }.by(1) + expect(result).to be_success expect(issue).to be_persisted expect(issue.related_issues(user)).to eq([related_issue]) end diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index 863df810d01..23180f75eb3 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -35,6 +35,23 @@ RSpec.describe Issues::MoveService do let!(:new_issue) { move_service.execute(old_issue, new_project) } end + context 'when issue creation fails' do + include_context 'user can move issue' + + before do + allow_next_instance_of(Issues::CreateService) do |create_service| + allow(create_service).to receive(:execute).and_return(ServiceResponse.error(message: 'some error')) + end + end + + it 'raises a move error' do + expect { move_service.execute(old_issue, new_project) }.to raise_error( + Issues::MoveService::MoveError, + 'some error' + ) + end + end + context 'issue movable' do include_context 'user can move issue' diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 634a4206d48..20b1a1f58bb 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -512,6 +512,20 @@ RSpec.describe Issues::UpdateService, :mailer do expect(note.note).to eq('changed the description') end + + it 'triggers GraphQL description updated subscription' do + expect(GraphqlTriggers).to receive(:issuable_description_updated).with(issue).and_call_original + + update_issue(description: 'Changed description') + end + end + + context 'when decription is not changed' do + it 'does not trigger GraphQL description updated subscription' do + expect(GraphqlTriggers).not_to receive(:issuable_description_updated) + + update_issue(title: 'Changed title') + end end context 'when issue turns confidential' do @@ -838,6 +852,24 @@ RSpec.describe Issues::UpdateService, :mailer do service.execute(issue) end + # At the moment of writting old associations are not necessary for update_task + # and doing this will prevent fetching associations from the DB and comparing old and new labels + it 'does not pass old_associations to the after_update method' do + params = { + update_task: { + index: 1, + checked: false, + line_source: '- [x] Task 1', + line_number: 1 + } + } + service = described_class.new(project: project, current_user: user, params: params) + + expect(service).to receive(:after_update).with(issue, {}) + + service.execute(issue) + end + it 'creates system note about task status change' do note1 = find_note('marked the checklist item **Task 1** as completed') note2 = find_note('marked the checklist item **Task 2** as completed') diff --git a/spec/services/jira_connect/create_asymmetric_jwt_service_spec.rb b/spec/services/jira_connect/create_asymmetric_jwt_service_spec.rb new file mode 100644 index 00000000000..f5359e5b643 --- /dev/null +++ b/spec/services/jira_connect/create_asymmetric_jwt_service_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe JiraConnect::CreateAsymmetricJwtService do + describe '#execute' do + let_it_be(:jira_connect_installation) { create(:jira_connect_installation) } + + let(:service) { described_class.new(jira_connect_installation) } + + subject(:jwt_token) { service.execute } + + it 'raises an error' do + expect { jwt_token }.to raise_error(ArgumentError, 'jira_connect_installation is not a proxy installation') + end + + context 'with proxy installation' do + let_it_be(:jira_connect_installation) { create(:jira_connect_installation, instance_url: 'https://gitlab.test') } + + let(:public_key_id) { Atlassian::Jwt.decode(jwt_token, nil, false, algorithm: 'RS256').last['kid'] } + let(:public_key_cdn) { 'https://gitlab.com/-/jira_connect/public_keys/' } + let(:jwt_verification_claims) do + { + aud: 'https://gitlab.test/-/jira_connect', + iss: jira_connect_installation.client_key, + qsh: Atlassian::Jwt.create_query_string_hash('https://gitlab.test/-/jira_connect/events/installed', 'POST', 'https://gitlab.test/-/jira_connect') + } + end + + subject(:jwt_token) { service.execute } + + it 'stores the public key' do + expect { JiraConnect::PublicKey.find(public_key_id) }.not_to raise_error + end + + it 'is produces a valid JWT' do + public_key = OpenSSL::PKey.read(JiraConnect::PublicKey.find(public_key_id).key) + options = jwt_verification_claims.except(:qsh).merge({ verify_aud: true, verify_iss: true, algorithm: 'RS256' }) + + decoded_token = Atlassian::Jwt.decode(jwt_token, public_key, true, options).first + + expect(decoded_token).to eq(jwt_verification_claims.stringify_keys) + end + end + end +end diff --git a/spec/services/jira_connect/sync_service_spec.rb b/spec/services/jira_connect/sync_service_spec.rb index 7242b1f41f9..32580a7735f 100644 --- a/spec/services/jira_connect/sync_service_spec.rb +++ b/spec/services/jira_connect/sync_service_spec.rb @@ -46,10 +46,11 @@ RSpec.describe JiraConnect::SyncService do context 'when a request returns an error' do it 'logs the response as an error' do - expect_next(client).to store_info([ - { 'errorMessages' => ['some error message'] }, - { 'errorMessages' => ['x'] } - ]) + expect_next(client).to store_info( + [ + { 'errorMessages' => ['some error message'] }, + { 'errorMessages' => ['x'] } + ]) expect_log(:error, { 'errorMessages' => ['some error message'] }) expect_log(:error, { 'errorMessages' => ['x'] }) diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb index 25696ca209e..756e1cf403c 100644 --- a/spec/services/members/create_service_spec.rb +++ b/spec/services/members/create_service_spec.rb @@ -110,12 +110,61 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ expect(execute_service[:status]).to eq(:success) end end + + context 'when only one user fails validations' do + let_it_be(:source) { create(:project, group: create(:group)) } + let(:user_id) { [member.id, user_invited_by_id.id] } + + before do + # validations will fail because we try to invite them to the project as a guest + source.group.add_developer(member) + end + + it 'triggers the members added event' do + expect(Gitlab::EventStore) + .to receive(:publish) + .with(an_instance_of(Members::MembersAddedEvent)) + .and_call_original + + expect(execute_service[:status]).to eq(:error) + expect(execute_service[:message]) + .to include 'Access level should be greater than or equal to Developer inherited membership from group' + expect(source.users).not_to include(member) + expect(source.users).to include(user_invited_by_id) + end + end + + context 'when all users fail validations' do + let_it_be(:source) { create(:project, group: create(:group)) } + let(:user_id) { [member.id, user_invited_by_id.id] } + + before do + # validations will fail because we try to invite them to the project as a guest + source.group.add_developer(member) + source.group.add_developer(user_invited_by_id) + end + + it 'does not trigger the members added event' do + expect(Gitlab::EventStore) + .not_to receive(:publish) + .with(an_instance_of(Members::MembersAddedEvent)) + + expect(execute_service[:status]).to eq(:error) + expect(execute_service[:message]) + .to include 'Access level should be greater than or equal to Developer inherited membership from group' + expect(source.users).not_to include(member, user_invited_by_id) + end + end end context 'when passing no user ids' do let(:user_id) { '' } it 'does not add a member' do + expect(Gitlab::EventStore) + .not_to receive(:publish) + .with(an_instance_of(Members::MembersAddedEvent)) + expect(execute_service[:status]).to eq(:error) expect(execute_service[:message]).to be_present expect(source.users).not_to include member diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 9f0daba3327..8559c02be57 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -95,6 +95,37 @@ RSpec.describe Members::DestroyService do end end + context 'With ExclusiveLeaseHelpers' do + let(:service_object) { described_class.new(current_user) } + let!(:member) { group_project.add_developer(member_user) } + + subject(:destroy_member) { service_object.execute(member, **opts) } + + before do + group_project.add_maintainer(current_user) + + allow(service_object).to receive(:in_lock) do |_, &block| + block.call if lock_obtained + end + end + + context 'when lock is obtained' do + let(:lock_obtained) { true } + + it 'destroys the membership' do + expect { destroy_member }.to change { group_project.members.count }.by(-1) + end + end + + context 'when the lock can not be obtained' do + let(:lock_obtained) { false } + + it 'does not destroy the membership' do + expect { destroy_member }.not_to change { group_project.members.count } + end + end + end + context 'with a member with access' do before do group_project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PRIVATE) diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 8f448184b45..b3c4ed4c544 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -9,6 +9,7 @@ RSpec.describe MergeRequests::CloseService do let(:merge_request) { create(:merge_request, assignees: [user2], author: create(:user)) } let(:project) { merge_request.project } let!(:todo) { create(:todo, :assigned, user: user, project: project, target: merge_request, author: user2) } + let(:service) { described_class.new(project: project, current_user: user) } before do project.add_maintainer(user) @@ -16,18 +17,20 @@ RSpec.describe MergeRequests::CloseService do project.add_guest(guest) end + def execute + service.execute(merge_request) + end + describe '#execute' do it_behaves_like 'cache counters invalidator' it_behaves_like 'merge request reviewers cache counters invalidator' context 'valid params' do - let(:service) { described_class.new(project: project, current_user: user) } - before do allow(service).to receive(:execute_hooks) perform_enqueued_jobs do - @merge_request = service.execute(merge_request) + @merge_request = execute end end @@ -73,7 +76,7 @@ RSpec.describe MergeRequests::CloseService do expect(metrics_service).to receive(:close) - described_class.new(project: project, current_user: user).execute(merge_request) + execute end it 'calls the merge request activity counter' do @@ -81,13 +84,11 @@ RSpec.describe MergeRequests::CloseService do .to receive(:track_close_mr_action) .with(user: user) - described_class.new(project: project, current_user: user).execute(merge_request) + execute end it 'refreshes the number of open merge requests for a valid MR', :use_clean_rails_memory_store_caching do - service = described_class.new(project: project, current_user: user) - - expect { service.execute(merge_request) } + expect { execute } .to change { project.open_merge_requests_count }.from(1).to(0) end @@ -96,25 +97,39 @@ RSpec.describe MergeRequests::CloseService do expect(service).to receive(:execute_for_merge_request_pipeline).with(merge_request) end - described_class.new(project: project, current_user: user).execute(merge_request) + execute end it 'schedules CleanupRefsService' do expect(MergeRequests::CleanupRefsService).to receive(:schedule).with(merge_request) - described_class.new(project: project, current_user: user).execute(merge_request) + execute + end + + it 'triggers GraphQL subscription mergeRequestMergeStatusUpdated' do + expect(GraphqlTriggers).to receive(:merge_request_merge_status_updated).with(merge_request) + + execute end context 'current user is not authorized to close merge request' do + let(:user) { guest } + before do perform_enqueued_jobs do - @merge_request = described_class.new(project: project, current_user: guest).execute(merge_request) + @merge_request = execute end end it 'does not close the merge request' do expect(@merge_request).to be_open end + + it 'does not trigger GraphQL subscription mergeRequestMergeStatusUpdated' do + expect(GraphqlTriggers).not_to receive(:merge_request_merge_status_updated) + + execute + end end end end diff --git a/spec/services/merge_requests/create_from_issue_service_spec.rb b/spec/services/merge_requests/create_from_issue_service_spec.rb index 749b30bff5f..0eefbed252b 100644 --- a/spec/services/merge_requests/create_from_issue_service_spec.rb +++ b/spec/services/merge_requests/create_from_issue_service_spec.rb @@ -34,19 +34,19 @@ RSpec.describe MergeRequests::CreateFromIssueService do expect(result[:message]).to eq('Invalid issue iid') end - it 'creates a branch based on issue title', :sidekiq_might_not_need_inline do + it 'creates a branch based on issue title' do service.execute expect(target_project.repository.branch_exists?(issue.to_branch_name)).to be_truthy end - it 'creates a branch using passed name', :sidekiq_might_not_need_inline do + it 'creates a branch using passed name' do service_with_custom_source_branch.execute expect(target_project.repository.branch_exists?(custom_source_branch)).to be_truthy end - it 'creates the new_merge_request system note', :sidekiq_might_not_need_inline do + it 'creates the new_merge_request system note' do expect(SystemNoteService).to receive(:new_merge_request).with(issue, project, user, instance_of(MergeRequest)) service.execute @@ -60,7 +60,7 @@ RSpec.describe MergeRequests::CreateFromIssueService do 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 + it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created' do expect_next_instance_of(MergeRequest) do |instance| expect(instance).to receive(:valid?).at_least(:once).and_return(false) end @@ -81,36 +81,36 @@ RSpec.describe MergeRequests::CreateFromIssueService do service.execute end - it 'creates a merge request', :sidekiq_might_not_need_inline do + it 'creates a merge request' do expect { service.execute }.to change(target_project.merge_requests, :count).by(1) end - it 'sets the merge request author to current user and assigns them', :sidekiq_might_not_need_inline do + it 'sets the merge request author to current user and assigns them' do result = service.execute expect(result[:merge_request].author).to eq(user) expect(result[:merge_request].assignees).to eq([user]) end - it 'sets the merge request source branch to the new issue branch', :sidekiq_might_not_need_inline do + it 'sets the merge request source branch to the new issue branch' do result = service.execute expect(result[:merge_request].source_branch).to eq(issue.to_branch_name) end - it 'sets the merge request source branch to the passed branch name', :sidekiq_might_not_need_inline do + it 'sets the merge request source branch to the passed branch name' do result = service_with_custom_source_branch.execute expect(result[:merge_request].source_branch).to eq(custom_source_branch) end - it 'sets the merge request target branch to the project default branch', :sidekiq_might_not_need_inline do + it 'sets the merge request target branch to the project default branch' do result = service.execute expect(result[:merge_request].target_branch).to eq(target_project.default_branch) end - it 'executes quick actions if the build service sets them in the description', :sidekiq_might_not_need_inline do + it 'executes quick actions if the build service sets them in the description' do allow(service).to receive(:merge_request).and_wrap_original do |m, *args| m.call(*args).tap do |merge_request| merge_request.description = "/assign #{user.to_reference}" @@ -122,7 +122,7 @@ RSpec.describe MergeRequests::CreateFromIssueService do expect(result[:merge_request].assignees).to eq([user]) end - context 'when ref branch is set', :sidekiq_might_not_need_inline do + context 'when ref branch is set' do subject { described_class.new(project: project, current_user: user, mr_params: { ref: 'feature', **service_params }).execute } it 'sets the merge request source branch to the new issue branch' do @@ -213,7 +213,7 @@ RSpec.describe MergeRequests::CreateFromIssueService do it_behaves_like 'a service that creates a merge request from an issue' - it 'sets the merge request title to: "Draft: $issue-branch-name', :sidekiq_might_not_need_inline do + it 'sets the merge request title to: "Draft: $issue-branch-name' do result = service.execute expect(result[:merge_request].title).to eq("Draft: #{issue.to_branch_name.titleize.humanize}") diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 4102cdc101e..0bc8258af42 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -102,7 +102,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do description: 'please fix', source_branch: 'feature', target_branch: 'master', - assignees: [user2] + assignee_ids: [user2.id] } end diff --git a/spec/services/merge_requests/ff_merge_service_spec.rb b/spec/services/merge_requests/ff_merge_service_spec.rb index aa5d6dcd1fb..5027acbba0a 100644 --- a/spec/services/merge_requests/ff_merge_service_spec.rb +++ b/spec/services/merge_requests/ff_merge_service_spec.rb @@ -108,7 +108,8 @@ RSpec.describe MergeRequests::FfMergeService do service.execute(merge_request) - expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error) + .with(hash_including(message: a_string_matching(error_message))) end it 'logs and saves error if there is an PreReceiveError exception' do @@ -122,7 +123,8 @@ RSpec.describe MergeRequests::FfMergeService do service.execute(merge_request) expect(merge_request.merge_error).to include(error_message) - expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error) + .with(hash_including(message: a_string_matching(error_message))) end it 'does not update squash_commit_sha if squash merge is not successful' do diff --git a/spec/services/merge_requests/link_lfs_objects_service_spec.rb b/spec/services/merge_requests/link_lfs_objects_service_spec.rb index 2fb6bbaf02f..96cb72baac2 100644 --- a/spec/services/merge_requests/link_lfs_objects_service_spec.rb +++ b/spec/services/merge_requests/link_lfs_objects_service_spec.rb @@ -52,10 +52,11 @@ RSpec.describe MergeRequests::LinkLfsObjectsService, :sidekiq_inline do it 'calls Projects::LfsPointers::LfsLinkService#execute with OIDs of LFS objects in merge request' do expect_next_instance_of(Projects::LfsPointers::LfsLinkService) do |service| - expect(service).to receive(:execute).with(%w[ - 8b12507783d5becacbf2ebe5b01a60024d8728a8f86dcc818bce699e8b3320bc - 94a72c074cfe574742c9e99e863322f73feff82981d065ff65a0308f44f19f62 - ]) + expect(service).to receive(:execute).with( + %w[ + 8b12507783d5becacbf2ebe5b01a60024d8728a8f86dcc818bce699e8b3320bc + 94a72c074cfe574742c9e99e863322f73feff82981d065ff65a0308f44f19f62 + ]) end execute diff --git a/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb b/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb index 4d7bd3d8800..8437876c3cf 100644 --- a/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb +++ b/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb @@ -15,20 +15,26 @@ RSpec.describe MergeRequests::MarkReviewerReviewedService do end describe '#execute' do - describe 'invalid permissions' do - let(:service) { described_class.new(project: project, current_user: create(:user)) } - + shared_examples_for 'failed service execution' do it 'returns an error' do expect(result[:status]).to eq :error end + + it_behaves_like 'does not trigger GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { result } + end + end + + describe 'invalid permissions' do + let(:service) { described_class.new(project: project, current_user: create(:user)) } + + it_behaves_like 'failed service execution' end describe 'reviewer does not exist' do let(:service) { described_class.new(project: project, current_user: create(:user)) } - it 'returns an error' do - expect(result[:status]).to eq :error - end + it_behaves_like 'failed service execution' end describe 'reviewer exists' do @@ -40,6 +46,10 @@ RSpec.describe MergeRequests::MarkReviewerReviewedService do expect(result[:status]).to eq :success expect(reviewer.state).to eq 'reviewed' end + + it_behaves_like 'triggers GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { result } + end end end end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index a2d73d8c9b1..d3bf203d6bb 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -95,6 +95,42 @@ RSpec.describe MergeRequests::MergeService do end end + context 'running the service once' do + let(:ref) { merge_request.to_reference(full: true) } + let(:jid) { SecureRandom.hex } + + let(:messages) do + [ + /#{ref} - Git merge started on JID #{jid}/, + /#{ref} - Git merge finished on JID #{jid}/, + /#{ref} - Post merge started on JID #{jid}/, + /#{ref} - Post merge finished on JID #{jid}/, + /#{ref} - Merge process finished on JID #{jid}/ + ] + end + + before do + merge_request.update!(merge_jid: jid) + ::Gitlab::ApplicationContext.push(caller_id: 'MergeWorker') + end + + it 'logs status messages' do + allow(Gitlab::AppLogger).to receive(:info).and_call_original + + messages.each do |message| + expect(Gitlab::AppLogger).to receive(:info).with( + hash_including( + 'meta.caller_id' => 'MergeWorker', + message: message, + merge_request_info: ref + ) + ).and_call_original + end + + service.execute(merge_request) + end + end + context 'running the service multiple time' do it 'is idempotent' do 2.times { service.execute(merge_request) } @@ -315,7 +351,9 @@ RSpec.describe MergeRequests::MergeService do service.execute(merge_request) expect(merge_request.merge_error).to eq(error_message) - expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error) + .with(hash_including(merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message))) end end @@ -328,7 +366,9 @@ RSpec.describe MergeRequests::MergeService do service.execute(merge_request) 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)) + expect(Gitlab::AppLogger).to have_received(:error) + .with(hash_including(merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message))) end it 'logs and saves error if user is not authorized' do @@ -354,7 +394,9 @@ RSpec.describe MergeRequests::MergeService do service.execute(merge_request) expect(merge_request.merge_error).to include('Something went wrong during merge pre-receive hook') - expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error) + .with(hash_including(merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message))) end it 'logs and saves error if commit is not created' do @@ -366,7 +408,9 @@ 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(described_class::GENERIC_ERROR_MESSAGE) - expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(described_class::GENERIC_ERROR_MESSAGE)) + expect(Gitlab::AppLogger).to have_received(:error) + .with(hash_including(merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(described_class::GENERIC_ERROR_MESSAGE))) end context 'when squashing is required' do @@ -385,7 +429,9 @@ RSpec.describe MergeRequests::MergeService do expect(merge_request.merge_commit_sha).to be_nil expect(merge_request.squash_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(Gitlab::AppLogger).to have_received(:error) + .with(hash_including(merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message))) end end @@ -406,7 +452,9 @@ RSpec.describe MergeRequests::MergeService do expect(merge_request.merge_commit_sha).to be_nil expect(merge_request.squash_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(Gitlab::AppLogger).to have_received(:error) + .with(hash_including(merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message))) end it 'logs and saves error if there is an PreReceiveError exception' do @@ -422,7 +470,9 @@ RSpec.describe MergeRequests::MergeService do expect(merge_request.merge_commit_sha).to be_nil expect(merge_request.squash_commit_sha).to be_nil expect(merge_request.merge_error).to include('Something went wrong during merge pre-receive hook') - expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error) + .with(hash_including(merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message))) end context 'when fast-forward merge is not allowed' do @@ -444,7 +494,9 @@ RSpec.describe MergeRequests::MergeService do expect(merge_request.merge_commit_sha).to be_nil expect(merge_request.squash_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(Gitlab::AppLogger).to have_received(:error) + .with(hash_including(merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message))) end end end @@ -461,7 +513,9 @@ RSpec.describe MergeRequests::MergeService do it 'logs and saves error' do service.execute(merge_request) - expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error) + .with(hash_including(merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message))) end end @@ -473,7 +527,9 @@ RSpec.describe MergeRequests::MergeService do it 'logs and saves error' do service.execute(merge_request) - expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error) + .with(hash_including(merge_request_info: merge_request.to_reference(full: true), + message: a_string_matching(error_message))) end context 'when passing `skip_discussions_check: true` as `options` parameter' do diff --git a/spec/services/merge_requests/mergeability/logger_spec.rb b/spec/services/merge_requests/mergeability/logger_spec.rb index a4d544884b9..3e2a1e9f9fd 100644 --- a/spec/services/merge_requests/mergeability/logger_spec.rb +++ b/spec/services/merge_requests/mergeability/logger_spec.rb @@ -94,25 +94,6 @@ RSpec.describe MergeRequests::Mergeability::Logger, :request_store do end end - context 'when disabled' do - before do - stub_feature_flags(mergeability_checks_logger: false) - end - - it "returns the block's value" do - expect(logger.instrument(mergeability_name: :expensive_operation) { 123 }).to eq(123) - end - - it 'does not call the logger' do - expect(Gitlab::AppJsonLogger).not_to receive(:new) - - expect(logger.instrument(mergeability_name: :expensive_operation) { Project.count + MergeRequest.count }) - .to eq(2) - - logger.commit - end - end - it 'raises an error when block is not provided' do expect { logger.instrument(mergeability_name: :expensive_operation) } .to raise_error(ArgumentError, 'block not given') 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 391377ad801..251bf6f0d9d 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -730,6 +730,15 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'with a deleted branch' it_behaves_like 'with the project default branch' + + context 'when passing in usernames' do + # makes sure that usernames starting with numbers aren't treated as IDs + let(:user2) { create(:user, username: '123user', developer_projects: [project]) } + let(:user3) { create(:user, username: '999user', developer_projects: [project]) } + let(:assigned) { { user2.username => 1, user3.username => 1 } } + + it_behaves_like 'with an existing branch that has a merge request open in foss' + end end describe '`unassign` push option' do @@ -743,6 +752,13 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'with a deleted branch' it_behaves_like 'with the project default branch' + + context 'when passing in usernames' do + let(:assigned) { { user2.username => 1, user3.username => 1 } } + let(:unassigned) { { user1.username => 1, user3.username => 1 } } + + it_behaves_like 'with an existing branch that has a merge request open in foss' + end end describe 'multiple pushed branches' do diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 09d06b8b2ab..5174ceaaa82 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe MergeRequests::RefreshService do include ProjectForksHelper - include ProjectHelpers + include UserHelpers let(:project) { create(:project, :repository) } let(:user) { create(:user) } @@ -189,7 +189,7 @@ RSpec.describe MergeRequests::RefreshService do subject { service.new(project: @project, current_user: @user).execute(@oldrev, @newrev, 'refs/heads/master') } - it 'updates the head_pipeline_id for @merge_request', :sidekiq_might_not_need_inline do + it 'updates the head_pipeline_id for @merge_request', :sidekiq_inline do expect { subject }.to change { @merge_request.reload.head_pipeline_id }.from(nil).to(pipeline.id) end @@ -306,7 +306,7 @@ RSpec.describe MergeRequests::RefreshService do subject end - it 'sets the latest detached merge request pipeline as a head pipeline', :sidekiq_might_not_need_inline do + it 'sets the latest detached merge request pipeline as a head pipeline' do @merge_request.reload expect(@merge_request.actual_head_pipeline).to be_merge_request_event end @@ -424,7 +424,7 @@ RSpec.describe MergeRequests::RefreshService do end end - context 'push to origin repo target branch', :sidekiq_might_not_need_inline do + context 'push to origin repo target branch' do context 'when all MRs to the target branch had diffs' do before do service.new(project: @project, current_user: @user).execute(@oldrev, @newrev, 'refs/heads/feature') @@ -474,7 +474,7 @@ RSpec.describe MergeRequests::RefreshService do end end - context 'manual merge of source branch', :sidekiq_might_not_need_inline do + context 'manual merge of source branch' do before do # Merge master -> feature branch @project.repository.merge(@user, @merge_request.diff_head_sha, @merge_request, 'Test message') @@ -496,7 +496,7 @@ RSpec.describe MergeRequests::RefreshService do end end - context 'push to fork repo source branch', :sidekiq_might_not_need_inline do + context 'push to fork repo source branch' do let(:refresh_service) { service.new(project: @fork_project, current_user: @user) } def refresh @@ -561,7 +561,7 @@ RSpec.describe MergeRequests::RefreshService do end end - context 'push to fork repo target branch', :sidekiq_might_not_need_inline do + context 'push to fork repo target branch' do describe 'changes to merge requests' do before do service.new(project: @fork_project, current_user: @user).execute(@oldrev, @newrev, 'refs/heads/feature') @@ -587,7 +587,7 @@ RSpec.describe MergeRequests::RefreshService do end end - context 'forked projects with the same source branch name as target branch', :sidekiq_might_not_need_inline do + context 'forked projects with the same source branch name as target branch' do let!(:first_commit) do @fork_project.repository.create_file(@user, 'test1.txt', 'Test data', message: 'Test commit', @@ -671,7 +671,7 @@ RSpec.describe MergeRequests::RefreshService do context 'push new branch that exists in a merge request' do let(:refresh_service) { service.new(project: @fork_project, current_user: @user) } - it 'refreshes the merge request', :sidekiq_might_not_need_inline do + it 'refreshes the merge request' do expect(refresh_service).to receive(:execute_hooks) .with(@fork_merge_request, 'update', old_rev: Gitlab::Git::BLANK_SHA) allow_any_instance_of(Repository).to receive(:merge_base).and_return(@oldrev) @@ -799,23 +799,24 @@ RSpec.describe MergeRequests::RefreshService do it 'does not mark as draft based on commits that do not belong to an MR' do allow(refresh_service).to receive(:find_new_commits) - refresh_service.instance_variable_set("@commits", [ - double( - id: 'aaaaaaa', - sha: 'aaaaaaa', - short_id: 'aaaaaaa', - title: 'Fix issue', - draft?: false - ), - double( - id: 'bbbbbbb', - sha: 'bbbbbbbb', - short_id: 'bbbbbbb', - title: 'fixup! Fix issue', - draft?: true, - to_reference: 'bbbbbbb' - ) - ]) + refresh_service.instance_variable_set("@commits", + [ + double( + id: 'aaaaaaa', + sha: 'aaaaaaa', + short_id: 'aaaaaaa', + title: 'Fix issue', + draft?: false + ), + double( + id: 'bbbbbbb', + sha: 'bbbbbbbb', + short_id: 'bbbbbbb', + title: 'fixup! Fix issue', + draft?: true, + to_reference: 'bbbbbbb' + ) + ]) refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') reload_mrs diff --git a/spec/services/merge_requests/request_review_service_spec.rb b/spec/services/merge_requests/request_review_service_spec.rb index 8bc31df605c..1d3f92b083f 100644 --- a/spec/services/merge_requests/request_review_service_spec.rb +++ b/spec/services/merge_requests/request_review_service_spec.rb @@ -25,20 +25,26 @@ RSpec.describe MergeRequests::RequestReviewService do end describe '#execute' do - describe 'invalid permissions' do - let(:service) { described_class.new(project: project, current_user: create(:user)) } - + shared_examples_for 'failed service execution' do it 'returns an error' do expect(result[:status]).to eq :error end + + it_behaves_like 'does not trigger GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { result } + end + end + + describe 'invalid permissions' do + let(:service) { described_class.new(project: project, current_user: create(:user)) } + + it_behaves_like 'failed service execution' end describe 'reviewer does not exist' do let(:result) { service.execute(merge_request, create(:user)) } - it 'returns an error' do - expect(result[:status]).to eq :error - end + it_behaves_like 'failed service execution' end describe 'reviewer exists' do @@ -64,6 +70,10 @@ RSpec.describe MergeRequests::RequestReviewService do service.execute(merge_request, user) end + + it_behaves_like 'triggers GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { result } + end end end end diff --git a/spec/services/merge_requests/update_assignees_service_spec.rb b/spec/services/merge_requests/update_assignees_service_spec.rb index 3a0b17c2768..2d80d75a262 100644 --- a/spec/services/merge_requests/update_assignees_service_spec.rb +++ b/spec/services/merge_requests/update_assignees_service_spec.rb @@ -36,6 +36,20 @@ RSpec.describe MergeRequests::UpdateAssigneesService do service.execute(merge_request) end + shared_examples 'it updates and enqueues the job' do + it 'correctly updates the MR and enqueues the job' do + expect_next(MergeRequests::HandleAssigneesChangeService, project: project, current_user: user) do |service| + expect(service) + .to receive(:async_execute).with(merge_request, [user3], execute_hooks: true) + end + + expect { update_merge_request } + .to change { merge_request.reload.assignees }.from([user3]).to(new_users) + .and change(merge_request, :updated_at) + .and change(merge_request, :updated_by).to(user) + end + end + shared_examples 'removing all assignees' do it 'removes all assignees' do expect(update_merge_request).to have_attributes(assignees: be_empty, errors: be_none) @@ -73,16 +87,8 @@ RSpec.describe MergeRequests::UpdateAssigneesService do it_behaves_like 'removing all assignees' end - it 'updates the MR, and queues the more expensive work for later' do - expect_next(MergeRequests::HandleAssigneesChangeService, project: project, current_user: user) do |service| - expect(service) - .to receive(:async_execute).with(merge_request, [user3], execute_hooks: true) - end - - expect { update_merge_request } - .to change { merge_request.reload.assignees }.from([user3]).to([user2]) - .and change(merge_request, :updated_at) - .and change(merge_request, :updated_by).to(user) + it_behaves_like 'it updates and enqueues the job' do + let(:new_users) { [user2] } end it 'does not update the assignees if they do not have access' do diff --git a/spec/services/merge_requests/update_reviewers_service_spec.rb b/spec/services/merge_requests/update_reviewers_service_spec.rb index 8920141adbb..9f935e1cecf 100644 --- a/spec/services/merge_requests/update_reviewers_service_spec.rb +++ b/spec/services/merge_requests/update_reviewers_service_spec.rb @@ -128,6 +128,10 @@ RSpec.describe MergeRequests::UpdateReviewersService do set_reviewers end + it_behaves_like 'triggers GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { set_reviewers } + end + it 'calls MergeRequest::ResolveTodosService#async_execute' do expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service| expect(service).to receive(:async_execute) @@ -149,6 +153,14 @@ RSpec.describe MergeRequests::UpdateReviewersService do set_reviewers end + context 'when reviewers did not change' do + let(:opts) { { reviewer_ids: merge_request.reviewer_ids } } + + it_behaves_like 'does not trigger GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { set_reviewers } + end + end + it 'does not update the reviewers if they do not have access' do opts[:reviewer_ids] = [create(:user).id] diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 8ebabd64d8a..1d67574b06d 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -425,16 +425,10 @@ RSpec.describe MergeRequests::UpdateService, :mailer do create(:merge_request, :simple, source_project: project, reviewer_ids: [user2.id]) end - context 'when merge_request_reviewer feature is enabled' do - before do - stub_feature_flags(merge_request_reviewer: true) - end - - let(:opts) { { reviewer_ids: [IssuableFinder::Params::NONE] } } + let(:opts) { { reviewer_ids: [IssuableFinder::Params::NONE] } } - it 'removes reviewers' do - expect(update_merge_request(opts).reviewers).to eq [] - end + it 'removes reviewers' do + expect(update_merge_request(opts).reviewers).to eq [] end end end @@ -625,6 +619,20 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(Todo.count).to eq(2) end + + it 'triggers GraphQL description updated subscription' do + expect(GraphqlTriggers).to receive(:issuable_description_updated).with(merge_request).and_call_original + + update_merge_request(description: 'updated description') + end + end + + context 'when decription is not changed' do + it 'does not trigger GraphQL description updated subscription' do + expect(GraphqlTriggers).not_to receive(:issuable_description_updated) + + update_merge_request(title: 'updated title') + end end context 'when is reassigned' do @@ -685,6 +693,16 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(user2.review_requested_open_merge_requests_count).to eq(1) expect(user3.review_requested_open_merge_requests_count).to eq(0) end + + it_behaves_like 'triggers GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { update_merge_request({ reviewer_ids: [user2.id] }) } + end + end + + context 'when reviewers did not change' do + it_behaves_like 'does not trigger GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { update_merge_request({ reviewer_ids: [merge_request.reviewer_ids] }) } + end end context 'when the milestone is removed' do @@ -827,6 +845,12 @@ RSpec.describe MergeRequests::UpdateService, :mailer do should_not_email(non_subscriber) end + it 'triggers GraphQL subscription mergeRequestMergeStatusUpdated' do + expect(GraphqlTriggers).to receive(:merge_request_merge_status_updated).with(merge_request) + + update_merge_request(title: 'New title') + end + context 'when removing through wip_event param' do it 'removes Draft from the title' do expect { update_merge_request({ wip_event: "ready" }) } @@ -853,6 +877,12 @@ RSpec.describe MergeRequests::UpdateService, :mailer do should_not_email(non_subscriber) end + it 'triggers GraphQL subscription mergeRequestMergeStatusUpdated' do + expect(GraphqlTriggers).to receive(:merge_request_merge_status_updated).with(merge_request) + + update_merge_request(title: 'Draft: New title') + end + context 'when adding through wip_event param' do it 'adds Draft to the title' do expect { update_merge_request({ wip_event: "draft" }) } diff --git a/spec/services/ml/experiment_tracking/candidate_repository_spec.rb b/spec/services/ml/experiment_tracking/candidate_repository_spec.rb new file mode 100644 index 00000000000..8002b2ebc86 --- /dev/null +++ b/spec/services/ml/experiment_tracking/candidate_repository_spec.rb @@ -0,0 +1,199 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ml::ExperimentTracking::CandidateRepository do + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be(:experiment) { create(:ml_experiments, user: user, project: project) } + let_it_be(:candidate) { create(:ml_candidates, user: user, experiment: experiment) } + + let(:repository) { described_class.new(project, user) } + + describe '#by_iid' do + let(:iid) { candidate.iid } + + subject { repository.by_iid(iid) } + + it { is_expected.to eq(candidate) } + + context 'when iid does not exist' do + let(:iid) { non_existing_record_iid.to_s } + + it { is_expected.to be_nil } + end + + context 'when iid belongs to a different project' do + let(:repository) { described_class.new(create(:project), user) } + + it { is_expected.to be_nil } + end + end + + describe '#create!' do + subject { repository.create!(experiment, 1234) } + + it 'creates the candidate' do + expect(subject.start_time).to eq(1234) + expect(subject.iid).not_to be_nil + expect(subject.end_time).to be_nil + end + end + + describe '#update' do + let(:end_time) { 123456 } + let(:status) { 'running' } + + subject { repository.update(candidate, status, end_time) } + + it { is_expected.to be_truthy } + + context 'when end_time is missing ' do + let(:end_time) { nil } + + it { is_expected.to be_truthy } + end + + context 'when status is wrong' do + let(:status) { 's' } + + it 'fails assigning the value' do + expect { subject }.to raise_error(ArgumentError) + end + end + + context 'when status is missing' do + let(:status) { nil } + + it { is_expected.to be_truthy } + end + end + + describe '#add_metric!' do + let(:props) { { name: 'abc', value: 1234, tracked: 12345678, step: 0 } } + let(:metrics_before) { candidate.metrics.size } + + before do + metrics_before + end + + subject { repository.add_metric!(candidate, props[:name], props[:value], props[:tracked], props[:step]) } + + it 'adds a new metric' do + expect { subject }.to change { candidate.metrics.size }.by(1) + end + + context 'when name missing' do + let(:props) { { value: 1234, tracked: 12345678, step: 0 } } + + it 'does not add metric' do + expect { subject }.to raise_error(ActiveRecord::RecordInvalid) + end + end + end + + describe '#add_param!' do + let(:props) { { name: 'abc', value: 'def' } } + + subject { repository.add_param!(candidate, props[:name], props[:value]) } + + it 'adds a new param' do + expect { subject }.to change { candidate.params.size }.by(1) + end + + context 'when name missing' do + let(:props) { { value: 1234 } } + + it 'throws RecordInvalid' do + expect { subject }.to raise_error(ActiveRecord::RecordInvalid) + end + end + + context 'when param was already added' do + it 'throws RecordInvalid' do + repository.add_param!(candidate, 'new', props[:value]) + + expect { repository.add_param!(candidate, 'new', props[:value]) }.to raise_error(ActiveRecord::RecordInvalid) + end + end + end + + describe "#add_params" do + let(:params) do + [{ key: 'model_class', value: 'LogisticRegression' }, { 'key': 'pythonEnv', value: '3.10' }] + end + + subject { repository.add_params(candidate, params) } + + it 'adds the parameters' do + expect { subject }.to change { candidate.reload.params.size }.by(2) + end + + context 'if parameter misses key' do + let(:params) do + [{ value: 'LogisticRegression' }] + end + + it 'does not throw and does not add' do + expect { subject }.to raise_error(ActiveRecord::ActiveRecordError) + end + end + + context 'if parameter misses value' do + let(:params) do + [{ key: 'pythonEnv2' }] + end + + it 'does not throw and does not add' do + expect { subject }.to raise_error(ActiveRecord::ActiveRecordError) + end + end + + context 'if parameter repeated do' do + let(:params) do + [ + { 'key': 'pythonEnv0', value: '2.7' }, + { 'key': 'pythonEnv1', value: '3.9' }, + { 'key': 'pythonEnv1', value: '3.10' } + ] + end + + before do + repository.add_param!(candidate, 'pythonEnv0', '0') + end + + it 'does not throw and adds only the first of each kind' do + expect { subject }.to change { candidate.reload.params.size }.by(1) + end + end + end + + describe "#add_metrics" do + let(:metrics) do + [ + { key: 'mae', value: 2.5, timestamp: 1552550804 }, + { key: 'rmse', value: 2.7, timestamp: 1552550804 } + ] + end + + subject { repository.add_metrics(candidate, metrics) } + + it 'adds the metrics' do + expect { subject }.to change { candidate.reload.metrics.size }.by(2) + end + + context 'when metrics have repeated keys' do + let(:metrics) do + [ + { key: 'mae', value: 2.5, timestamp: 1552550804 }, + { key: 'rmse', value: 2.7, timestamp: 1552550804 }, + { key: 'mae', value: 2.7, timestamp: 1552550805 } + ] + end + + it 'adds all of them' do + expect { subject }.to change { candidate.reload.metrics.size }.by(3) + end + end + end +end diff --git a/spec/services/ml/experiment_tracking/experiment_repository_spec.rb b/spec/services/ml/experiment_tracking/experiment_repository_spec.rb new file mode 100644 index 00000000000..80e1fa025d1 --- /dev/null +++ b/spec/services/ml/experiment_tracking/experiment_repository_spec.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ml::ExperimentTracking::ExperimentRepository do + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be(:experiment) { create(:ml_experiments, user: user, project: project) } + let_it_be(:experiment2) { create(:ml_experiments, user: user, project: project) } + let_it_be(:experiment3) { create(:ml_experiments, user: user, project: project) } + let_it_be(:experiment4) { create(:ml_experiments, user: user) } + + let(:repository) { described_class.new(project, user) } + + describe '#by_iid_or_name' do + let(:iid) { experiment.iid } + let(:name) { nil } + + subject { repository.by_iid_or_name(iid: iid, name: name) } + + context 'when iid passed' do + it('fetches the experiment') { is_expected.to eq(experiment) } + + context 'and name passed' do + let(:name) { experiment2.name } + + it('ignores the name') { is_expected.to eq(experiment) } + end + + context 'and does not exist' do + let(:iid) { non_existing_record_iid } + + it { is_expected.to eq(nil) } + end + end + + context 'when iid is not passed', 'and name is passed' do + let(:iid) { nil } + + context 'when name exists' do + let(:name) { experiment2.name } + + it('fetches the experiment') { is_expected.to eq(experiment2) } + end + + context 'when name does not exist' do + let(:name) { non_existing_record_iid } + + it { is_expected.to eq(nil) } + end + end + end + + describe '#all' do + it 'fetches experiments for project' do + expect(repository.all).to match_array([experiment, experiment2, experiment3]) + end + end + + describe '#create!' do + let(:name) { 'hello' } + + subject { repository.create!(name) } + + it 'creates the candidate' do + expect { subject }.to change { repository.all.size }.by(1) + end + + context 'when name exists' do + let(:name) { experiment.name } + + it 'throws error' do + expect { subject }.to raise_error(ActiveRecord::ActiveRecordError) + end + end + + context 'when name is missing' do + let(:name) { nil } + + it 'throws error' do + expect { subject }.to raise_error(ActiveRecord::ActiveRecordError) + end + end + end +end diff --git a/spec/services/namespaces/package_settings/update_service_spec.rb b/spec/services/namespaces/package_settings/update_service_spec.rb index ed385f1cd7f..10926c5ef57 100644 --- a/spec/services/namespaces/package_settings/update_service_spec.rb +++ b/spec/services/namespaces/package_settings/update_service_spec.rb @@ -33,8 +33,29 @@ RSpec.describe ::Namespaces::PackageSettings::UpdateService do shared_examples 'updating the namespace package setting' do it_behaves_like 'updating the namespace package setting attributes', - from: { maven_duplicates_allowed: true, maven_duplicate_exception_regex: 'SNAPSHOT', generic_duplicates_allowed: true, generic_duplicate_exception_regex: 'foo' }, - to: { maven_duplicates_allowed: false, maven_duplicate_exception_regex: 'RELEASE', generic_duplicates_allowed: false, generic_duplicate_exception_regex: 'bar' } + from: { + maven_duplicates_allowed: true, + maven_duplicate_exception_regex: 'SNAPSHOT', + generic_duplicates_allowed: true, + generic_duplicate_exception_regex: 'foo', + maven_package_requests_forwarding: true, + lock_maven_package_requests_forwarding: false, + npm_package_requests_forwarding: nil, + lock_npm_package_requests_forwarding: false, + pypi_package_requests_forwarding: nil, + lock_pypi_package_requests_forwarding: false + }, to: { + maven_duplicates_allowed: false, + maven_duplicate_exception_regex: 'RELEASE', + generic_duplicates_allowed: false, + generic_duplicate_exception_regex: 'bar', + maven_package_requests_forwarding: true, + lock_maven_package_requests_forwarding: true, + npm_package_requests_forwarding: true, + lock_npm_package_requests_forwarding: true, + pypi_package_requests_forwarding: true, + lock_pypi_package_requests_forwarding: true + } it_behaves_like 'returning a success' @@ -63,10 +84,18 @@ RSpec.describe ::Namespaces::PackageSettings::UpdateService do context 'with existing namespace package setting' do let_it_be(:package_settings) { create(:namespace_package_setting, namespace: namespace) } let_it_be(:params) do - { maven_duplicates_allowed: false, + { + maven_duplicates_allowed: false, maven_duplicate_exception_regex: 'RELEASE', generic_duplicates_allowed: false, - generic_duplicate_exception_regex: 'bar' } + generic_duplicate_exception_regex: 'bar', + maven_package_requests_forwarding: true, + lock_maven_package_requests_forwarding: true, + npm_package_requests_forwarding: true, + lock_npm_package_requests_forwarding: true, + pypi_package_requests_forwarding: true, + lock_pypi_package_requests_forwarding: true + } end where(:user_role, :shared_examples_name) do diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 935dcef1011..8fbf023cda0 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -337,6 +337,27 @@ RSpec.describe NotificationService, :mailer do end end end + + describe '#access_token_revoked' do + let_it_be(:user) { create(:user) } + let_it_be(:pat) { create(:personal_access_token, user: user) } + + subject(:notification_service) { notification.access_token_revoked(user, pat.name) } + + it 'sends email to the token owner' do + expect { notification_service }.to have_enqueued_email(user, pat.name, mail: "access_token_revoked_email") + end + + context 'when user is not allowed to receive notifications' do + before do + user.block! + end + + it 'does not send email to the token owner' do + expect { notification_service }.not_to have_enqueued_email(user, pat.name, mail: "access_token_revoked_email") + end + end + end end describe 'SSH Keys' do diff --git a/spec/services/onboarding/progress_service_spec.rb b/spec/services/onboarding/progress_service_spec.rb index e9b8ea2e859..8f3f723613e 100644 --- a/spec/services/onboarding/progress_service_spec.rb +++ b/spec/services/onboarding/progress_service_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Onboarding::ProgressService do context 'when not onboarded' do it 'does not schedule a worker' do - expect(Namespaces::OnboardingProgressWorker).not_to receive(:perform_async) + expect(Onboarding::ProgressWorker).not_to receive(:perform_async) execute_service end @@ -28,7 +28,7 @@ RSpec.describe Onboarding::ProgressService do end it 'does not schedule a worker' do - expect(Namespaces::OnboardingProgressWorker).not_to receive(:perform_async) + expect(Onboarding::ProgressWorker).not_to receive(:perform_async) execute_service end @@ -36,7 +36,7 @@ RSpec.describe Onboarding::ProgressService do context 'when action is not yet completed' do it 'schedules a worker' do - expect(Namespaces::OnboardingProgressWorker).to receive(:perform_async) + expect(Onboarding::ProgressWorker).to receive(:perform_async) execute_service end diff --git a/spec/services/packages/debian/create_package_file_service_spec.rb b/spec/services/packages/debian/create_package_file_service_spec.rb index c8292b2d5c2..291f6df991c 100644 --- a/spec/services/packages/debian/create_package_file_service_spec.rb +++ b/spec/services/packages/debian/create_package_file_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Packages::Debian::CreatePackageFileService do include WorkhorseHelpers let_it_be(:package) { create(:debian_incoming, without_package_files: true) } + let_it_be(:current_user) { create(:user) } describe '#execute' do let(:file_name) { 'libsample0_1.2.3~alpha2_amd64.deb' } @@ -20,12 +21,13 @@ RSpec.describe Packages::Debian::CreatePackageFileService do }.with_indifferent_access end - let(:service) { described_class.new(package, params) } + let(:service) { described_class.new(package: package, current_user: current_user, params: params) } subject(:package_file) { service.execute } shared_examples 'a valid deb' do it 'creates a new package file', :aggregate_failures do + expect(::Packages::Debian::ProcessChangesWorker).not_to receive(:perform_async) expect(package_file).to be_valid expect(package_file.file.read).to start_with('!<arch>') expect(package_file.size).to eq(1124) @@ -40,6 +42,24 @@ RSpec.describe Packages::Debian::CreatePackageFileService do end end + shared_examples 'a valid changes' do + it 'creates a new package file', :aggregate_failures do + expect(::Packages::Debian::ProcessChangesWorker).to receive(:perform_async) + + expect(package_file).to be_valid + expect(package_file.file.read).to start_with('Format: 1.8') + expect(package_file.size).to eq(2143) + expect(package_file.file_name).to eq(file_name) + expect(package_file.file_sha1).to eq('54321') + expect(package_file.file_sha256).to eq('543212345') + expect(package_file.file_md5).to eq('12345') + expect(package_file.debian_file_metadatum).to be_valid + expect(package_file.debian_file_metadatum.file_type).to eq('unknown') + expect(package_file.debian_file_metadatum.architecture).to be_nil + expect(package_file.debian_file_metadatum.fields).to be_nil + end + end + context 'with temp file' do let!(:file) do upload_path = ::Packages::PackageFileUploader.workhorse_local_upload_path @@ -52,6 +72,21 @@ RSpec.describe Packages::Debian::CreatePackageFileService do end it_behaves_like 'a valid deb' + + context 'with a .changes file' do + let(:file_name) { 'sample_1.2.3~alpha2_amd64.changes' } + let(:fixture_path) { "spec/fixtures/packages/debian/#{file_name}" } + + it_behaves_like 'a valid changes' + end + + context 'when current_user is missing' do + let(:current_user) { nil } + + it 'raises an error' do + expect { package_file }.to raise_error(ArgumentError, 'Invalid user') + end + end end context 'with remote file' do @@ -77,37 +112,37 @@ RSpec.describe Packages::Debian::CreatePackageFileService do it_behaves_like 'a valid deb' end - context 'package is missing' do + context 'when package is missing' do let(:package) { nil } let(:params) { {} } it 'raises an error' do - expect { subject.execute }.to raise_error(ArgumentError, 'Invalid package') + expect { package_file }.to raise_error(ArgumentError, 'Invalid package') end end - context 'params is empty' do + context 'when params is empty' do let(:params) { {} } it 'raises an error' do - expect { subject.execute }.to raise_error(ActiveRecord::RecordInvalid) + expect { package_file }.to raise_error(ActiveRecord::RecordInvalid) end end - context 'file is missing' do + context 'when file is missing' do let(:file_name) { 'libsample0_1.2.3~alpha2_amd64.deb' } let(:file) { nil } it 'raises an error' do - expect { subject.execute }.to raise_error(ActiveRecord::RecordInvalid) + expect { package_file }.to raise_error(ActiveRecord::RecordInvalid) end end - context 'FIPS mode enabled', :fips_mode do + context 'when FIPS mode enabled', :fips_mode do let(:file) { nil } it 'raises an error' do - expect { subject.execute }.to raise_error(::Packages::FIPS::DisabledError) + expect { package_file }.to raise_error(::Packages::FIPS::DisabledError) end end end diff --git a/spec/services/packages/mark_packages_for_destruction_service_spec.rb b/spec/services/packages/mark_packages_for_destruction_service_spec.rb new file mode 100644 index 00000000000..5c043b89de8 --- /dev/null +++ b/spec/services/packages/mark_packages_for_destruction_service_spec.rb @@ -0,0 +1,107 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::MarkPackagesForDestructionService, :sidekiq_inline do + let_it_be(:project) { create(:project) } + let_it_be_with_reload(:packages) { create_list(:npm_package, 3, project: project) } + + let(:user) { project.owner } + + # The service only accepts ActiveRecord relationships and not arrays. + let(:service) { described_class.new(packages: ::Packages::Package.id_in(package_ids), current_user: user) } + let(:package_ids) { packages.map(&:id) } + + describe '#execute' do + subject { service.execute } + + context 'when the user is authorized' do + before do + project.add_maintainer(user) + end + + context 'when it is successful' do + it 'marks the packages as pending destruction' do + expect(::Packages::Maven::Metadata::SyncService).not_to receive(:new) + + expect { subject }.to change { ::Packages::Package.pending_destruction.count }.from(0).to(3) + .and change { Packages::PackageFile.pending_destruction.count }.from(0).to(3) + packages.each { |pkg| expect(pkg.reload).to be_pending_destruction } + + expect(subject).to be_a(ServiceResponse) + expect(subject).to be_success + expect(subject.message).to eq('Packages were successfully marked as pending destruction') + end + + context 'with maven packages' do + let_it_be_with_reload(:packages) { create_list(:maven_package, 3, project: project) } + + it 'marks the packages as pending destruction' do + expect(::Packages::Maven::Metadata::SyncService).to receive(:new).once.and_call_original + + expect { subject }.to change { ::Packages::Package.pending_destruction.count }.from(0).to(3) + .and change { Packages::PackageFile.pending_destruction.count }.from(0).to(9) + packages.each { |pkg| expect(pkg.reload).to be_pending_destruction } + + expect(subject).to be_a(ServiceResponse) + expect(subject).to be_success + expect(subject.message).to eq('Packages were successfully marked as pending destruction') + end + + context 'without version' do + before do + ::Packages::Package.id_in(package_ids).update_all(version: nil) + end + + it 'marks the packages as pending destruction' do + expect(::Packages::Maven::Metadata::SyncService).not_to receive(:new) + + expect { subject }.to change { ::Packages::Package.pending_destruction.count }.from(0).to(3) + .and change { Packages::PackageFile.pending_destruction.count }.from(0).to(9) + packages.each { |pkg| expect(pkg.reload).to be_pending_destruction } + + expect(subject).to be_a(ServiceResponse) + expect(subject).to be_success + expect(subject.message).to eq('Packages were successfully marked as pending destruction') + end + end + end + end + + context 'when it is not successful' do + before do + allow(service).to receive(:can_destroy_packages?).and_raise(StandardError, 'test') + end + + it 'returns an error ServiceResponse' do + expect(::Packages::Maven::Metadata::SyncService).not_to receive(:new) + + expect { subject }.to not_change { ::Packages::Package.pending_destruction.count } + .and not_change { ::Packages::PackageFile.pending_destruction.count } + + expect(subject).to be_a(ServiceResponse) + expect(subject).to be_error + expect(subject.message).to eq("Failed to mark the packages as pending destruction") + expect(subject.status).to eq(:error) + end + end + end + + context 'when the user is not authorized' do + let(:user) { nil } + + it 'returns an error ServiceResponse' do + expect(::Packages::Maven::Metadata::SyncService).not_to receive(:new) + + expect { subject }.to not_change { ::Packages::Package.pending_destruction.count } + .and not_change { ::Packages::PackageFile.pending_destruction.count } + + expect(subject).to be_a(ServiceResponse) + expect(subject).to be_error + expect(subject.message).to eq("You don't have the permission to perform this action") + expect(subject.status).to eq(:error) + expect(subject.reason).to eq(:unauthorized) + end + end + end +end diff --git a/spec/services/packages/rpm/parse_package_service_spec.rb b/spec/services/packages/rpm/parse_package_service_spec.rb new file mode 100644 index 00000000000..f330587bfa0 --- /dev/null +++ b/spec/services/packages/rpm/parse_package_service_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Packages::Rpm::ParsePackageService do + let(:package_file) { File.open('spec/fixtures/packages/rpm/hello-0.0.1-1.fc29.x86_64.rpm') } + + describe 'dynamic private methods' do + described_class::BUILD_ATTRIBUTES_METHOD_NAMES.each do |attribute| + it 'define dynamic build attribute method' do + expect(described_class).to be_private_method_defined("build_#{attribute}") + end + end + end + + describe '#execute' do + subject { described_class.new(package_file).execute } + + shared_examples 'valid package parsing' do + it 'return hash' do + expect(subject).to be_a(Hash) + end + + it 'has all static attribute keys' do + expect(subject.keys).to include(*described_class::STATIC_ATTRIBUTES) + end + + it 'includes epoch attribute' do + expect(subject[:epoch]).not_to be_blank + end + + it 'has all built attributes with array values' do + result = subject + described_class::BUILD_ATTRIBUTES_METHOD_NAMES.each do |attribute| + expect(result).to have_key(attribute) + expect(result[attribute]).to be_a(Array) + end + end + end + + context 'when wrong format file received' do + let(:package_file) { File.open('spec/fixtures/rails_sample.jpg') } + + it 'raise error' do + expect { subject }.to raise_error(ArgumentError) + end + end + + context 'when valid file uploaded' do + context 'when .rpm file uploaded' do + it_behaves_like 'valid package parsing' + end + + context 'when .src.rpm file uploaded' do + let(:package_file) { File.open('spec/fixtures/packages/rpm/hello-0.0.1-1.fc29.src.rpm') } + + it_behaves_like 'valid package parsing' + end + end + end +end diff --git a/spec/services/packages/rpm/repository_metadata/base_builder_spec.rb b/spec/services/packages/rpm/repository_metadata/base_builder_spec.rb index 0fb58cc27d5..524c224177b 100644 --- a/spec/services/packages/rpm/repository_metadata/base_builder_spec.rb +++ b/spec/services/packages/rpm/repository_metadata/base_builder_spec.rb @@ -3,7 +3,10 @@ require 'spec_helper' RSpec.describe Packages::Rpm::RepositoryMetadata::BaseBuilder do describe '#execute' do - subject { described_class.new.execute } + subject { described_class.new(xml: xml, data: data).execute } + + let(:xml) { nil } + let(:data) { {} } before do stub_const("#{described_class}::ROOT_TAG", 'test') @@ -18,5 +21,13 @@ RSpec.describe Packages::Rpm::RepositoryMetadata::BaseBuilder do expect(result.children.first.attributes['foo1'].value).to eq('bar1') expect(result.children.first.attributes['foo2'].value).to eq('bar2') end + + context 'when call with parameters' do + let(:xml) { 'test' } + + it 'raise NotImplementedError' do + expect { subject }.to raise_error NotImplementedError + end + end end end diff --git a/spec/services/packages/rpm/repository_metadata/build_primary_xml_spec.rb b/spec/services/packages/rpm/repository_metadata/build_primary_xml_spec.rb index f5294d6f7f7..147d5862a71 100644 --- a/spec/services/packages/rpm/repository_metadata/build_primary_xml_spec.rb +++ b/spec/services/packages/rpm/repository_metadata/build_primary_xml_spec.rb @@ -3,18 +3,32 @@ require 'spec_helper' RSpec.describe Packages::Rpm::RepositoryMetadata::BuildPrimaryXml do describe '#execute' do - subject { described_class.new.execute } + subject { described_class.new(xml: xml, data: data).execute } - context "when generate empty xml" do - let(:expected_xml) do - <<~XML - <?xml version="1.0" encoding="UTF-8"?> - <metadata xmlns="http://linux.duke.edu/metadata/common" xmlns:rpm="http://linux.duke.edu/metadata/rpm" packages="0"/> - XML - end + let(:empty_xml) do + <<~XML + <?xml version="1.0" encoding="UTF-8"?> + <metadata xmlns="http://linux.duke.edu/metadata/common" xmlns:rpm="http://linux.duke.edu/metadata/rpm" packages="0"/> + XML + end + + it_behaves_like 'handling rpm xml file' + + context 'when updating existing xml' do + include_context 'with rpm package data' + + let(:xml) { empty_xml } + let(:data) { xml_update_params } + let(:required_text_only_attributes) { %i[description summary arch name] } + + it 'adds node with required_text_only_attributes' do + result = Nokogiri::XML::Document.parse(subject).remove_namespaces! - it 'generate expected xml' do - expect(subject).to eq(expected_xml) + required_text_only_attributes.each do |attribute| + expect( + result.at("//#{described_class::ROOT_TAG}/package/#{attribute}").text + ).to eq(data[attribute]) + end end end end diff --git a/spec/services/packages/rpm/repository_metadata/build_repomd_xml_spec.rb b/spec/services/packages/rpm/repository_metadata/build_repomd_xml_spec.rb index 29b0f73e3c1..0843a983b7e 100644 --- a/spec/services/packages/rpm/repository_metadata/build_repomd_xml_spec.rb +++ b/spec/services/packages/rpm/repository_metadata/build_repomd_xml_spec.rb @@ -62,5 +62,25 @@ RSpec.describe Packages::Rpm::RepositoryMetadata::BuildRepomdXml do end end end + + context 'when data values has unexpected keys' do + let(:data) do + { + filelists: described_class::ALLOWED_DATA_VALUE_KEYS.each_with_object({}) do |key, result| + result[:"#{key}-wrong"] = { value: 'value' } + end + } + end + + it 'ignores wrong keys' do + result = Nokogiri::XML::Document.parse(subject).remove_namespaces! + + data.each do |tag_name, tag_attributes| + tag_attributes.each_key do |key| + expect(result.at("//repomd/data[@type=\"#{tag_name}\"]/#{key}")).to be_nil + end + end + end + end end end diff --git a/spec/services/pages_domains/create_acme_order_service_spec.rb b/spec/services/pages_domains/create_acme_order_service_spec.rb index b882c253613..35b2cc56973 100644 --- a/spec/services/pages_domains/create_acme_order_service_spec.rb +++ b/spec/services/pages_domains/create_acme_order_service_spec.rb @@ -38,21 +38,13 @@ RSpec.describe PagesDomains::CreateAcmeOrderService do expect(challenge).to have_received(:request_validation).ordered end - it 'generates and saves private key: rsa' do - stub_feature_flags(pages_lets_encrypt_ecdsa: false) + it 'generates and saves private key' do service.execute saved_order = PagesDomainAcmeOrder.last expect { OpenSSL::PKey::RSA.new(saved_order.private_key) }.not_to raise_error end - it 'generates and saves private key: ec' do - service.execute - - saved_order = PagesDomainAcmeOrder.last - expect { OpenSSL::PKey::EC.new(saved_order.private_key) }.not_to raise_error - end - it 'properly saves order attributes' do service.execute diff --git a/spec/services/pages_domains/create_service_spec.rb b/spec/services/pages_domains/create_service_spec.rb new file mode 100644 index 00000000000..cac941fb134 --- /dev/null +++ b/spec/services/pages_domains/create_service_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::PagesDomains::CreateService do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :in_subgroup) } + + let(:domain) { 'new.domain.com' } + let(:attributes) { { domain: domain } } + + subject(:service) { described_class.new(project, user, attributes) } + + context 'when the user does not have the required permissions' do + it 'does not create a pages domain and does not publish a PagesDomainCreatedEvent' do + expect(service.execute).to be_nil + + expect { service.execute } + .to not_publish_event(PagesDomains::PagesDomainCreatedEvent) + .and not_change(project.pages_domains, :count) + end + end + + context 'when the user has the required permissions' do + before do + project.add_maintainer(user) + end + + context 'when it saves the domain successfully' do + it 'creates the domain and publishes a PagesDomainCreatedEvent' do + pages_domain = nil + + expect { pages_domain = service.execute } + .to change(project.pages_domains, :count) + .and publish_event(PagesDomains::PagesDomainCreatedEvent) + .with( + project_id: project.id, + namespace_id: project.namespace.id, + root_namespace_id: project.root_namespace.id, + domain: domain + ) + + expect(pages_domain).to be_persisted + end + end + + context 'when it fails to save the domain' do + let(:domain) { nil } + + it 'does not create a pages domain and does not publish a PagesDomainCreatedEvent' do + pages_domain = nil + + expect { pages_domain = service.execute } + .to not_publish_event(PagesDomains::PagesDomainCreatedEvent) + .and not_change(project.pages_domains, :count) + + expect(pages_domain).not_to be_persisted + end + end + end +end diff --git a/spec/services/pages_domains/delete_service_spec.rb b/spec/services/pages_domains/delete_service_spec.rb new file mode 100644 index 00000000000..5f98fe3c7f7 --- /dev/null +++ b/spec/services/pages_domains/delete_service_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::PagesDomains::DeleteService do + let_it_be(:user) { create(:user) } + let_it_be(:pages_domain) { create(:pages_domain, :with_project) } + + let(:params) do + attributes_for(:pages_domain, :with_trusted_chain).slice(:key, :certificate).tap do |params| + params[:user_provided_key] = params.delete(:key) + params[:user_provided_certificate] = params.delete(:certificate) + end + end + + subject(:service) { described_class.new(pages_domain.project, user, params) } + + context 'when the user does not have the required permissions' do + it 'does not delete the pages domain and does not publish a PagesDomainDeletedEvent' do + result_match = -> { expect(service.execute(pages_domain)).to be_nil } + + expect(&result_match) + .to not_publish_event(PagesDomains::PagesDomainDeletedEvent) + end + end + + context 'when the user has the required permissions' do + before do + pages_domain.project.add_maintainer(user) + end + + context 'when it updates the domain successfully' do + it 'deletes the domain and publishes a PagesDomainDeletedEvent' do + result_match = -> { expect(service.execute(pages_domain)).not_to be_nil } + + expect(&result_match) + .to publish_event(PagesDomains::PagesDomainDeletedEvent) + .with( + project_id: pages_domain.project.id, + namespace_id: pages_domain.project.namespace.id, + root_namespace_id: pages_domain.project.root_namespace.id, + domain: pages_domain.domain + ) + end + end + end +end diff --git a/spec/services/pages_domains/update_service_spec.rb b/spec/services/pages_domains/update_service_spec.rb new file mode 100644 index 00000000000..f6558f56422 --- /dev/null +++ b/spec/services/pages_domains/update_service_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe PagesDomains::UpdateService do + let_it_be(:user) { create(:user) } + let_it_be(:pages_domain) { create(:pages_domain, :with_project) } + + let(:params) do + attributes_for(:pages_domain, :with_trusted_chain).slice(:key, :certificate).tap do |params| + params[:user_provided_key] = params.delete(:key) + params[:user_provided_certificate] = params.delete(:certificate) + end + end + + subject(:service) { described_class.new(pages_domain.project, user, params) } + + context 'when the user does not have the required permissions' do + it 'does not update the pages domain and does not publish a PagesDomainUpdatedEvent' do + expect do + expect(service.execute(pages_domain)).to be_nil + end.to not_publish_event(PagesDomains::PagesDomainUpdatedEvent) + end + end + + context 'when the user has the required permissions' do + before do + pages_domain.project.add_maintainer(user) + end + + context 'when it updates the domain successfully' do + it 'updates the domain' do + expect(service.execute(pages_domain)).to eq(true) + end + + it 'publishes a PagesDomainUpdatedEvent' do + expect { service.execute(pages_domain) } + .to publish_event(PagesDomains::PagesDomainUpdatedEvent) + .with( + project_id: pages_domain.project.id, + namespace_id: pages_domain.project.namespace.id, + root_namespace_id: pages_domain.project.root_namespace.id, + domain: pages_domain.domain + ) + end + end + + context 'when it fails to update the domain' do + let(:params) { { user_provided_certificate: 'blabla' } } + + it 'does not update a pages domain' do + expect(service.execute(pages_domain)).to be(false) + end + + it 'does not publish a PagesDomainUpdatedEvent' do + expect { service.execute(pages_domain) } + .not_to publish_event(PagesDomains::PagesDomainUpdatedEvent) + end + end + end +end diff --git a/spec/services/projects/autocomplete_service_spec.rb b/spec/services/projects/autocomplete_service_spec.rb index 54a21d2f22b..bc95a1f3c8b 100644 --- a/spec/services/projects/autocomplete_service_spec.rb +++ b/spec/services/projects/autocomplete_service_spec.rb @@ -154,23 +154,49 @@ RSpec.describe Projects::AutocompleteService do let_it_be(:project) { create(:project, group: group) } let_it_be(:contact_1) { create(:contact, group: group) } let_it_be(:contact_2) { create(:contact, group: group) } + let_it_be(:contact_3) { create(:contact, :inactive, group: group) } - subject { described_class.new(project, user).contacts.as_json } + let(:issue) { nil } + + subject { described_class.new(project, user).contacts(issue).as_json } before do group.add_developer(user) end - it 'returns contact data correctly' do + it 'returns CRM contacts from group' do expected_contacts = [ { 'id' => contact_1.id, 'email' => contact_1.email, - 'first_name' => contact_1.first_name, 'last_name' => contact_1.last_name }, + 'first_name' => contact_1.first_name, 'last_name' => contact_1.last_name, 'state' => contact_1.state }, { 'id' => contact_2.id, 'email' => contact_2.email, - 'first_name' => contact_2.first_name, 'last_name' => contact_2.last_name } + 'first_name' => contact_2.first_name, 'last_name' => contact_2.last_name, 'state' => contact_2.state }, + { 'id' => contact_3.id, 'email' => contact_3.email, + 'first_name' => contact_3.first_name, 'last_name' => contact_3.last_name, 'state' => contact_3.state } ] expect(subject).to match_array(expected_contacts) end + + context 'some contacts are already assigned to the issue' do + let(:issue) { create(:issue, project: project) } + + before do + issue.customer_relations_contacts << [contact_2, contact_3] + end + + it 'marks already assigned contacts as set' do + expected_contacts = [ + { 'id' => contact_1.id, 'email' => contact_1.email, + 'first_name' => contact_1.first_name, 'last_name' => contact_1.last_name, 'state' => contact_1.state, 'set' => false }, + { 'id' => contact_2.id, 'email' => contact_2.email, + 'first_name' => contact_2.first_name, 'last_name' => contact_2.last_name, 'state' => contact_2.state, 'set' => true }, + { 'id' => contact_3.id, 'email' => contact_3.email, + 'first_name' => contact_3.first_name, 'last_name' => contact_3.last_name, 'state' => contact_3.state, 'set' => true } + ] + + expect(subject).to match_array(expected_contacts) + end + end end describe '#labels_as_hash' do 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 2008de195ab..8311c4e4d9b 100644 --- a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb @@ -2,372 +2,134 @@ require 'spec_helper' -RSpec.describe Projects::ContainerRepository::CleanupTagsService, :clean_gitlab_redis_cache do - using RSpec::Parameterized::TableSyntax +RSpec.describe Projects::ContainerRepository::CleanupTagsService do + let_it_be_with_reload(:container_repository) { create(:container_repository) } + let_it_be(:user) { container_repository.project.owner } - include_context 'for a cleanup tags service' - - let_it_be(:user) { create(:user) } - let_it_be(:project, reload: true) { create(:project, :private) } - - let(:repository) { create(:container_repository, :root, project: project) } - let(:service) { described_class.new(container_repository: repository, current_user: user, params: params) } - let(:tags) { %w[latest A Ba Bb C D E] } + let(:params) { {} } + let(:extra_params) { {} } + let(:service) { described_class.new(container_repository: container_repository, current_user: user, params: params.merge(extra_params)) } before do - project.add_maintainer(user) if user - stub_container_registry_config(enabled: true) - - stub_container_registry_tags( - repository: repository.path, - tags: tags - ) - - stub_tag_digest('latest', 'sha256:configA') - stub_tag_digest('A', 'sha256:configA') - stub_tag_digest('Ba', 'sha256:configB') - stub_tag_digest('Bb', 'sha256:configB') - stub_tag_digest('C', 'sha256:configC') - stub_tag_digest('D', 'sha256:configD') - stub_tag_digest('E', nil) - - stub_digest_config('sha256:configA', 1.hour.ago) - stub_digest_config('sha256:configB', 5.days.ago) - stub_digest_config('sha256:configC', 1.month.ago) - stub_digest_config('sha256:configD', nil) end describe '#execute' do subject { service.execute } - it_behaves_like 'handling invalid params', - service_response_extra: { - before_truncate_size: 0, - after_truncate_size: 0, - before_delete_size: 0, - cached_tags_count: 0 - }, - supports_caching: true - - it_behaves_like 'when regex matching everything is specified', - delete_expectations: [%w(A Ba Bb C D E)], - service_response_extra: { - before_truncate_size: 6, - after_truncate_size: 6, - before_delete_size: 6, - cached_tags_count: 0 - }, - supports_caching: true - - it_behaves_like 'when delete regex matching specific tags is used', - service_response_extra: { - before_truncate_size: 2, - after_truncate_size: 2, - before_delete_size: 2, - cached_tags_count: 0 - }, - supports_caching: true - - it_behaves_like 'when delete regex matching specific tags is used with overriding allow regex', - service_response_extra: { - before_truncate_size: 1, - after_truncate_size: 1, - before_delete_size: 1, - cached_tags_count: 0 - }, - supports_caching: true - - it_behaves_like 'with allow regex value', - delete_expectations: [%w(A C D E)], - service_response_extra: { - before_truncate_size: 4, - after_truncate_size: 4, - before_delete_size: 4, - cached_tags_count: 0 - }, - supports_caching: true - - it_behaves_like 'when keeping only N tags', - delete_expectations: [%w(Bb Ba C)], - service_response_extra: { - before_truncate_size: 4, - after_truncate_size: 4, - before_delete_size: 3, - cached_tags_count: 0 - }, - supports_caching: true - - it_behaves_like 'when not keeping N tags', - delete_expectations: [%w(A Ba Bb C)], - service_response_extra: { - before_truncate_size: 4, - after_truncate_size: 4, - before_delete_size: 4, - cached_tags_count: 0 - }, - supports_caching: true - - it_behaves_like 'when removing keeping only 3', - delete_expectations: [%w(Bb Ba C)], - service_response_extra: { - before_truncate_size: 6, - after_truncate_size: 6, - before_delete_size: 3, - cached_tags_count: 0 - }, - supports_caching: true - - it_behaves_like 'when removing older than 1 day', - delete_expectations: [%w(Ba Bb C)], - service_response_extra: { - before_truncate_size: 6, - after_truncate_size: 6, - before_delete_size: 3, - cached_tags_count: 0 - }, - supports_caching: true - - it_behaves_like 'when combining all parameters', - delete_expectations: [%w(Bb Ba C)], - service_response_extra: { - before_truncate_size: 6, - after_truncate_size: 6, - before_delete_size: 3, - cached_tags_count: 0 - }, - supports_caching: true - - it_behaves_like 'when running a container_expiration_policy', - delete_expectations: [%w(Bb Ba C)], - service_response_extra: { - before_truncate_size: 6, - after_truncate_size: 6, - before_delete_size: 3, - cached_tags_count: 0 - }, - supports_caching: true - - context 'when running a container_expiration_policy with caching' do - let(:user) { nil } - let(:params) do - { - 'name_regex_delete' => '.*', - 'keep_n' => 1, - 'older_than' => '1 day', - 'container_expiration_policy' => true - } - end - - it 'expects caching to be used' do - expect_delete(%w(Bb Ba C), container_expiration_policy: true) - expect_caching - - subject - end - - context 'when setting set to false' do - before do - stub_application_setting(container_registry_expiration_policies_caching: false) - end - - it 'does not use caching' do - expect_delete(%w(Bb Ba C), container_expiration_policy: true) - expect_no_caching + shared_examples 'returning error message' do |message| + it "returns error #{message}" do + expect(::Projects::ContainerRepository::Gitlab::CleanupTagsService).not_to receive(:new) + expect(::Projects::ContainerRepository::ThirdParty::CleanupTagsService).not_to receive(:new) + expect(service).not_to receive(:log_info) - subject - end + expect(subject).to eq(status: :error, message: message) end end - context 'truncating the tags list' do - let(:params) do - { - 'name_regex_delete' => '.*', - 'keep_n' => 1 - } - end - - shared_examples 'returning the response' do |status:, original_size:, before_truncate_size:, after_truncate_size:, before_delete_size:| - it 'returns the response' do - expect_no_caching + shared_examples 'handling invalid regular expressions' do + shared_examples 'handling invalid regex' do + it_behaves_like 'returning error message', 'invalid regex' - result = subject + it 'calls error tracking service' do + expect(::Gitlab::ErrorTracking).to receive(:log_exception).and_call_original - service_response = expected_service_response( - status: status, - original_size: original_size, - deleted: nil - ).merge( - before_truncate_size: before_truncate_size, - after_truncate_size: after_truncate_size, - before_delete_size: before_delete_size, - cached_tags_count: 0 - ) - - expect(result).to eq(service_response) + subject end end - where(:max_list_size, :delete_tags_service_status, :expected_status, :expected_truncated) do - 10 | :success | :success | false - 10 | :error | :error | false - 3 | :success | :error | true - 3 | :error | :error | true - 0 | :success | :success | false - 0 | :error | :error | false - end + context 'when name_regex_delete is invalid' do + let(:extra_params) { { 'name_regex_delete' => '*test*' } } - with_them do - before do - stub_application_setting(container_registry_cleanup_tags_service_max_list_size: max_list_size) - allow_next_instance_of(Projects::ContainerRepository::DeleteTagsService) do |service| - expect(service).to receive(:execute).and_return(status: delete_tags_service_status) - end - end - - original_size = 7 - keep_n = 1 - - it_behaves_like( - 'returning the response', - status: params[:expected_status], - original_size: original_size, - before_truncate_size: original_size - keep_n, - after_truncate_size: params[:expected_truncated] ? params[:max_list_size] + keep_n : original_size - keep_n, - before_delete_size: params[:expected_truncated] ? params[:max_list_size] : original_size - keep_n - 1 # one tag is filtered out with older_than filter - ) + it_behaves_like 'handling invalid regex' end - end - context 'caching', :freeze_time do - let(:params) do - { - 'name_regex_delete' => '.*', - 'keep_n' => 1, - 'older_than' => '1 day', - 'container_expiration_policy' => true - } - end + context 'when name_regex is invalid' do + let(:extra_params) { { 'name_regex' => '*test*' } } - let(:tags_and_created_ats) do - { - 'A' => 1.hour.ago, - 'Ba' => 5.days.ago, - 'Bb' => 5.days.ago, - 'C' => 1.month.ago, - 'D' => nil, - 'E' => nil - } + it_behaves_like 'handling invalid regex' end - let(:cacheable_tags) { tags_and_created_ats.reject { |_, value| value.nil? } } + context 'when name_regex_keep is invalid' do + let(:extra_params) { { 'name_regex_keep' => '*test*' } } - before do - expect_delete(%w(Bb Ba C), container_expiration_policy: true) - # We froze time so we need to set the created_at stubs again - stub_digest_config('sha256:configA', 1.hour.ago) - stub_digest_config('sha256:configB', 5.days.ago) - stub_digest_config('sha256:configC', 1.month.ago) + it_behaves_like 'handling invalid regex' end + end - it 'caches the created_at values' do - expect_mget(tags_and_created_ats.keys) - expect_set(cacheable_tags) - - expect(subject).to include(cached_tags_count: 0) + shared_examples 'handling all types of container repositories' do + shared_examples 'calling service' do |service_class, extra_log_data: {}| + let(:service_double) { instance_double(service_class.to_s) } + + it "uses cleanup tags service #{service_class}" do + expect(service_class).to receive(:new).with(container_repository: container_repository, current_user: user, params: params).and_return(service_double) + expect(service_double).to receive(:execute).and_return('return value') + expect(service).to receive(:log_info) + .with( + { + container_repository_id: container_repository.id, + container_repository_path: container_repository.path, + project_id: container_repository.project.id + }.merge(extra_log_data)) + expect(subject).to eq('return value') + end end - context 'with cached values' do + context 'with a migrated repository' do before do - ::Gitlab::Redis::Cache.with do |redis| - redis.set(cache_key('C'), rfc3339(1.month.ago)) - end + container_repository.update_column(:migration_state, :import_done) end - it 'uses them' do - expect_mget(tags_and_created_ats.keys) - - # because C is already in cache, it should not be cached again - expect_set(cacheable_tags.except('C')) - - # We will ping the container registry for all tags *except* for C because it's cached - expect(ContainerRegistry::Blob).to receive(:new).with(repository, { "digest" => "sha256:configA" }).and_call_original - expect(ContainerRegistry::Blob).to receive(:new).with(repository, { "digest" => "sha256:configB" }).twice.and_call_original - expect(ContainerRegistry::Blob).not_to receive(:new).with(repository, { "digest" => "sha256:configC" }) - expect(ContainerRegistry::Blob).to receive(:new).with(repository, { "digest" => "sha256:configD" }).and_call_original - - expect(subject).to include(cached_tags_count: 1) - end - end + context 'supporting the gitlab api' do + before do + allow(container_repository.gitlab_api_client).to receive(:supports_gitlab_api?).and_return(true) + end - def expect_mget(keys) - Gitlab::Redis::Cache.with do |redis| - expect(redis).to receive(:mget).with(keys.map(&method(:cache_key))).and_call_original + it_behaves_like 'calling service', ::Projects::ContainerRepository::Gitlab::CleanupTagsService, extra_log_data: { gitlab_cleanup_tags_service: true } end - end - - def expect_set(tags) - selected_tags = tags.map do |tag_name, created_at| - ex = 1.day.seconds - (Time.zone.now - created_at).seconds - [tag_name, created_at, ex.to_i] if ex.positive? - end.compact - - return if selected_tags.count.zero? - Gitlab::Redis::Cache.with do |redis| - expect(redis).to receive(:pipelined).and_call_original - - expect_next_instance_of(Redis::PipelinedConnection) do |pipeline| - selected_tags.each do |tag_name, created_at, ex| - expect(pipeline).to receive(:set).with(cache_key(tag_name), rfc3339(created_at), ex: ex) - end + context 'not supporting the gitlab api' do + before do + allow(container_repository.gitlab_api_client).to receive(:supports_gitlab_api?).and_return(false) end + + it_behaves_like 'calling service', ::Projects::ContainerRepository::ThirdParty::CleanupTagsService, extra_log_data: { third_party_cleanup_tags_service: true } end end - def cache_key(tag_name) - "container_repository:{#{repository.id}}:tag:#{tag_name}:created_at" - end + context 'with a non migrated repository' do + before do + container_repository.update_column(:migration_state, :default) + container_repository.update!(created_at: ContainerRepository::MIGRATION_PHASE_1_ENDED_AT - 1.week) + end - def rfc3339(date_time) - # DateTime rfc3339 is different ActiveSupport::TimeWithZone rfc3339 - # The caching will use DateTime rfc3339 - DateTime.rfc3339(date_time.rfc3339).rfc3339 + it_behaves_like 'calling service', ::Projects::ContainerRepository::ThirdParty::CleanupTagsService, extra_log_data: { third_party_cleanup_tags_service: true } end end - end - private + context 'with valid user' do + it_behaves_like 'handling invalid regular expressions' + it_behaves_like 'handling all types of container repositories' + end - def stub_tag_digest(tag, digest) - allow_any_instance_of(ContainerRegistry::Client) - .to receive(:repository_tag_digest) - .with(repository.path, tag) { digest } + context 'for container expiration policy' do + let(:user) { nil } + let(:params) { { 'container_expiration_policy' => true } } - allow_any_instance_of(ContainerRegistry::Client) - .to receive(:repository_manifest) - .with(repository.path, tag) do - { 'config' => { 'digest' => digest } } if digest + it_behaves_like 'handling invalid regular expressions' + it_behaves_like 'handling all types of container repositories' end - end - def stub_digest_config(digest, created_at) - allow_any_instance_of(ContainerRegistry::Client) - .to receive(:blob) - .with(repository.path, digest, nil) do - { 'created' => created_at.to_datetime.rfc3339 }.to_json if created_at + context 'with not allowed user' do + let_it_be(:user) { create(:user) } + + it_behaves_like 'returning error message', 'access denied' end - end - def expect_caching - ::Gitlab::Redis::Cache.with do |redis| - expect(redis).to receive(:mget).and_call_original - expect(redis).to receive(:pipelined).and_call_original + context 'with no user' do + let(:user) { nil } - expect_next_instance_of(Redis::PipelinedConnection) do |pipeline| - expect(pipeline).to receive(:set).and_call_original - end + it_behaves_like 'returning error message', 'access denied' end end end diff --git a/spec/services/projects/container_repository/gitlab/cleanup_tags_service_spec.rb b/spec/services/projects/container_repository/gitlab/cleanup_tags_service_spec.rb index d2cdb667659..59827ea035e 100644 --- a/spec/services/projects/container_repository/gitlab/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/gitlab/cleanup_tags_service_spec.rb @@ -46,8 +46,6 @@ RSpec.describe Projects::ContainerRepository::Gitlab::CleanupTagsService do context 'with several tags pages' do let(:tags_page_size) { 2 } - it_behaves_like 'handling invalid params' - it_behaves_like 'when regex matching everything is specified', delete_expectations: [%w[A], %w[Ba Bb], %w[C D], %w[E]] @@ -105,8 +103,6 @@ RSpec.describe Projects::ContainerRepository::Gitlab::CleanupTagsService do context 'with a single tags page' do let(:tags_page_size) { 1000 } - it_behaves_like 'handling invalid params' - it_behaves_like 'when regex matching everything is specified', delete_expectations: [%w[A Ba Bb C D E]] diff --git a/spec/services/projects/container_repository/third_party/cleanup_tags_service_spec.rb b/spec/services/projects/container_repository/third_party/cleanup_tags_service_spec.rb new file mode 100644 index 00000000000..2d034d577ac --- /dev/null +++ b/spec/services/projects/container_repository/third_party/cleanup_tags_service_spec.rb @@ -0,0 +1,370 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::ContainerRepository::ThirdParty::CleanupTagsService, :clean_gitlab_redis_cache do + using RSpec::Parameterized::TableSyntax + + include_context 'for a cleanup tags service' + + let_it_be(:user) { create(:user) } + let_it_be(:project, reload: true) { create(:project, :private) } + + let(:repository) { create(:container_repository, :root, project: project) } + let(:service) { described_class.new(container_repository: repository, current_user: user, params: params) } + let(:tags) { %w[latest A Ba Bb C D E] } + + before do + project.add_maintainer(user) if user + + stub_container_registry_config(enabled: true) + + stub_container_registry_tags( + repository: repository.path, + tags: tags + ) + + stub_tag_digest('latest', 'sha256:configA') + stub_tag_digest('A', 'sha256:configA') + stub_tag_digest('Ba', 'sha256:configB') + stub_tag_digest('Bb', 'sha256:configB') + stub_tag_digest('C', 'sha256:configC') + stub_tag_digest('D', 'sha256:configD') + stub_tag_digest('E', nil) + + stub_digest_config('sha256:configA', 1.hour.ago) + stub_digest_config('sha256:configB', 5.days.ago) + stub_digest_config('sha256:configC', 1.month.ago) + stub_digest_config('sha256:configD', nil) + end + + describe '#execute' do + subject { service.execute } + + it_behaves_like 'when regex matching everything is specified', + delete_expectations: [%w[A Ba Bb C D E]], + service_response_extra: { + before_truncate_size: 6, + after_truncate_size: 6, + before_delete_size: 6, + cached_tags_count: 0 + }, + supports_caching: true + + it_behaves_like 'when delete regex matching specific tags is used', + service_response_extra: { + before_truncate_size: 2, + after_truncate_size: 2, + before_delete_size: 2, + cached_tags_count: 0 + }, + supports_caching: true + + it_behaves_like 'when delete regex matching specific tags is used with overriding allow regex', + service_response_extra: { + before_truncate_size: 1, + after_truncate_size: 1, + before_delete_size: 1, + cached_tags_count: 0 + }, + supports_caching: true + + it_behaves_like 'with allow regex value', + delete_expectations: [%w[A C D E]], + service_response_extra: { + before_truncate_size: 4, + after_truncate_size: 4, + before_delete_size: 4, + cached_tags_count: 0 + }, + supports_caching: true + + it_behaves_like 'when keeping only N tags', + delete_expectations: [%w[Bb Ba C]], + service_response_extra: { + before_truncate_size: 4, + after_truncate_size: 4, + before_delete_size: 3, + cached_tags_count: 0 + }, + supports_caching: true + + it_behaves_like 'when not keeping N tags', + delete_expectations: [%w[A Ba Bb C]], + service_response_extra: { + before_truncate_size: 4, + after_truncate_size: 4, + before_delete_size: 4, + cached_tags_count: 0 + }, + supports_caching: true + + it_behaves_like 'when removing keeping only 3', + delete_expectations: [%w[Bb Ba C]], + service_response_extra: { + before_truncate_size: 6, + after_truncate_size: 6, + before_delete_size: 3, + cached_tags_count: 0 + }, + supports_caching: true + + it_behaves_like 'when removing older than 1 day', + delete_expectations: [%w[Ba Bb C]], + service_response_extra: { + before_truncate_size: 6, + after_truncate_size: 6, + before_delete_size: 3, + cached_tags_count: 0 + }, + supports_caching: true + + it_behaves_like 'when combining all parameters', + delete_expectations: [%w[Bb Ba C]], + service_response_extra: { + before_truncate_size: 6, + after_truncate_size: 6, + before_delete_size: 3, + cached_tags_count: 0 + }, + supports_caching: true + + it_behaves_like 'when running a container_expiration_policy', + delete_expectations: [%w[Bb Ba C]], + service_response_extra: { + before_truncate_size: 6, + after_truncate_size: 6, + before_delete_size: 3, + cached_tags_count: 0 + }, + supports_caching: true + + context 'when running a container_expiration_policy with caching' do + let(:user) { nil } + let(:params) do + { + 'name_regex_delete' => '.*', + 'keep_n' => 1, + 'older_than' => '1 day', + 'container_expiration_policy' => true + } + end + + it 'expects caching to be used' do + expect_delete(%w[Bb Ba C], container_expiration_policy: true) + expect_caching + + subject + end + + context 'when setting set to false' do + before do + stub_application_setting(container_registry_expiration_policies_caching: false) + end + + it 'does not use caching' do + expect_delete(%w[Bb Ba C], container_expiration_policy: true) + expect_no_caching + + subject + end + end + end + + context 'when truncating the tags list' do + let(:params) do + { + 'name_regex_delete' => '.*', + 'keep_n' => 1 + } + end + + shared_examples 'returning the response' do + |status:, original_size:, before_truncate_size:, after_truncate_size:, before_delete_size:| + it 'returns the response' do + expect_no_caching + + result = subject + + service_response = expected_service_response( + status: status, + original_size: original_size, + deleted: nil + ).merge( + before_truncate_size: before_truncate_size, + after_truncate_size: after_truncate_size, + before_delete_size: before_delete_size, + cached_tags_count: 0 + ) + + expect(result).to eq(service_response) + end + end + + where(:max_list_size, :delete_tags_service_status, :expected_status, :expected_truncated) do + 10 | :success | :success | false + 10 | :error | :error | false + 3 | :success | :error | true + 3 | :error | :error | true + 0 | :success | :success | false + 0 | :error | :error | false + end + + with_them do + before do + stub_application_setting(container_registry_cleanup_tags_service_max_list_size: max_list_size) + allow_next_instance_of(Projects::ContainerRepository::DeleteTagsService) do |service| + allow(service).to receive(:execute).and_return(status: delete_tags_service_status) + end + end + + original_size = 7 + keep_n = 1 + + it_behaves_like( + 'returning the response', + status: params[:expected_status], + original_size: original_size, + before_truncate_size: original_size - keep_n, + after_truncate_size: params[:expected_truncated] ? params[:max_list_size] + keep_n : original_size - keep_n, + # one tag is filtered out with older_than filter + before_delete_size: params[:expected_truncated] ? params[:max_list_size] : original_size - keep_n - 1 + ) + end + end + + context 'with caching', :freeze_time do + let(:params) do + { + 'name_regex_delete' => '.*', + 'keep_n' => 1, + 'older_than' => '1 day', + 'container_expiration_policy' => true + } + end + + let(:tags_and_created_ats) do + { + 'A' => 1.hour.ago, + 'Ba' => 5.days.ago, + 'Bb' => 5.days.ago, + 'C' => 1.month.ago, + 'D' => nil, + 'E' => nil + } + end + + let(:cacheable_tags) { tags_and_created_ats.reject { |_, value| value.nil? } } + + before do + expect_delete(%w[Bb Ba C], container_expiration_policy: true) + # We froze time so we need to set the created_at stubs again + stub_digest_config('sha256:configA', 1.hour.ago) + stub_digest_config('sha256:configB', 5.days.ago) + stub_digest_config('sha256:configC', 1.month.ago) + end + + it 'caches the created_at values' do + expect_mget(tags_and_created_ats.keys) + expect_set(cacheable_tags) + + expect(subject).to include(cached_tags_count: 0) + end + + context 'with cached values' do + before do + ::Gitlab::Redis::Cache.with do |redis| + redis.set(cache_key('C'), rfc3339(1.month.ago)) + end + end + + it 'uses them' do + expect_mget(tags_and_created_ats.keys) + + # because C is already in cache, it should not be cached again + expect_set(cacheable_tags.except('C')) + + # We will ping the container registry for all tags *except* for C because it's cached + expect(ContainerRegistry::Blob) + .to receive(:new).with(repository, { "digest" => "sha256:configA" }).and_call_original + expect(ContainerRegistry::Blob) + .to receive(:new).with(repository, { "digest" => "sha256:configB" }).twice.and_call_original + expect(ContainerRegistry::Blob).not_to receive(:new).with(repository, { "digest" => "sha256:configC" }) + expect(ContainerRegistry::Blob) + .to receive(:new).with(repository, { "digest" => "sha256:configD" }).and_call_original + + expect(subject).to include(cached_tags_count: 1) + end + end + + def expect_mget(keys) + Gitlab::Redis::Cache.with do |redis| + parameters = keys.map { |k| cache_key(k) } + expect(redis).to receive(:mget).with(parameters).and_call_original + end + end + + def expect_set(tags) + selected_tags = tags.map do |tag_name, created_at| + ex = 1.day.seconds - (Time.zone.now - created_at).seconds + [tag_name, created_at, ex.to_i] if ex.positive? + end.compact + + return if selected_tags.count.zero? + + Gitlab::Redis::Cache.with do |redis| + expect(redis).to receive(:pipelined).and_call_original + + expect_next_instance_of(Redis::PipelinedConnection) do |pipeline| + selected_tags.each do |tag_name, created_at, ex| + expect(pipeline).to receive(:set).with(cache_key(tag_name), rfc3339(created_at), ex: ex) + end + end + end + end + + def cache_key(tag_name) + "container_repository:{#{repository.id}}:tag:#{tag_name}:created_at" + end + + def rfc3339(date_time) + # DateTime rfc3339 is different ActiveSupport::TimeWithZone rfc3339 + # The caching will use DateTime rfc3339 + DateTime.rfc3339(date_time.rfc3339).rfc3339 + end + end + end + + private + + def stub_tag_digest(tag, digest) + allow(repository.client) + .to receive(:repository_tag_digest) + .with(repository.path, tag) { digest } + + allow(repository.client) + .to receive(:repository_manifest) + .with(repository.path, tag) do + { 'config' => { 'digest' => digest } } if digest + end + end + + def stub_digest_config(digest, created_at) + allow(repository.client) + .to receive(:blob) + .with(repository.path, digest, nil) do + { 'created' => created_at.to_datetime.rfc3339 }.to_json if created_at + end + end + + def expect_caching + ::Gitlab::Redis::Cache.with do |redis| + expect(redis).to receive(:mget).and_call_original + expect(redis).to receive(:pipelined).and_call_original + + expect_next_instance_of(Redis::PipelinedConnection) do |pipeline| + expect(pipeline).to receive(:set).and_call_original + end + end + end +end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 8269dbebccb..f7f02769f6a 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -146,20 +146,6 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi expect { destroy_project(project, user, {}) }.to change(MergeRequestDiff, :count).by(-1) expect { another_project_mr.reload }.not_to raise_error end - - context 'when extract_mr_diff_deletions feature flag is disabled' do - before do - stub_feature_flags(extract_mr_diff_deletions: false) - end - - it 'also deletes merge request diffs' do - merge_request_diffs = merge_request.merge_request_diffs - expect(merge_request_diffs.size).to eq(1) - - expect { destroy_project(project, user, {}) }.to change(MergeRequestDiff, :count).by(-1) - expect { another_project_mr.reload }.not_to raise_error - end - end end it_behaves_like 'deleting the project' diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index ab9f99f893d..6dc72948541 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -276,6 +276,15 @@ RSpec.describe Projects::ImportService do expect(result[:status]).to eq :error expect(result[:message]).to include('Only allowed ports are 80, 443') end + + it 'fails with file scheme' do + project.import_url = "file:///tmp/dir.git" + + result = subject.execute + + expect(result[:status]).to eq :error + expect(result[:message]).to include('Only allowed schemes are http, https') + end end it_behaves_like 'measurable service' do diff --git a/spec/services/projects/update_repository_storage_service_spec.rb b/spec/services/projects/update_repository_storage_service_spec.rb index 17d01a57221..ee8f7fb2ef2 100644 --- a/spec/services/projects/update_repository_storage_service_spec.rb +++ b/spec/services/projects/update_repository_storage_service_spec.rb @@ -37,10 +37,6 @@ RSpec.describe Projects::UpdateRepositoryStorageService do context 'when the move succeeds' do it 'moves the repository to the new storage and unmarks the repository as read-only' do - old_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do - project.repository.path_to_repo - end - expect(project_repository_double).to receive(:replicate) .with(project.repository.raw) expect(project_repository_double).to receive(:checksum) @@ -53,7 +49,6 @@ RSpec.describe Projects::UpdateRepositoryStorageService do expect(result).to be_success expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('test_second_storage') - expect(gitlab_shell.repository_exists?('default', old_path)).to be(false) expect(project.project_repository.shard_name).to eq('test_second_storage') end end diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 85d3e99109d..7d8951bf111 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -11,10 +11,27 @@ RSpec.describe Projects::UpdateService do create(:project, creator: user, namespace: user.namespace) end + shared_examples 'publishing Projects::ProjectAttributesChangedEvent' do |params:, attributes:| + it "publishes Projects::ProjectAttributesChangedEvent" do + expect { update_project(project, user, params) } + .to publish_event(Projects::ProjectAttributesChangedEvent) + .with( + project_id: project.id, + namespace_id: project.namespace_id, + root_namespace_id: project.root_namespace.id, + attributes: attributes + ) + end + end + describe '#execute' do let(:admin) { create(:admin) } context 'when changing visibility level' do + it_behaves_like 'publishing Projects::ProjectAttributesChangedEvent', + params: { visibility_level: Gitlab::VisibilityLevel::INTERNAL }, + attributes: %w[updated_at visibility_level] + context 'when visibility_level changes to INTERNAL' do it 'updates the project to internal' do expect(TodosDestroyer::ProjectPrivateWorker).not_to receive(:perform_in) @@ -290,7 +307,7 @@ RSpec.describe Projects::UpdateService do context 'when we update project but not enabling a wiki' do it 'does not try to create an empty wiki' do - TestEnv.rm_storage_dir(project.repository_storage, project.wiki.path) + project.wiki.repository.raw.remove result = update_project(project, user, { name: 'test1' }) @@ -311,7 +328,7 @@ RSpec.describe Projects::UpdateService do context 'when enabling a wiki' do it 'creates a wiki' do project.project_feature.update!(wiki_access_level: ProjectFeature::DISABLED) - TestEnv.rm_storage_dir(project.repository_storage, project.wiki.path) + project.wiki.repository.raw.remove result = update_project(project, user, project_feature_attributes: { wiki_access_level: ProjectFeature::ENABLED }) @@ -323,7 +340,7 @@ RSpec.describe Projects::UpdateService do it 'logs an error and creates a metric when wiki can not be created' do project.project_feature.update!(wiki_access_level: ProjectFeature::DISABLED) - expect_any_instance_of(ProjectWiki).to receive(:wiki).and_raise(Wiki::CouldNotCreateWikiError) + expect_any_instance_of(ProjectWiki).to receive(:create_wiki_repository).and_raise(Wiki::CouldNotCreateWikiError) expect_any_instance_of(described_class).to receive(:log_error).with("Could not create wiki for #{project.full_name}") counter = double(:counter) @@ -348,7 +365,37 @@ RSpec.describe Projects::UpdateService do end end + context 'when changes project features' do + # Using some sample features for testing. + # Not using all the features because some of them must be enabled/disabled together + %w[issues wiki forking].each do |feature_name| + let(:feature) { "#{feature_name}_access_level" } + let(:params) do + { project_feature_attributes: { feature => ProjectFeature::ENABLED } } + end + + before do + project.project_feature.update!(feature => ProjectFeature::DISABLED) + end + + it 'publishes Projects::ProjectFeaturesChangedEvent' do + expect { update_project(project, user, params) } + .to publish_event(Projects::ProjectFeaturesChangedEvent) + .with( + project_id: project.id, + namespace_id: project.namespace_id, + root_namespace_id: project.root_namespace.id, + features: ["updated_at", feature] + ) + end + end + end + context 'when archiving a project' do + it_behaves_like 'publishing Projects::ProjectAttributesChangedEvent', + params: { archived: true }, + attributes: %w[updated_at archived] + it 'publishes a ProjectTransferedEvent' do expect { update_project(project, user, archived: true) } .to publish_event(Projects::ProjectArchivedEvent) diff --git a/spec/services/repositories/changelog_service_spec.rb b/spec/services/repositories/changelog_service_spec.rb index 3615747e191..47ebd55022f 100644 --- a/spec/services/repositories/changelog_service_spec.rb +++ b/spec/services/repositories/changelog_service_spec.rb @@ -67,10 +67,11 @@ RSpec.describe Repositories::ChangelogService do 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 } - ]) + .and_return( + [ + { sha: sha2, merge_request_id: mr1.id }, + { sha: sha3, merge_request_id: mr2.id } + ]) service = described_class .new(project, creator, version: '1.0.0', from: sha1, to: sha3) @@ -135,10 +136,11 @@ RSpec.describe Repositories::ChangelogService do 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 } - ]) + .and_return( + [ + { sha: sha2, merge_request_id: mr1.id }, + { sha: sha3, merge_request_id: mr2.id } + ]) service = described_class .new(project, creator, version: '1.0.0', from: sha1, to: sha3) diff --git a/spec/services/resource_events/merge_into_notes_service_spec.rb b/spec/services/resource_events/merge_into_notes_service_spec.rb index abe00e72f20..ebfd942066f 100644 --- a/spec/services/resource_events/merge_into_notes_service_spec.rb +++ b/spec/services/resource_events/merge_into_notes_service_spec.rb @@ -33,7 +33,7 @@ RSpec.describe ResourceEvents::MergeIntoNotesService do notes = described_class.new(resource, user).execute([note1, note2]) - expected = [note1, event1, note2, event2].map(&:discussion_id) + expected = [note1, event1, note2, event2].map(&:reload).map(&:discussion_id) expect(notes.map(&:discussion_id)).to eq expected end @@ -65,7 +65,7 @@ RSpec.describe ResourceEvents::MergeIntoNotesService do last_fetched_at: 2.days.ago).execute expect(notes.count).to eq 1 - expect(notes.first.discussion_id).to eq event.discussion_id + expect(notes.first.discussion_id).to eq event.reload.discussion_id end it "preloads the note author's status" do diff --git a/spec/services/resource_events/synthetic_milestone_notes_builder_service_spec.rb b/spec/services/resource_events/synthetic_milestone_notes_builder_service_spec.rb index 9c6b6a33b57..f368e107c60 100644 --- a/spec/services/resource_events/synthetic_milestone_notes_builder_service_spec.rb +++ b/spec/services/resource_events/synthetic_milestone_notes_builder_service_spec.rb @@ -19,10 +19,11 @@ RSpec.describe ResourceEvents::SyntheticMilestoneNotesBuilderService do notes = described_class.new(issue, user).execute expect(notes.map(&:created_at)).to eq(events.map(&:created_at)) - expect(notes.map(&:note)).to eq([ - "changed milestone to %#{milestone.iid}", - 'removed milestone' - ]) + expect(notes.map(&:note)).to eq( + [ + "changed milestone to %#{milestone.iid}", + 'removed milestone' + ]) end it_behaves_like 'filters by paginated notes', :resource_milestone_event diff --git a/spec/services/snippets/update_repository_storage_service_spec.rb b/spec/services/snippets/update_repository_storage_service_spec.rb index fdea3615fb1..9874189f73a 100644 --- a/spec/services/snippets/update_repository_storage_service_spec.rb +++ b/spec/services/snippets/update_repository_storage_service_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' RSpec.describe Snippets::UpdateRepositoryStorageService do - include Gitlab::ShellAdapter - subject { described_class.new(repository_storage_move) } describe "#execute" do @@ -32,10 +30,6 @@ RSpec.describe Snippets::UpdateRepositoryStorageService do context 'when the move succeeds' do it 'moves the repository to the new storage and unmarks the repository as read-only' do - old_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do - snippet.repository.path_to_repo - end - expect(snippet_repository_double).to receive(:replicate) .with(snippet.repository.raw) expect(snippet_repository_double).to receive(:checksum) @@ -48,7 +42,6 @@ RSpec.describe Snippets::UpdateRepositoryStorageService do expect(result).to be_success expect(snippet).not_to be_repository_read_only expect(snippet.repository_storage).to eq(destination) - expect(gitlab_shell.repository_exists?('default', old_path)).to be(false) expect(snippet.snippet_repository.shard_name).to eq(destination) end end diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index b32599d4af8..03e1811c8a5 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -388,24 +388,95 @@ RSpec.describe Users::DestroyService do context 'batched nullify' do let(:other_user) { create(:user) } + # rubocop:disable Layout/LineLength + def nullify_in_batches_regexp(table, column, user, batch_size: 100) + %r{^UPDATE "#{table}" SET "#{column}" = NULL WHERE "#{table}"."id" IN \(SELECT "#{table}"."id" FROM "#{table}" WHERE "#{table}"."#{column}" = #{user.id} LIMIT #{batch_size}\)} + end + + def delete_in_batches_regexps(table, column, user, items, batch_size: 1000) + select_query = %r{^SELECT "#{table}".* FROM "#{table}" WHERE "#{table}"."#{column}" = #{user.id}.*ORDER BY "#{table}"."id" ASC LIMIT #{batch_size}} + + [select_query] + items.map { |item| %r{^DELETE FROM "#{table}" WHERE "#{table}"."id" = #{item.id}} } + end + # rubocop:enable Layout/LineLength + it 'nullifies related associations in batches' do expect(other_user).to receive(:nullify_dependent_associations_in_batches).and_call_original described_class.new(user).execute(other_user, skip_authorization: true) end - it 'nullifies last_updated_issues, closed_issues, resource_label_events' do + it 'nullifies issues and resource associations', :aggregate_failures do issue = create(:issue, closed_by: other_user, updated_by: other_user) resource_label_event = create(:resource_label_event, user: other_user) + resource_state_event = create(:resource_state_event, user: other_user) + todos = create_list(:todo, 2, project: issue.project, user: other_user, author: other_user, target: issue) + event = create(:event, project: issue.project, author: other_user) - described_class.new(user).execute(other_user, skip_authorization: true) + query_recorder = ActiveRecord::QueryRecorder.new do + described_class.new(user).execute(other_user, skip_authorization: true) + end issue.reload resource_label_event.reload + resource_state_event.reload expect(issue.closed_by).to be_nil expect(issue.updated_by).to be_nil expect(resource_label_event.user).to be_nil + expect(resource_state_event.user).to be_nil + expect(other_user.authored_todos).to be_empty + expect(other_user.todos).to be_empty + expect(other_user.authored_events).to be_empty + + expected_queries = [ + nullify_in_batches_regexp(:issues, :updated_by_id, other_user), + nullify_in_batches_regexp(:issues, :closed_by_id, other_user), + nullify_in_batches_regexp(:resource_label_events, :user_id, other_user), + nullify_in_batches_regexp(:resource_state_events, :user_id, other_user) + ] + + expected_queries += delete_in_batches_regexps(:todos, :user_id, other_user, todos) + expected_queries += delete_in_batches_regexps(:todos, :author_id, other_user, todos) + expected_queries += delete_in_batches_regexps(:events, :author_id, other_user, [event]) + + expect(query_recorder.log).to include(*expected_queries) + end + + it 'nullifies merge request associations', :aggregate_failures do + merge_request = create(:merge_request, source_project: project, target_project: project, + assignee: other_user, updated_by: other_user, merge_user: other_user) + merge_request.metrics.update!(merged_by: other_user, latest_closed_by: other_user) + merge_request.reviewers = [other_user] + merge_request.assignees = [other_user] + + query_recorder = ActiveRecord::QueryRecorder.new do + described_class.new(user).execute(other_user, skip_authorization: true) + end + + merge_request.reload + + expect(merge_request.updated_by).to be_nil + expect(merge_request.assignee).to be_nil + expect(merge_request.assignee_id).to be_nil + expect(merge_request.metrics.merged_by).to be_nil + expect(merge_request.metrics.latest_closed_by).to be_nil + expect(merge_request.reviewers).to be_empty + expect(merge_request.assignees).to be_empty + + expected_queries = [ + nullify_in_batches_regexp(:merge_requests, :updated_by_id, other_user), + nullify_in_batches_regexp(:merge_requests, :assignee_id, other_user), + nullify_in_batches_regexp(:merge_request_metrics, :merged_by_id, other_user), + nullify_in_batches_regexp(:merge_request_metrics, :latest_closed_by_id, other_user) + ] + + expected_queries += delete_in_batches_regexps(:merge_request_assignees, :user_id, other_user, + merge_request.assignees) + expected_queries += delete_in_batches_regexps(:merge_request_reviewers, :user_id, other_user, + merge_request.reviewers) + + expect(query_recorder.log).to include(*expected_queries) end end end diff --git a/spec/services/users/dismiss_namespace_callout_service_spec.rb b/spec/services/users/dismiss_namespace_callout_service_spec.rb deleted file mode 100644 index fbcdb66c9e8..00000000000 --- a/spec/services/users/dismiss_namespace_callout_service_spec.rb +++ /dev/null @@ -1,24 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Users::DismissNamespaceCalloutService do - describe '#execute' do - let_it_be(:user) { create(:user) } - - let(:params) { { feature_name: feature_name, namespace_id: user.namespace.id } } - let(:feature_name) { Users::NamespaceCallout.feature_names.each_key.first } - - subject(:execute) do - described_class.new( - container: nil, current_user: user, params: params - ).execute - end - - it_behaves_like 'dismissing user callout', Users::NamespaceCallout - - it 'sets the namespace_id' do - expect(execute.namespace_id).to eq(user.namespace.id) - end - end -end diff --git a/spec/services/users/refresh_authorized_projects_service_spec.rb b/spec/services/users/refresh_authorized_projects_service_spec.rb index e6ccb2b16e7..e33886d2add 100644 --- a/spec/services/users/refresh_authorized_projects_service_spec.rb +++ b/spec/services/users/refresh_authorized_projects_service_spec.rb @@ -108,8 +108,8 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do describe '#update_authorizations' do context 'when there are no rows to add and remove' do it 'does not change authorizations' do - expect(user).not_to receive(:remove_project_authorizations) - expect(ProjectAuthorization).not_to receive(:insert_authorizations) + expect(ProjectAuthorization).not_to receive(:delete_all_in_batches_for_user) + expect(ProjectAuthorization).not_to receive(:insert_all_in_batches) service.update_authorizations([], []) end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index fed3ae7a543..551c3dbcc82 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -75,7 +75,8 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state 'Content-Type' => 'application/json', 'User-Agent' => "GitLab/#{Gitlab::VERSION}", 'X-Gitlab-Event' => 'Push Hook', - 'X-Gitlab-Event-UUID' => uuid + 'X-Gitlab-Event-UUID' => uuid, + 'X-Gitlab-Instance' => Gitlab.config.gitlab.base_url } end @@ -164,7 +165,7 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state end end - it 'POSTs the data as JSON' do + it 'POSTs the data as JSON and returns expected headers' do stub_full_request(project_hook.url, method: :post) service_instance.execute @@ -174,6 +175,22 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state ).once end + context 'when webhooks_gitlab_instance_header flag is disabled' do + before do + stub_feature_flags(webhooks_gitlab_instance_header: false) + end + + it 'excludes the X-Gitlab-Instance header' do + stub_full_request(project_hook.url, method: :post) + + service_instance.execute + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).with( + headers: headers.except('X-Gitlab-Instance') + ).once + end + end + context 'when the data is a Gitlab::DataBuilder::Pipeline' do let(:pipeline) { create(:ci_pipeline, project: project) } let(:data) { ::Gitlab::DataBuilder::Pipeline.new(pipeline) } diff --git a/spec/services/web_hooks/log_execution_service_spec.rb b/spec/services/web_hooks/log_execution_service_spec.rb index 1967a8368fb..1b8ff9f2a05 100644 --- a/spec/services/web_hooks/log_execution_service_spec.rb +++ b/spec/services/web_hooks/log_execution_service_spec.rb @@ -41,12 +41,21 @@ RSpec.describe WebHooks::LogExecutionService do service.execute end + it 'does not update the last failure when the feature flag is disabled' do + stub_feature_flags(web_hooks_disable_failed: false) + + expect(project_hook).not_to receive(:update_last_failure) + + service.execute + end + context 'obtaining an exclusive lease' do let(:lease_key) { "web_hooks:update_hook_failure_state:#{project_hook.id}" } it 'updates failure state using a lease that ensures fresh state is written' do service = described_class.new(hook: project_hook, log_data: data, response_category: :error) - WebHook.find(project_hook.id).update!(backoff_count: 1) + # Write state somewhere else, so that the hook is out-of-date + WebHook.find(project_hook.id).update!(recent_failures: 5, disabled_until: 10.minutes.from_now, backoff_count: 1) lease = stub_exclusive_lease(lease_key, timeout: described_class::LOCK_TTL) @@ -69,6 +78,8 @@ RSpec.describe WebHooks::LogExecutionService do subject(:service) { described_class.new(hook: project_hook, log_data: data, response_category: response_category) } before do + # stub LOCK_RETRY to be 0 in order for tests to run quicker + stub_const("#{described_class.name}::LOCK_RETRY", 0) stub_exclusive_lease_taken(lease_key, timeout: described_class::LOCK_TTL) allow(project_hook).to receive(:executable?).and_return(executable) end @@ -146,36 +157,10 @@ RSpec.describe WebHooks::LogExecutionService do data[:response_status] = '500' end - it 'does not increment the failure count' do - expect { service.execute }.not_to change(project_hook, :recent_failures) - end - it 'backs off' do - expect { service.execute }.to change(project_hook, :disabled_until) - end - - it 'increases the backoff count' do - expect { service.execute }.to change(project_hook, :backoff_count).by(1) - end - - context 'when the previous cool-off was near the maximum' do - before do - project_hook.update!(disabled_until: 5.minutes.ago, backoff_count: 8) - end + expect(project_hook).to receive(:backoff!) - it 'sets the disabled_until attribute' do - expect { service.execute }.to change(project_hook, :disabled_until).to(1.day.from_now) - end - end - - context 'when we have backed-off many many times' do - before do - project_hook.update!(disabled_until: 5.minutes.ago, backoff_count: 365) - end - - it 'sets the disabled_until attribute' do - expect { service.execute }.to change(project_hook, :disabled_until).to(1.day.from_now) - end + service.execute end end end diff --git a/spec/services/work_items/update_service_spec.rb b/spec/services/work_items/update_service_spec.rb index e8b82b0b4f2..1761d1104dd 100644 --- a/spec/services/work_items/update_service_spec.rb +++ b/spec/services/work_items/update_service_spec.rb @@ -88,6 +88,26 @@ RSpec.describe WorkItems::UpdateService do end end + context 'when decription is changed' do + let(:opts) { { description: 'description changed' } } + + it 'triggers GraphQL description updated subscription' do + expect(GraphqlTriggers).to receive(:issuable_description_updated).with(work_item).and_call_original + + update_work_item + end + end + + context 'when decription is not changed' do + let(:opts) { { title: 'title changed' } } + + it 'does not trigger GraphQL description updated subscription' do + expect(GraphqlTriggers).not_to receive(:issuable_description_updated) + + update_work_item + end + end + context 'when updating state_event' do context 'when state_event is close' do let(:opts) { { state_event: 'close' } } @@ -292,5 +312,65 @@ RSpec.describe WorkItems::UpdateService do end end end + + describe 'label updates' do + let_it_be(:label1) { create(:label, project: project) } + let_it_be(:label2) { create(:label, project: project) } + + context 'when labels are changed' do + let(:label) { create(:label, project: project) } + let(:opts) { { label_ids: [label1.id] } } + + it 'tracks users updating work item labels' do + expect(Gitlab::UsageDataCounters::WorkItemActivityUniqueCounter).to receive(:track_work_item_labels_changed_action).with(author: current_user) + + update_work_item + end + + it_behaves_like 'broadcasting issuable labels updates' do + let(:issuable) { work_item } + let(:label_a) { label1 } + let(:label_b) { label2 } + + def update_issuable(update_params) + described_class.new( + project: project, + current_user: current_user, + params: update_params, + spam_params: spam_params, + widget_params: widget_params + ).execute(work_item) + end + end + end + + context 'when labels are not changed' do + shared_examples 'work item update that does not track label updates' do + it 'does not track users updating work item labels' do + expect(Gitlab::UsageDataCounters::WorkItemActivityUniqueCounter).not_to receive(:track_work_item_labels_changed_action) + + update_work_item + end + end + + context 'when labels param is not provided' do + let(:opts) { { title: 'not updating labels' } } + + it_behaves_like 'work item update that does not track label updates' + end + + context 'when labels param is provided but labels remain unchanged' do + let(:opts) { { label_ids: [] } } + + it_behaves_like 'work item update that does not track label updates' + end + + context 'when labels param is provided invalid values' do + let(:opts) { { label_ids: [non_existing_record_id] } } + + it_behaves_like 'work item update that does not track label updates' + end + end + end end end |