diff options
| author | GitLab Bot <gitlab-bot@gitlab.com> | 2019-10-22 11:31:16 +0000 |
|---|---|---|
| committer | GitLab Bot <gitlab-bot@gitlab.com> | 2019-10-22 11:31:16 +0000 |
| commit | 905c1110b08f93a19661cf42a276c7ea90d0a0ff (patch) | |
| tree | 756d138db422392c00471ab06acdff92c5a9b69c /spec/services | |
| parent | 50d93f8d1686950fc58dda4823c4835fd0d8c14b (diff) | |
| download | gitlab-ce-905c1110b08f93a19661cf42a276c7ea90d0a0ff.tar.gz | |
Add latest changes from gitlab-org/gitlab@12-4-stable-ee
Diffstat (limited to 'spec/services')
62 files changed, 2671 insertions, 1005 deletions
diff --git a/spec/services/application_settings/update_service_spec.rb b/spec/services/application_settings/update_service_spec.rb index 51fb43907a6..6e1fdb7aad0 100644 --- a/spec/services/application_settings/update_service_spec.rb +++ b/spec/services/application_settings/update_service_spec.rb @@ -147,35 +147,44 @@ describe ApplicationSettings::UpdateService do using RSpec::Parameterized::TableSyntax where(:params_performance_bar_enabled, - :params_performance_bar_allowed_group_path, - :previous_performance_bar_allowed_group_id, - :expected_performance_bar_allowed_group_id) do - true | '' | nil | nil - true | '' | 42_000_000 | nil - true | nil | nil | nil - true | nil | 42_000_000 | nil - true | 'foo' | nil | nil - true | 'foo' | 42_000_000 | nil - true | 'group_a' | nil | 42_000_000 - true | 'group_b' | 42_000_000 | 43_000_000 - true | 'group_a' | 42_000_000 | 42_000_000 - false | '' | nil | nil - false | '' | 42_000_000 | nil - false | nil | nil | nil - false | nil | 42_000_000 | nil - false | 'foo' | nil | nil - false | 'foo' | 42_000_000 | nil - false | 'group_a' | nil | nil - false | 'group_b' | 42_000_000 | nil - false | 'group_a' | 42_000_000 | nil + :params_performance_bar_allowed_group_path, + :previous_performance_bar_allowed_group_id, + :expected_performance_bar_allowed_group_id, + :expected_valid) do + true | '' | nil | nil | true + true | '' | 42_000_000 | nil | true + true | nil | nil | nil | true + true | nil | 42_000_000 | nil | true + true | 'foo' | nil | nil | false + true | 'foo' | 42_000_000 | 42_000_000 | false + true | 'group_a' | nil | 42_000_000 | true + true | 'group_b' | 42_000_000 | 43_000_000 | true + true | 'group_b/' | 42_000_000 | 43_000_000 | true + true | 'group_a' | 42_000_000 | 42_000_000 | true + false | '' | nil | nil | true + false | '' | 42_000_000 | nil | true + false | nil | nil | nil | true + false | nil | 42_000_000 | nil | true + false | 'foo' | nil | nil | true + false | 'foo' | 42_000_000 | nil | true + false | 'group_a' | nil | nil | true + false | 'group_b' | 42_000_000 | nil | true + false | 'group_a' | 42_000_000 | nil | true + nil | '' | nil | nil | true + nil | 'foo' | nil | nil | false + nil | 'group_a' | nil | 42_000_000 | true end with_them do let(:params) do { - performance_bar_enabled: params_performance_bar_enabled, performance_bar_allowed_group_path: params_performance_bar_allowed_group_path - } + }.tap do |params_hash| + # Treat nil in the table as missing + unless params_performance_bar_enabled.nil? + params_hash[:performance_bar_enabled] = params_performance_bar_enabled + end + end end before do @@ -202,6 +211,14 @@ describe ApplicationSettings::UpdateService do .not_to change(application_settings, :performance_bar_allowed_group_id) end end + + it 'adds errors to the model for invalid params' do + expect(subject.execute).to eq(expected_valid) + + unless expected_valid + expect(application_settings.errors[:performance_bar_allowed_group_id]).to be_present + end + end end context 'when :performance_bar_allowed_group_path is not present' do @@ -221,7 +238,7 @@ describe ApplicationSettings::UpdateService do let(:group) { create(:group) } let(:params) { { performance_bar_allowed_group_path: group.full_path } } - it 'implicitely defaults to true' do + it 'implicitly defaults to true' do expect { subject.execute } .to change(application_settings, :performance_bar_allowed_group_id) .from(nil).to(group.id) @@ -295,4 +312,26 @@ describe ApplicationSettings::UpdateService do expect(application_settings.raw_blob_request_limit).to eq(600) end end + + context 'when protected path settings are passed' do + let(:params) do + { + throttle_protected_paths_enabled: 1, + throttle_protected_paths_period_in_seconds: 600, + throttle_protected_paths_requests_per_period: 100, + protected_paths_raw: "/users/password\r\n/users/sign_in\r\n" + } + end + + it 'updates protected path settings' do + subject.execute + + application_settings.reload + + expect(application_settings.throttle_protected_paths_enabled).to be_truthy + expect(application_settings.throttle_protected_paths_period_in_seconds).to eq(600) + expect(application_settings.throttle_protected_paths_requests_per_period).to eq(100) + expect(application_settings.protected_paths).to eq(['/users/password', '/users/sign_in']) + end + end end diff --git a/spec/services/boards/issues/create_service_spec.rb b/spec/services/boards/issues/create_service_spec.rb index 33637419f83..ef7b7fdbaac 100644 --- a/spec/services/boards/issues/create_service_spec.rb +++ b/spec/services/boards/issues/create_service_spec.rb @@ -10,7 +10,7 @@ describe Boards::Issues::CreateService do let(:label) { create(:label, project: project, name: 'in-progress') } let!(:list) { create(:list, board: board, label: label, position: 0) } - subject(:service) { described_class.new(board.parent, project, user, board_id: board.id, list_id: list.id, title: 'New issue') } + subject(:service) { described_class.new(board.resource_parent, project, user, board_id: board.id, list_id: list.id, title: 'New issue') } before do project.add_developer(user) diff --git a/spec/services/boards/lists/update_service_spec.rb b/spec/services/boards/lists/update_service_spec.rb index a5411a2fb3a..243e0fc50ad 100644 --- a/spec/services/boards/lists/update_service_spec.rb +++ b/spec/services/boards/lists/update_service_spec.rb @@ -9,9 +9,9 @@ describe Boards::Lists::UpdateService do shared_examples 'moving list' do context 'when user can admin list' do it 'calls Lists::MoveService to update list position' do - board.parent.add_developer(user) + board.resource_parent.add_developer(user) - expect(Boards::Lists::MoveService).to receive(:new).with(board.parent, user, params).and_call_original + expect(Boards::Lists::MoveService).to receive(:new).with(board.resource_parent, user, params).and_call_original expect_any_instance_of(Boards::Lists::MoveService).to receive(:execute).with(list) service.execute(list) @@ -30,7 +30,7 @@ describe Boards::Lists::UpdateService do shared_examples 'updating list preferences' do context 'when user can read list' do it 'updates list preference for user' do - board.parent.add_guest(user) + board.resource_parent.add_guest(user) service.execute(list) @@ -48,7 +48,7 @@ describe Boards::Lists::UpdateService do end describe '#execute' do - let(:service) { described_class.new(board.parent, user, params) } + let(:service) { described_class.new(board.resource_parent, user, params) } context 'when position parameter is present' do let(:params) { { position: 1 } } diff --git a/spec/services/boards/visits/create_service_spec.rb b/spec/services/boards/visits/create_service_spec.rb index 6baf7ac9deb..203c287f396 100644 --- a/spec/services/boards/visits/create_service_spec.rb +++ b/spec/services/boards/visits/create_service_spec.rb @@ -10,7 +10,7 @@ describe Boards::Visits::CreateService do let(:project) { create(:project) } let(:project_board) { create(:board, project: project) } - subject(:service) { described_class.new(project_board.parent, user) } + subject(:service) { described_class.new(project_board.resource_parent, user) } it 'returns nil when there is no user' do service.current_user = nil @@ -35,7 +35,7 @@ describe Boards::Visits::CreateService do let(:group) { create(:group) } let(:group_board) { create(:board, group: group) } - subject(:service) { described_class.new(group_board.parent, user) } + subject(:service) { described_class.new(group_board.resource_parent, user) } it 'returns nil when there is no user' do service.current_user = nil diff --git a/spec/services/bulk_push_event_payload_service_spec.rb b/spec/services/bulk_push_event_payload_service_spec.rb new file mode 100644 index 00000000000..661c3540aa0 --- /dev/null +++ b/spec/services/bulk_push_event_payload_service_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe BulkPushEventPayloadService do + let(:event) { create(:push_event) } + + let(:push_data) do + { + action: :created, + ref_count: 4, + ref_type: :branch + } + end + + subject { described_class.new(event, push_data) } + + it 'creates a PushEventPayload' do + push_event_payload = subject.execute + + expect(push_event_payload).to be_persisted + expect(push_event_payload.action).to eq(push_data[:action].to_s) + expect(push_event_payload.commit_count).to eq(0) + expect(push_event_payload.ref_count).to eq(push_data[:ref_count]) + expect(push_event_payload.ref_type).to eq(push_data[:ref_type].to_s) + end +end diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb new file mode 100644 index 00000000000..40a3b115cb5 --- /dev/null +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Ci::CreatePipelineService do + context 'rules' do + let(:user) { create(:admin) } + let(:ref) { 'refs/heads/master' } + let(:source) { :push } + let(:service) { described_class.new(project, user, { ref: ref }) } + let(:pipeline) { service.execute(source) } + let(:build_names) { pipeline.builds.pluck(:name) } + + before do + stub_ci_pipeline_yaml_file(config) + allow_any_instance_of(Ci::BuildScheduleWorker).to receive(:perform).and_return(true) + end + + context 'exists:' do + let(:config) do + <<-EOY + regular-job: + script: 'echo Hello, World!' + + rules-job: + script: "echo hello world, $CI_COMMIT_REF_NAME" + rules: + - exists: + - README.md + when: manual + - exists: + - app.rb + when: on_success + + delayed-job: + script: "echo See you later, World!" + rules: + - exists: + - README.md + when: delayed + start_in: 4 hours + EOY + end + let(:regular_job) { pipeline.builds.find_by(name: 'regular-job') } + let(:rules_job) { pipeline.builds.find_by(name: 'rules-job') } + let(:delayed_job) { pipeline.builds.find_by(name: 'delayed-job') } + + context 'with matches' do + let(:project) { create(:project, :custom_repo, files: { 'README.md' => '' }) } + + it 'creates two jobs' do + expect(pipeline).to be_persisted + expect(build_names).to contain_exactly('regular-job', 'rules-job', 'delayed-job') + end + + it 'sets when: for all jobs' do + expect(regular_job.when).to eq('on_success') + expect(rules_job.when).to eq('manual') + expect(delayed_job.when).to eq('delayed') + expect(delayed_job.options[:start_in]).to eq('4 hours') + end + end + + context 'with matches on the second rule' do + let(:project) { create(:project, :custom_repo, files: { 'app.rb' => '' }) } + + it 'includes both jobs' do + expect(pipeline).to be_persisted + expect(build_names).to contain_exactly('regular-job', 'rules-job') + end + + it 'sets when: for the created rules job based on the second clause' do + expect(regular_job.when).to eq('on_success') + expect(rules_job.when).to eq('on_success') + end + end + + context 'without matches' do + let(:project) { create(:project, :custom_repo, files: { 'useless_script.rb' => '' }) } + + it 'only persists the job without rules' do + expect(pipeline).to be_persisted + expect(regular_job).to be_persisted + expect(rules_job).to be_nil + expect(delayed_job).to be_nil + end + + it 'sets when: for the created job' do + expect(regular_job.when).to eq('on_success') + end + end + end + end +end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 1a5fdac1c95..fd5f72c4c46 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -71,6 +71,7 @@ describe Ci::CreatePipelineService do expect(Gitlab::Metrics).to receive(:counter) .with(:pipelines_created_total, "Counter of pipelines created") .and_call_original + allow(Gitlab::Metrics).to receive(:counter).and_call_original # allow other counters pipeline end @@ -735,6 +736,28 @@ describe Ci::CreatePipelineService do end end + context 'when environment with duplicate names' do + let(:ci_yaml) do + { + deploy: { environment: { name: 'production' }, script: 'ls' }, + deploy_2: { environment: { name: 'production' }, script: 'ls' } + } + end + + before do + stub_ci_pipeline_yaml_file(YAML.dump(ci_yaml)) + end + + it 'creates a pipeline with the environment' do + result = execute_service + + expect(result).to be_persisted + expect(Environment.find_by(name: 'production')).to be_present + expect(result.builds.first.deployment).to be_persisted + expect(result.builds.first.deployment.deployable).to be_a(Ci::Build) + end + end + context 'when builds with auto-retries are configured' do let(:pipeline) { execute_service } let(:rspec_job) { pipeline.builds.find_by(name: 'rspec') } diff --git a/spec/services/ci/pipeline_trigger_service_spec.rb b/spec/services/ci/pipeline_trigger_service_spec.rb index 76251b5b557..24d42f402f4 100644 --- a/spec/services/ci/pipeline_trigger_service_spec.rb +++ b/spec/services/ci/pipeline_trigger_service_spec.rb @@ -11,76 +11,158 @@ describe Ci::PipelineTriggerService do describe '#execute' do let(:user) { create(:user) } - let(:trigger) { create(:ci_trigger, project: project, owner: user) } let(:result) { described_class.new(project, user, params).execute } before do project.add_developer(user) end - context 'when trigger belongs to a different project' do - let(:params) { { token: trigger.token, ref: 'master', variables: nil } } - let(:trigger) { create(:ci_trigger, project: create(:project), owner: user) } + context 'with a trigger token' do + let(:trigger) { create(:ci_trigger, project: project, owner: user) } - it 'does nothing' do - expect { result }.not_to change { Ci::Pipeline.count } - end - end - - context 'when params have an existsed trigger token' do - context 'when params have an existsed ref' do + context 'when trigger belongs to a different project' do let(:params) { { token: trigger.token, ref: 'master', variables: nil } } + let(:trigger) { create(:ci_trigger, project: create(:project), owner: user) } - it 'triggers a pipeline' do - expect { result }.to change { Ci::Pipeline.count }.by(1) - expect(result[:pipeline].ref).to eq('master') - expect(result[:pipeline].project).to eq(project) - expect(result[:pipeline].user).to eq(trigger.owner) - expect(result[:pipeline].trigger_requests.to_a) - .to eq(result[:pipeline].builds.map(&:trigger_request).uniq) - expect(result[:status]).to eq(:success) + it 'does nothing' do + expect { result }.not_to change { Ci::Pipeline.count } end + end - context 'when commit message has [ci skip]' do - before do - allow_any_instance_of(Ci::Pipeline).to receive(:git_commit_message) { '[ci skip]' } - end + context 'when params have an existsed trigger token' do + context 'when params have an existsed ref' do + let(:params) { { token: trigger.token, ref: 'master', variables: nil } } - it 'ignores [ci skip] and create as general' do + it 'triggers a pipeline' do expect { result }.to change { Ci::Pipeline.count }.by(1) + expect(result[:pipeline].ref).to eq('master') + expect(result[:pipeline].project).to eq(project) + expect(result[:pipeline].user).to eq(trigger.owner) + expect(result[:pipeline].trigger_requests.to_a) + .to eq(result[:pipeline].builds.map(&:trigger_request).uniq) expect(result[:status]).to eq(:success) end + + context 'when commit message has [ci skip]' do + before do + allow_any_instance_of(Ci::Pipeline).to receive(:git_commit_message) { '[ci skip]' } + end + + it 'ignores [ci skip] and create as general' do + expect { result }.to change { Ci::Pipeline.count }.by(1) + expect(result[:status]).to eq(:success) + end + end + + context 'when params have a variable' do + let(:params) { { token: trigger.token, ref: 'master', variables: variables } } + let(:variables) { { 'AAA' => 'AAA123' } } + + it 'has a variable' do + expect { result }.to change { Ci::PipelineVariable.count }.by(1) + .and change { Ci::TriggerRequest.count }.by(1) + expect(result[:pipeline].variables.map { |v| { v.key => v.value } }.first).to eq(variables) + expect(result[:pipeline].trigger_requests.last.variables).to be_nil + end + end end - context 'when params have a variable' do - let(:params) { { token: trigger.token, ref: 'master', variables: variables } } - let(:variables) { { 'AAA' => 'AAA123' } } + context 'when params have a non-existsed ref' do + let(:params) { { token: trigger.token, ref: 'invalid-ref', variables: nil } } - it 'has a variable' do - expect { result }.to change { Ci::PipelineVariable.count }.by(1) - .and change { Ci::TriggerRequest.count }.by(1) - expect(result[:pipeline].variables.map { |v| { v.key => v.value } }.first).to eq(variables) - expect(result[:pipeline].trigger_requests.last.variables).to be_nil + it 'does not trigger a pipeline' do + expect { result }.not_to change { Ci::Pipeline.count } + expect(result[:http_status]).to eq(400) end end end - context 'when params have a non-existsed ref' do - let(:params) { { token: trigger.token, ref: 'invalid-ref', variables: nil } } + context 'when params have a non-existsed trigger token' do + let(:params) { { token: 'invalid-token', ref: nil, variables: nil } } it 'does not trigger a pipeline' do expect { result }.not_to change { Ci::Pipeline.count } - expect(result[:http_status]).to eq(400) + expect(result).to be_nil end end end - context 'when params have a non-existsed trigger token' do - let(:params) { { token: 'invalid-token', ref: nil, variables: nil } } + context 'with a pipeline job token' do + let!(:pipeline) { create(:ci_empty_pipeline, project: project) } + let(:job) { create(:ci_build, :running, pipeline: pipeline, user: user) } + + context 'when job user does not have a permission to read a project' do + let(:params) { { token: job.token, ref: 'master', variables: nil } } + let(:job) { create(:ci_build, pipeline: pipeline, user: create(:user)) } + + it 'does nothing' do + expect { result }.not_to change { Ci::Pipeline.count } + end + end + + context 'when job is not running' do + let(:params) { { token: job.token, ref: 'master', variables: nil } } + let(:job) { create(:ci_build, :success, pipeline: pipeline, user: user) } + + it 'does nothing' do + expect { result }.not_to change { Ci::Pipeline.count } + expect(result[:message]).to eq('400 Job has to be running') + end + end - it 'does not trigger a pipeline' do - expect { result }.not_to change { Ci::Pipeline.count } - expect(result).to be_nil + context 'when params have an existsed job token' do + context 'when params have an existsed ref' do + let(:params) { { token: job.token, ref: 'master', variables: nil } } + + it 'triggers a pipeline' do + expect { result }.to change { Ci::Pipeline.count }.by(1) + expect(result[:pipeline].ref).to eq('master') + expect(result[:pipeline].project).to eq(project) + expect(result[:pipeline].user).to eq(job.user) + expect(result[:status]).to eq(:success) + end + + context 'when commit message has [ci skip]' do + before do + allow_any_instance_of(Ci::Pipeline).to receive(:git_commit_message) { '[ci skip]' } + end + + it 'ignores [ci skip] and create as general' do + expect { result }.to change { Ci::Pipeline.count }.by(1) + expect(result[:status]).to eq(:success) + end + end + + context 'when params have a variable' do + let(:params) { { token: job.token, ref: 'master', variables: variables } } + let(:variables) { { 'AAA' => 'AAA123' } } + + it 'has a variable' do + expect { result }.to change { Ci::PipelineVariable.count }.by(1) + .and change { Ci::Sources::Pipeline.count }.by(1) + expect(result[:pipeline].variables.map { |v| { v.key => v.value } }.first).to eq(variables) + expect(job.sourced_pipelines.last.pipeline_id).to eq(result[:pipeline].id) + end + end + end + + context 'when params have a non-existsed ref' do + let(:params) { { token: job.token, ref: 'invalid-ref', variables: nil } } + + it 'does not job a pipeline' do + expect { result }.not_to change { Ci::Pipeline.count } + expect(result[:http_status]).to eq(400) + end + end + end + + context 'when params have a non-existsed trigger token' do + let(:params) { { token: 'invalid-token', ref: nil, variables: nil } } + + it 'does not trigger a pipeline' do + expect { result }.not_to change { Ci::Pipeline.count } + expect(result).to be_nil + end end end end diff --git a/spec/services/ci/process_pipeline_service_spec.rb b/spec/services/ci/process_pipeline_service_spec.rb index 1b28d2d4d02..05adec8b745 100644 --- a/spec/services/ci/process_pipeline_service_spec.rb +++ b/spec/services/ci/process_pipeline_service_spec.rb @@ -422,6 +422,18 @@ describe Ci::ProcessPipelineService, '#execute' do end end + context 'when an exception is raised during a persistent ref creation' do + before do + successful_build('test', stage_idx: 0) + + allow_any_instance_of(Ci::PersistentRef).to receive(:delete_refs) { raise ArgumentError } + end + + it 'process the pipeline' do + expect { process_pipeline }.not_to raise_error + end + end + context 'when there are manual action in earlier stages' do context 'when first stage has only optional manual actions' do before do @@ -907,6 +919,10 @@ describe Ci::ProcessPipelineService, '#execute' do create(:ci_build, :created, pipeline: pipeline, name: name, **opts) end + def successful_build(name, **opts) + create(:ci_build, :success, pipeline: pipeline, name: name, **opts) + end + def delayed_options { when: 'delayed', options: { script: %w(echo), start_in: '1 minute' } } end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 874945c8585..2f2c525ccc4 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -501,6 +501,22 @@ module Ci expect(pending_job).to be_archived_failure end end + + context 'when an exception is raised during a persistent ref creation' do + before do + allow_any_instance_of(Ci::PersistentRef).to receive(:exist?) { false } + allow_any_instance_of(Ci::PersistentRef).to receive(:create_ref) { raise ArgumentError } + end + + subject { execute(specific_runner, {}) } + + it 'picks the build' do + expect(subject).to eq(pending_job) + + pending_job.reload + expect(pending_job).to be_running + end + end end describe '#register_success' do diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 281c7438eee..b1368f7776b 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -204,6 +204,16 @@ describe Ci::RetryBuildService do expect(build).to be_retried expect(build.reload).to be_retried end + + context 'when build with deployment is retried' do + let!(:build) do + create(:ci_build, :with_deployment, :deploy_to_production, pipeline: pipeline, stage_id: stage.id) + end + + it 'creates a new deployment' do + expect { new_build }.to change { Deployment.count }.by(1) + end + end end context 'when user does not have ability to execute build' do diff --git a/spec/services/ci/stop_environments_service_spec.rb b/spec/services/ci/stop_environments_service_spec.rb index 890fa5bc009..ed92625a2cc 100644 --- a/spec/services/ci/stop_environments_service_spec.rb +++ b/spec/services/ci/stop_environments_service_spec.rb @@ -121,8 +121,8 @@ describe Ci::StopEnvironmentsService do merge_requests_as_head_pipeline: [merge_request]) end - let!(:review_job) { create(:ci_build, :start_review_app, pipeline: pipeline, project: project) } - let!(:stop_review_job) { create(:ci_build, :stop_review_app, :manual, pipeline: pipeline, project: project) } + let!(:review_job) { create(:ci_build, :with_deployment, :start_review_app, pipeline: pipeline, project: project) } + let!(:stop_review_job) { create(:ci_build, :with_deployment, :stop_review_app, :manual, pipeline: pipeline, project: project) } before do review_job.deployment.success! diff --git a/spec/services/clusters/gcp/finalize_creation_service_spec.rb b/spec/services/clusters/gcp/finalize_creation_service_spec.rb index 5f91acb8e84..43dbea959a2 100644 --- a/spec/services/clusters/gcp/finalize_creation_service_spec.rb +++ b/spec/services/clusters/gcp/finalize_creation_service_spec.rb @@ -107,6 +107,9 @@ describe Clusters::Gcp::FinalizeCreationService, '#execute' do namespace: 'default' } ) + + stub_kubeclient_get_cluster_role_binding_error(api_url, 'gitlab-admin') + stub_kubeclient_create_cluster_role_binding(api_url) end end @@ -133,9 +136,6 @@ describe Clusters::Gcp::FinalizeCreationService, '#execute' do context 'With an RBAC cluster' do before do provider.legacy_abac = false - - stub_kubeclient_get_cluster_role_binding_error(api_url, 'gitlab-admin') - stub_kubeclient_create_cluster_role_binding(api_url) end include_context 'kubernetes information successfully fetched' @@ -152,4 +152,22 @@ describe Clusters::Gcp::FinalizeCreationService, '#execute' do it_behaves_like 'kubernetes information not successfully fetched' end + + context 'With a Cloud Run cluster' do + before do + provider.cloud_run = true + end + + include_context 'kubernetes information successfully fetched' + + it_behaves_like 'success' + + it 'has knative pre-installed' do + subject + cluster.reload + + expect(cluster.application_knative).to be_present + expect(cluster.application_knative).to be_pre_installed + end + end end diff --git a/spec/services/create_snippet_service_spec.rb b/spec/services/create_snippet_service_spec.rb index 7d2491b3a49..1751029a78c 100644 --- a/spec/services/create_snippet_service_spec.rb +++ b/spec/services/create_snippet_service_spec.rb @@ -3,26 +3,28 @@ require 'spec_helper' describe CreateSnippetService do - before do - @user = create :user - @admin = create :user, admin: true - @opts = { + let(:user) { create(:user) } + let(:admin) { create(:user, :admin) } + let(:opts) { base_opts.merge(extra_opts) } + let(:base_opts) do + { title: 'Test snippet', file_name: 'snippet.rb', content: 'puts "hello world"', visibility_level: Gitlab::VisibilityLevel::PRIVATE } end + let(:extra_opts) { {} } context 'When public visibility is restricted' do + let(:extra_opts) { { visibility_level: Gitlab::VisibilityLevel::PUBLIC } } + before do stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) - - @opts.merge!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) end it 'non-admins are not able to create a public snippet' do - snippet = create_snippet(nil, @user, @opts) + snippet = create_snippet(nil, user, opts) expect(snippet.errors.messages).to have_key(:visibility_level) expect(snippet.errors.messages[:visibility_level].first).to( match('has been restricted') @@ -30,37 +32,81 @@ describe CreateSnippetService do end it 'admins are able to create a public snippet' do - snippet = create_snippet(nil, @admin, @opts) + snippet = create_snippet(nil, admin, opts) expect(snippet.errors.any?).to be_falsey expect(snippet.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC) end describe "when visibility level is passed as a string" do + let(:extra_opts) { { visibility: 'internal' } } + before do - @opts[:visibility] = 'internal' - @opts.delete(:visibility_level) + base_opts.delete(:visibility_level) end it "assigns the correct visibility level" do - snippet = create_snippet(nil, @user, @opts) + snippet = create_snippet(nil, user, opts) expect(snippet.errors.any?).to be_falsey expect(snippet.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL) end end end + context 'checking spam' do + shared_examples 'marked as spam' do + let(:snippet) { create_snippet(nil, admin, opts) } + + it 'marks a snippet as a spam ' do + expect(snippet).to be_spam + end + + it 'invalidates the snippet' do + expect(snippet).to be_invalid + end + + it 'creates a new spam_log' do + expect { snippet } + .to log_spam(title: snippet.title, noteable_type: 'PersonalSnippet') + end + + it 'assigns a spam_log to an issue' do + expect(snippet.spam_log).to eq(SpamLog.last) + end + end + + let(:extra_opts) do + { visibility_level: Gitlab::VisibilityLevel::PUBLIC, request: double(:request, env: {}) } + end + + before do + expect_next_instance_of(AkismetService) do |akismet_service| + expect(akismet_service).to receive_messages(spam?: true) + end + end + + [true, false, nil].each do |allow_possible_spam| + context "when recaptcha_disabled flag is #{allow_possible_spam.inspect}" do + before do + stub_feature_flags(allow_possible_spam: allow_possible_spam) unless allow_possible_spam.nil? + end + + it_behaves_like 'marked as spam' + end + end + end + describe 'usage counter' do let(:counter) { Gitlab::UsageDataCounters::SnippetCounter } it 'increments count' do expect do - create_snippet(nil, @admin, @opts) + create_snippet(nil, admin, opts) end.to change { counter.read(:create) }.by 1 end it 'does not increment count if create fails' do expect do - create_snippet(nil, @admin, {}) + create_snippet(nil, admin, {}) end.not_to change { counter.read(:create) } end end diff --git a/spec/services/update_deployment_service_spec.rb b/spec/services/deployments/after_create_service_spec.rb index 7dc52f6816a..b34483ea85b 100644 --- a/spec/services/update_deployment_service_spec.rb +++ b/spec/services/deployments/after_create_service_spec.rb @@ -2,13 +2,14 @@ require 'spec_helper' -describe UpdateDeploymentService do +describe Deployments::AfterCreateService do let(:user) { create(:user) } let(:project) { create(:project, :repository) } let(:options) { { name: 'production' } } let(:job) do create(:ci_build, + :with_deployment, ref: 'master', tag: false, environment: 'production', @@ -114,6 +115,7 @@ describe UpdateDeploymentService do context 'when yaml environment uses $CI_COMMIT_REF_NAME' do let(:job) do create(:ci_build, + :with_deployment, ref: 'master', environment: 'production', project: project, @@ -126,6 +128,7 @@ describe UpdateDeploymentService do context 'when yaml environment uses $CI_ENVIRONMENT_SLUG' do let(:job) do create(:ci_build, + :with_deployment, ref: 'master', environment: 'prod-slug', project: project, @@ -138,6 +141,7 @@ describe UpdateDeploymentService do context 'when yaml environment uses yaml_variables containing symbol keys' do let(:job) do create(:ci_build, + :with_deployment, yaml_variables: [{ key: :APP_HOST, value: 'host' }], environment: 'production', project: project, @@ -148,7 +152,7 @@ describe UpdateDeploymentService do end context 'when yaml environment does not have url' do - let(:job) { create(:ci_build, environment: 'staging', project: project) } + let(:job) { create(:ci_build, :with_deployment, environment: 'staging', project: project) } it 'returns the external_url from persisted environment' do is_expected.to be_nil @@ -174,6 +178,7 @@ describe UpdateDeploymentService do context 'when job deploys to staging' do let(:job) do create(:ci_build, + :with_deployment, ref: 'master', tag: false, environment: 'staging', diff --git a/spec/services/deployments/create_service_spec.rb b/spec/services/deployments/create_service_spec.rb new file mode 100644 index 00000000000..e41c8259ea9 --- /dev/null +++ b/spec/services/deployments/create_service_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Deployments::CreateService do + let(:environment) do + double( + :environment, + deployment_platform: double(:platform, cluster_id: 1), + project_id: 2, + id: 3 + ) + end + + let(:user) { double(:user) } + + describe '#execute' do + let(:service) { described_class.new(environment, user, {}) } + + it 'does not run the AfterCreateService service if the deployment is not persisted' do + deploy = double(:deployment, persisted?: false) + + expect(service) + .to receive(:create_deployment) + .and_return(deploy) + + expect(Deployments::AfterCreateService) + .not_to receive(:new) + + expect(service.execute).to eq(deploy) + end + + it 'runs the AfterCreateService service if the deployment is persisted' do + deploy = double(:deployment, persisted?: true) + after_service = double(:after_create_service) + + expect(service) + .to receive(:create_deployment) + .and_return(deploy) + + expect(Deployments::AfterCreateService) + .to receive(:new) + .with(deploy) + .and_return(after_service) + + expect(after_service) + .to receive(:execute) + + expect(service.execute).to eq(deploy) + end + end + + describe '#create_deployment' do + it 'creates a deployment' do + environment = build(:environment) + service = described_class.new(environment, user, {}) + + expect(environment.deployments) + .to receive(:create) + .with(an_instance_of(Hash)) + + service.create_deployment + end + end + + describe '#deployment_attributes' do + it 'only includes attributes that we want to persist' do + service = described_class.new( + environment, + user, + ref: 'master', + tag: true, + sha: '123', + foo: 'bar', + on_stop: 'stop', + status: 'running' + ) + + expect(service.deployment_attributes).to eq( + cluster_id: 1, + project_id: 2, + environment_id: 3, + ref: 'master', + tag: true, + sha: '123', + user: user, + on_stop: 'stop', + status: 'running' + ) + end + end +end diff --git a/spec/services/deployments/update_service_spec.rb b/spec/services/deployments/update_service_spec.rb new file mode 100644 index 00000000000..a923099b82c --- /dev/null +++ b/spec/services/deployments/update_service_spec.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Deployments::UpdateService do + let(:deploy) { create(:deployment, :running) } + let(:service) { described_class.new(deploy, status: 'success') } + + describe '#execute' do + it 'updates the status of a deployment' do + expect(service.execute).to eq(true) + expect(deploy.status).to eq('success') + end + end +end diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index 9f2c3fec62c..eb738ac80b1 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -113,40 +113,21 @@ describe EventCreateService do end end - describe '#push', :clean_gitlab_redis_shared_state do - let(:project) { create(:project) } - let(:user) { create(:user) } - - let(:push_data) do - { - commits: [ - { - id: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2', - message: 'This is a commit' - } - ], - before: '0000000000000000000000000000000000000000', - after: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2', - total_commits_count: 1, - ref: 'refs/heads/my-branch' - } - end - + shared_examples_for 'service for creating a push event' do |service_class| it 'creates a new event' do - expect { service.push(project, user, push_data) }.to change { Event.count } + expect { subject }.to change { Event.count } end it 'creates the push event payload' do - expect(PushEventPayloadService).to receive(:new) + expect(service_class).to receive(:new) .with(an_instance_of(PushEvent), push_data) .and_call_original - service.push(project, user, push_data) + subject end it 'updates user last activity' do - expect { service.push(project, user, push_data) } - .to change { user.last_activity_on }.to(Date.today) + expect { subject }.to change { user.last_activity_on }.to(Date.today) end it 'caches the last push event for the user' do @@ -154,7 +135,7 @@ describe EventCreateService do .to receive(:cache_last_push_event) .with(an_instance_of(PushEvent)) - service.push(project, user, push_data) + subject end it 'does not create any event data when an error is raised' do @@ -163,17 +144,56 @@ describe EventCreateService do allow(payload_service).to receive(:execute) .and_raise(RuntimeError) - allow(PushEventPayloadService).to receive(:new) + allow(service_class).to receive(:new) .and_return(payload_service) - expect { service.push(project, user, push_data) } - .to raise_error(RuntimeError) - + expect { subject }.to raise_error(RuntimeError) expect(Event.count).to eq(0) expect(PushEventPayload.count).to eq(0) end end + describe '#push', :clean_gitlab_redis_shared_state do + let(:project) { create(:project) } + let(:user) { create(:user) } + + let(:push_data) do + { + commits: [ + { + id: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2', + message: 'This is a commit' + } + ], + before: '0000000000000000000000000000000000000000', + after: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2', + total_commits_count: 1, + ref: 'refs/heads/my-branch' + } + end + + subject { service.push(project, user, push_data) } + + it_behaves_like 'service for creating a push event', PushEventPayloadService + end + + describe '#bulk_push', :clean_gitlab_redis_shared_state do + let(:project) { create(:project) } + let(:user) { create(:user) } + + let(:push_data) do + { + action: :created, + ref_count: 4, + ref_type: :branch + } + end + + subject { service.bulk_push(project, user, push_data) } + + it_behaves_like 'service for creating a push event', BulkPushEventPayloadService + end + describe 'Project' do let(:user) { create :user } let(:project) { create(:project) } diff --git a/spec/services/git/base_hooks_service_spec.rb b/spec/services/git/base_hooks_service_spec.rb index 874df9a68cd..f3f6b36a18d 100644 --- a/spec/services/git/base_hooks_service_spec.rb +++ b/spec/services/git/base_hooks_service_spec.rb @@ -8,14 +8,12 @@ describe Git::BaseHooksService do let(:user) { create(:user) } let(:project) { create(:project, :repository) } - let(:service) { described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) } - let(:oldrev) { Gitlab::Git::BLANK_SHA } let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0 let(:ref) { 'refs/tags/v1.1.0' } - describe '#execute_project_hooks' do - class TestService < described_class + let(:test_service) do + Class.new(described_class) do def hook_name :push_hooks end @@ -24,12 +22,44 @@ describe Git::BaseHooksService do [] end end + end - let(:project) { create(:project, :repository) } + subject { test_service.new(project, user, params) } + + let(:params) do + { + change: { + oldrev: oldrev, + newrev: newrev, + ref: ref + } + } + end - subject { TestService.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) } + describe 'push event' do + it 'creates push event' do + expect_next_instance_of(EventCreateService) do |service| + expect(service).to receive(:push) + end + + subject.execute + end - context '#execute_hooks' do + context 'create_push_event is set to false' do + before do + params[:create_push_event] = false + end + + it 'does not create push event' do + expect(EventCreateService).not_to receive(:new) + + subject.execute + end + end + end + + describe 'project hooks and services' do + context 'hooks' do before do expect(project).to receive(:has_active_hooks?).and_return(active) end @@ -57,7 +87,7 @@ describe Git::BaseHooksService do end end - context '#execute_services' do + context 'services' do before do expect(project).to receive(:has_active_services?).and_return(active) end @@ -84,78 +114,20 @@ describe Git::BaseHooksService do end end end - end - - describe 'with remote mirrors' do - class TestService < described_class - def commits - [] - end - end - - let(:project) { create(:project, :repository, :remote_mirror) } - - subject { TestService.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) } - - before do - expect(subject).to receive(:execute_project_hooks) - end - - context 'when remote mirror feature is enabled' do - it 'fails stuck remote mirrors' do - allow(project).to receive(:update_remote_mirrors).and_return(project.remote_mirrors) - expect(project).to receive(:mark_stuck_remote_mirrors_as_failed!) - - subject.execute - end - - it 'updates remote mirrors' do - expect(project).to receive(:update_remote_mirrors) - - subject.execute - end - end - context 'when remote mirror feature is disabled' do + context 'execute_project_hooks param set to false' do before do - stub_application_setting(mirror_available: false) - end - - context 'with remote mirrors global setting overridden' do - before do - project.remote_mirror_available_overridden = true - end + params[:execute_project_hooks] = false - it 'fails stuck remote mirrors' do - allow(project).to receive(:update_remote_mirrors).and_return(project.remote_mirrors) - expect(project).to receive(:mark_stuck_remote_mirrors_as_failed!) - - subject.execute - end - - it 'updates remote mirrors' do - expect(project).to receive(:update_remote_mirrors) - - subject.execute - end + allow(project).to receive(:has_active_hooks?).and_return(true) + allow(project).to receive(:has_active_services?).and_return(true) end - context 'without remote mirrors global setting overridden' do - before do - project.remote_mirror_available_overridden = false - end - - it 'does not fails stuck remote mirrors' do - expect(project).not_to receive(:mark_stuck_remote_mirrors_as_failed!) + it 'does not execute hooks and services' do + expect(project).not_to receive(:execute_hooks) + expect(project).not_to receive(:execute_services) - subject.execute - end - - it 'does not updates remote mirrors' do - expect(project).not_to receive(:update_remote_mirrors) - - subject.execute - end + subject.execute end end end diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb index 2bf7dc32436..085b49f31ab 100644 --- a/spec/services/git/branch_hooks_service_spec.rb +++ b/spec/services/git/branch_hooks_service_spec.rb @@ -16,13 +16,7 @@ describe Git::BranchHooksService do let(:newrev) { commit.id } let(:service) do - described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) - end - - it 'update remote mirrors' do - expect(service).to receive(:update_remote_mirrors).and_call_original - - service.execute + described_class.new(project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref }) end describe "Git Push Data" do @@ -356,7 +350,7 @@ describe Git::BranchHooksService do let(:forked_project) { fork_project(upstream_project, user, repository: true) } let!(:forked_service) do - described_class.new(forked_project, user, oldrev: oldrev, newrev: newrev, ref: ref) + described_class.new(forked_project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref }) end context 'when commits already exists in the upstream project' do diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index f4d1a1e34cd..bf68eb0af20 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -19,7 +19,7 @@ describe Git::BranchPushService, services: true do describe 'Push branches' do subject do - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end context 'new branch' do @@ -70,7 +70,7 @@ describe Git::BranchPushService, services: true do end describe "Pipelines" do - subject { execute_service(project, user, oldrev, newrev, ref) } + subject { execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) } before do stub_ci_pipeline_to_return_yaml_file @@ -121,7 +121,7 @@ describe Git::BranchPushService, services: true do .to receive(:perform_async) .with(project.id, user.id, blankrev, 'newrev', ref) - execute_service(project, user, blankrev, 'newrev', ref ) + execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref) end end @@ -130,13 +130,13 @@ describe Git::BranchPushService, services: true do it "calls the copy attributes method for the first push to the default branch" do expect(project.repository).to receive(:copy_gitattributes).with('master') - execute_service(project, user, blankrev, 'newrev', ref) + execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref) end it "calls the copy attributes method for changes to the default branch" do expect(project.repository).to receive(:copy_gitattributes).with(ref) - execute_service(project, user, 'oldrev', 'newrev', ref) + execute_service(project, user, oldrev: 'oldrev', newrev: 'newrev', ref: ref) end end @@ -149,7 +149,7 @@ describe Git::BranchPushService, services: true do it "does not call copy attributes method" do expect(project.repository).not_to receive(:copy_gitattributes) - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end end end @@ -163,7 +163,7 @@ describe Git::BranchPushService, services: true do it "when pushing a branch for the first time" do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - execute_service(project, user, blankrev, 'newrev', ref) + execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref) expect(project.protected_branches).not_to be_empty expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER]) expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER]) @@ -174,7 +174,7 @@ describe Git::BranchPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - execute_service(project, user, blankrev, 'newrev', ref) + execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref) expect(project.protected_branches).to be_empty end @@ -184,7 +184,7 @@ describe Git::BranchPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - execute_service(project, user, blankrev, 'newrev', ref) + execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref) expect(project.protected_branches).not_to be_empty expect(project.protected_branches.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER]) @@ -199,7 +199,7 @@ describe Git::BranchPushService, services: true do expect(project.default_branch).to eq("master") expect(ProtectedBranches::CreateService).not_to receive(:new) - execute_service(project, user, blankrev, 'newrev', ref) + execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref) expect(project.protected_branches).not_to be_empty expect(project.protected_branches.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::NO_ACCESS]) @@ -211,7 +211,7 @@ describe Git::BranchPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - execute_service(project, user, blankrev, 'newrev', ref) + execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref) expect(project.protected_branches).not_to be_empty expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER]) expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER]) @@ -219,7 +219,7 @@ describe Git::BranchPushService, services: true do it "when pushing new commits to existing branch" do expect(project).to receive(:execute_hooks) - execute_service(project, user, 'oldrev', 'newrev', ref) + execute_service(project, user, oldrev: 'oldrev', newrev: 'newrev', ref: ref) end end end @@ -249,7 +249,7 @@ describe Git::BranchPushService, services: true do it "creates a note if a pushed commit mentions an issue" do expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end it "only creates a cross-reference note if one doesn't already exist" do @@ -257,7 +257,7 @@ describe Git::BranchPushService, services: true do expect(SystemNoteService).not_to receive(:cross_reference).with(issue, commit, commit_author) - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end it "defaults to the pushing user if the commit's author is not known" do @@ -267,7 +267,7 @@ describe Git::BranchPushService, services: true do ) expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, user) - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end it "finds references in the first push to a non-default branch" do @@ -276,7 +276,7 @@ describe Git::BranchPushService, services: true do expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) - execute_service(project, user, blankrev, newrev, 'refs/heads/other') + execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: 'refs/heads/other') end end @@ -306,14 +306,14 @@ describe Git::BranchPushService, services: true do context "while saving the 'first_mentioned_in_commit_at' metric for an issue" do it 'sets the metric for referenced issues' do - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) expect(issue.reload.metrics.first_mentioned_in_commit_at).to be_like_time(commit_time) end it 'does not set the metric for non-referenced issues' do non_referenced_issue = create(:issue, project: project) - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) expect(non_referenced_issue.reload.metrics.first_mentioned_in_commit_at).to be_nil end @@ -345,18 +345,18 @@ describe Git::BranchPushService, services: true do context "to default branches" do it "closes issues" do - execute_service(project, commit_author, oldrev, newrev, ref) + execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) expect(Issue.find(issue.id)).to be_closed end it "adds a note indicating that the issue is now closed" do expect(SystemNoteService).to receive(:change_status).with(issue, project, commit_author, "closed", closing_commit) - execute_service(project, commit_author, oldrev, newrev, ref) + execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) end it "doesn't create additional cross-reference notes" do expect(SystemNoteService).not_to receive(:cross_reference) - execute_service(project, commit_author, oldrev, newrev, ref) + execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) end end @@ -368,11 +368,11 @@ describe Git::BranchPushService, services: true do it "creates cross-reference notes" do expect(SystemNoteService).to receive(:cross_reference).with(issue, closing_commit, commit_author) - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end it "doesn't close issues" do - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) expect(Issue.find(issue.id)).to be_opened end end @@ -408,7 +408,7 @@ describe Git::BranchPushService, services: true do let(:message) { "this is some work.\n\nrelated to JIRA-1" } it "initiates one api call to jira server to mention the issue" do - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) expect(WebMock).to have_requested(:post, jira_api_comment_url('JIRA-1')).with( body: /mentioned this issue in/ @@ -436,13 +436,13 @@ describe Git::BranchPushService, services: true do context "using right markdown" do it "initiates one api call to jira server to close the issue" do - execute_service(project, commit_author, oldrev, newrev, ref) + execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) expect(WebMock).to have_requested(:post, jira_api_transition_url('JIRA-1')).once end it "initiates one api call to jira server to comment on the issue" do - execute_service(project, commit_author, oldrev, newrev, ref) + execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) expect(WebMock).to have_requested(:post, jira_api_comment_url('JIRA-1')).with( body: comment_body @@ -459,13 +459,13 @@ describe Git::BranchPushService, services: true do let(:message) { "this is some work.\n\ncloses #1" } it "does not initiates one api call to jira server to close the issue" do - execute_service(project, commit_author, oldrev, newrev, ref) + execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) expect(WebMock).not_to have_requested(:post, jira_api_transition_url('JIRA-1')) end it "does not initiates one api call to jira server to comment on the issue" do - execute_service(project, commit_author, oldrev, newrev, ref) + execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) expect(WebMock).not_to have_requested(:post, jira_api_comment_url('JIRA-1')).with( body: comment_body @@ -478,13 +478,13 @@ describe Git::BranchPushService, services: true do let(:message) { "this is some work.\n\ncloses JIRA-1 \n\n closes #{issue.to_reference}" } it "initiates one api call to jira server to close the jira issue" do - execute_service(project, commit_author, oldrev, newrev, ref) + execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) expect(WebMock).to have_requested(:post, jira_api_transition_url('JIRA-1')).once end it "initiates one api call to jira server to comment on the jira issue" do - execute_service(project, commit_author, oldrev, newrev, ref) + execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) expect(WebMock).to have_requested(:post, jira_api_comment_url('JIRA-1')).with( body: comment_body @@ -492,14 +492,14 @@ describe Git::BranchPushService, services: true do end it "closes the internal issue" do - execute_service(project, commit_author, oldrev, newrev, ref) + execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) expect(issue.reload).to be_closed end it "adds a note indicating that the issue is now closed" do expect(SystemNoteService).to receive(:change_status) .with(issue, project, commit_author, "closed", closing_commit) - execute_service(project, commit_author, oldrev, newrev, ref) + execute_service(project, commit_author, oldrev: oldrev, newrev: newrev, ref: ref) end end end @@ -517,7 +517,7 @@ describe Git::BranchPushService, services: true do end it 'push to first branch updates HEAD' do - execute_service(project, user, blankrev, newrev, new_ref) + execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: new_ref) end end @@ -542,7 +542,7 @@ describe Git::BranchPushService, services: true do it 'does not perform housekeeping when not needed' do expect(housekeeping).not_to receive(:execute) - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end context 'when housekeeping is needed' do @@ -553,20 +553,20 @@ describe Git::BranchPushService, services: true do it 'performs housekeeping' do expect(housekeeping).to receive(:execute) - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end it 'does not raise an exception' do allow(housekeeping).to receive(:try_obtain_lease).and_return(false) - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end end it 'increments the push counter' do expect(housekeeping).to receive(:increment!) - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end end @@ -577,7 +577,7 @@ describe Git::BranchPushService, services: true do it 'does nothing' do expect(::Ci::StopEnvironmentsService).not_to receive(:new) - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end end @@ -585,7 +585,7 @@ describe Git::BranchPushService, services: true do it 'does nothing' do expect(::Ci::StopEnvironmentsService).not_to receive(:new) - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end end @@ -599,7 +599,7 @@ describe Git::BranchPushService, services: true do expect(stop_service).to receive(:execute).with(branch) end - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end end end @@ -611,15 +611,17 @@ describe Git::BranchPushService, services: true do expect(hooks_service.project).to eq(project) expect(hooks_service.current_user).to eq(user) expect(hooks_service.params).to include( - oldrev: oldrev, - newrev: newrev, - ref: ref + change: { + oldrev: oldrev, + newrev: newrev, + ref: ref + } ) expect(hooks_service).to receive(:execute) end - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end end @@ -629,13 +631,13 @@ describe Git::BranchPushService, services: true do it 'does nothing' do expect(::Git::BranchHooksService).not_to receive(:new) - execute_service(project, user, oldrev, newrev, ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end end end - def execute_service(project, user, oldrev, newrev, ref) - service = described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + def execute_service(project, user, change) + service = described_class.new(project, user, change: change) service.execute service end diff --git a/spec/services/git/process_ref_changes_service_spec.rb b/spec/services/git/process_ref_changes_service_spec.rb new file mode 100644 index 00000000000..35ddf95b5f6 --- /dev/null +++ b/spec/services/git/process_ref_changes_service_spec.rb @@ -0,0 +1,193 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Git::ProcessRefChangesService do + let(:project) { create(:project, :repository) } + let(:user) { project.owner } + let(:params) { { changes: git_changes } } + + subject { described_class.new(project, user, params) } + + shared_examples_for 'service for processing ref changes' do |push_service_class| + let(:service) { double(execute: true) } + let(:git_changes) { double(branch_changes: [], tag_changes: []) } + + def multiple_changes(change, count) + Array.new(count).map.with_index do |n, index| + { index: index, oldrev: change[:oldrev], newrev: change[:newrev], ref: "#{change[:ref]}#{n}" } + end + end + + let(:changes) do + [ + { index: 0, oldrev: Gitlab::Git::BLANK_SHA, newrev: '789012', ref: "#{ref_prefix}/create" }, + { index: 1, oldrev: '123456', newrev: '789012', ref: "#{ref_prefix}/update" }, + { index: 2, oldrev: '123456', newrev: Gitlab::Git::BLANK_SHA, ref: "#{ref_prefix}/delete" } + ] + end + + before do + allow(git_changes).to receive(changes_method).and_return(changes) + end + + it "calls #{push_service_class}" do + expect(push_service_class) + .to receive(:new) + .with(project, project.owner, hash_including(execute_project_hooks: true, create_push_event: true)) + .exactly(changes.count).times + .and_return(service) + + subject.execute + end + + context 'changes exceed push_event_hooks_limit' do + let(:push_event_hooks_limit) { 3 } + + let(:changes) do + multiple_changes( + { oldrev: '123456', newrev: '789012', ref: "#{ref_prefix}/test" }, + push_event_hooks_limit + 1 + ) + end + + before do + stub_application_setting(push_event_hooks_limit: push_event_hooks_limit) + end + + context 'git_push_execute_all_project_hooks is disabled' do + before do + stub_feature_flags(git_push_execute_all_project_hooks: false) + end + + it "calls #{push_service_class} with execute_project_hooks set to false" do + expect(push_service_class) + .to receive(:new) + .with(project, project.owner, hash_including(execute_project_hooks: false)) + .exactly(changes.count).times + .and_return(service) + + subject.execute + end + end + + context 'git_push_execute_all_project_hooks is enabled' do + before do + stub_feature_flags(git_push_execute_all_project_hooks: true) + end + + it "calls #{push_service_class} with execute_project_hooks set to true" do + expect(push_service_class) + .to receive(:new) + .with(project, project.owner, hash_including(execute_project_hooks: true)) + .exactly(changes.count).times + .and_return(service) + + subject.execute + end + end + end + + context 'changes exceed push_event_activities_limit per action' do + let(:push_event_activities_limit) { 3 } + + let(:changes) do + [ + { oldrev: Gitlab::Git::BLANK_SHA, newrev: '789012', ref: "#{ref_prefix}/create" }, + { oldrev: '123456', newrev: '789012', ref: "#{ref_prefix}/update" }, + { oldrev: '123456', newrev: Gitlab::Git::BLANK_SHA, ref: "#{ref_prefix}/delete" } + ].map do |change| + multiple_changes(change, push_event_activities_limit + 1) + end.flatten + end + + before do + stub_application_setting(push_event_activities_limit: push_event_activities_limit) + end + + it "calls #{push_service_class} with create_push_event set to false" do + expect(push_service_class) + .to receive(:new) + .with(project, project.owner, hash_including(create_push_event: false)) + .exactly(changes.count).times + .and_return(service) + + subject.execute + end + + it 'creates events per action' do + allow(push_service_class).to receive(:new).and_return(service) + + expect { subject.execute }.to change { Event.count }.by(3) + end + end + + context 'pipeline creation' do + context 'with valid .gitlab-ci.yml' do + before do + stub_ci_pipeline_to_return_yaml_file + + allow_any_instance_of(Project) + .to receive(:commit) + .and_return(project.commit) + + allow_any_instance_of(Repository) + .to receive(:branch_exists?) + .and_return(true) + end + + context 'when git_push_create_all_pipelines is disabled' do + before do + stub_feature_flags(git_push_create_all_pipelines: false) + end + + it 'creates pipeline for branches and tags' do + subject.execute + + expect(Ci::Pipeline.pluck(:ref)).to contain_exactly('create', 'update', 'delete') + end + + it "creates exactly #{described_class::PIPELINE_PROCESS_LIMIT} pipelines" do + stub_const("#{described_class}::PIPELINE_PROCESS_LIMIT", changes.count - 1) + + expect { subject.execute }.to change { Ci::Pipeline.count }.by(described_class::PIPELINE_PROCESS_LIMIT) + end + end + + context 'when git_push_create_all_pipelines is enabled' do + before do + stub_feature_flags(git_push_create_all_pipelines: true) + end + + it 'creates all pipelines' do + expect { subject.execute }.to change { Ci::Pipeline.count }.by(changes.count) + end + end + end + + context 'with invalid .gitlab-ci.yml' do + before do + stub_ci_pipeline_yaml_file(nil) + end + + it 'does not create a pipeline' do + expect { subject.execute }.not_to change { Ci::Pipeline.count } + end + end + end + end + + context 'branch changes' do + let(:changes_method) { :branch_changes } + let(:ref_prefix) { 'refs/heads' } + + it_behaves_like 'service for processing ref changes', Git::BranchPushService + end + + context 'tag changes' do + let(:changes_method) { :tag_changes } + let(:ref_prefix) { 'refs/tags' } + + it_behaves_like 'service for processing ref changes', Git::TagPushService + end +end diff --git a/spec/services/git/tag_hooks_service_spec.rb b/spec/services/git/tag_hooks_service_spec.rb index e362577d289..abb5b9b130b 100644 --- a/spec/services/git/tag_hooks_service_spec.rb +++ b/spec/services/git/tag_hooks_service_spec.rb @@ -15,13 +15,7 @@ describe Git::TagHooksService, :service do let(:commit) { tag.dereferenced_target } let(:service) do - described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) - end - - it 'update remote mirrors' do - expect(service).to receive(:update_remote_mirrors).and_call_original - - service.execute + described_class.new(project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref }) end describe 'System hooks' do @@ -64,6 +58,7 @@ describe Git::TagHooksService, :service do describe 'Push data' do shared_examples_for 'tag push data expectations' do subject(:push_data) { service.send(:push_data) } + it 'has expected push data attributes' do is_expected.to match a_hash_including( object_kind: 'tag_push', diff --git a/spec/services/git/tag_push_service_spec.rb b/spec/services/git/tag_push_service_spec.rb index 7e008637182..9688041c08c 100644 --- a/spec/services/git/tag_push_service_spec.rb +++ b/spec/services/git/tag_push_service_spec.rb @@ -8,7 +8,7 @@ describe Git::TagPushService do let(:user) { create(:user) } let(:project) { create(:project, :repository) } - let(:service) { described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) } + let(:service) { described_class.new(project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref }) } let(:oldrev) { Gitlab::Git::BLANK_SHA } let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0 diff --git a/spec/services/grafana/proxy_service_spec.rb b/spec/services/grafana/proxy_service_spec.rb new file mode 100644 index 00000000000..694d531c9fc --- /dev/null +++ b/spec/services/grafana/proxy_service_spec.rb @@ -0,0 +1,139 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Grafana::ProxyService do + include ReactiveCachingHelpers + + let_it_be(:project) { create(:project) } + let_it_be(:grafana_integration) { create(:grafana_integration, project: project) } + + let(:proxy_path) { 'api/v1/query_range' } + let(:datasource_id) { '1' } + let(:query_params) do + { + 'query' => 'rate(relevant_metric)', + 'start' => '1570441248', + 'end' => '1570444848', + 'step' => '900' + } + end + + let(:cache_params) { [project.id, datasource_id, proxy_path, query_params] } + + let(:service) do + described_class.new(project, datasource_id, proxy_path, query_params) + end + + shared_examples_for 'initializes an instance' do + it 'initializes an instance of ProxyService class' do + expect(subject).to be_an_instance_of(described_class) + expect(subject.project).to eq(project) + expect(subject.datasource_id).to eq('1') + expect(subject.proxy_path).to eq('api/v1/query_range') + expect(subject.query_params).to eq(query_params) + end + end + + describe '.from_cache' do + subject { described_class.from_cache(*cache_params) } + + it_behaves_like 'initializes an instance' + end + + describe '#initialize' do + subject { service } + + it_behaves_like 'initializes an instance' + end + + describe '#execute' do + subject(:result) { service.execute } + + context 'when grafana integration is not configured' do + before do + allow(project).to receive(:grafana_integration).and_return(nil) + end + + it 'returns error' do + expect(result).to eq( + status: :error, + message: 'Proxy support for this API is not available currently' + ) + end + end + + context 'with caching', :use_clean_rails_memory_store_caching do + context 'when value not present in cache' do + it 'returns nil' do + expect(ReactiveCachingWorker) + .to receive(:perform_async) + .with(service.class, service.id, *cache_params) + + expect(result).to eq(nil) + end + end + + context 'when value present in cache' do + let(:return_value) { { 'http_status' => 200, 'body' => 'body' } } + + before do + stub_reactive_cache(service, return_value, cache_params) + end + + it 'returns cached value' do + expect(ReactiveCachingWorker) + .not_to receive(:perform_async) + .with(service.class, service.id, *cache_params) + + expect(result[:http_status]).to eq(return_value[:http_status]) + expect(result[:body]).to eq(return_value[:body]) + end + end + end + + context 'call prometheus api' do + let(:client) { service.send(:client) } + + before do + synchronous_reactive_cache(service) + end + + context 'connection to grafana datasource succeeds' do + let(:response) { instance_double(Gitlab::HTTP::Response) } + let(:status_code) { 400 } + let(:body) { 'body' } + + before do + allow(client).to receive(:proxy_datasource).and_return(response) + + allow(response).to receive(:code).and_return(status_code) + allow(response).to receive(:body).and_return(body) + end + + it 'returns the http status code and body from prometheus' do + expect(result).to eq( + http_status: status_code, + body: body, + status: :success + ) + end + end + + context 'connection to grafana datasource fails' do + before do + allow(client).to receive(:proxy_datasource) + .and_raise(Grafana::Client::Error, 'Network connection error') + end + + it 'returns error' do + expect(result).to eq( + status: :error, + message: 'Network connection error', + http_status: :service_unavailable + ) + end + end + end + end +end diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index 1ab2e994b7e..d13739cefd9 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -8,7 +8,7 @@ describe Groups::DestroyService do let!(:user) { create(:user) } let!(:group) { create(:group) } let!(:nested_group) { create(:group, parent: group) } - let!(:project) { create(:project, :legacy_storage, namespace: group) } + let!(:project) { create(:project, :repository, :legacy_storage, namespace: group) } let!(:notification_setting) { create(:notification_setting, source: group)} let(:gitlab_shell) { Gitlab::Shell.new } let(:remove_path) { group.path + "+#{group.id}+deleted" } @@ -119,7 +119,7 @@ describe Groups::DestroyService do let!(:project) { create(:project, :legacy_storage, :empty_repo, namespace: group) } it 'removes repository' do - expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey end end @@ -127,7 +127,7 @@ describe Groups::DestroyService do let!(:project) { create(:project, :empty_repo, namespace: group) } it 'removes repository' do - expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey end end end diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index 0cbb3122bb0..5ef1fb1932f 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -426,5 +426,22 @@ describe Groups::TransferService do end end end + + context 'when a project in group has container images' do + let(:group) { create(:group, :public, :nested) } + let!(:project) { create(:project, :repository, :public, namespace: group) } + + before do + stub_container_registry_tags(repository: /image/, tags: %w[rc1]) + create(:container_repository, project: project, name: :image) + create(:group_member, :owner, group: new_parent_group, user: user) + end + + it 'does not allow group to be transferred' do + transfer_service.execute(new_parent_group) + + expect(transfer_service.error).to match(/Docker images in their Container Registry/) + end + end end end diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 12e9c2b2f3a..ca8eaf4c970 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -148,6 +148,30 @@ describe Groups::UpdateService do end end + context 'projects in group have container images' do + let(:service) { described_class.new(public_group, user, path: SecureRandom.hex) } + let(:project) { create(:project, :internal, group: public_group) } + + before do + stub_container_registry_tags(repository: /image/, tags: %w[rc1]) + create(:container_repository, project: project, name: :image) + end + + it 'does not allow path to be changed' do + result = described_class.new(public_group, user, path: 'new-path').execute + + expect(result).to eq false + expect(public_group.errors[:base].first).to match(/Docker images in their Container Registry/) + end + + it 'allows other settings to be changed' do + result = described_class.new(public_group, user, name: 'new-name').execute + + expect(result).to eq true + expect(public_group.reload.name).to eq('new-name') + end + end + context 'for a subgroup' do let(:subgroup) { create(:group, :private, parent: private_group) } diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 642a49d57d5..1f7d564b6ec 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -8,6 +8,7 @@ describe Issues::CloseService do let(:user2) { create(:user, email: "user2@example.com") } let(:guest) { create(:user) } let(:issue) { create(:issue, title: "My issue", project: project, assignees: [user2], author: create(:user)) } + let(:external_issue) { ExternalIssue.new('JIRA-123', project) } let(:closing_merge_request) { create(:merge_request, source_project: project) } let(:closing_commit) { create(:commit, project: project) } let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) } @@ -36,6 +37,16 @@ describe Issues::CloseService do expect(service.execute(issue)).to eq(issue) end + it 'closes the external issue even when the user is not authorized to do so' do + allow(service).to receive(:can?).with(user, :update_issue, external_issue) + .and_return(false) + + expect(service).to receive(:close_issue) + .with(external_issue, closed_via: nil, notifications: true, system_note: true) + + service.execute(external_issue) + end + it 'closes the issue when the user is authorized to do so' do allow(service).to receive(:can?).with(user, :update_issue, issue) .and_return(true) diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index b7bedc2f97e..5dc6b6176ee 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -344,7 +344,7 @@ describe Issues::CreateService do end before do - allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) + stub_feature_flags(allow_possible_spam: false) end context 'when recaptcha was verified' do @@ -384,31 +384,67 @@ describe Issues::CreateService do end context 'when recaptcha was not verified' do + before do + expect_next_instance_of(SpamService) do |spam_service| + expect(spam_service).to receive_messages(check_for_spam?: true) + end + end + context 'when akismet detects spam' do before do - allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) + expect_next_instance_of(AkismetService) do |akismet_service| + expect(akismet_service).to receive_messages(spam?: true) + end end - it 'marks an issue as a spam ' do - expect(issue).to be_spam - end + context 'when issuables_recaptcha_enabled feature flag is true' do + it 'marks an issue as a spam ' do + expect(issue).to be_spam + end - it 'an issue is not valid ' do - expect(issue.valid?).to be_falsey - end + it 'invalidates the issue' do + expect(issue).to be_invalid + end + + it 'creates a new spam_log' do + expect { issue } + .to log_spam(title: issue.title, description: issue.description, user_id: user.id, noteable_type: 'Issue') + end - it 'creates a new spam_log' do - expect {issue}.to change {SpamLog.count}.from(0).to(1) + it 'assigns a spam_log to an issue' do + expect(issue.spam_log).to eq(SpamLog.last) + end end - it 'assigns a spam_log to an issue' do - expect(issue.spam_log).to eq(SpamLog.last) + context 'when issuable_recaptcha_enabled feature flag is false' do + before do + stub_feature_flags(allow_possible_spam: true) + end + + it 'does not mark an issue as a spam ' do + expect(issue).not_to be_spam + end + + it 'accepts the ​issue as valid' do + expect(issue).to be_valid + end + + it 'creates a new spam_log' do + expect { issue } + .to log_spam(title: issue.title, description: issue.description, user_id: user.id, noteable_type: 'Issue') + end + + it 'assigns a spam_log to an issue' do + expect(issue.spam_log).to eq(SpamLog.last) + end end end context 'when akismet does not detect spam' do before do - allow_any_instance_of(AkismetService).to receive(:spam?).and_return(false) + expect_next_instance_of(AkismetService) do |akismet_service| + expect(akismet_service).to receive_messages(spam?: false) + end end it 'does not mark an issue as a spam ' do diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 55254b61ac8..154bfec0da2 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -6,7 +6,8 @@ describe Issues::UpdateService, :mailer do let(:user) { create(:user) } let(:user2) { create(:user) } let(:user3) { create(:user) } - let(:project) { create(:project) } + let(:group) { create(:group, :public) } + let(:project) { create(:project, :repository, group: group) } let(:label) { create(:label, project: project) } let(:label2) { create(:label) } @@ -667,6 +668,7 @@ describe Issues::UpdateService, :mailer do context 'updating mentions' do let(:mentionable) { issue } + include_examples 'updating mentions', described_class end diff --git a/spec/services/issues/zoom_link_service_spec.rb b/spec/services/issues/zoom_link_service_spec.rb index baa6d774864..ba3f007c917 100644 --- a/spec/services/issues/zoom_link_service_spec.rb +++ b/spec/services/issues/zoom_link_service_spec.rb @@ -38,12 +38,6 @@ describe Issues::ZoomLinkService do end end - shared_context 'feature flag disabled' do - before do - stub_feature_flags(issue_zoom_integration: false) - end - end - shared_context 'insufficient permissions' do before do project.add_guest(user) @@ -57,6 +51,12 @@ describe Issues::ZoomLinkService do expect(result.payload[:description]) .to eq("#{issue.description}\n\n#{zoom_link}") end + + it 'tracks the add event' do + expect(Gitlab::Tracking).to receive(:event) + .with('IncidentManagement::ZoomIntegration', 'add_zoom_meeting', label: 'Issue ID', value: issue.id) + result + end end shared_examples 'cannot add link' do @@ -78,11 +78,6 @@ describe Issues::ZoomLinkService do include_examples 'cannot add link' end - context 'when feature flag is disabled' do - include_context 'feature flag disabled' - include_examples 'cannot add link' - end - context 'with insufficient permissions' do include_context 'insufficient permissions' include_examples 'cannot add link' @@ -113,12 +108,6 @@ describe Issues::ZoomLinkService do it { is_expected.to eq(true) } - context 'when feature flag is disabled' do - include_context 'feature flag disabled' - - it { is_expected.to eq(false) } - end - context 'with insufficient permissions' do include_context 'insufficient permissions' @@ -152,9 +141,11 @@ describe Issues::ZoomLinkService do .to eq(issue.description.delete_suffix("\n\n#{zoom_link}")) end - context 'when feature flag is disabled' do - include_context 'feature flag disabled' - include_examples 'cannot remove link' + it 'tracks the remove event' do + expect(Gitlab::Tracking).to receive(:event) + .with('IncidentManagement::ZoomIntegration', 'remove_zoom_meeting', label: 'Issue ID', value: issue.id) + + result end context 'with insufficient permissions' do @@ -187,12 +178,6 @@ describe Issues::ZoomLinkService do it { is_expected.to eq(true) } - context 'when feature flag is disabled' do - include_context 'feature flag disabled' - - it { is_expected.to eq(false) } - end - context 'with insufficient permissions' do include_context 'insufficient permissions' diff --git a/spec/services/members/approve_access_request_service_spec.rb b/spec/services/members/approve_access_request_service_spec.rb index f56c31e51f6..5bbceac3dd0 100644 --- a/spec/services/members/approve_access_request_service_spec.rb +++ b/spec/services/members/approve_access_request_service_spec.rb @@ -3,8 +3,8 @@ require 'spec_helper' describe Members::ApproveAccessRequestService do - let(:project) { create(:project, :public, :access_requestable) } - let(:group) { create(:group, :public, :access_requestable) } + let(:project) { create(:project, :public) } + let(:group) { create(:group, :public) } let(:current_user) { create(:user) } let(:access_requester_user) { create(:user) } let(:access_requester) { source.requesters.find_by!(user_id: access_requester_user.id) } diff --git a/spec/services/members/request_access_service_spec.rb b/spec/services/members/request_access_service_spec.rb index 2e5275eb3f2..a0f7ae91bdb 100644 --- a/spec/services/members/request_access_service_spec.rb +++ b/spec/services/members/request_access_service_spec.rb @@ -41,7 +41,7 @@ describe Members::RequestAccessService do context 'when access requests are disabled' do %i[project group].each do |source_type| it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do - let(:source) { create(source_type, :public) } + let(:source) { create(source_type, :public, :request_access_disabled) } end end end @@ -49,7 +49,7 @@ describe Members::RequestAccessService do context 'when current user can request access to the project' do %i[project group].each do |source_type| it_behaves_like 'a service creating a access request' do - let(:source) { create(source_type, :public, :access_requestable) } + let(:source) { create(source_type, :public) } 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 07e0218e1df..51a5c51f6c3 100644 --- a/spec/services/merge_requests/create_from_issue_service_spec.rb +++ b/spec/services/merge_requests/create_from_issue_service_spec.rb @@ -13,6 +13,7 @@ describe MergeRequests::CreateFromIssueService do let(:custom_source_branch) { 'custom-source-branch' } subject(:service) { described_class.new(project, user, service_params) } + subject(:service_with_custom_source_branch) { described_class.new(project, user, branch_name: custom_source_branch, **service_params) } before do diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index ffc86f68469..fff6ddf3928 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -56,9 +56,11 @@ describe MergeRequests::PostMergeService do issue = create(:issue, project: project) allow(merge_request).to receive(:visible_closing_issues_for).and_return([issue]) - allow_any_instance_of(Issues::CloseService).to receive(:execute).with(issue, commit: merge_request).and_raise + expect_next_instance_of(Issues::CloseService) do |service| + allow(service).to receive(:execute).with(issue, commit: merge_request).and_raise(RuntimeError) + end - expect { described_class.new(project, user, {}).execute(merge_request) }.to raise_error + expect { described_class.new(project, user).execute(merge_request) }.to raise_error(RuntimeError) expect(merge_request.reload).to be_merged end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 6ba67c7165c..2dc932c9f2c 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe MergeRequests::RefreshService do include ProjectForksHelper + include ProjectHelpers let(:project) { create(:project, :repository) } let(:user) { create(:user) } @@ -756,4 +757,127 @@ describe MergeRequests::RefreshService do end end end + + describe '#abort_ff_merge_requests_with_when_pipeline_succeeds' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:source_project) { project } + let_it_be(:target_project) { project } + let_it_be(:author) { create_user_from_membership(target_project, :developer) } + let_it_be(:user) { create(:user) } + + let_it_be(:forked_project) do + fork_project(target_project, author, repository: true) + end + + let_it_be(:merge_request) do + create(:merge_request, + author: author, + source_project: source_project, + source_branch: 'feature', + target_branch: 'master', + target_project: target_project, + auto_merge_enabled: true, + auto_merge_strategy: AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS, + merge_user: user) + end + + let_it_be(:newrev) do + target_project + .repository + .create_file(user, 'test1.txt', 'Test data', + message: 'Test commit', branch_name: 'master') + end + + let_it_be(:oldrev) do + target_project + .repository + .commit(newrev) + .parent_id + end + + let(:refresh_service) { service.new(project, user) } + + before do + target_project.merge_method = merge_method + target_project.save! + + refresh_service.execute(oldrev, newrev, 'refs/heads/master') + merge_request.reload + end + + let(:aborted_message) do + /aborted the automatic merge because target branch was updated/ + end + + shared_examples 'aborted MWPS' do + it 'aborts auto_merge' do + expect(merge_request.auto_merge_enabled?).to be_falsey + expect(merge_request.notes.last.note).to match(aborted_message) + end + + it 'removes merge_user' do + expect(merge_request.merge_user).to be_nil + end + + it 'does not add todos for merge user' do + expect(user.todos.for_target(merge_request)).to be_empty + end + + it 'adds todos for merge author' do + expect(author.todos.for_target(merge_request)).to be_present.and be_all(&:pending?) + end + end + + context 'when Project#merge_method is set to FF' do + let(:merge_method) { :ff } + + it_behaves_like 'aborted MWPS' + + context 'with forked project' do + let(:source_project) { forked_project } + + it_behaves_like 'aborted MWPS' + end + end + + context 'when Project#merge_method is set to rebase_merge' do + let(:merge_method) { :rebase_merge } + + it_behaves_like 'aborted MWPS' + + context 'with forked project' do + let(:source_project) { forked_project } + + it_behaves_like 'aborted MWPS' + end + end + + context 'when Project#merge_method is set to merge' do + let(:merge_method) { :merge } + + shared_examples 'maintained MWPS' do + it 'does not cancel auto merge' do + expect(merge_request.auto_merge_enabled?).to be_truthy + expect(merge_request.notes).to be_empty + end + + it 'does not change merge_user' do + expect(merge_request.merge_user).to eq(user) + end + + it 'does not add todos' do + expect(author.todos.for_target(merge_request)).to be_empty + expect(user.todos.for_target(merge_request)).to be_empty + end + end + + it_behaves_like 'maintained MWPS' + + context 'with forked project' do + let(:source_project) { forked_project } + + it_behaves_like 'maintained MWPS' + end + end + end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 9688e02d6ac..d3c4c436901 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -5,7 +5,8 @@ require 'spec_helper' describe MergeRequests::UpdateService, :mailer do include ProjectForksHelper - let(:project) { create(:project, :repository) } + let(:group) { create(:group, :public) } + let(:project) { create(:project, :repository, group: group) } let(:user) { create(:user) } let(:user2) { create(:user) } let(:user3) { create(:user) } @@ -472,6 +473,7 @@ describe MergeRequests::UpdateService, :mailer do context 'updating mentions' do let(:mentionable) { merge_request } + include_examples 'updating mentions', described_class end diff --git a/spec/services/note_summary_spec.rb b/spec/services/note_summary_spec.rb index e59731207a5..aa4e41f4d8c 100644 --- a/spec/services/note_summary_spec.rb +++ b/spec/services/note_summary_spec.rb @@ -46,5 +46,17 @@ describe NoteSummary do it 'returns metadata hash' do expect(create_note_summary.metadata).to eq(action: 'icon', commit_count: 5) end + + context 'description action and noteable has saved_description_version' do + before do + noteable.saved_description_version = 1 + end + + subject { described_class.new(noteable, project, user, 'note', action: 'description') } + + it 'sets the description_version metadata' do + expect(subject.metadata).to include(description_version: 1) + end + end end end diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index 717eb97fa5a..73fcdd787aa 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -3,17 +3,25 @@ require 'spec_helper' describe Notes::UpdateService do - let(:project) { create(:project) } + let(:group) { create(:group, :public) } + let(:project) { create(:project, :public, group: group) } + let(:private_group) { create(:group, :private) } + let(:private_project) { create(:project, :private, group: private_group) } let(:user) { create(:user) } let(:user2) { create(:user) } let(:user3) { create(:user) } let(:issue) { create(:issue, project: project) } + let(:issue2) { create(:issue, project: private_project) } let(:note) { create(:note, project: project, noteable: issue, author: user, note: "Old note #{user2.to_reference}") } before do project.add_maintainer(user) project.add_developer(user2) project.add_developer(user3) + group.add_developer(user3) + private_group.add_developer(user) + private_group.add_developer(user2) + private_project.add_developer(user3) end describe '#execute' do @@ -46,13 +54,17 @@ describe Notes::UpdateService do end context 'todos' do - let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) } + shared_examples 'does not update todos' do + it 'keep todos' do + expect(todo.reload).to be_pending + end - context 'when the note change' do - before do - update_note({ note: "New note #{user2.to_reference} #{user3.to_reference}" }) + it 'does not create any new todos' do + expect(Todo.count).to eq(1) end + end + shared_examples 'creates one todo' do it 'marks todos as done' do expect(todo.reload).to be_done end @@ -62,17 +74,75 @@ describe Notes::UpdateService do end end - context 'when the note does not change' do - before do - update_note({ note: "Old note #{user2.to_reference}" }) + context 'when note includes a user mention' do + let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) } + + context 'when the note does not change mentions' do + before do + update_note({ note: "Old note #{user2.to_reference}" }) + end + + it_behaves_like 'does not update todos' end - it 'keep todos' do - expect(todo.reload).to be_pending + context 'when the note changes to include one more user mention' do + before do + update_note({ note: "New note #{user2.to_reference} #{user3.to_reference}" }) + end + + it_behaves_like 'creates one todo' end - it 'does not create any new todos' do - expect(Todo.count).to eq(1) + context 'when the note changes to include a group mentions' do + before do + update_note({ note: "New note #{private_group.to_reference}" }) + end + + it_behaves_like 'creates one todo' + end + end + + context 'when note includes a group mention' do + context 'when the group is public' do + let(:note) { create(:note, project: project, noteable: issue, author: user, note: "Old note #{group.to_reference}") } + let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) } + + context 'when the note does not change mentions' do + before do + update_note({ note: "Old note #{group.to_reference}" }) + end + + it_behaves_like 'does not update todos' + end + + context 'when the note changes mentions' do + before do + update_note({ note: "New note #{user2.to_reference} #{user3.to_reference}" }) + end + + it_behaves_like 'creates one todo' + end + end + + context 'when the group is private' do + let(:note) { create(:note, project: project, noteable: issue, author: user, note: "Old note #{private_group.to_reference}") } + let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) } + + context 'when the note does not change mentions' do + before do + update_note({ note: "Old note #{private_group.to_reference}" }) + end + + it_behaves_like 'does not update todos' + end + + context 'when the note changes mentions' do + before do + update_note({ note: "New note #{user2.to_reference} #{user3.to_reference}" }) + end + + it_behaves_like 'creates one todo' + end end end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index bd6734634cb..aa67b87a645 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -678,6 +678,27 @@ describe NotificationService, :mailer do end end + describe '#send_new_release_notifications' do + context 'when recipients for a new release exist' do + let(:release) { create(:release) } + + it 'calls new_release_email for each relevant recipient' do + user_1 = create(:user) + user_2 = create(:user) + user_3 = create(:user) + recipient_1 = NotificationRecipient.new(user_1, :custom, custom_action: :new_release) + recipient_2 = NotificationRecipient.new(user_2, :custom, custom_action: :new_release) + allow(NotificationRecipientService).to receive(:build_new_release_recipients).and_return([recipient_1, recipient_2]) + + release + + should_email(user_1) + should_email(user_2) + should_not_email(user_3) + end + end + end + describe 'Participating project notification settings have priority over group and global settings if available' do let!(:group) { create(:group) } let!(:maintainer) { group.add_owner(create(:user, username: 'maintainer')).user } @@ -1942,7 +1963,7 @@ describe NotificationService, :mailer do let(:developer) { create(:user) } let!(:group) do - create(:group, :public, :access_requestable) do |group| + create(:group, :public) do |group| group.add_owner(owner) group.add_maintainer(maintainer) group.add_developer(developer) @@ -1968,7 +1989,7 @@ describe NotificationService, :mailer do end it_behaves_like 'sends notification only to a maximum of ten, most recently active group owners' do - let(:group) { create(:group, :public, :access_requestable) } + let(:group) { create(:group, :public) } let(:notification_trigger) { group.request_access(added_user) } end end @@ -2029,7 +2050,7 @@ describe NotificationService, :mailer do let(:maintainer) { create(:user) } let!(:project) do - create(:project, :public, :access_requestable) do |project| + create(:project, :public) do |project| project.add_developer(developer) project.add_maintainer(maintainer) end @@ -2053,7 +2074,7 @@ describe NotificationService, :mailer do end it_behaves_like 'sends notification only to a maximum of ten, most recently active project maintainers' do - let(:project) { create(:project, :public, :access_requestable) } + let(:project) { create(:project, :public) } let(:notification_trigger) { project.request_access(added_user) } end end @@ -2064,7 +2085,7 @@ describe NotificationService, :mailer do context 'when the project has no maintainers' do context 'when the group has at least one owner' do - let!(:project) { create(:project, :public, :access_requestable, namespace: group) } + let!(:project) { create(:project, :public, namespace: group) } before do reset_delivered_emails! @@ -2079,14 +2100,14 @@ describe NotificationService, :mailer do end it_behaves_like 'sends notification only to a maximum of ten, most recently active group owners' do - let(:group) { create(:group, :public, :access_requestable) } + let(:group) { create(:group, :public) } let(:notification_trigger) { project.request_access(added_user) } end end context 'when the group does not have any owners' do let(:group) { create(:group) } - let!(:project) { create(:project, :public, :access_requestable, namespace: group) } + let!(:project) { create(:project, :public, namespace: group) } context 'recipients' do before do @@ -2107,7 +2128,7 @@ describe NotificationService, :mailer do let(:developer) { create(:user) } let!(:project) do - create(:project, :public, :access_requestable, namespace: group) do |project| + create(:project, :public, namespace: group) do |project| project.add_maintainer(maintainer) project.add_developer(developer) end @@ -2128,7 +2149,7 @@ describe NotificationService, :mailer do end it_behaves_like 'sends notification only to a maximum of ten, most recently active project maintainers' do - let(:project) { create(:project, :public, :access_requestable, namespace: group) } + let(:project) { create(:project, :public, namespace: group) } let(:notification_trigger) { project.request_access(added_user) } end end diff --git a/spec/services/projects/after_import_service_spec.rb b/spec/services/projects/after_import_service_spec.rb index 51d3fd18881..27e8f3c45ba 100644 --- a/spec/services/projects/after_import_service_spec.rb +++ b/spec/services/projects/after_import_service_spec.rb @@ -19,6 +19,8 @@ describe Projects::AfterImportService do allow(housekeeping_service) .to receive(:execute).and_yield + + expect(housekeeping_service).to receive(:increment!) end it 'performs housekeeping' 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 14247f1c71e..14772d172e8 100644 --- a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb @@ -157,6 +157,6 @@ describe Projects::ContainerRepository::CleanupTagsService do def expect_delete(digest) expect_any_instance_of(ContainerRegistry::Client) .to receive(:delete_repository_tag) - .with(repository.path, digest) + .with(repository.path, digest) { true } end end diff --git a/spec/services/projects/container_repository/delete_tags_service_spec.rb b/spec/services/projects/container_repository/delete_tags_service_spec.rb new file mode 100644 index 00000000000..f296ef3a776 --- /dev/null +++ b/spec/services/projects/container_repository/delete_tags_service_spec.rb @@ -0,0 +1,135 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Projects::ContainerRepository::DeleteTagsService do + set(:user) { create(:user) } + set(:project) { create(:project, :private) } + set(:repository) { create(:container_repository, :root, project: project) } + + let(:params) { { tags: tags } } + let(:service) { described_class.new(project, user, params) } + + before do + stub_container_registry_config(enabled: true, + api_url: 'http://registry.gitlab', + host_port: 'registry.gitlab') + + stub_container_registry_tags( + repository: repository.path, + tags: %w(latest A Ba Bb C D E)) + + stub_tag_digest('latest', 'sha256:configA') + stub_tag_digest('A', 'sha256:configA') + stub_tag_digest('Ba', 'sha256:configB') + end + + describe '#execute' do + let(:tags) { %w[A] } + subject { service.execute(repository) } + + context 'without permissions' do + it { is_expected.to include(status: :error) } + end + + context 'with permissions' do + before do + project.add_developer(user) + end + + context 'when no params are specified' do + let(:params) { {} } + + it 'does not remove anything' do + expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag) + + is_expected.to include(status: :error) + end + end + + context 'with empty tags' do + let(:tags) { [] } + + it 'does not remove anything' do + expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag) + + is_expected.to include(status: :error) + end + end + + context 'with dummy tags disabled' do + let(:tags) { %w[A Ba] } + + before do + stub_feature_flags(container_registry_smart_delete: false) + end + + it 'deletes tags one by one' do + expect_delete_tag('sha256:configA') + expect_delete_tag('sha256:configB') + is_expected.to include(status: :success) + end + end + + context 'with dummy tags enabled' do + let(:tags) { %w[A Ba] } + + it 'deletes the tags using a dummy image' do + stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + + stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/A") + .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) + + stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/Ba") + .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) + + expect_delete_tag('sha256:dummy') + + is_expected.to include(status: :success) + end + + it 'succedes when tag delete returns 404' do + stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + + stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/A") + .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) + + stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/Ba") + .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) + + stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:dummy") + .to_return(status: 404, body: "", headers: {}) + + is_expected.to include(status: :success) + end + end + end + end + + private + + def stub_tag_digest(tag, digest) + stub_request(:head, "http://registry.gitlab/v2/#{repository.path}/manifests/#{tag}") + .to_return(status: 200, body: "", headers: { 'docker-content-digest' => digest }) + 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 + end + end + + def stub_upload(content, digest) + expect_any_instance_of(ContainerRegistry::Client) + .to receive(:upload_blob) + .with(repository.path, content, digest) { double(success?: true ) } + end + + def expect_delete_tag(digest) + expect_any_instance_of(ContainerRegistry::Client) + .to receive(:delete_repository_tag) + .with(repository.path, digest) { true } + end +end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 9a6f64b825a..2331281bd8e 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -24,8 +24,8 @@ describe Projects::DestroyService do it 'deletes the project' do expect(Project.unscoped.all).not_to include(project) - expect(project.gitlab_shell.exists?(project.repository_storage, path + '.git')).to be_falsey - expect(project.gitlab_shell.exists?(project.repository_storage, remove_path + '.git')).to be_falsey + expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey + expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey end end diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index b759830d603..7e7e80ca240 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -50,6 +50,7 @@ describe Projects::ForkService do it { expect(to_project.star_count).to be_zero } it { expect(to_project.description).to eq(@from_project.description) } it { expect(to_project.avatar.file).to be_exists } + it { expect(to_project.ci_config_path).to eq(@from_project.ci_config_path) } # This test is here because we had a bug where the from-project lost its # avatar after being forked. @@ -215,7 +216,8 @@ describe Projects::ForkService do @project = create(:project, :repository, creator_id: @group_owner.id, star_count: 777, - description: 'Wow, such a cool project!') + description: 'Wow, such a cool project!', + ci_config_path: 'debian/salsa-ci.yml') @group = create(:group) @group.add_user(@group_owner, GroupMember::OWNER) @group.add_user(@developer, GroupMember::DEVELOPER) @@ -228,14 +230,15 @@ describe Projects::ForkService do it 'group owner successfully forks project into the group' do to_project = fork_project(@project, @group_owner, @opts) - expect(to_project).to be_persisted - expect(to_project.errors).to be_empty - expect(to_project.owner).to eq(@group) - expect(to_project.namespace).to eq(@group) - expect(to_project.name).to eq(@project.name) - expect(to_project.path).to eq(@project.path) - expect(to_project.description).to eq(@project.description) - expect(to_project.star_count).to be_zero + expect(to_project).to be_persisted + expect(to_project.errors).to be_empty + expect(to_project.owner).to eq(@group) + expect(to_project.namespace).to eq(@group) + expect(to_project.name).to eq(@project.name) + expect(to_project.path).to eq(@project.path) + expect(to_project.description).to eq(@project.description) + expect(to_project.ci_config_path).to eq(@project.ci_config_path) + expect(to_project.star_count).to be_zero end end diff --git a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb index 5b778f16b5a..70785c606a5 100644 --- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb @@ -48,8 +48,8 @@ describe Projects::HashedStorage::MigrateRepositoryService do it 'renames project and wiki repositories' do service.execute - expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.git")).to be_truthy - expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_truthy + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.git")).to be_truthy + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_truthy end it 'updates project to be hashed and not read-only' do @@ -84,14 +84,13 @@ describe Projects::HashedStorage::MigrateRepositoryService do service.execute - expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.git")).to be_falsey - expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_falsey + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.git")).to be_falsey + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_falsey expect(project.repository_read_only?).to be_falsey end context 'when rollback fails' do before do - hashed_storage.ensure_storage_path_exists gitlab_shell.mv_repository(project.repository_storage, old_disk_path, new_disk_path) end diff --git a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb index bd4354a7df3..3ca9ee5bee5 100644 --- a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb @@ -48,8 +48,8 @@ describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis it 'renames project and wiki repositories' do service.execute - expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.git")).to be_truthy - expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_truthy + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.git")).to be_truthy + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_truthy end it 'updates project to be legacy and not read-only' do @@ -84,14 +84,13 @@ describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis service.execute - expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.git")).to be_falsey - expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_falsey + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.git")).to be_falsey + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_falsey expect(project.repository_read_only?).to be_falsey end context 'when rollback fails' do before do - legacy_storage.ensure_storage_path_exists gitlab_shell.mv_repository(project.repository_storage, old_disk_path, new_disk_path) end diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb index f651db70cbd..c99054d9fd5 100644 --- a/spec/services/projects/housekeeping_service_spec.rb +++ b/spec/services/projects/housekeeping_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe Projects::HousekeepingService do subject { described_class.new(project) } + set(:project) { create(:project, :repository) } before do diff --git a/spec/services/projects/import_export/export_service_spec.rb b/spec/services/projects/import_export/export_service_spec.rb index 404bb55629a..146d656c909 100644 --- a/spec/services/projects/import_export/export_service_spec.rb +++ b/spec/services/projects/import_export/export_service_spec.rb @@ -35,20 +35,27 @@ describe Projects::ImportExport::ExportService do end it 'saves the repo' do + # This spec errors when run against the EE codebase as there will be a third repository + # saved (the EE-specific design repository). + # + # Instead, skip this test when run within EE. There is a spec for the EE-specific design repo + # in the corresponding EE spec. + skip if Gitlab.ee? + # once for the normal repo, once for the wiki expect(Gitlab::ImportExport::RepoSaver).to receive(:new).twice.and_call_original service.execute end - it 'saves the lfs objects' do - expect(Gitlab::ImportExport::LfsSaver).to receive(:new).and_call_original + it 'saves the wiki repo' do + expect(Gitlab::ImportExport::WikiRepoSaver).to receive(:new).and_call_original service.execute end - it 'saves the wiki repo' do - expect(Gitlab::ImportExport::WikiRepoSaver).to receive(:new).and_call_original + it 'saves the lfs objects' do + expect(Gitlab::ImportExport::LfsSaver).to receive(:new).and_call_original service.execute end @@ -98,9 +105,9 @@ describe Projects::ImportExport::ExportService do end end - context 'when saver services fail' do + context 'when saving services fail' do before do - allow(service).to receive(:save_services).and_return(false) + allow(service).to receive(:save_exporters).and_return(false) end after do @@ -122,7 +129,7 @@ describe Projects::ImportExport::ExportService do expect(Rails.logger).to receive(:error) end - it 'the after export strategy is not called' do + it 'does not call the export strategy' do expect(service).not_to receive(:execute_after_export_action) end end diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb index 7e765659b9d..b2f9fd6df79 100644 --- a/spec/services/projects/operations/update_service_spec.rb +++ b/spec/services/projects/operations/update_service_spec.rb @@ -170,5 +170,61 @@ describe Projects::Operations::UpdateService do expect(project.reload.name).to eq(original_name) end end + + context 'grafana integration' do + let(:params) do + { + grafana_integration_attributes: { + grafana_url: 'http://new.grafana.com', + token: 'VerySecureToken=' + } + } + end + + context 'without existing grafana integration' do + it 'creates an integration' do + expect(result[:status]).to eq(:success) + + expected_attrs = params[:grafana_integration_attributes] + integration = project.reload.grafana_integration + + expect(integration.grafana_url).to eq(expected_attrs[:grafana_url]) + expect(integration.token).to eq(expected_attrs[:token]) + end + end + + context 'with an existing grafana integration' do + before do + create(:grafana_integration, project: project) + end + + it 'updates the settings' do + expect(result[:status]).to eq(:success) + + expected_attrs = params[:grafana_integration_attributes] + integration = project.reload.grafana_integration + + expect(integration.grafana_url).to eq(expected_attrs[:grafana_url]) + expect(integration.token).to eq(expected_attrs[:token]) + end + + context 'with all grafana attributes blank in params' do + let(:params) do + { + grafana_integration_attributes: { + grafana_url: '', + token: '' + } + } + end + + it 'destroys the metrics_setting entry in DB' do + expect(result[:status]).to eq(:success) + + expect(project.reload.grafana_integration).to be_nil + end + end + end + end end end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 6b906f9372c..26d8ac9b479 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -103,7 +103,7 @@ describe Projects::TransferService do it 'rolls back repo location' do attempt_project_transfer - expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be(true) + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{project.disk_path}.git")).to be(true) expect(original_path).to eq current_path end diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index b597717c347..fe92b53cd91 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -40,6 +40,7 @@ describe Projects::UpdatePagesService do it "doesn't delete artifacts after deploying" do expect(execute).to eq(:success) + expect(project.pages_metadatum).to be_deployed expect(build.artifacts?).to eq(true) end end @@ -47,6 +48,7 @@ describe Projects::UpdatePagesService do it 'succeeds' do expect(project.pages_deployed?).to be_falsey expect(execute).to eq(:success) + expect(project.pages_metadatum).to be_deployed expect(project.pages_deployed?).to be_truthy # Check that all expected files are extracted @@ -63,16 +65,23 @@ describe Projects::UpdatePagesService do it 'removes pages after destroy' do expect(PagesWorker).to receive(:perform_in) expect(project.pages_deployed?).to be_falsey + expect(execute).to eq(:success) + + expect(project.pages_metadatum).to be_deployed expect(project.pages_deployed?).to be_truthy + project.destroy + expect(project.pages_deployed?).to be_falsey + expect(ProjectPagesMetadatum.find_by_project_id(project)).to be_nil end it 'fails if sha on branch is not latest' do build.update(ref: 'feature') expect(execute).not_to eq(:success) + expect(project.pages_metadatum).not_to be_deployed end context 'when using empty file' do @@ -94,6 +103,7 @@ describe Projects::UpdatePagesService do it 'succeeds to extract' do expect(execute).to eq(:success) + expect(project.pages_metadatum).to be_deployed end end end @@ -109,6 +119,7 @@ describe Projects::UpdatePagesService do build.reload expect(deploy_status).to be_failed + expect(project.pages_metadatum).not_to be_deployed end end @@ -125,6 +136,7 @@ describe Projects::UpdatePagesService do build.reload expect(deploy_status).to be_failed + expect(project.pages_metadatum).not_to be_deployed end end @@ -138,6 +150,7 @@ describe Projects::UpdatePagesService do build.reload expect(deploy_status).to be_failed + expect(project.pages_metadatum).not_to be_deployed end end end @@ -179,6 +192,7 @@ describe Projects::UpdatePagesService do expect(deploy_status.description) .to match(/artifacts for pages are too large/) expect(deploy_status).to be_script_failure + expect(project.pages_metadatum).not_to be_deployed end end @@ -196,6 +210,7 @@ describe Projects::UpdatePagesService do subject.execute expect(deploy_status.description).not_to be_present + expect(project.pages_metadatum).to be_deployed end end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index b65ee16c189..788f83cc233 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -586,6 +586,22 @@ describe QuickActions::InterpretService do expect(message).to eq('Made this issue confidential.') end + + context 'when issuable is already confidential' do + before do + issuable.update(confidential: true) + end + + it 'does not return the success message' do + _, _, message = service.execute(content, issuable) + + expect(message).to be_empty + end + + it 'is not part of the available commands' do + expect(service.available_commands(issuable)).not_to include(a_hash_including(name: :confidential)) + end + end end shared_examples 'shrug command' do @@ -1529,12 +1545,20 @@ describe QuickActions::InterpretService do end it 'limits to commands passed ' do - content = "/shrug\n/close" + content = "/shrug test\n/close" text, commands = service.execute(content, issue, only: [:shrug]) expect(commands).to be_empty - expect(text).to eq("#{described_class::SHRUG}\n/close") + expect(text).to eq("test #{described_class::SHRUG}\n/close") + end + + it 'preserves leading whitespace ' do + content = " - list\n\n/close\n\ntest\n\n" + + text, _ = service.execute(content, issue) + + expect(text).to eq(" - list\n\ntest") end context '/create_merge_request command' do diff --git a/spec/services/spam_service_spec.rb b/spec/services/spam_service_spec.rb index b9e5e844c1f..76f77583612 100644 --- a/spec/services/spam_service_spec.rb +++ b/spec/services/spam_service_spec.rb @@ -44,30 +44,50 @@ describe SpamService do end context 'when indicated as spam by akismet' do + shared_examples 'akismet spam' do + it 'doesnt check as spam when request is missing' do + check_spam(issue, nil, false) + + expect(issue).not_to be_spam + end + + it 'creates a spam log' do + expect { check_spam(issue, request, false) } + .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue') + end + + it 'does not yield to the block' do + expect(check_spam(issue, request, false)) + .to eql(SpamLog.last) + end + end + before do allow(AkismetService).to receive(:new).and_return(double(spam?: true)) end - it 'doesnt check as spam when request is missing' do - check_spam(issue, nil, false) + context 'when allow_possible_spam feature flag is false' do + before do + stub_feature_flags(allow_possible_spam: false) + end - expect(issue.spam).to be_falsey - end + it_behaves_like 'akismet spam' - it 'checks as spam' do - check_spam(issue, request, false) + it 'checks as spam' do + check_spam(issue, request, false) - expect(issue.spam).to be_truthy + expect(issue.spam).to be_truthy + end end - it 'creates a spam log' do - expect { check_spam(issue, request, false) } - .to change { SpamLog.count }.from(0).to(1) - end + context 'when allow_possible_spam feature flag is true' do + it_behaves_like 'akismet spam' + + it 'does not check as spam' do + check_spam(issue, request, false) - it 'doesnt yield block' do - expect(check_spam(issue, request, false)) - .to eql(SpamLog.last) + expect(issue.spam).to be_nil + end end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 910fe3b50b7..83101add724 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -3,7 +3,6 @@ require 'spec_helper' describe SystemNoteService do - include ProjectForksHelper include Gitlab::Routing include RepoHelpers include AssetsHelpers @@ -14,270 +13,65 @@ describe SystemNoteService do let(:noteable) { create(:issue, project: project) } let(:issue) { noteable } - shared_examples_for 'a note with overridable created_at' do - let(:noteable) { create(:issue, project: project, system_note_timestamp: Time.at(42)) } - - it 'the note has the correct time' do - expect(subject.created_at).to eq Time.at(42) - end - end - - shared_examples_for 'a system note' do - let(:expected_noteable) { noteable } - let(:commit_count) { nil } - - it 'has the correct attributes', :aggregate_failures do - expect(subject).to be_valid - expect(subject).to be_system - - expect(subject.noteable).to eq expected_noteable - expect(subject.project).to eq project - expect(subject.author).to eq author - - expect(subject.system_note_metadata.action).to eq(action) - expect(subject.system_note_metadata.commit_count).to eq(commit_count) - end - end - describe '.add_commits' do - subject { described_class.add_commits(noteable, project, author, new_commits, old_commits, oldrev) } - - let(:noteable) { create(:merge_request, source_project: project, target_project: project) } - let(:new_commits) { noteable.commits } - let(:old_commits) { [] } - let(:oldrev) { nil } - - it_behaves_like 'a system note' do - let(:commit_count) { new_commits.size } - let(:action) { 'commit' } - end - - describe 'note body' do - let(:note_lines) { subject.note.split("\n").reject(&:blank?) } - - describe 'comparison diff link line' do - it 'adds the comparison text' do - expect(note_lines[2]).to match "[Compare with previous version]" - end - end - - context 'without existing commits' do - it 'adds a message header' do - expect(note_lines[0]).to eq "added #{new_commits.size} commits" - end + let(:new_commits) { double } + let(:old_commits) { double } + let(:oldrev) { double } - it 'adds a message for each commit' do - decoded_note_content = HTMLEntities.new.decode(subject.note) - - new_commits.each do |commit| - expect(decoded_note_content).to include("<li>#{commit.short_id} - #{commit.title}</li>") - end - end + it 'calls CommitService' do + expect_next_instance_of(::SystemNotes::CommitService) do |service| + expect(service).to receive(:add_commits).with(new_commits, old_commits, oldrev) end - describe 'summary line for existing commits' do - let(:summary_line) { note_lines[1] } - - context 'with one existing commit' do - let(:old_commits) { [noteable.commits.last] } - - it 'includes the existing commit' do - expect(summary_line).to start_with("<ul><li>#{old_commits.first.short_id} - 1 commit from branch <code>feature</code>") - end - end - - context 'with multiple existing commits' do - let(:old_commits) { noteable.commits[3..-1] } - - context 'with oldrev' do - let(:oldrev) { noteable.commits[2].id } - - it 'includes a commit range and count' do - expect(summary_line) - .to start_with("<ul><li>#{Commit.truncate_sha(oldrev)}...#{old_commits.last.short_id} - 26 commits from branch <code>feature</code>") - end - end - - context 'without oldrev' do - it 'includes a commit range and count' do - expect(summary_line) - .to start_with("<ul><li>#{old_commits[0].short_id}..#{old_commits[-1].short_id} - 26 commits from branch <code>feature</code>") - end - end - - context 'on a fork' do - before do - expect(noteable).to receive(:for_fork?).and_return(true) - end - - it 'includes the project namespace' do - expect(summary_line).to include("<code>#{noteable.target_project_namespace}:feature</code>") - end - end - end - end + described_class.add_commits(noteable, project, author, new_commits, old_commits, oldrev) end end describe '.tag_commit' do - let(:noteable) do - project.commit - end - let(:tag_name) { 'v1.2.3' } - - subject { described_class.tag_commit(noteable, project, author, tag_name) } - - it_behaves_like 'a system note' do - let(:action) { 'tag' } - end + let(:tag_name) { double } - it 'sets the note text' do - link = "/#{project.full_path}/-/tags/#{tag_name}" + it 'calls CommitService' do + expect_next_instance_of(::SystemNotes::CommitService) do |service| + expect(service).to receive(:tag_commit).with(tag_name) + end - expect(subject.note).to eq "tagged commit #{noteable.sha} to [`#{tag_name}`](#{link})" + described_class.tag_commit(noteable, project, author, tag_name) end end describe '.change_assignee' do - subject { described_class.change_assignee(noteable, project, author, assignee) } - - let(:assignee) { create(:user) } + let(:assignee) { double } - it_behaves_like 'a system note' do - let(:action) { 'assignee' } - end - - context 'when assignee added' do - it_behaves_like 'a note with overridable created_at' - - it 'sets the note text' do - expect(subject.note).to eq "assigned to @#{assignee.username}" + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_assignee).with(assignee) end - end - context 'when assignee removed' do - let(:assignee) { nil } - - it_behaves_like 'a note with overridable created_at' - - it 'sets the note text' do - expect(subject.note).to eq 'removed assignee' - end + described_class.change_assignee(noteable, project, author, assignee) end end describe '.change_issuable_assignees' do - subject { described_class.change_issuable_assignees(noteable, project, author, [assignee]) } - - let(:assignee) { create(:user) } - let(:assignee1) { create(:user) } - let(:assignee2) { create(:user) } - let(:assignee3) { create(:user) } - - it_behaves_like 'a system note' do - let(:action) { 'assignee' } - end + let(:assignees) { [double, double] } - def build_note(old_assignees, new_assignees) - issue.assignees = new_assignees - described_class.change_issuable_assignees(issue, project, author, old_assignees).note - end - - it_behaves_like 'a note with overridable created_at' - - it 'builds a correct phrase when an assignee is added to a non-assigned issue' do - expect(build_note([], [assignee1])).to eq "assigned to @#{assignee1.username}" - end - - it 'builds a correct phrase when assignee removed' do - expect(build_note([assignee1], [])).to eq "unassigned @#{assignee1.username}" - end - - it 'builds a correct phrase when assignees changed' do - expect(build_note([assignee1], [assignee2])).to eq \ - "assigned to @#{assignee2.username} and unassigned @#{assignee1.username}" - end - - it 'builds a correct phrase when three assignees removed and one added' do - expect(build_note([assignee, assignee1, assignee2], [assignee3])).to eq \ - "assigned to @#{assignee3.username} and unassigned @#{assignee.username}, @#{assignee1.username}, and @#{assignee2.username}" - end - - it 'builds a correct phrase when one assignee changed from a set' do - expect(build_note([assignee, assignee1], [assignee, assignee2])).to eq \ - "assigned to @#{assignee2.username} and unassigned @#{assignee1.username}" - end - - it 'builds a correct phrase when one assignee removed from a set' do - expect(build_note([assignee, assignee1, assignee2], [assignee, assignee1])).to eq \ - "unassigned @#{assignee2.username}" - end - - it 'builds a correct phrase when the locale is different' do - Gitlab::I18n.with_locale('pt-BR') do - expect(build_note([assignee, assignee1, assignee2], [assignee3])).to eq \ - "assigned to @#{assignee3.username} and unassigned @#{assignee.username}, @#{assignee1.username}, and @#{assignee2.username}" + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_issuable_assignees).with(assignees) end + + described_class.change_issuable_assignees(noteable, project, author, assignees) end end describe '.change_milestone' do - context 'for a project milestone' do - subject { described_class.change_milestone(noteable, project, author, milestone) } + let(:milestone) { double } - let(:milestone) { create(:milestone, project: project) } - - it_behaves_like 'a system note' do - let(:action) { 'milestone' } - end - - context 'when milestone added' do - it 'sets the note text' do - reference = milestone.to_reference(format: :iid) - - expect(subject.note).to eq "changed milestone to #{reference}" - end - - it_behaves_like 'a note with overridable created_at' - end - - context 'when milestone removed' do - let(:milestone) { nil } - - it 'sets the note text' do - expect(subject.note).to eq 'removed milestone' - end - - it_behaves_like 'a note with overridable created_at' - end - end - - context 'for a group milestone' do - subject { described_class.change_milestone(noteable, project, author, milestone) } - - let(:milestone) { create(:milestone, group: group) } - - it_behaves_like 'a system note' do - let(:action) { 'milestone' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_milestone).with(milestone) end - context 'when milestone added' do - it 'sets the note text to use the milestone name' do - expect(subject.note).to eq "changed milestone to #{milestone.to_reference(format: :name)}" - end - - it_behaves_like 'a note with overridable created_at' - end - - context 'when milestone removed' do - let(:milestone) { nil } - - it 'sets the note text' do - expect(subject.note).to eq 'removed milestone' - end - - it_behaves_like 'a note with overridable created_at' - end + described_class.change_milestone(noteable, project, author, milestone) end end @@ -308,28 +102,15 @@ describe SystemNoteService do end describe '.change_status' do - subject { described_class.change_status(noteable, project, author, status, source) } - - context 'with status reopened' do - let(:status) { 'reopened' } - let(:source) { nil } + let(:status) { double } + let(:source) { double } - it_behaves_like 'a note with overridable created_at' - - it_behaves_like 'a system note' do - let(:action) { 'opened' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_status).with(status, source) end - end - - context 'with a source' do - let(:status) { 'opened' } - let(:source) { double('commit', gfm_reference: 'commit 123456') } - it_behaves_like 'a note with overridable created_at' - - it 'sets the note text' do - expect(subject.note).to eq "#{status} via commit 123456" - end + described_class.change_status(noteable, project, author, status, source) end end @@ -383,65 +164,34 @@ describe SystemNoteService do end describe '.change_title' do - let(:noteable) { create(:issue, project: project, title: 'Lorem ipsum') } + let(:title) { double } - subject { described_class.change_title(noteable, project, author, 'Old title') } - - context 'when noteable responds to `title`' do - it_behaves_like 'a system note' do - let(:action) { 'title' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_title).with(title) end - it_behaves_like 'a note with overridable created_at' - - it 'sets the note text' do - expect(subject.note) - .to eq "changed title from **{-Old title-}** to **{+Lorem ipsum+}**" - end + described_class.change_title(noteable, project, author, title) end end describe '.change_description' do - subject { described_class.change_description(noteable, project, author) } - - context 'when noteable responds to `description`' do - it_behaves_like 'a system note' do - let(:action) { 'description' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_description) end - it_behaves_like 'a note with overridable created_at' - - it 'sets the note text' do - expect(subject.note).to eq('changed the description') - end + described_class.change_description(noteable, project, author) end end describe '.change_issue_confidentiality' do - subject { described_class.change_issue_confidentiality(noteable, project, author) } - - context 'issue has been made confidential' do - before do - noteable.update_attribute(:confidential, true) - end - - it_behaves_like 'a system note' do - let(:action) { 'confidential' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_issue_confidentiality) end - it 'sets the note text' do - expect(subject.note).to eq 'made the issue confidential' - end - end - - context 'issue has been made visible' do - it_behaves_like 'a system note' do - let(:action) { 'visible' } - end - - it 'sets the note text' do - expect(subject.note).to eq 'made the issue visible to everyone' - end + described_class.change_issue_confidentiality(noteable, project, author) end end @@ -521,295 +271,71 @@ describe SystemNoteService do end describe '.zoom_link_added' do - subject { described_class.zoom_link_added(issue, project, author) } - - it_behaves_like 'a system note' do - let(:action) { 'pinned_embed' } - end + it 'calls ZoomService' do + expect_next_instance_of(::SystemNotes::ZoomService) do |service| + expect(service).to receive(:zoom_link_added) + end - it 'sets the zoom link added note text' do - expect(subject.note).to eq('added a Zoom call to this issue') + described_class.zoom_link_added(noteable, project, author) end end describe '.zoom_link_removed' do - subject { described_class.zoom_link_removed(issue, project, author) } - - it_behaves_like 'a system note' do - let(:action) { 'pinned_embed' } - end + it 'calls ZoomService' do + expect_next_instance_of(::SystemNotes::ZoomService) do |service| + expect(service).to receive(:zoom_link_removed) + end - it 'sets the zoom link removed note text' do - expect(subject.note).to eq('removed a Zoom call from this issue') + described_class.zoom_link_removed(noteable, project, author) end end describe '.cross_reference' do - subject { described_class.cross_reference(noteable, mentioner, author) } - - let(:mentioner) { create(:issue, project: project) } - - it_behaves_like 'a system note' do - let(:action) { 'cross_reference' } - end - - context 'when cross-reference disallowed' do - before do - expect(described_class).to receive(:cross_reference_disallowed?).and_return(true) - end - - it 'returns nil' do - expect(subject).to be_nil - end - - it 'does not create a system note metadata record' do - expect { subject }.not_to change { SystemNoteMetadata.count } - end - end - - context 'when cross-reference allowed' do - before do - expect(described_class).to receive(:cross_reference_disallowed?).and_return(false) - end + let(:mentioner) { double } - it_behaves_like 'a system note' do - let(:action) { 'cross_reference' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:cross_reference).with(mentioner) end - it_behaves_like 'a note with overridable created_at' - - describe 'note_body' do - context 'cross-project' do - let(:project2) { create(:project, :repository) } - let(:mentioner) { create(:issue, project: project2) } - - context 'from Commit' do - let(:mentioner) { project2.repository.commit } - - it 'references the mentioning commit' do - expect(subject.note).to eq "mentioned in commit #{mentioner.to_reference(project)}" - end - end - - context 'from non-Commit' do - it 'references the mentioning object' do - expect(subject.note).to eq "mentioned in issue #{mentioner.to_reference(project)}" - end - end - end - - context 'within the same project' do - context 'from Commit' do - let(:mentioner) { project.repository.commit } - - it 'references the mentioning commit' do - expect(subject.note).to eq "mentioned in commit #{mentioner.to_reference}" - end - end - - context 'from non-Commit' do - it 'references the mentioning object' do - expect(subject.note).to eq "mentioned in issue #{mentioner.to_reference}" - end - end - end - end + described_class.cross_reference(double, mentioner, double) end end describe '.cross_reference_disallowed?' do - context 'when mentioner is not a MergeRequest' do - it 'is falsey' do - mentioner = noteable.dup - expect(described_class.cross_reference_disallowed?(noteable, mentioner)) - .to be_falsey - end - end + let(:mentioner) { double } - context 'when mentioner is a MergeRequest' do - let(:mentioner) { create(:merge_request, :simple, source_project: project) } - let(:noteable) { project.commit } - - it 'is truthy when noteable is in commits' do - expect(mentioner).to receive(:commits).and_return([noteable]) - expect(described_class.cross_reference_disallowed?(noteable, mentioner)) - .to be_truthy - end - - it 'is falsey when noteable is not in commits' do - expect(mentioner).to receive(:commits).and_return([]) - expect(described_class.cross_reference_disallowed?(noteable, mentioner)) - .to be_falsey + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:cross_reference_disallowed?).with(mentioner) end - end - context 'when notable is an ExternalIssue' do - let(:noteable) { ExternalIssue.new('EXT-1234', project) } - it 'is truthy' do - mentioner = noteable.dup - expect(described_class.cross_reference_disallowed?(noteable, mentioner)) - .to be_truthy - end + described_class.cross_reference_disallowed?(double, mentioner) end end describe '.cross_reference_exists?' do - let(:commit0) { project.commit } - let(:commit1) { project.commit('HEAD~2') } - - context 'issue from commit' do - before do - # Mention issue (noteable) from commit0 - described_class.cross_reference(noteable, commit0, author) - end - - it 'is truthy when already mentioned' do - expect(described_class.cross_reference_exists?(noteable, commit0)) - .to be_truthy - end - - it 'is falsey when not already mentioned' do - expect(described_class.cross_reference_exists?(noteable, commit1)) - .to be_falsey - end - - context 'legacy capitalized cross reference' do - before do - # Mention issue (noteable) from commit0 - system_note = described_class.cross_reference(noteable, commit0, author) - system_note.update(note: system_note.note.capitalize) - end - - it 'is truthy when already mentioned' do - expect(described_class.cross_reference_exists?(noteable, commit0)) - .to be_truthy - end - end - end - - context 'commit from commit' do - before do - # Mention commit1 from commit0 - described_class.cross_reference(commit0, commit1, author) - end - - it 'is truthy when already mentioned' do - expect(described_class.cross_reference_exists?(commit0, commit1)) - .to be_truthy - end + let(:mentioner) { double } - it 'is falsey when not already mentioned' do - expect(described_class.cross_reference_exists?(commit1, commit0)) - .to be_falsey + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:cross_reference_exists?).with(mentioner) end - context 'legacy capitalized cross reference' do - before do - # Mention commit1 from commit0 - system_note = described_class.cross_reference(commit0, commit1, author) - system_note.update(note: system_note.note.capitalize) - end - - it 'is truthy when already mentioned' do - expect(described_class.cross_reference_exists?(commit0, commit1)) - .to be_truthy - end - end - end - - context 'commit with cross-reference from fork' do - let(:author2) { create(:project_member, :reporter, user: create(:user), project: project).user } - let(:forked_project) { fork_project(project, author2, repository: true) } - let(:commit2) { forked_project.commit } - - before do - described_class.cross_reference(noteable, commit0, author2) - end - - it 'is true when a fork mentions an external issue' do - expect(described_class.cross_reference_exists?(noteable, commit2)) - .to be true - end - - context 'legacy capitalized cross reference' do - before do - system_note = described_class.cross_reference(noteable, commit0, author2) - system_note.update(note: system_note.note.capitalize) - end - - it 'is true when a fork mentions an external issue' do - expect(described_class.cross_reference_exists?(noteable, commit2)) - .to be true - end - end + described_class.cross_reference_exists?(double, mentioner) end end describe '.noteable_moved' do - let(:new_project) { create(:project) } - let(:new_noteable) { create(:issue, project: new_project) } - - subject do - described_class.noteable_moved(noteable, project, new_noteable, author, direction: direction) - end - - shared_examples 'cross project mentionable' do - include MarkupHelper - - it 'contains cross reference to new noteable' do - expect(subject.note).to include cross_project_reference(new_project, new_noteable) - end - - it 'mentions referenced noteable' do - expect(subject.note).to include new_noteable.to_reference - end - - it 'mentions referenced project' do - expect(subject.note).to include new_project.full_path - end - end - - context 'moved to' do - let(:direction) { :to } - - it_behaves_like 'cross project mentionable' - it_behaves_like 'a system note' do - let(:action) { 'moved' } - end + let(:noteable_ref) { double } + let(:direction) { double } - it 'notifies about noteable being moved to' do - expect(subject.note).to match('moved to') + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:noteable_moved).with(noteable_ref, direction) end - end - context 'moved from' do - let(:direction) { :from } - - it_behaves_like 'cross project mentionable' - it_behaves_like 'a system note' do - let(:action) { 'moved' } - end - - it 'notifies about noteable being moved from' do - expect(subject.note).to match('moved from') - end - end - - context 'invalid direction' do - let(:direction) { :invalid } - - it 'raises error' do - expect { subject }.to raise_error StandardError, /Invalid direction/ - end - end - end - - describe '.new_commit_summary' do - it 'escapes HTML titles' do - commit = double(title: '<pre>This is a test</pre>', short_id: '12345678') - escaped = '<pre>This is a test</pre>' - - expect(described_class.new_commit_summary([commit])).to all(match(/- #{escaped}/)) + described_class.noteable_moved(double, double, noteable_ref, double, direction: direction) end end @@ -1171,17 +697,14 @@ describe SystemNoteService do end describe '.change_task_status' do - let(:noteable) { create(:issue, project: project) } - let(:task) { double(:task, complete?: true, source: 'task') } - - subject { described_class.change_task_status(noteable, project, author, task) } + let(:new_task) { double } - it_behaves_like 'a system note' do - let(:action) { 'task' } - end + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_task_status).with(new_task) + end - it "posts the 'marked the task as complete' system note" do - expect(subject.note).to eq("marked the task **task** as completed") + described_class.change_task_status(noteable, project, author, new_task) end end @@ -1259,90 +782,42 @@ describe SystemNoteService do end describe '.mark_duplicate_issue' do - subject { described_class.mark_duplicate_issue(noteable, project, author, canonical_issue) } + let(:canonical_issue) { double } - context 'within the same project' do - let(:canonical_issue) { create(:issue, project: project) } - - it_behaves_like 'a system note' do - let(:action) { 'duplicate' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:mark_duplicate_issue).with(canonical_issue) end - it { expect(subject.note).to eq "marked this issue as a duplicate of #{canonical_issue.to_reference}" } - end - - context 'across different projects' do - let(:other_project) { create(:project) } - let(:canonical_issue) { create(:issue, project: other_project) } - - it_behaves_like 'a system note' do - let(:action) { 'duplicate' } - end - - it { expect(subject.note).to eq "marked this issue as a duplicate of #{canonical_issue.to_reference(project)}" } + described_class.mark_duplicate_issue(noteable, project, author, canonical_issue) end end describe '.mark_canonical_issue_of_duplicate' do - subject { described_class.mark_canonical_issue_of_duplicate(noteable, project, author, duplicate_issue) } + let(:duplicate_issue) { double } - context 'within the same project' do - let(:duplicate_issue) { create(:issue, project: project) } - - it_behaves_like 'a system note' do - let(:action) { 'duplicate' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:mark_canonical_issue_of_duplicate).with(duplicate_issue) end - it { expect(subject.note).to eq "marked #{duplicate_issue.to_reference} as a duplicate of this issue" } - end - - context 'across different projects' do - let(:other_project) { create(:project) } - let(:duplicate_issue) { create(:issue, project: other_project) } - - it_behaves_like 'a system note' do - let(:action) { 'duplicate' } - end - - it { expect(subject.note).to eq "marked #{duplicate_issue.to_reference(project)} as a duplicate of this issue" } + described_class.mark_canonical_issue_of_duplicate(noteable, project, author, duplicate_issue) end end describe '.discussion_lock' do - subject { described_class.discussion_lock(noteable, author) } - - context 'discussion unlocked' do - it_behaves_like 'a system note' do - let(:action) { 'unlocked' } - end - - it 'creates the note text correctly' do - [:issue, :merge_request].each do |type| - issuable = create(type) + let(:issuable) { double } - expect(described_class.discussion_lock(issuable, author).note) - .to eq("unlocked this #{type.to_s.titleize.downcase}") - end - end + before do + allow(issuable).to receive(:project).and_return(double) end - context 'discussion locked' do - before do - noteable.update_attribute(:discussion_locked, true) - end - - it_behaves_like 'a system note' do - let(:action) { 'locked' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:discussion_lock) end - it 'creates the note text correctly' do - [:issue, :merge_request].each do |type| - issuable = create(type, discussion_locked: true) - - expect(described_class.discussion_lock(issuable, author).note) - .to eq("locked this #{type.to_s.titleize.downcase}") - end - end + described_class.discussion_lock(issuable, double) end end end diff --git a/spec/services/system_notes/base_service_spec.rb b/spec/services/system_notes/base_service_spec.rb new file mode 100644 index 00000000000..96788b05829 --- /dev/null +++ b/spec/services/system_notes/base_service_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe SystemNotes::BaseService do + let(:noteable) { double } + let(:project) { double } + let(:author) { double } + + let(:base_service) { described_class.new(noteable: noteable, project: project, author: author) } + + describe '#noteable' do + subject { base_service.noteable } + + it { is_expected.to eq(noteable) } + + it 'returns nil if no arguments are given' do + instance = described_class.new + expect(instance.noteable).to be_nil + end + end + + describe '#project' do + subject { base_service.project } + + it { is_expected.to eq(project) } + + it 'returns nil if no arguments are given' do + instance = described_class.new + expect(instance.project).to be_nil + end + end + + describe '#author' do + subject { base_service.author } + + it { is_expected.to eq(author) } + + it 'returns nil if no arguments are given' do + instance = described_class.new + expect(instance.author).to be_nil + end + end +end diff --git a/spec/services/system_notes/commit_service_spec.rb b/spec/services/system_notes/commit_service_spec.rb new file mode 100644 index 00000000000..4d4403be59a --- /dev/null +++ b/spec/services/system_notes/commit_service_spec.rb @@ -0,0 +1,117 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe SystemNotes::CommitService do + set(:group) { create(:group) } + set(:project) { create(:project, :repository, group: group) } + set(:author) { create(:user) } + + let(:commit_service) { described_class.new(noteable: noteable, project: project, author: author) } + + describe '#add_commits' do + subject { commit_service.add_commits(new_commits, old_commits, oldrev) } + + let(:noteable) { create(:merge_request, source_project: project, target_project: project) } + let(:new_commits) { noteable.commits } + let(:old_commits) { [] } + let(:oldrev) { nil } + + it_behaves_like 'a system note' do + let(:commit_count) { new_commits.size } + let(:action) { 'commit' } + end + + describe 'note body' do + let(:note_lines) { subject.note.split("\n").reject(&:blank?) } + + describe 'comparison diff link line' do + it 'adds the comparison text' do + expect(note_lines[2]).to match "[Compare with previous version]" + end + end + + context 'without existing commits' do + it 'adds a message header' do + expect(note_lines[0]).to eq "added #{new_commits.size} commits" + end + + it 'adds a message for each commit' do + decoded_note_content = HTMLEntities.new.decode(subject.note) + + new_commits.each do |commit| + expect(decoded_note_content).to include("<li>#{commit.short_id} - #{commit.title}</li>") + end + end + end + + describe 'summary line for existing commits' do + let(:summary_line) { note_lines[1] } + + context 'with one existing commit' do + let(:old_commits) { [noteable.commits.last] } + + it 'includes the existing commit' do + expect(summary_line).to start_with("<ul><li>#{old_commits.first.short_id} - 1 commit from branch <code>feature</code>") + end + end + + context 'with multiple existing commits' do + let(:old_commits) { noteable.commits[3..-1] } + + context 'with oldrev' do + let(:oldrev) { noteable.commits[2].id } + + it 'includes a commit range and count' do + expect(summary_line) + .to start_with("<ul><li>#{Commit.truncate_sha(oldrev)}...#{old_commits.last.short_id} - 26 commits from branch <code>feature</code>") + end + end + + context 'without oldrev' do + it 'includes a commit range and count' do + expect(summary_line) + .to start_with("<ul><li>#{old_commits[0].short_id}..#{old_commits[-1].short_id} - 26 commits from branch <code>feature</code>") + end + end + + context 'on a fork' do + before do + expect(noteable).to receive(:for_fork?).and_return(true) + end + + it 'includes the project namespace' do + expect(summary_line).to include("<code>#{noteable.target_project_namespace}:feature</code>") + end + end + end + end + end + end + + describe '#tag_commit' do + let(:noteable) { project.commit } + let(:tag_name) { 'v1.2.3' } + + subject { commit_service.tag_commit(tag_name) } + + it_behaves_like 'a system note' do + let(:action) { 'tag' } + end + + it 'sets the note text' do + link = "/#{project.full_path}/-/tags/#{tag_name}" + + expect(subject.note).to eq "tagged commit #{noteable.sha} to [`#{tag_name}`](#{link})" + end + end + + describe '#new_commit_summary' do + it 'escapes HTML titles' do + commit = double(title: '<pre>This is a test</pre>', short_id: '12345678') + escaped = '<pre>This is a test</pre>' + + expect(described_class.new.new_commit_summary([commit])).to all(match(/- #{escaped}/)) + end + end +end diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb new file mode 100644 index 00000000000..5023abad4cd --- /dev/null +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -0,0 +1,628 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ::SystemNotes::IssuablesService do + include ProjectForksHelper + + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :repository, group: group) } + let_it_be(:author) { create(:user) } + let(:noteable) { create(:issue, project: project) } + let(:issue) { noteable } + + let(:service) { described_class.new(noteable: noteable, project: project, author: author) } + + describe '#change_assignee' do + subject { service.change_assignee(assignee) } + + let(:assignee) { create(:user) } + + it_behaves_like 'a system note' do + let(:action) { 'assignee' } + end + + context 'when assignee added' do + it_behaves_like 'a note with overridable created_at' + + it 'sets the note text' do + expect(subject.note).to eq "assigned to @#{assignee.username}" + end + end + + context 'when assignee removed' do + let(:assignee) { nil } + + it_behaves_like 'a note with overridable created_at' + + it 'sets the note text' do + expect(subject.note).to eq 'removed assignee' + end + end + end + + describe '#change_issuable_assignees' do + subject { service.change_issuable_assignees([assignee]) } + + let(:assignee) { create(:user) } + let(:assignee1) { create(:user) } + let(:assignee2) { create(:user) } + let(:assignee3) { create(:user) } + + it_behaves_like 'a system note' do + let(:action) { 'assignee' } + end + + def build_note(old_assignees, new_assignees) + issue.assignees = new_assignees + service.change_issuable_assignees(old_assignees).note + end + + it_behaves_like 'a note with overridable created_at' + + it 'builds a correct phrase when an assignee is added to a non-assigned issue' do + expect(build_note([], [assignee1])).to eq "assigned to @#{assignee1.username}" + end + + it 'builds a correct phrase when assignee removed' do + expect(build_note([assignee1], [])).to eq "unassigned @#{assignee1.username}" + end + + it 'builds a correct phrase when assignees changed' do + expect(build_note([assignee1], [assignee2])).to eq \ + "assigned to @#{assignee2.username} and unassigned @#{assignee1.username}" + end + + it 'builds a correct phrase when three assignees removed and one added' do + expect(build_note([assignee, assignee1, assignee2], [assignee3])).to eq \ + "assigned to @#{assignee3.username} and unassigned @#{assignee.username}, @#{assignee1.username}, and @#{assignee2.username}" + end + + it 'builds a correct phrase when one assignee changed from a set' do + expect(build_note([assignee, assignee1], [assignee, assignee2])).to eq \ + "assigned to @#{assignee2.username} and unassigned @#{assignee1.username}" + end + + it 'builds a correct phrase when one assignee removed from a set' do + expect(build_note([assignee, assignee1, assignee2], [assignee, assignee1])).to eq \ + "unassigned @#{assignee2.username}" + end + + it 'builds a correct phrase when the locale is different' do + Gitlab::I18n.with_locale('pt-BR') do + expect(build_note([assignee, assignee1, assignee2], [assignee3])).to eq \ + "assigned to @#{assignee3.username} and unassigned @#{assignee.username}, @#{assignee1.username}, and @#{assignee2.username}" + end + end + end + + describe '#change_milestone' do + subject { service.change_milestone(milestone) } + + context 'for a project milestone' do + let(:milestone) { create(:milestone, project: project) } + + it_behaves_like 'a system note' do + let(:action) { 'milestone' } + end + + context 'when milestone added' do + it 'sets the note text' do + reference = milestone.to_reference(format: :iid) + + expect(subject.note).to eq "changed milestone to #{reference}" + end + + it_behaves_like 'a note with overridable created_at' + end + + context 'when milestone removed' do + let(:milestone) { nil } + + it 'sets the note text' do + expect(subject.note).to eq 'removed milestone' + end + + it_behaves_like 'a note with overridable created_at' + end + end + + context 'for a group milestone' do + let(:milestone) { create(:milestone, group: group) } + + it_behaves_like 'a system note' do + let(:action) { 'milestone' } + end + + context 'when milestone added' do + it 'sets the note text to use the milestone name' do + expect(subject.note).to eq "changed milestone to #{milestone.to_reference(format: :name)}" + end + + it_behaves_like 'a note with overridable created_at' + end + + context 'when milestone removed' do + let(:milestone) { nil } + + it 'sets the note text' do + expect(subject.note).to eq 'removed milestone' + end + + it_behaves_like 'a note with overridable created_at' + end + end + end + + describe '#change_status' do + subject { service.change_status(status, source) } + + context 'with status reopened' do + let(:status) { 'reopened' } + let(:source) { nil } + + it_behaves_like 'a note with overridable created_at' + + it_behaves_like 'a system note' do + let(:action) { 'opened' } + end + end + + context 'with a source' do + let(:status) { 'opened' } + let(:source) { double('commit', gfm_reference: 'commit 123456') } + + it_behaves_like 'a note with overridable created_at' + + it 'sets the note text' do + expect(subject.note).to eq "#{status} via commit 123456" + end + end + end + + describe '#change_title' do + let(:noteable) { create(:issue, project: project, title: 'Lorem ipsum') } + + subject { service.change_title('Old title') } + + context 'when noteable responds to `title`' do + it_behaves_like 'a system note' do + let(:action) { 'title' } + end + + it_behaves_like 'a note with overridable created_at' + + it 'sets the note text' do + expect(subject.note) + .to eq "changed title from **{-Old title-}** to **{+Lorem ipsum+}**" + end + end + end + + describe '#change_description' do + subject { service.change_description } + + context 'when noteable responds to `description`' do + it_behaves_like 'a system note' do + let(:action) { 'description' } + end + + it_behaves_like 'a note with overridable created_at' + + it 'sets the note text' do + expect(subject.note).to eq('changed the description') + end + + it 'associates the related description version' do + noteable.update!(description: 'New description') + + description_version_id = subject.system_note_metadata.description_version_id + + expect(description_version_id).not_to be_nil + expect(description_version_id).to eq(noteable.saved_description_version.id) + end + end + end + + describe '#change_issue_confidentiality' do + subject { service.change_issue_confidentiality } + + context 'issue has been made confidential' do + before do + noteable.update_attribute(:confidential, true) + end + + it_behaves_like 'a system note' do + let(:action) { 'confidential' } + end + + it 'sets the note text' do + expect(subject.note).to eq 'made the issue confidential' + end + end + + context 'issue has been made visible' do + it_behaves_like 'a system note' do + let(:action) { 'visible' } + end + + it 'sets the note text' do + expect(subject.note).to eq 'made the issue visible to everyone' + end + end + end + + describe '#cross_reference' do + let(:service) { described_class.new(noteable: noteable, author: author) } + + let(:mentioner) { create(:issue, project: project) } + + subject { service.cross_reference(mentioner) } + + it_behaves_like 'a system note' do + let(:action) { 'cross_reference' } + end + + context 'when cross-reference disallowed' do + before do + expect_any_instance_of(described_class).to receive(:cross_reference_disallowed?).and_return(true) + end + + it 'returns nil' do + expect(subject).to be_nil + end + + it 'does not create a system note metadata record' do + expect { subject }.not_to change { SystemNoteMetadata.count } + end + end + + context 'when cross-reference allowed' do + before do + expect_any_instance_of(described_class).to receive(:cross_reference_disallowed?).and_return(false) + end + + it_behaves_like 'a system note' do + let(:action) { 'cross_reference' } + end + + it_behaves_like 'a note with overridable created_at' + + describe 'note_body' do + context 'cross-project' do + let(:project2) { create(:project, :repository) } + let(:mentioner) { create(:issue, project: project2) } + + context 'from Commit' do + let(:mentioner) { project2.repository.commit } + + it 'references the mentioning commit' do + expect(subject.note).to eq "mentioned in commit #{mentioner.to_reference(project)}" + end + end + + context 'from non-Commit' do + it 'references the mentioning object' do + expect(subject.note).to eq "mentioned in issue #{mentioner.to_reference(project)}" + end + end + end + + context 'within the same project' do + context 'from Commit' do + let(:mentioner) { project.repository.commit } + + it 'references the mentioning commit' do + expect(subject.note).to eq "mentioned in commit #{mentioner.to_reference}" + end + end + + context 'from non-Commit' do + it 'references the mentioning object' do + expect(subject.note).to eq "mentioned in issue #{mentioner.to_reference}" + end + end + end + end + end + end + + describe '#cross_reference_exists?' do + let(:commit0) { project.commit } + let(:commit1) { project.commit('HEAD~2') } + + context 'issue from commit' do + before do + # Mention issue (noteable) from commit0 + service.cross_reference(commit0) + end + + it 'is truthy when already mentioned' do + expect(service.cross_reference_exists?(commit0)) + .to be_truthy + end + + it 'is falsey when not already mentioned' do + expect(service.cross_reference_exists?(commit1)) + .to be_falsey + end + + context 'legacy capitalized cross reference' do + before do + # Mention issue (noteable) from commit0 + system_note = service.cross_reference(commit0) + system_note.update(note: system_note.note.capitalize) + end + + it 'is truthy when already mentioned' do + expect(service.cross_reference_exists?(commit0)) + .to be_truthy + end + end + end + + context 'commit from commit' do + let(:service) { described_class.new(noteable: commit0, author: author) } + + before do + # Mention commit1 from commit0 + service.cross_reference(commit1) + end + + it 'is truthy when already mentioned' do + expect(service.cross_reference_exists?(commit1)) + .to be_truthy + end + + it 'is falsey when not already mentioned' do + service = described_class.new(noteable: commit1, author: author) + + expect(service.cross_reference_exists?(commit0)) + .to be_falsey + end + + context 'legacy capitalized cross reference' do + before do + # Mention commit1 from commit0 + system_note = service.cross_reference(commit1) + system_note.update(note: system_note.note.capitalize) + end + + it 'is truthy when already mentioned' do + expect(service.cross_reference_exists?(commit1)) + .to be_truthy + end + end + end + + context 'commit with cross-reference from fork' do + let(:author2) { create(:project_member, :reporter, user: create(:user), project: project).user } + let(:forked_project) { fork_project(project, author2, repository: true) } + let(:commit2) { forked_project.commit } + + let(:service) { described_class.new(noteable: noteable, author: author2) } + + before do + service.cross_reference(commit0) + end + + it 'is true when a fork mentions an external issue' do + expect(service.cross_reference_exists?(commit2)) + .to be true + end + + context 'legacy capitalized cross reference' do + before do + system_note = service.cross_reference(commit0) + system_note.update(note: system_note.note.capitalize) + end + + it 'is true when a fork mentions an external issue' do + expect(service.cross_reference_exists?(commit2)) + .to be true + end + end + end + end + + describe '#change_task_status' do + let(:noteable) { create(:issue, project: project) } + let(:task) { double(:task, complete?: true, source: 'task') } + + subject { service.change_task_status(task) } + + it_behaves_like 'a system note' do + let(:action) { 'task' } + end + + it "posts the 'marked the task as complete' system note" do + expect(subject.note).to eq("marked the task **task** as completed") + end + end + + describe '#noteable_moved' do + let(:new_project) { create(:project) } + let(:new_noteable) { create(:issue, project: new_project) } + + subject do + # service = described_class.new(noteable: noteable, project: project, author: author) + service.noteable_moved(new_noteable, direction) + end + + shared_examples 'cross project mentionable' do + include MarkupHelper + + it 'contains cross reference to new noteable' do + expect(subject.note).to include cross_project_reference(new_project, new_noteable) + end + + it 'mentions referenced noteable' do + expect(subject.note).to include new_noteable.to_reference + end + + it 'mentions referenced project' do + expect(subject.note).to include new_project.full_path + end + end + + context 'moved to' do + let(:direction) { :to } + + it_behaves_like 'cross project mentionable' + it_behaves_like 'a system note' do + let(:action) { 'moved' } + end + + it 'notifies about noteable being moved to' do + expect(subject.note).to match('moved to') + end + end + + context 'moved from' do + let(:direction) { :from } + + it_behaves_like 'cross project mentionable' + it_behaves_like 'a system note' do + let(:action) { 'moved' } + end + + it 'notifies about noteable being moved from' do + expect(subject.note).to match('moved from') + end + end + + context 'invalid direction' do + let(:direction) { :invalid } + + it 'raises error' do + expect { subject }.to raise_error StandardError, /Invalid direction/ + end + end + end + + describe '#mark_duplicate_issue' do + subject { service.mark_duplicate_issue(canonical_issue) } + + context 'within the same project' do + let(:canonical_issue) { create(:issue, project: project) } + + it_behaves_like 'a system note' do + let(:action) { 'duplicate' } + end + + it { expect(subject.note).to eq "marked this issue as a duplicate of #{canonical_issue.to_reference}" } + end + + context 'across different projects' do + let(:other_project) { create(:project) } + let(:canonical_issue) { create(:issue, project: other_project) } + + it_behaves_like 'a system note' do + let(:action) { 'duplicate' } + end + + it { expect(subject.note).to eq "marked this issue as a duplicate of #{canonical_issue.to_reference(project)}" } + end + end + + describe '#mark_canonical_issue_of_duplicate' do + subject { service.mark_canonical_issue_of_duplicate(duplicate_issue) } + + context 'within the same project' do + let(:duplicate_issue) { create(:issue, project: project) } + + it_behaves_like 'a system note' do + let(:action) { 'duplicate' } + end + + it { expect(subject.note).to eq "marked #{duplicate_issue.to_reference} as a duplicate of this issue" } + end + + context 'across different projects' do + let(:other_project) { create(:project) } + let(:duplicate_issue) { create(:issue, project: other_project) } + + it_behaves_like 'a system note' do + let(:action) { 'duplicate' } + end + + it { expect(subject.note).to eq "marked #{duplicate_issue.to_reference(project)} as a duplicate of this issue" } + end + end + + describe '#discussion_lock' do + subject { service.discussion_lock } + + context 'discussion unlocked' do + it_behaves_like 'a system note' do + let(:action) { 'unlocked' } + end + + it 'creates the note text correctly' do + [:issue, :merge_request].each do |type| + issuable = create(type) + + service = described_class.new(noteable: issuable, author: author) + expect(service.discussion_lock.note) + .to eq("unlocked this #{type.to_s.titleize.downcase}") + end + end + end + + context 'discussion locked' do + before do + noteable.update_attribute(:discussion_locked, true) + end + + it_behaves_like 'a system note' do + let(:action) { 'locked' } + end + + it 'creates the note text correctly' do + [:issue, :merge_request].each do |type| + issuable = create(type, discussion_locked: true) + + service = described_class.new(noteable: issuable, author: author) + expect(service.discussion_lock.note) + .to eq("locked this #{type.to_s.titleize.downcase}") + end + end + end + end + + describe '#cross_reference_disallowed?' do + context 'when mentioner is not a MergeRequest' do + it 'is falsey' do + mentioner = noteable.dup + expect(service.cross_reference_disallowed?(mentioner)) + .to be_falsey + end + end + + context 'when mentioner is a MergeRequest' do + let(:mentioner) { create(:merge_request, :simple, source_project: project) } + let(:noteable) { project.commit } + + it 'is truthy when noteable is in commits' do + expect(mentioner).to receive(:commits).and_return([noteable]) + expect(service.cross_reference_disallowed?(mentioner)) + .to be_truthy + end + + it 'is falsey when noteable is not in commits' do + expect(mentioner).to receive(:commits).and_return([]) + expect(service.cross_reference_disallowed?(mentioner)) + .to be_falsey + end + end + + context 'when notable is an ExternalIssue' do + let(:noteable) { ExternalIssue.new('EXT-1234', project) } + it 'is truthy' do + mentioner = noteable.dup + expect(service.cross_reference_disallowed?(mentioner)) + .to be_truthy + end + end + end +end diff --git a/spec/services/system_notes/zoom_service_spec.rb b/spec/services/system_notes/zoom_service_spec.rb new file mode 100644 index 00000000000..435cdb5748e --- /dev/null +++ b/spec/services/system_notes/zoom_service_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ::SystemNotes::ZoomService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:author) { create(:user) } + + let(:noteable) { create(:issue, project: project) } + + let(:service) { described_class.new(noteable: noteable, project: project, author: author) } + + describe '#zoom_link_added' do + subject { service.zoom_link_added } + + it_behaves_like 'a system note' do + let(:action) { 'pinned_embed' } + end + + it 'sets the zoom link added note text' do + expect(subject.note).to eq('added a Zoom call to this issue') + end + end + + describe '#zoom_link_removed' do + subject { service.zoom_link_removed } + + it_behaves_like 'a system note' do + let(:action) { 'pinned_embed' } + end + + it 'sets the zoom link removed note text' do + expect(subject.note).to eq('removed a Zoom call from this issue') + end + end +end diff --git a/spec/services/todos/destroy/private_features_service_spec.rb b/spec/services/todos/destroy/private_features_service_spec.rb index 7831e3a47e0..dfe9f42e8b1 100644 --- a/spec/services/todos/destroy/private_features_service_spec.rb +++ b/spec/services/todos/destroy/private_features_service_spec.rb @@ -27,7 +27,7 @@ describe Todos::Destroy::PrivateFeaturesService do context 'when user_id is provided' do subject { described_class.new(project.id, user.id).execute } - context 'when all feaures have same visibility as the project' do + context 'when all features have same visibility as the project' do it 'removes only user issue todos' do expect { subject }.not_to change { Todo.count } end @@ -92,7 +92,7 @@ describe Todos::Destroy::PrivateFeaturesService do context 'when user_id is not provided' do subject { described_class.new(project.id).execute } - context 'when all feaures have same visibility as the project' do + context 'when all features have same visibility as the project' do it 'does not remove any todos' do expect { subject }.not_to change { Todo.count } end diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index 4a5f4509a7b..23a0c71175e 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -183,7 +183,7 @@ describe Users::DestroyService do let!(:project) { create(:project, :empty_repo, :legacy_storage, namespace: user.namespace) } it 'removes repository' do - expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey end end @@ -191,7 +191,7 @@ describe Users::DestroyService do let!(:project) { create(:project, :empty_repo, namespace: user.namespace) } it 'removes repository' do - expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey end end end |