diff options
Diffstat (limited to 'spec/services')
73 files changed, 3289 insertions, 705 deletions
diff --git a/spec/services/application_settings/update_service_spec.rb b/spec/services/application_settings/update_service_spec.rb index a641828faa5..ab06c1a1209 100644 --- a/spec/services/application_settings/update_service_spec.rb +++ b/spec/services/application_settings/update_service_spec.rb @@ -62,6 +62,54 @@ describe ApplicationSettings::UpdateService do end end + describe 'updating outbound_local_requests_whitelist' do + context 'when params is blank' do + let(:params) { {} } + + it 'does not add to whitelist' do + expect { subject.execute }.not_to change { + application_settings.outbound_local_requests_whitelist + } + end + end + + context 'when param add_to_outbound_local_requests_whitelist contains values' do + before do + application_settings.outbound_local_requests_whitelist = ['127.0.0.1'] + end + + let(:params) { { add_to_outbound_local_requests_whitelist: ['example.com', ''] } } + + it 'adds to whitelist' do + expect { subject.execute }.to change { + application_settings.outbound_local_requests_whitelist + } + + expect(application_settings.outbound_local_requests_whitelist).to contain_exactly( + '127.0.0.1', 'example.com' + ) + end + end + + context 'when param outbound_local_requests_whitelist_raw is passed' do + before do + application_settings.outbound_local_requests_whitelist = ['127.0.0.1'] + end + + let(:params) { { outbound_local_requests_whitelist_raw: 'example.com;gitlab.com' } } + + it 'overwrites the existing whitelist' do + expect { subject.execute }.to change { + application_settings.outbound_local_requests_whitelist + } + + expect(application_settings.outbound_local_requests_whitelist).to contain_exactly( + 'example.com', 'gitlab.com' + ) + end + end + end + describe 'performance bar settings' do using RSpec::Parameterized::TableSyntax @@ -153,6 +201,24 @@ describe ApplicationSettings::UpdateService do enable_external_authorization_service_check end + it 'does not validate labels if external authorization gets disabled' do + expect_any_instance_of(described_class).not_to receive(:validate_classification_label) + + described_class.new(application_settings, admin, { external_authorization_service_enabled: false }).execute + end + + it 'does validate labels if external authorization gets enabled ' do + expect_any_instance_of(described_class).to receive(:validate_classification_label) + + described_class.new(application_settings, admin, { external_authorization_service_enabled: true }).execute + end + + it 'does validate labels if external authorization is left unchanged' do + expect_any_instance_of(described_class).to receive(:validate_classification_label) + + described_class.new(application_settings, admin, { external_authorization_service_default_label: 'new-label' }).execute + end + it 'does not save the settings with an error if the service denies access' do expect(::Gitlab::ExternalAuthorization) .to receive(:access_allowed?).with(admin, 'new-label') { false } @@ -180,4 +246,20 @@ describe ApplicationSettings::UpdateService do described_class.new(application_settings, admin, { home_page_url: 'http://foo.bar' }).execute end end + + context 'when raw_blob_request_limit is passsed' do + let(:params) do + { + raw_blob_request_limit: 600 + } + end + + it 'updates raw_blob_request_limit value' do + subject.execute + + application_settings.reload + + expect(application_settings.raw_blob_request_limit).to eq(600) + end + end end diff --git a/spec/services/audit_event_service_spec.rb b/spec/services/audit_event_service_spec.rb index 32fd98e6ef9..e42bff607b2 100644 --- a/spec/services/audit_event_service_spec.rb +++ b/spec/services/audit_event_service_spec.rb @@ -10,11 +10,8 @@ describe AuditEventService do let(:logger) { instance_double(Gitlab::AuditJsonLogger) } describe '#security_event' do - before do - expect(service).to receive(:file_logger).and_return(logger) - end - it 'creates an event and logs to a file' do + expect(service).to receive(:file_logger).and_return(logger) expect(logger).to receive(:info).with(author_id: user.id, entity_id: project.id, entity_type: "Project", @@ -22,5 +19,32 @@ describe AuditEventService do expect { service.security_event }.to change(SecurityEvent, :count).by(1) end + + it 'formats from and to fields' do + service = described_class.new( + user, project, + { + from: true, + to: false, + action: :create, + target_id: 1 + }) + expect(service).to receive(:file_logger).and_return(logger) + expect(logger).to receive(:info).with(author_id: user.id, + entity_type: 'Project', + entity_id: project.id, + from: 'true', + to: 'false', + action: :create, + target_id: 1) + + expect { service.security_event }.to change(SecurityEvent, :count).by(1) + + details = SecurityEvent.last.details + expect(details[:from]).to be true + expect(details[:to]).to be false + expect(details[:action]).to eq(:create) + expect(details[:target_id]).to eq(1) + end end end diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb index 4f4776bbb27..3ca389ba25b 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -145,6 +145,19 @@ describe Auth::ContainerRegistryAuthenticationService do it_behaves_like 'not a container repository factory' end + describe '#pull_access_token' do + let(:project) { create(:project) } + let(:token) { described_class.pull_access_token(project.full_path) } + + subject { { token: token } } + + it_behaves_like 'an accessible' do + let(:actions) { ['pull'] } + end + + it_behaves_like 'not a container repository factory' + end + context 'user authorization' do let(:current_user) { create(:user) } diff --git a/spec/services/award_emojis/add_service_spec.rb b/spec/services/award_emojis/add_service_spec.rb new file mode 100644 index 00000000000..037db39ba80 --- /dev/null +++ b/spec/services/award_emojis/add_service_spec.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AwardEmojis::AddService do + set(:user) { create(:user) } + set(:project) { create(:project) } + set(:awardable) { create(:note, project: project) } + let(:name) { 'thumbsup' } + subject(:service) { described_class.new(awardable, name, user) } + + describe '#execute' do + context 'when user is not authorized' do + it 'does not add an emoji' do + expect { service.execute }.not_to change { AwardEmoji.count } + end + + it 'returns an error state' do + result = service.execute + + expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(:forbidden) + end + end + + context 'when user is authorized' do + before do + project.add_developer(user) + end + + it 'creates an award emoji' do + expect { service.execute }.to change { AwardEmoji.count }.by(1) + end + + it 'returns the award emoji' do + result = service.execute + + expect(result[:award]).to be_kind_of(AwardEmoji) + end + + it 'return a success status' do + result = service.execute + + expect(result[:status]).to eq(:success) + end + + it 'sets the correct properties on the award emoji' do + award = service.execute[:award] + + expect(award.name).to eq(name) + expect(award.user).to eq(user) + end + + describe 'marking Todos as done' do + subject { service.execute } + + include_examples 'creating award emojis marks Todos as done' + end + + context 'when the awardable cannot have emoji awarded to it' do + before do + expect(awardable).to receive(:emoji_awardable?).and_return(false) + end + + it 'does not add an emoji' do + expect { service.execute }.not_to change { AwardEmoji.count } + end + + it 'returns an error status' do + result = service.execute + + expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(:unprocessable_entity) + end + end + + context 'when the awardable is invalid' do + before do + expect_next_instance_of(AwardEmoji) do |award| + expect(award).to receive(:valid?).and_return(false) + expect(award).to receive_message_chain(:errors, :full_messages).and_return(['Error 1', 'Error 2']) + end + end + + it 'does not add an emoji' do + expect { service.execute }.not_to change { AwardEmoji.count } + end + + it 'returns an error status' do + result = service.execute + + expect(result[:status]).to eq(:error) + end + + it 'returns an error message' do + result = service.execute + + expect(result[:message]).to eq('Error 1 and Error 2') + end + end + end + end +end diff --git a/spec/services/award_emojis/collect_user_emoji_service_spec.rb b/spec/services/award_emojis/collect_user_emoji_service_spec.rb new file mode 100644 index 00000000000..a0dea31b403 --- /dev/null +++ b/spec/services/award_emojis/collect_user_emoji_service_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AwardEmojis::CollectUserEmojiService do + describe '#execute' do + it 'returns an Array containing the awarded emoji names' do + user = create(:user) + + create(:award_emoji, user: user, name: 'thumbsup') + create(:award_emoji, user: user, name: 'thumbsup') + create(:award_emoji, user: user, name: 'thumbsdown') + + awarded = described_class.new(user).execute + + expect(awarded).to eq([{ name: 'thumbsup' }, { name: 'thumbsdown' }]) + end + + it 'returns an empty Array when no user is given' do + awarded = described_class.new.execute + + expect(awarded).to be_empty + end + end +end diff --git a/spec/services/award_emojis/destroy_service_spec.rb b/spec/services/award_emojis/destroy_service_spec.rb new file mode 100644 index 00000000000..c4a7d5ec20e --- /dev/null +++ b/spec/services/award_emojis/destroy_service_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AwardEmojis::DestroyService do + set(:user) { create(:user) } + set(:awardable) { create(:note) } + set(:project) { awardable.project } + let(:name) { 'thumbsup' } + let!(:award_from_other_user) do + create(:award_emoji, name: name, awardable: awardable, user: create(:user)) + end + subject(:service) { described_class.new(awardable, name, user) } + + describe '#execute' do + shared_examples_for 'a service that does not authorize the user' do |error:| + it 'does not remove the emoji' do + expect { service.execute }.not_to change { AwardEmoji.count } + end + + it 'returns an error state' do + result = service.execute + + expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(:forbidden) + end + + it 'returns a nil award' do + result = service.execute + + expect(result).to have_key(:award) + expect(result[:award]).to be_nil + end + + it 'returns the error' do + result = service.execute + + expect(result[:message]).to eq(error) + expect(result[:errors]).to eq([error]) + end + end + + context 'when user is not authorized' do + it_behaves_like 'a service that does not authorize the user', + error: 'User cannot destroy emoji on the awardable' + end + + context 'when the user is authorized' do + before do + project.add_developer(user) + end + + context 'when user has not awarded an emoji to the awardable' do + let!(:award_from_user) { create(:award_emoji, name: name, user: user) } + + it_behaves_like 'a service that does not authorize the user', + error: 'User has not awarded emoji of type thumbsup on the awardable' + end + + context 'when user has awarded an emoji to the awardable' do + let!(:award_from_user) { create(:award_emoji, name: name, awardable: awardable, user: user) } + + it 'removes the emoji' do + expect { service.execute }.to change { AwardEmoji.count }.by(-1) + end + + it 'returns a success status' do + result = service.execute + + expect(result[:status]).to eq(:success) + end + + it 'returns no errors' do + result = service.execute + + expect(result).not_to have_key(:error) + expect(result).not_to have_key(:errors) + end + + it 'returns the destroyed award' do + result = service.execute + + expect(result[:award]).to eq(award_from_user) + expect(result[:award]).to be_destroyed + end + end + end + end +end diff --git a/spec/services/award_emojis/toggle_service_spec.rb b/spec/services/award_emojis/toggle_service_spec.rb new file mode 100644 index 00000000000..972a1d5fc06 --- /dev/null +++ b/spec/services/award_emojis/toggle_service_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AwardEmojis::ToggleService do + set(:user) { create(:user) } + set(:project) { create(:project, :public) } + set(:awardable) { create(:note, project: project) } + let(:name) { 'thumbsup' } + subject(:service) { described_class.new(awardable, name, user) } + + describe '#execute' do + context 'when user has awarded an emoji' do + let!(:award_from_other_user) { create(:award_emoji, name: name, awardable: awardable, user: create(:user)) } + let!(:award) { create(:award_emoji, name: name, awardable: awardable, user: user) } + + it 'calls AwardEmojis::DestroyService' do + expect(AwardEmojis::AddService).not_to receive(:new) + + expect_next_instance_of(AwardEmojis::DestroyService) do |service| + expect(service).to receive(:execute) + end + + service.execute + end + + it 'destroys an AwardEmoji' do + expect { service.execute }.to change { AwardEmoji.count }.by(-1) + end + + it 'returns the result of DestroyService#execute' do + mock_result = double(foo: true) + + expect_next_instance_of(AwardEmojis::DestroyService) do |service| + expect(service).to receive(:execute).and_return(mock_result) + end + + result = service.execute + + expect(result).to eq(mock_result) + end + end + + context 'when user has not awarded an emoji' do + it 'calls AwardEmojis::AddService' do + expect_next_instance_of(AwardEmojis::AddService) do |service| + expect(service).to receive(:execute) + end + + expect(AwardEmojis::DestroyService).not_to receive(:new) + + service.execute + end + + it 'creates an AwardEmoji' do + expect { service.execute }.to change { AwardEmoji.count }.by(1) + end + + it 'returns the result of AddService#execute' do + mock_result = double(foo: true) + + expect_next_instance_of(AwardEmojis::AddService) do |service| + expect(service).to receive(:execute).and_return(mock_result) + end + + result = service.execute + + expect(result).to eq(mock_result) + end + end + end +end diff --git a/spec/services/boards/issues/list_service_spec.rb b/spec/services/boards/issues/list_service_spec.rb index 40878e24cb4..931b67b2950 100644 --- a/spec/services/boards/issues/list_service_spec.rb +++ b/spec/services/boards/issues/list_service_spec.rb @@ -112,7 +112,7 @@ describe Boards::Issues::ListService do it_behaves_like 'issues list service' end - context 'and group is an ancestor', :nested_groups do + context 'and group is an ancestor' do let(:parent) { create(:group) } let(:group) { create(:group, parent: parent) } let!(:backlog) { create(:backlog_list, board: board) } diff --git a/spec/services/boards/issues/move_service_spec.rb b/spec/services/boards/issues/move_service_spec.rb index 16e2a2fba6b..cf84ec8fd4c 100644 --- a/spec/services/boards/issues/move_service_spec.rb +++ b/spec/services/boards/issues/move_service_spec.rb @@ -52,5 +52,91 @@ describe Boards::Issues::MoveService do it_behaves_like 'issues move service', true end + + describe '#execute_multiple' do + set(:group) { create(:group) } + set(:user) { create(:user) } + set(:project) { create(:project, namespace: group) } + set(:board1) { create(:board, group: group) } + set(:development) { create(:group_label, group: group, name: 'Development') } + set(:testing) { create(:group_label, group: group, name: 'Testing') } + set(:list1) { create(:list, board: board1, label: development, position: 0) } + set(:list2) { create(:list, board: board1, label: testing, position: 1) } + let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list2.id } } + + before do + project.add_developer(user) + end + + it 'returns the expected result if list of issues is empty' do + expect(described_class.new(group, user, params).execute_multiple([])).to eq({ count: 0, success: false, issues: [] }) + end + + context 'moving multiple issues' do + let(:issue1) { create(:labeled_issue, project: project, labels: [development]) } + let(:issue2) { create(:labeled_issue, project: project, labels: [development]) } + + it 'moves multiple issues from one list to another' do + expect(described_class.new(group, user, params).execute_multiple([issue1, issue2])).to be_truthy + + expect(issue1.labels).to eq([testing]) + expect(issue2.labels).to eq([testing]) + end + end + + context 'moving a single issue' do + let(:issue1) { create(:labeled_issue, project: project, labels: [development]) } + + it 'moves one issue' do + expect(described_class.new(group, user, params).execute_multiple([issue1])).to be_truthy + + expect(issue1.labels).to eq([testing]) + end + end + + context 'moving issues visually after an existing issue' do + let(:existing_issue) { create(:labeled_issue, project: project, labels: [testing], relative_position: 10) } + let(:issue1) { create(:labeled_issue, project: project, labels: [development]) } + let(:issue2) { create(:labeled_issue, project: project, labels: [development]) } + + let(:move_params) do + params.dup.tap do |hash| + hash[:move_before_id] = existing_issue.id + end + end + + it 'moves one issue' do + expect(described_class.new(group, user, move_params).execute_multiple([issue1, issue2])).to be_truthy + + expect(issue1.labels).to eq([testing]) + expect(issue2.labels).to eq([testing]) + + expect(issue1.relative_position > existing_issue.relative_position).to eq(true) + expect(issue2.relative_position > issue1.relative_position).to eq(true) + end + end + + context 'moving issues visually before an existing issue' do + let(:existing_issue) { create(:labeled_issue, project: project, labels: [testing], relative_position: 10) } + let(:issue1) { create(:labeled_issue, project: project, labels: [development]) } + let(:issue2) { create(:labeled_issue, project: project, labels: [development]) } + + let(:move_params) do + params.dup.tap do |hash| + hash[:move_after_id] = existing_issue.id + end + end + + it 'moves one issue' do + expect(described_class.new(group, user, move_params).execute_multiple([issue1, issue2])).to be_truthy + + expect(issue1.labels).to eq([testing]) + expect(issue2.labels).to eq([testing]) + + expect(issue2.relative_position < existing_issue.relative_position).to eq(true) + expect(issue1.relative_position < issue2.relative_position).to eq(true) + end + end + end end end diff --git a/spec/services/ci/archive_trace_service_spec.rb b/spec/services/ci/archive_trace_service_spec.rb index 44a77c29086..454db3d5a48 100644 --- a/spec/services/ci/archive_trace_service_spec.rb +++ b/spec/services/ci/archive_trace_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Ci::ArchiveTraceService, '#execute' do - subject { described_class.new.execute(job) } + subject { described_class.new.execute(job, worker_name: ArchiveTraceWorker.name) } context 'when job is finished' do let(:job) { create(:ci_build, :success, :trace_live) } @@ -25,6 +25,34 @@ describe Ci::ArchiveTraceService, '#execute' do expect { subject }.not_to change { Ci::JobArtifact.trace.count } end end + + context 'when job does not have trace' do + let(:job) { create(:ci_build, :success) } + + it 'leaves a warning message in sidekiq log' do + expect(Sidekiq.logger).to receive(:warn).with( + class: ArchiveTraceWorker.name, + message: 'The job does not have live trace but going to be archived.', + job_id: job.id) + + subject + end + end + + context 'when job failed to archive trace but did not raise an exception' do + before do + allow_any_instance_of(Gitlab::Ci::Trace).to receive(:archive!) {} + end + + it 'leaves a warning message in sidekiq log' do + expect(Sidekiq.logger).to receive(:warn).with( + class: ArchiveTraceWorker.name, + message: 'The job does not have archived trace after archiving.', + job_id: job.id) + + subject + end + end end context 'when job is running' do @@ -37,10 +65,10 @@ describe Ci::ArchiveTraceService, '#execute' do issue_url: 'https://gitlab.com/gitlab-org/gitlab-ce/issues/51502', extra: { job_id: job.id } ).once - expect(Rails.logger) - .to receive(:error) - .with("Failed to archive trace. id: #{job.id} message: Job is not finished yet") - .and_call_original + expect(Sidekiq.logger).to receive(:warn).with( + class: ArchiveTraceWorker.name, + message: "Failed to archive trace. message: Job is not finished yet.", + job_id: job.id).and_call_original expect(Gitlab::Metrics) .to receive(:counter) diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index d9b61dfe503..deb68899309 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -1099,6 +1099,63 @@ describe Ci::CreatePipelineService do end end end + + context 'when needs is used' do + let(:pipeline) { execute_service } + + let(:config) do + { + build_a: { + stage: "build", + script: "ls", + only: %w[master] + }, + test_a: { + stage: "test", + script: "ls", + only: %w[master feature], + needs: %w[build_a] + }, + deploy: { + stage: "deploy", + script: "ls", + only: %w[tags] + } + } + end + + before do + stub_ci_pipeline_yaml_file(YAML.dump(config)) + end + + context 'when pipeline on master is created' do + let(:ref_name) { 'refs/heads/master' } + + it 'creates a pipeline with build_a and test_a' do + expect(pipeline).to be_persisted + expect(pipeline.builds.map(&:name)).to contain_exactly("build_a", "test_a") + end + end + + context 'when pipeline on feature is created' do + let(:ref_name) { 'refs/heads/feature' } + + it 'does not create a pipeline as test_a depends on build_a' do + expect(pipeline).not_to be_persisted + expect(pipeline.builds).to be_empty + expect(pipeline.errors[:base]).to contain_exactly("test_a: needs 'build_a'") + end + end + + context 'when pipeline on v1.0.0 is created' do + let(:ref_name) { 'refs/tags/v1.0.0' } + + it 'does create a pipeline only with deploy' do + expect(pipeline).to be_persisted + expect(pipeline.builds.map(&:name)).to contain_exactly("deploy") + end + end + end end describe '#execute!' do diff --git a/spec/services/ci/play_build_service_spec.rb b/spec/services/ci/play_build_service_spec.rb index 1e68b7956ea..cf39f3da4fe 100644 --- a/spec/services/ci/play_build_service_spec.rb +++ b/spec/services/ci/play_build_service_spec.rb @@ -60,6 +60,19 @@ describe Ci::PlayBuildService, '#execute' do expect(build.reload.user).to eq user end + + context 'when variables are supplied' do + let(:job_variables) do + [{ key: 'first', secret_value: 'first' }, + { key: 'second', secret_value: 'second' }] + end + + it 'assigns the variables to the build' do + service.execute(build, job_variables) + + expect(build.reload.job_variables.map(&:key)).to contain_exactly('first', 'second') + end + end end context 'when build is not a playable manual action' do diff --git a/spec/services/ci/process_pipeline_service_spec.rb b/spec/services/ci/process_pipeline_service_spec.rb index cadb519ccee..1b28d2d4d02 100644 --- a/spec/services/ci/process_pipeline_service_spec.rb +++ b/spec/services/ci/process_pipeline_service_spec.rb @@ -700,6 +700,138 @@ describe Ci::ProcessPipelineService, '#execute' do end end + context 'when pipeline with needs is created' do + let!(:linux_build) { create_build('linux:build', stage: 'build', stage_idx: 0) } + let!(:mac_build) { create_build('mac:build', stage: 'build', stage_idx: 0) } + let!(:linux_rspec) { create_build('linux:rspec', stage: 'test', stage_idx: 1) } + let!(:linux_rubocop) { create_build('linux:rubocop', stage: 'test', stage_idx: 1) } + let!(:mac_rspec) { create_build('mac:rspec', stage: 'test', stage_idx: 1) } + let!(:mac_rubocop) { create_build('mac:rubocop', stage: 'test', stage_idx: 1) } + let!(:deploy) { create_build('deploy', stage: 'deploy', stage_idx: 2) } + + let!(:linux_rspec_on_build) { create(:ci_build_need, build: linux_rspec, name: 'linux:build') } + let!(:linux_rubocop_on_build) { create(:ci_build_need, build: linux_rubocop, name: 'linux:build') } + + let!(:mac_rspec_on_build) { create(:ci_build_need, build: mac_rspec, name: 'mac:build') } + let!(:mac_rubocop_on_build) { create(:ci_build_need, build: mac_rubocop, name: 'mac:build') } + + it 'when linux:* finishes first it runs it out of order' do + expect(process_pipeline).to be_truthy + + expect(stages).to eq(%w(pending created created)) + expect(builds.pending).to contain_exactly(linux_build, mac_build) + + # we follow the single path of linux + linux_build.reset.success! + + expect(stages).to eq(%w(running pending created)) + expect(builds.success).to contain_exactly(linux_build) + expect(builds.pending).to contain_exactly(mac_build, linux_rspec, linux_rubocop) + + linux_rspec.reset.success! + + expect(stages).to eq(%w(running running created)) + expect(builds.success).to contain_exactly(linux_build, linux_rspec) + expect(builds.pending).to contain_exactly(mac_build, linux_rubocop) + + linux_rubocop.reset.success! + + expect(stages).to eq(%w(running running created)) + expect(builds.success).to contain_exactly(linux_build, linux_rspec, linux_rubocop) + expect(builds.pending).to contain_exactly(mac_build) + + mac_build.reset.success! + mac_rspec.reset.success! + mac_rubocop.reset.success! + + expect(stages).to eq(%w(success success pending)) + expect(builds.success).to contain_exactly( + linux_build, linux_rspec, linux_rubocop, mac_build, mac_rspec, mac_rubocop) + expect(builds.pending).to contain_exactly(deploy) + end + + context 'when feature ci_dag_support is disabled' do + before do + stub_feature_flags(ci_dag_support: false) + end + + it 'when linux:build finishes first it follows stages' do + expect(process_pipeline).to be_truthy + + expect(stages).to eq(%w(pending created created)) + expect(builds.pending).to contain_exactly(linux_build, mac_build) + + # we follow the single path of linux + linux_build.reset.success! + + expect(stages).to eq(%w(running created created)) + expect(builds.success).to contain_exactly(linux_build) + expect(builds.pending).to contain_exactly(mac_build) + + mac_build.reset.success! + + expect(stages).to eq(%w(success pending created)) + expect(builds.success).to contain_exactly(linux_build, mac_build) + expect(builds.pending).to contain_exactly( + linux_rspec, linux_rubocop, mac_rspec, mac_rubocop) + + linux_rspec.reset.success! + linux_rubocop.reset.success! + mac_rspec.reset.success! + mac_rubocop.reset.success! + + expect(stages).to eq(%w(success success pending)) + expect(builds.success).to contain_exactly( + linux_build, linux_rspec, linux_rubocop, mac_build, mac_rspec, mac_rubocop) + expect(builds.pending).to contain_exactly(deploy) + end + end + + context 'when one of the jobs is run on a failure' do + let!(:linux_notify) { create_build('linux:notify', stage: 'deploy', stage_idx: 2, when: 'on_failure') } + + let!(:linux_notify_on_build) { create(:ci_build_need, build: linux_notify, name: 'linux:build') } + + context 'when another job in build phase fails first' do + context 'when ci_dag_support is enabled' do + it 'does skip linux:notify' do + expect(process_pipeline).to be_truthy + + mac_build.reset.drop! + linux_build.reset.success! + + expect(linux_notify.reset).to be_skipped + end + end + + context 'when ci_dag_support is disabled' do + before do + stub_feature_flags(ci_dag_support: false) + end + + it 'does run linux:notify' do + expect(process_pipeline).to be_truthy + + mac_build.reset.drop! + linux_build.reset.success! + + expect(linux_notify.reset).to be_pending + end + end + end + + context 'when linux:build job fails first' do + it 'does run linux:notify' do + expect(process_pipeline).to be_truthy + + linux_build.reset.drop! + + expect(linux_notify.reset).to be_pending + end + end + end + end + def process_pipeline described_class.new(pipeline.project, user).execute(pipeline) end @@ -712,6 +844,10 @@ describe Ci::ProcessPipelineService, '#execute' do all_builds.where.not(status: [:created, :skipped]) end + def stages + pipeline.reset.stages.map(&:status) + end + def builds_names builds.pluck(:name) end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 11b06ef5019..fe7c6fe4700 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -30,7 +30,8 @@ describe Ci::RetryBuildService do job_artifacts_sast job_artifacts_dependency_scanning job_artifacts_container_scanning job_artifacts_dast job_artifacts_license_management job_artifacts_performance - job_artifacts_codequality job_artifacts_metrics scheduled_at].freeze + job_artifacts_codequality job_artifacts_metrics scheduled_at + job_variables].freeze IGNORE_ACCESSORS = %i[type lock_version target_url base_tags trace_sections @@ -65,6 +66,9 @@ describe Ci::RetryBuildService do file_type: file_type, job: build, expire_at: build.artifacts_expire_at) end + create(:ci_job_variable, job: build) + create(:ci_build_need, build: build) + build.reload end diff --git a/spec/services/ci/update_build_queue_service_spec.rb b/spec/services/ci/update_build_queue_service_spec.rb index 4b869385128..522dd1ba1c2 100644 --- a/spec/services/ci/update_build_queue_service_spec.rb +++ b/spec/services/ci/update_build_queue_service_spec.rb @@ -7,84 +7,108 @@ describe Ci::UpdateBuildQueueService do let(:build) { create(:ci_build, pipeline: pipeline) } let(:pipeline) { create(:ci_pipeline, project: project) } - context 'when updating specific runners' do - let(:runner) { create(:ci_runner, :project, projects: [project]) } - - context 'when there is a runner that can pick build' do - it 'ticks runner queue value' do - expect { subject.execute(build) }.to change { runner.ensure_runner_queue_value } - end + shared_examples 'refreshes runner' do + it 'ticks runner queue value' do + expect { subject.execute(build) }.to change { runner.ensure_runner_queue_value } end + end - context 'when there is no runner that can pick build' do - let(:another_project) { create(:project) } - let(:runner) { create(:ci_runner, :project, projects: [another_project]) } - - it 'does not tick runner queue value' do - expect { subject.execute(build) }.not_to change { runner.ensure_runner_queue_value } - end + shared_examples 'does not refresh runner' do + it 'ticks runner queue value' do + expect { subject.execute(build) }.not_to change { runner.ensure_runner_queue_value } end end - context 'when updating shared runners' do - let(:runner) { create(:ci_runner, :instance) } - - context 'when there is no runner that can pick build' do - it 'ticks runner queue value' do - expect { subject.execute(build) }.to change { runner.ensure_runner_queue_value } + shared_examples 'matching build' do + context 'when there is a online runner that can pick build' do + before do + runner.update!(contacted_at: 30.minutes.ago) end + + it_behaves_like 'refreshes runner' end + end + shared_examples 'mismatching tags' do context 'when there is no runner that can pick build due to tag mismatch' do before do build.tag_list = [:docker] end - it 'does not tick runner queue value' do - expect { subject.execute(build) }.not_to change { runner.ensure_runner_queue_value } - end + it_behaves_like 'does not refresh runner' end + end - context 'when there is no runner that can pick build due to being disabled on project' do + shared_examples 'recent runner queue' do + context 'when there is runner with expired cache' do before do - build.project.shared_runners_enabled = false + runner.update!(contacted_at: Ci::Runner.recent_queue_deadline) end - it 'does not tick runner queue value' do - expect { subject.execute(build) }.not_to change { runner.ensure_runner_queue_value } + context 'when ci_update_queues_for_online_runners is enabled' do + before do + stub_feature_flags(ci_update_queues_for_online_runners: true) + end + + it_behaves_like 'does not refresh runner' + end + + context 'when ci_update_queues_for_online_runners is disabled' do + before do + stub_feature_flags(ci_update_queues_for_online_runners: false) + end + + it_behaves_like 'refreshes runner' end end end - context 'when updating group runners' do - let(:group) { create(:group) } - let(:project) { create(:project, group: group) } - let(:runner) { create(:ci_runner, :group, groups: [group]) } + context 'when updating specific runners' do + let(:runner) { create(:ci_runner, :project, projects: [project]) } - context 'when there is a runner that can pick build' do - it 'ticks runner queue value' do - expect { subject.execute(build) }.to change { runner.ensure_runner_queue_value } - end + it_behaves_like 'matching build' + it_behaves_like 'mismatching tags' + it_behaves_like 'recent runner queue' + + context 'when the runner is assigned to another project' do + let(:another_project) { create(:project) } + let(:runner) { create(:ci_runner, :project, projects: [another_project]) } + + it_behaves_like 'does not refresh runner' end + end - context 'when there is no runner that can pick build due to tag mismatch' do + context 'when updating shared runners' do + let(:runner) { create(:ci_runner, :instance) } + + it_behaves_like 'matching build' + it_behaves_like 'mismatching tags' + it_behaves_like 'recent runner queue' + + context 'when there is no runner that can pick build due to being disabled on project' do before do - build.tag_list = [:docker] + build.project.shared_runners_enabled = false end - it 'does not tick runner queue value' do - expect { subject.execute(build) }.not_to change { runner.ensure_runner_queue_value } - end + it_behaves_like 'does not refresh runner' end + end + + context 'when updating group runners' do + let(:group) { create(:group) } + let(:project) { create(:project, group: group) } + let(:runner) { create(:ci_runner, :group, groups: [group]) } + + it_behaves_like 'matching build' + it_behaves_like 'mismatching tags' + it_behaves_like 'recent runner queue' context 'when there is no runner that can pick build due to being disabled on project' do before do build.project.group_runners_enabled = false end - it 'does not tick runner queue value' do - expect { subject.execute(build) }.not_to change { runner.ensure_runner_queue_value } - end + it_behaves_like 'does not refresh runner' end end end diff --git a/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb b/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb index 9ab83d913f5..a948b442441 100644 --- a/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb +++ b/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb @@ -41,7 +41,7 @@ describe Clusters::Applications::CheckUninstallProgressService do end end - context 'when application is installing' do + context 'when application is uninstalling' do RESCHEDULE_PHASES.each { |phase| it_behaves_like 'a not yet terminated installation', phase } context 'when installation POD succeeded' do @@ -56,6 +56,12 @@ describe Clusters::Applications::CheckUninstallProgressService do service.execute end + it 'runs application post_uninstall' do + expect(application).to receive(:post_uninstall).and_call_original + + service.execute + end + it 'destroys the application' do expect(worker_class).not_to receive(:perform_in) diff --git a/spec/services/clusters/build_kubernetes_namespace_service_spec.rb b/spec/services/clusters/build_kubernetes_namespace_service_spec.rb new file mode 100644 index 00000000000..36c05469542 --- /dev/null +++ b/spec/services/clusters/build_kubernetes_namespace_service_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Clusters::BuildKubernetesNamespaceService do + let(:cluster) { create(:cluster, :project, :provided_by_gcp) } + let(:environment) { create(:environment) } + let(:project) { environment.project } + + let(:namespace_generator) { double(from_environment_slug: namespace) } + let(:namespace) { 'namespace' } + + subject { described_class.new(cluster, environment: environment).execute } + + before do + allow(Gitlab::Kubernetes::DefaultNamespace).to receive(:new).and_return(namespace_generator) + end + + shared_examples 'shared attributes' do + it 'initializes a new namespace and sets default values' do + expect(subject).to be_new_record + expect(subject.cluster).to eq cluster + expect(subject.project).to eq project + expect(subject.namespace).to eq namespace + expect(subject.service_account_name).to eq "#{namespace}-service-account" + end + end + + include_examples 'shared attributes' + + it 'sets cluster_project and environment' do + expect(subject.cluster_project).to eq cluster.cluster_project + expect(subject.environment).to eq environment + end + + context 'namespace per environment is disabled' do + let(:cluster) { create(:cluster, :project, :provided_by_gcp, :namespace_per_environment_disabled) } + + include_examples 'shared attributes' + + it 'does not set environment' do + expect(subject.cluster_project).to eq cluster.cluster_project + expect(subject.environment).to be_nil + end + end + + context 'group cluster' do + let(:cluster) { create(:cluster, :group, :provided_by_gcp) } + + include_examples 'shared attributes' + + it 'does not set cluster_project' do + expect(subject.cluster_project).to be_nil + expect(subject.environment).to eq environment + end + end +end diff --git a/spec/services/clusters/gcp/kubernetes/create_or_update_namespace_service_spec.rb b/spec/services/clusters/gcp/kubernetes/create_or_update_namespace_service_spec.rb index be052a07da7..e44cc3f5a78 100644 --- a/spec/services/clusters/gcp/kubernetes/create_or_update_namespace_service_spec.rb +++ b/spec/services/clusters/gcp/kubernetes/create_or_update_namespace_service_spec.rb @@ -9,8 +9,9 @@ describe Clusters::Gcp::Kubernetes::CreateOrUpdateNamespaceService, '#execute' d let(:platform) { cluster.platform } let(:api_url) { 'https://kubernetes.example.com' } let(:project) { cluster.project } + let(:environment) { create(:environment, project: project) } let(:cluster_project) { cluster.cluster_project } - let(:namespace) { "#{project.path}-#{project.id}" } + let(:namespace) { "#{project.name}-#{project.id}-#{environment.slug}" } subject do described_class.new( @@ -34,6 +35,8 @@ describe Clusters::Gcp::Kubernetes::CreateOrUpdateNamespaceService, '#execute' d stub_kubeclient_create_service_account(api_url, namespace: namespace) stub_kubeclient_create_secret(api_url, namespace: namespace) stub_kubeclient_put_secret(api_url, "#{namespace}-token", namespace: namespace) + stub_kubeclient_put_role(api_url, Clusters::Gcp::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_NAME, namespace: namespace) + stub_kubeclient_put_role_binding(api_url, Clusters::Gcp::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_BINDING_NAME, namespace: namespace) stub_kubeclient_get_secret( api_url, @@ -77,7 +80,8 @@ describe Clusters::Gcp::Kubernetes::CreateOrUpdateNamespaceService, '#execute' d let(:kubernetes_namespace) do build(:cluster_kubernetes_namespace, cluster: cluster, - project: project) + project: project, + environment: environment) end it_behaves_like 'successful creation of kubernetes namespace' @@ -90,20 +94,22 @@ describe Clusters::Gcp::Kubernetes::CreateOrUpdateNamespaceService, '#execute' d build(:cluster_kubernetes_namespace, cluster: cluster, project: cluster_project.project, - cluster_project: cluster_project) + cluster_project: cluster_project, + environment: environment) end it_behaves_like 'successful creation of kubernetes namespace' end context 'when there is a Kubernetes Namespace associated' do - let(:namespace) { 'new-namespace' } + let(:namespace) { "new-namespace-#{environment.slug}" } let(:kubernetes_namespace) do create(:cluster_kubernetes_namespace, cluster: cluster, project: cluster_project.project, - cluster_project: cluster_project) + cluster_project: cluster_project, + environment: environment) end before do diff --git a/spec/services/clusters/gcp/kubernetes/create_or_update_service_account_service_spec.rb b/spec/services/clusters/gcp/kubernetes/create_or_update_service_account_service_spec.rb index 382b9043566..8b874989758 100644 --- a/spec/services/clusters/gcp/kubernetes/create_or_update_service_account_service_spec.rb +++ b/spec/services/clusters/gcp/kubernetes/create_or_update_service_account_service_spec.rb @@ -143,6 +143,8 @@ describe Clusters::Gcp::Kubernetes::CreateOrUpdateServiceAccountService do stub_kubeclient_get_role_binding_error(api_url, role_binding_name, namespace: namespace) stub_kubeclient_create_role_binding(api_url, namespace: namespace) + stub_kubeclient_put_role(api_url, Clusters::Gcp::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_NAME, namespace: namespace) + stub_kubeclient_put_role_binding(api_url, Clusters::Gcp::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_BINDING_NAME, namespace: namespace) end it_behaves_like 'creates service account and token' @@ -169,6 +171,24 @@ describe Clusters::Gcp::Kubernetes::CreateOrUpdateServiceAccountService do ) ) end + + it 'creates a role and role binding granting knative serving permissions to the service account' do + subject + + expect(WebMock).to have_requested(:put, api_url + "/apis/rbac.authorization.k8s.io/v1/namespaces/#{namespace}/roles/#{Clusters::Gcp::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_NAME}").with( + body: hash_including( + metadata: { + name: Clusters::Gcp::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_NAME, + namespace: namespace + }, + rules: [{ + apiGroups: %w(serving.knative.dev), + resources: %w(configurations configurationgenerations routes revisions revisionuids autoscalers services), + verbs: %w(get list create update delete patch watch) + }] + ) + ) + end end end end diff --git a/spec/services/clusters/refresh_service_spec.rb b/spec/services/clusters/refresh_service_spec.rb deleted file mode 100644 index 5bc8a709941..00000000000 --- a/spec/services/clusters/refresh_service_spec.rb +++ /dev/null @@ -1,113 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Clusters::RefreshService do - shared_examples 'creates a kubernetes namespace' do - let(:token) { 'aaaaaa' } - let(:service_account_creator) { double(Clusters::Gcp::Kubernetes::CreateOrUpdateServiceAccountService, execute: true) } - let(:secrets_fetcher) { double(Clusters::Gcp::Kubernetes::FetchKubernetesTokenService, execute: token) } - - it 'creates a kubernetes namespace' do - expect(Clusters::Gcp::Kubernetes::CreateOrUpdateServiceAccountService).to receive(:namespace_creator).and_return(service_account_creator) - expect(Clusters::Gcp::Kubernetes::FetchKubernetesTokenService).to receive(:new).and_return(secrets_fetcher) - - expect { subject }.to change(project.kubernetes_namespaces, :count) - - kubernetes_namespace = cluster.kubernetes_namespaces.first - expect(kubernetes_namespace).to be_present - expect(kubernetes_namespace.project).to eq(project) - end - end - - shared_examples 'does not create a kubernetes namespace' do - it 'does not create a new kubernetes namespace' do - expect(Clusters::Gcp::Kubernetes::CreateOrUpdateServiceAccountService).not_to receive(:namespace_creator) - expect(Clusters::Gcp::Kubernetes::FetchKubernetesTokenService).not_to receive(:new) - - expect { subject }.not_to change(Clusters::KubernetesNamespace, :count) - end - end - - describe '.create_or_update_namespaces_for_cluster' do - let(:cluster) { create(:cluster, :provided_by_user, :project) } - let(:project) { cluster.project } - - subject { described_class.create_or_update_namespaces_for_cluster(cluster) } - - context 'cluster is project level' do - include_examples 'creates a kubernetes namespace' - - context 'when project already has kubernetes namespace' do - before do - create(:cluster_kubernetes_namespace, project: project, cluster: cluster) - end - - include_examples 'does not create a kubernetes namespace' - end - end - - context 'cluster is group level' do - let(:cluster) { create(:cluster, :provided_by_user, :group) } - let(:group) { cluster.group } - let(:project) { create(:project, group: group) } - - include_examples 'creates a kubernetes namespace' - - context 'when project already has kubernetes namespace' do - before do - create(:cluster_kubernetes_namespace, project: project, cluster: cluster) - end - - include_examples 'does not create a kubernetes namespace' - end - end - end - - describe '.create_or_update_namespaces_for_project' do - let(:project) { create(:project) } - - subject { described_class.create_or_update_namespaces_for_project(project) } - - it 'creates no kubernetes namespaces' do - expect { subject }.not_to change(project.kubernetes_namespaces, :count) - end - - context 'project has a project cluster' do - let!(:cluster) { create(:cluster, :provided_by_gcp, cluster_type: :project_type, projects: [project]) } - - include_examples 'creates a kubernetes namespace' - - context 'when project already has kubernetes namespace' do - before do - create(:cluster_kubernetes_namespace, project: project, cluster: cluster) - end - - include_examples 'does not create a kubernetes namespace' - end - end - - context 'project belongs to a group cluster' do - let!(:cluster) { create(:cluster, :provided_by_gcp, :group) } - - let(:group) { cluster.group } - let(:project) { create(:project, group: group) } - - include_examples 'does not create a kubernetes namespace' - - context 'when project already has kubernetes namespace' do - before do - create(:cluster_kubernetes_namespace, project: project, cluster: cluster) - end - - include_examples 'does not create a kubernetes namespace' - end - end - - context 'cluster is not managed' do - let!(:cluster) { create(:cluster, :project, :not_managed, projects: [project]) } - - include_examples 'does not create a kubernetes namespace' - end - end -end diff --git a/spec/services/create_snippet_service_spec.rb b/spec/services/create_snippet_service_spec.rb index f6b6989b955..9b83f65a17e 100644 --- a/spec/services/create_snippet_service_spec.rb +++ b/spec/services/create_snippet_service_spec.rb @@ -36,6 +36,22 @@ describe CreateSnippetService do end end + describe 'usage counter' do + let(:counter) { Gitlab::UsageDataCounters::SnippetCounter } + + it 'increments count' do + expect do + 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, {}) + end.not_to change { counter.read(:create) } + end + end + def create_snippet(project, user, opts) CreateSnippetService.new(project, user, opts).execute end diff --git a/spec/services/git/base_hooks_service_spec.rb b/spec/services/git/base_hooks_service_spec.rb index 4a2ec769116..874df9a68cd 100644 --- a/spec/services/git/base_hooks_service_spec.rb +++ b/spec/services/git/base_hooks_service_spec.rb @@ -14,6 +14,78 @@ describe Git::BaseHooksService do 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 + def hook_name + :push_hooks + end + + def commits + [] + end + end + + let(:project) { create(:project, :repository) } + + subject { TestService.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) } + + context '#execute_hooks' do + before do + expect(project).to receive(:has_active_hooks?).and_return(active) + end + + context 'active hooks' do + let(:active) { true } + + it 'executes the hooks' do + expect(subject).to receive(:push_data).at_least(:once).and_call_original + expect(project).to receive(:execute_hooks) + + subject.execute + end + end + + context 'inactive hooks' do + let(:active) { false } + + it 'does not execute the hooks' do + expect(subject).not_to receive(:push_data) + expect(project).not_to receive(:execute_hooks) + + subject.execute + end + end + end + + context '#execute_services' do + before do + expect(project).to receive(:has_active_services?).and_return(active) + end + + context 'active services' do + let(:active) { true } + + it 'executes the services' do + expect(subject).to receive(:push_data).at_least(:once).and_call_original + expect(project).to receive(:execute_services) + + subject.execute + end + end + + context 'inactive services' do + let(:active) { false } + + it 'does not execute the services' do + expect(subject).not_to receive(:push_data) + expect(project).not_to receive(:execute_services) + + subject.execute + end + end + end + end + describe 'with remote mirrors' do class TestService < described_class def commits diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb index 23be400059e..2bf7dc32436 100644 --- a/spec/services/git/branch_hooks_service_spec.rb +++ b/spec/services/git/branch_hooks_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe Git::BranchHooksService do include RepoHelpers + include ProjectForksHelper let(:project) { create(:project, :repository) } let(:user) { project.creator } @@ -25,7 +26,7 @@ describe Git::BranchHooksService do end describe "Git Push Data" do - subject(:push_data) { service.execute } + subject(:push_data) { service.send(:push_data) } it 'has expected push data attributes' do is_expected.to match a_hash_including( @@ -109,6 +110,7 @@ describe Git::BranchHooksService do expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) expect(event.push_event_payload.commit_from).to eq(oldrev) expect(event.push_event_payload.commit_to).to eq(newrev) + expect(event.push_event_payload.commit_title).to eq('Change some files') expect(event.push_event_payload.ref).to eq('master') expect(event.push_event_payload.commit_count).to eq(1) end @@ -124,6 +126,7 @@ describe Git::BranchHooksService do expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) expect(event.push_event_payload.commit_from).to be_nil expect(event.push_event_payload.commit_to).to eq(newrev) + expect(event.push_event_payload.commit_title).to eq('Initial commit') expect(event.push_event_payload.ref).to eq('master') expect(event.push_event_payload.commit_count).to be > 1 end @@ -156,9 +159,13 @@ describe Git::BranchHooksService do let(:blank_sha) { Gitlab::Git::BLANK_SHA } def clears_cache(extended: []) - expect(ProjectCacheWorker) - .to receive(:perform_async) - .with(project.id, extended, %i[commit_count repository_size]) + expect(service).to receive(:invalidated_file_types).and_return(extended) + + if extended.present? + expect(ProjectCacheWorker) + .to receive(:perform_async) + .with(project.id, extended, [], false) + end service.execute end @@ -266,10 +273,10 @@ describe Git::BranchHooksService do end describe 'Processing commit messages' do - # Create 4 commits, 2 of which have references. Limiting to 2 commits, we - # expect to see one commit message processor enqueued. - let(:commit_ids) do - Array.new(4) do |i| + # Create 6 commits, 3 of which have references. Limiting to 4 commits, we + # expect to see two commit message processors enqueued. + let!(:commit_ids) do + Array.new(6) do |i| message = "Issue #{'#' if i.even?}#{i}" project.repository.update_file( user, 'README.md', '', message: message, branch_name: branch @@ -277,18 +284,18 @@ describe Git::BranchHooksService do end end - let(:oldrev) { commit_ids.first } + let(:oldrev) { project.commit(commit_ids.first).parent_id } let(:newrev) { commit_ids.last } before do - stub_const("::Git::BaseHooksService::PROCESS_COMMIT_LIMIT", 2) + stub_const("::Git::BaseHooksService::PROCESS_COMMIT_LIMIT", 4) end context 'creating the default branch' do let(:oldrev) { Gitlab::Git::BLANK_SHA } it 'processes a limited number of commit messages' do - expect(ProcessCommitWorker).to receive(:perform_async).once + expect(ProcessCommitWorker).to receive(:perform_async).twice service.execute end @@ -296,7 +303,7 @@ describe Git::BranchHooksService do context 'updating the default branch' do it 'processes a limited number of commit messages' do - expect(ProcessCommitWorker).to receive(:perform_async).once + expect(ProcessCommitWorker).to receive(:perform_async).twice service.execute end @@ -317,7 +324,7 @@ describe Git::BranchHooksService do let(:oldrev) { Gitlab::Git::BLANK_SHA } it 'processes a limited number of commit messages' do - expect(ProcessCommitWorker).to receive(:perform_async).once + expect(ProcessCommitWorker).to receive(:perform_async).twice service.execute end @@ -327,7 +334,7 @@ describe Git::BranchHooksService do let(:branch) { 'fix' } it 'processes a limited number of commit messages' do - expect(ProcessCommitWorker).to receive(:perform_async).once + expect(ProcessCommitWorker).to receive(:perform_async).twice service.execute end @@ -343,6 +350,55 @@ describe Git::BranchHooksService do service.execute end end + + context 'when the project is forked' do + let(:upstream_project) { project } + 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) + end + + context 'when commits already exists in the upstream project' do + it 'does not process commit messages' do + expect(ProcessCommitWorker).not_to receive(:perform_async) + + forked_service.execute + end + end + + context 'when a commit does not exist in the upstream repo' do + # On top of the existing 6 commits, 3 of which have references, + # create 2 more, 1 of which has a reference. Limiting to 4 commits, we + # expect to see one commit message processor enqueued. + let!(:forked_commit_ids) do + Array.new(2) do |i| + message = "Issue #{'#' if i.even?}#{i}" + forked_project.repository.update_file( + user, 'README.md', '', message: message, branch_name: branch + ) + end + end + + let(:newrev) { forked_commit_ids.last } + + it 'processes the commit message' do + expect(ProcessCommitWorker).to receive(:perform_async).once + + forked_service.execute + end + end + + context 'when the upstream project no longer exists' do + it 'processes the commit messages' do + upstream_project.destroy! + + expect(ProcessCommitWorker).to receive(:perform_async).twice + + forked_service.execute + end + end + end end describe 'New branch detection' do diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index 6e39fa6b3c0..d9e607cd251 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -76,9 +76,28 @@ describe Git::BranchPushService, services: true do stub_ci_pipeline_to_return_yaml_file end + it 'creates a pipeline with the right parameters' do + expect(Ci::CreatePipelineService) + .to receive(:new) + .with(project, + user, + { + before: oldrev, + after: newrev, + ref: ref, + checkout_sha: SeedRepo::Commit::ID, + push_options: {} + }).and_call_original + + subject + end + it "creates a new pipeline" do expect { subject }.to change { Ci::Pipeline.count } - expect(Ci::Pipeline.last).to be_push + + pipeline = Ci::Pipeline.last + expect(pipeline).to be_push + expect(Gitlab::Git::BRANCH_REF_PREFIX + pipeline.ref).to eq(ref) end end @@ -123,6 +142,10 @@ describe Git::BranchPushService, services: true do describe "Webhooks" do context "execute webhooks" do + before do + create(:project_hook, push_events: true, project: project) + end + it "when pushing a branch for the first time" do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") diff --git a/spec/services/git/tag_hooks_service_spec.rb b/spec/services/git/tag_hooks_service_spec.rb index f5938a5c708..e362577d289 100644 --- a/spec/services/git/tag_hooks_service_spec.rb +++ b/spec/services/git/tag_hooks_service_spec.rb @@ -26,7 +26,8 @@ describe Git::TagHooksService, :service do describe 'System hooks' do it 'Executes system hooks' do - push_data = service.execute + push_data = service.send(:push_data) + expect(project).to receive(:has_active_hooks?).and_return(true) expect_next_instance_of(SystemHooksService) do |system_hooks_service| expect(system_hooks_service) @@ -40,6 +41,7 @@ describe Git::TagHooksService, :service do describe "Webhooks" do it "executes hooks on the project" do + expect(project).to receive(:has_active_hooks?).and_return(true) expect(project).to receive(:execute_hooks) service.execute @@ -61,7 +63,7 @@ describe Git::TagHooksService, :service do describe 'Push data' do shared_examples_for 'tag push data expectations' do - subject(:push_data) { service.execute } + 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 418952b52da..7e008637182 100644 --- a/spec/services/git/tag_push_service_spec.rb +++ b/spec/services/git/tag_push_service_spec.rb @@ -26,8 +26,8 @@ describe Git::TagPushService do subject end - it 'flushes the tags cache' do - expect(project.repository).to receive(:expire_tags_cache) + it 'does not flush the tags cache' do + expect(project.repository).not_to receive(:expire_tags_cache) subject end diff --git a/spec/services/groups/auto_devops_service_spec.rb b/spec/services/groups/auto_devops_service_spec.rb index 7f8ab517cef..7591b2f6f12 100644 --- a/spec/services/groups/auto_devops_service_spec.rb +++ b/spec/services/groups/auto_devops_service_spec.rb @@ -47,7 +47,7 @@ describe Groups::AutoDevopsService, '#execute' do expect(subgroup_1.auto_devops_enabled?).to eq(false) end - context 'when subgroups have projects', :nested_groups do + context 'when subgroups have projects' do it 'reflects changes on projects' do subgroup_1 = create(:group, parent: group) project_1 = create(:project, namespace: subgroup_1) diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index c5ff6cdbacd..0f9f20de586 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -44,7 +44,7 @@ describe Groups::CreateService, '#execute' do end end - describe 'creating subgroup', :nested_groups do + describe 'creating subgroup' do let!(:group) { create(:group) } let!(:service) { described_class.new(user, group_params.merge(parent_id: group.id)) } @@ -54,39 +54,31 @@ describe Groups::CreateService, '#execute' do end it { is_expected.to be_persisted } + end - context 'when nested groups feature is disabled' do - it 'does not save group and returns an error' do - allow(Group).to receive(:supports_nested_objects?).and_return(false) + context 'as guest' do + it 'does not save group and returns an error' do + is_expected.not_to be_persisted - is_expected.not_to be_persisted - expect(subject.errors[:parent_id]).to include('You don’t have permission to create a subgroup in this group.') - expect(subject.parent_id).to be_nil - end + expect(subject.errors[:parent_id].first).to eq('You don’t have permission to create a subgroup in this group.') + expect(subject.parent_id).to be_nil end end - context 'when nested groups feature is enabled' do + context 'as owner' do before do - allow(Group).to receive(:supports_nested_objects?).and_return(true) + group.add_owner(user) end - context 'as guest' do - it 'does not save group and returns an error' do - is_expected.not_to be_persisted + it { is_expected.to be_persisted } + end - expect(subject.errors[:parent_id].first).to eq('You don’t have permission to create a subgroup in this group.') - expect(subject.parent_id).to be_nil - end + context 'as maintainer' do + before do + group.add_maintainer(user) end - context 'as owner' do - before do - group.add_owner(user) - end - - it { is_expected.to be_persisted } - end + it { is_expected.to be_persisted } end end diff --git a/spec/services/groups/nested_create_service_spec.rb b/spec/services/groups/nested_create_service_spec.rb index 13acf9e055b..b30392c1b12 100644 --- a/spec/services/groups/nested_create_service_spec.rb +++ b/spec/services/groups/nested_create_service_spec.rb @@ -28,35 +28,7 @@ describe Groups::NestedCreateService do end end - describe 'without subgroups' do - let(:params) { { group_path: 'a-group' } } - - before do - allow(Group).to receive(:supports_nested_objects?) { false } - end - - it 'creates the group' do - group = service.execute - - expect(group).to be_persisted - end - - it 'returns the group if it already existed' do - existing_group = create(:group, path: 'a-group') - - expect(service.execute).to eq(existing_group) - end - - it 'raises an error when tring to create a subgroup' do - service = described_class.new(user, group_path: 'a-group/a-sub-group') - - expect { service.execute }.to raise_error('Nested groups are not supported on MySQL') - end - - it_behaves_like 'with a visibility level' - end - - describe 'with subgroups', :nested_groups do + describe 'with subgroups' do let(:params) { { group_path: 'a-group/a-sub-group' } } describe "#execute" do diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index b5708ebba76..f3af8cf5f3b 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -2,28 +2,13 @@ require 'rails_helper' -describe Groups::TransferService, :postgresql do +describe Groups::TransferService do let(:user) { create(:user) } let(:new_parent_group) { create(:group, :public) } let!(:group_member) { create(:group_member, :owner, group: group, user: user) } let(:transfer_service) { described_class.new(group, user) } shared_examples 'ensuring allowed transfer for a group' do - context 'with other database than PostgreSQL' do - before do - allow(Group).to receive(:supports_nested_objects?).and_return(false) - end - - it 'returns false' do - expect(transfer_service.execute(new_parent_group)).to be_falsy - end - - it 'adds an error on group' do - transfer_service.execute(new_parent_group) - expect(transfer_service.error).to eq('Transfer failed: Database is not supported.') - end - end - context "when there's an exception on GitLab shell directories" do let(:new_parent_group) { create(:group, :public) } diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index d081c20f669..12e9c2b2f3a 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -86,6 +86,7 @@ describe Groups::UpdateService do context "unauthorized visibility_level validation" do let!(:service) { described_class.new(internal_group, user, visibility_level: 99) } + before do internal_group.add_user(user, Gitlab::Access::MAINTAINER) end @@ -96,6 +97,20 @@ describe Groups::UpdateService do end end + context 'when updating #emails_disabled' do + let(:service) { described_class.new(internal_group, user, emails_disabled: true) } + + it 'updates the attribute' do + internal_group.add_user(user, Gitlab::Access::OWNER) + + expect { service.execute }.to change { internal_group.emails_disabled }.to(true) + end + + it 'does not update when not group owner' do + expect { service.execute }.not_to change { internal_group.emails_disabled } + end + end + context 'rename group' do let!(:service) { described_class.new(internal_group, user, path: SecureRandom.hex) } @@ -133,7 +148,7 @@ describe Groups::UpdateService do end end - context 'for a subgroup', :nested_groups do + context 'for a subgroup' do let(:subgroup) { create(:group, :private, parent: private_group) } context 'when the parent group share_with_group_lock is enabled' do diff --git a/spec/services/issuable/bulk_update_service_spec.rb b/spec/services/issuable/bulk_update_service_spec.rb index 3d2d4b5f216..e3a728f2566 100644 --- a/spec/services/issuable/bulk_update_service_spec.rb +++ b/spec/services/issuable/bulk_update_service_spec.rb @@ -16,22 +16,174 @@ describe Issuable::BulkUpdateService do shared_examples 'updates milestones' do it 'succeeds' do - result = bulk_update(issues, milestone_id: milestone.id) + result = bulk_update(issuables, milestone_id: milestone.id) expect(result[:success]).to be_truthy - expect(result[:count]).to eq(issues.count) + expect(result[:count]).to eq(issuables.count) end - it 'updates the issues milestone' do - bulk_update(issues, milestone_id: milestone.id) + it 'updates the issuables milestone' do + bulk_update(issuables, milestone_id: milestone.id) - issues.each do |issue| - expect(issue.reload.milestone).to eq(milestone) + issuables.each do |issuable| + expect(issuable.reload.milestone).to eq(milestone) end end end - context 'with project issues' do + shared_examples 'updating labels' do + def create_issue_with_labels(labels) + create(:labeled_issue, project: project, labels: labels) + end + + let(:issue_all_labels) { create_issue_with_labels([bug, regression, merge_requests]) } + let(:issue_bug_and_regression) { create_issue_with_labels([bug, regression]) } + let(:issue_bug_and_merge_requests) { create_issue_with_labels([bug, merge_requests]) } + let(:issue_no_labels) { create(:issue, project: project) } + let(:issues) { [issue_all_labels, issue_bug_and_regression, issue_bug_and_merge_requests, issue_no_labels] } + + let(:labels) { [] } + let(:add_labels) { [] } + let(:remove_labels) { [] } + + let(:bulk_update_params) do + { + label_ids: labels.map(&:id), + add_label_ids: add_labels.map(&:id), + remove_label_ids: remove_labels.map(&:id) + } + end + + before do + bulk_update(issues, bulk_update_params) + end + + context 'when label_ids are passed' do + let(:issues) { [issue_all_labels, issue_no_labels] } + let(:labels) { [bug, regression] } + + it 'updates the labels of all issues passed to the labels passed' do + expect(issues.map(&:reload).map(&:label_ids)).to all(match_array(labels.map(&:id))) + end + + it 'does not update issues not passed in' do + expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id) + end + + context 'when those label IDs are empty' do + let(:labels) { [] } + + it 'updates the issues passed to have no labels' do + expect(issues.map(&:reload).map(&:label_ids)).to all(be_empty) + end + end + end + + context 'when add_label_ids are passed' do + let(:issues) { [issue_all_labels, issue_bug_and_merge_requests, issue_no_labels] } + let(:add_labels) { [bug, regression, merge_requests] } + + it 'adds those label IDs to all issues passed' do + expect(issues.map(&:reload).map(&:label_ids)).to all(include(*add_labels.map(&:id))) + end + + it 'does not update issues not passed in' do + expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id) + end + end + + context 'when remove_label_ids are passed' do + let(:issues) { [issue_all_labels, issue_bug_and_merge_requests, issue_no_labels] } + let(:remove_labels) { [bug, regression, merge_requests] } + + it 'removes those label IDs from all issues passed' do + expect(issues.map(&:reload).map(&:label_ids)).to all(be_empty) + end + + it 'does not update issues not passed in' do + expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id) + end + end + + context 'when add_label_ids and remove_label_ids are passed' do + let(:issues) { [issue_all_labels, issue_bug_and_merge_requests, issue_no_labels] } + let(:add_labels) { [bug] } + let(:remove_labels) { [merge_requests] } + + it 'adds the label IDs to all issues passed' do + expect(issues.map(&:reload).map(&:label_ids)).to all(include(bug.id)) + end + + it 'removes the label IDs from all issues passed' do + expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(merge_requests.id) + end + + it 'does not update issues not passed in' do + expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id) + end + end + + context 'when add_label_ids and label_ids are passed' do + let(:issues) { [issue_all_labels, issue_bug_and_regression, issue_bug_and_merge_requests] } + let(:labels) { [merge_requests] } + let(:add_labels) { [regression] } + + it 'adds the label IDs to all issues passed' do + expect(issues.map(&:reload).map(&:label_ids)).to all(include(regression.id)) + end + + it 'ignores the label IDs parameter' do + expect(issues.map(&:reload).map(&:label_ids)).to all(include(bug.id)) + end + + it 'does not update issues not passed in' do + expect(issue_no_labels.label_ids).to be_empty + end + end + + context 'when remove_label_ids and label_ids are passed' do + let(:issues) { [issue_no_labels, issue_bug_and_regression] } + let(:labels) { [merge_requests] } + let(:remove_labels) { [regression] } + + it 'removes the label IDs from all issues passed' do + expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(regression.id) + end + + it 'ignores the label IDs parameter' do + expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(merge_requests.id) + end + + it 'does not update issues not passed in' do + expect(issue_all_labels.label_ids).to contain_exactly(bug.id, regression.id, merge_requests.id) + end + end + + context 'when add_label_ids, remove_label_ids, and label_ids are passed' do + let(:issues) { [issue_bug_and_merge_requests, issue_no_labels] } + let(:labels) { [regression] } + let(:add_labels) { [bug] } + let(:remove_labels) { [merge_requests] } + + it 'adds the label IDs to all issues passed' do + expect(issues.map(&:reload).map(&:label_ids)).to all(include(bug.id)) + end + + it 'removes the label IDs from all issues passed' do + expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(merge_requests.id) + end + + it 'ignores the label IDs parameter' do + expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(regression.id) + end + + it 'does not update issues not passed in' do + expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id) + end + end + end + + context 'with issuables at a project level' do describe 'close issues' do let(:issues) { create_list(:issue, 2, project: project) } @@ -165,172 +317,24 @@ describe Issuable::BulkUpdateService do context 'when the new assignee ID is not present' do it 'does not unassign' do expect { bulk_update(issue, assignee_ids: []) } - .not_to change { issue.reload.assignees } + .not_to change(issue.assignees, :count) end end end describe 'updating milestones' do - let(:issues) { [create(:issue, project: project)] } + let(:issuables) { [create(:issue, project: project)] } let(:milestone) { create(:milestone, project: project) } it_behaves_like 'updates milestones' end describe 'updating labels' do - def create_issue_with_labels(labels) - create(:labeled_issue, project: project, labels: labels) - end - let(:bug) { create(:label, project: project) } let(:regression) { create(:label, project: project) } let(:merge_requests) { create(:label, project: project) } - let(:issue_all_labels) { create_issue_with_labels([bug, regression, merge_requests]) } - let(:issue_bug_and_regression) { create_issue_with_labels([bug, regression]) } - let(:issue_bug_and_merge_requests) { create_issue_with_labels([bug, merge_requests]) } - let(:issue_no_labels) { create(:issue, project: project) } - let(:issues) { [issue_all_labels, issue_bug_and_regression, issue_bug_and_merge_requests, issue_no_labels] } - - let(:labels) { [] } - let(:add_labels) { [] } - let(:remove_labels) { [] } - - let(:bulk_update_params) do - { - label_ids: labels.map(&:id), - add_label_ids: add_labels.map(&:id), - remove_label_ids: remove_labels.map(&:id) - } - end - - before do - bulk_update(issues, bulk_update_params) - end - - context 'when label_ids are passed' do - let(:issues) { [issue_all_labels, issue_no_labels] } - let(:labels) { [bug, regression] } - - it 'updates the labels of all issues passed to the labels passed' do - expect(issues.map(&:reload).map(&:label_ids)).to all(match_array(labels.map(&:id))) - end - - it 'does not update issues not passed in' do - expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id) - end - - context 'when those label IDs are empty' do - let(:labels) { [] } - - it 'updates the issues passed to have no labels' do - expect(issues.map(&:reload).map(&:label_ids)).to all(be_empty) - end - end - end - - context 'when add_label_ids are passed' do - let(:issues) { [issue_all_labels, issue_bug_and_merge_requests, issue_no_labels] } - let(:add_labels) { [bug, regression, merge_requests] } - - it 'adds those label IDs to all issues passed' do - expect(issues.map(&:reload).map(&:label_ids)).to all(include(*add_labels.map(&:id))) - end - - it 'does not update issues not passed in' do - expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id) - end - end - - context 'when remove_label_ids are passed' do - let(:issues) { [issue_all_labels, issue_bug_and_merge_requests, issue_no_labels] } - let(:remove_labels) { [bug, regression, merge_requests] } - - it 'removes those label IDs from all issues passed' do - expect(issues.map(&:reload).map(&:label_ids)).to all(be_empty) - end - - it 'does not update issues not passed in' do - expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id) - end - end - - context 'when add_label_ids and remove_label_ids are passed' do - let(:issues) { [issue_all_labels, issue_bug_and_merge_requests, issue_no_labels] } - let(:add_labels) { [bug] } - let(:remove_labels) { [merge_requests] } - - it 'adds the label IDs to all issues passed' do - expect(issues.map(&:reload).map(&:label_ids)).to all(include(bug.id)) - end - - it 'removes the label IDs from all issues passed' do - expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(merge_requests.id) - end - - it 'does not update issues not passed in' do - expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id) - end - end - - context 'when add_label_ids and label_ids are passed' do - let(:issues) { [issue_all_labels, issue_bug_and_regression, issue_bug_and_merge_requests] } - let(:labels) { [merge_requests] } - let(:add_labels) { [regression] } - - it 'adds the label IDs to all issues passed' do - expect(issues.map(&:reload).map(&:label_ids)).to all(include(regression.id)) - end - - it 'ignores the label IDs parameter' do - expect(issues.map(&:reload).map(&:label_ids)).to all(include(bug.id)) - end - - it 'does not update issues not passed in' do - expect(issue_no_labels.label_ids).to be_empty - end - end - - context 'when remove_label_ids and label_ids are passed' do - let(:issues) { [issue_no_labels, issue_bug_and_regression] } - let(:labels) { [merge_requests] } - let(:remove_labels) { [regression] } - - it 'removes the label IDs from all issues passed' do - expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(regression.id) - end - - it 'ignores the label IDs parameter' do - expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(merge_requests.id) - end - - it 'does not update issues not passed in' do - expect(issue_all_labels.label_ids).to contain_exactly(bug.id, regression.id, merge_requests.id) - end - end - - context 'when add_label_ids, remove_label_ids, and label_ids are passed' do - let(:issues) { [issue_bug_and_merge_requests, issue_no_labels] } - let(:labels) { [regression] } - let(:add_labels) { [bug] } - let(:remove_labels) { [merge_requests] } - - it 'adds the label IDs to all issues passed' do - expect(issues.map(&:reload).map(&:label_ids)).to all(include(bug.id)) - end - - it 'removes the label IDs from all issues passed' do - expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(merge_requests.id) - end - - it 'ignores the label IDs parameter' do - expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(regression.id) - end - - it 'does not update issues not passed in' do - expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id) - end - end + it_behaves_like 'updating labels' end describe 'subscribe to issues' do @@ -360,22 +364,45 @@ describe Issuable::BulkUpdateService do end end - context 'with group issues' do + context 'with issuables at a group level' do let(:group) { create(:group) } - context 'updating milestone' do + describe 'updating milestones' do let(:milestone) { create(:milestone, group: group) } - let(:project1) { create(:project, :repository, group: group) } - let(:project2) { create(:project, :repository, group: group) } - let(:issue1) { create(:issue, project: project1) } - let(:issue2) { create(:issue, project: project2) } - let(:issues) { [issue1, issue2] } + let(:project) { create(:project, :repository, group: group) } before do group.add_maintainer(user) end - it_behaves_like 'updates milestones' + context 'when issues' do + let(:issue1) { create(:issue, project: project) } + let(:issue2) { create(:issue, project: project) } + let(:issuables) { [issue1, issue2] } + + it_behaves_like 'updates milestones' + end + + context 'when merge requests' do + let(:merge_request1) { create(:merge_request, source_project: project, source_branch: 'branch-1') } + let(:merge_request2) { create(:merge_request, source_project: project, source_branch: 'branch-2') } + let(:issuables) { [merge_request1, merge_request2] } + + it_behaves_like 'updates milestones' + end + end + + describe 'updating labels' do + let(:project) { create(:project, :repository, group: group) } + let(:bug) { create(:group_label, group: group) } + let(:regression) { create(:group_label, group: group) } + let(:merge_requests) { create(:group_label, group: group) } + + before do + group.add_reporter(user) + end + + it_behaves_like 'updating labels' end end end diff --git a/spec/services/issuable/clone/content_rewriter_spec.rb b/spec/services/issuable/clone/content_rewriter_spec.rb index 230e1123280..3479c20862a 100644 --- a/spec/services/issuable/clone/content_rewriter_spec.rb +++ b/spec/services/issuable/clone/content_rewriter_spec.rb @@ -165,5 +165,18 @@ describe Issuable::Clone::ContentRewriter do expect(note.note_html).not_to eq(new_note.note_html) end end + + context "discussion notes" do + let(:note) { create(:note, noteable: original_issue, note: "sample note", project: project1) } + let!(:discussion) { create(:discussion_note_on_issue, in_reply_to: note, note: "reply to sample note") } + + it 'rewrites discussion correctly' do + subject.execute + + expect(new_issue.notes.count).to eq(original_issue.notes.count) + expect(new_issue.notes.where(discussion_id: discussion.discussion_id).count).to eq(0) + expect(original_issue.notes.where(discussion_id: discussion.discussion_id).count).to eq(1) + end + end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 468e7c286d5..fd9a63b79cc 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -116,7 +116,7 @@ describe Issues::UpdateService, :mailer do expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position) end - context 'when moving issue between issues from different projects', :nested_groups do + context 'when moving issue between issues from different projects' do let(:group) { create(:group) } let(:subgroup) { create(:group, parent: group) } @@ -179,7 +179,7 @@ describe Issues::UpdateService, :mailer do it 'sends email to user2 about assign of new issue and email to user3 about issue unassignment' do deliveries = ActionMailer::Base.deliveries email = deliveries.last - recipients = deliveries.last(2).map(&:to).flatten + recipients = deliveries.last(2).flat_map(&:to) expect(recipients).to include(user2.email, user3.email) expect(email.subject).to include(issue.title) end @@ -226,6 +226,15 @@ describe Issues::UpdateService, :mailer do end end + it 'creates zoom_link_added system note when a zoom link is added to the description' do + update_issue(description: 'Changed description https://zoom.us/j/5873603787') + + note = find_note('added a Zoom call') + + expect(note).not_to be_nil + expect(note.note).to eq('added a Zoom call to this issue') + end + context 'when issue turns confidential' do let(:opts) do { @@ -703,7 +712,7 @@ describe Issues::UpdateService, :mailer do end end - context 'when moving an issue ', :nested_groups do + context 'when moving an issue ' do it 'raises an error for invalid move ids within a project' do opts = { move_between_ids: [9000, 9999] } diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 52f9a305d8f..7dce7f035d4 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -267,15 +267,15 @@ describe Members::DestroyService do expect(group.members.map(&:user)).not_to include(member_user) end - it 'removes the subgroup membership', :postgresql do + it 'removes the subgroup membership' do expect(subgroup.members.map(&:user)).not_to include(member_user) end - it 'removes the subsubgroup membership', :postgresql do + it 'removes the subsubgroup membership' do expect(subsubgroup.members.map(&:user)).not_to include(member_user) end - it 'removes the subsubproject membership', :postgresql do + it 'removes the subsubproject membership' do expect(subsubproject.members.map(&:user)).not_to include(member_user) end diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index 5c3b209086c..f18239f6d39 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -1,5 +1,4 @@ # frozen_string_literal: true - require 'spec_helper' describe MergeRequests::BuildService do @@ -225,6 +224,11 @@ describe MergeRequests::BuildService do let(:label_ids) { [label2.id] } let(:milestone_id) { milestone2.id } + before do + # Guests are not able to assign labels or milestones to an issue + project.add_developer(user) + end + it 'assigns milestone_id and label_ids instead of issue labels and milestone' do expect(merge_request.milestone).to eq(milestone2) expect(merge_request.labels).to match_array([label2]) @@ -479,4 +483,35 @@ describe MergeRequests::BuildService do end end end + + context 'when assigning labels' do + let(:label_ids) { [create(:label, project: project).id] } + + context 'for members with less than developer access' do + it 'is not allowed' do + expect(merge_request.label_ids).to be_empty + end + end + + context 'for users allowed to assign labels' do + before do + project.add_developer(user) + end + + context 'for labels in the project' do + it 'is allowed for developers' do + expect(merge_request.label_ids).to contain_exactly(*label_ids) + end + end + + context 'for unrelated labels' do + let(:project_label) { create(:label, project: project) } + let(:label_ids) { [create(:label).id, project_label.id] } + + it 'only assigns related labels' do + expect(merge_request.label_ids).to contain_exactly(project_label.id) + end + end + end + end end diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index ed48f4b1e44..699f2a98088 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe MergeRequests::CreateService do +describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do include ProjectForksHelper let(:project) { create(:project, :repository) } @@ -285,6 +285,12 @@ describe MergeRequests::CreateService do end end end + + it 'increments the usage data counter of create event' do + counter = Gitlab::UsageDataCounters::MergeRequestCounter + + expect { service.execute }.to change { counter.read(:create) }.by(1) + end end it_behaves_like 'new issuable record that supports quick actions' do diff --git a/spec/services/merge_requests/get_urls_service_spec.rb b/spec/services/merge_requests/get_urls_service_spec.rb index 0933c6d4336..9e7a5260ca4 100644 --- a/spec/services/merge_requests/get_urls_service_spec.rb +++ b/spec/services/merge_requests/get_urls_service_spec.rb @@ -8,8 +8,8 @@ describe MergeRequests::GetUrlsService do let(:project) { create(:project, :public, :repository) } let(:service) { described_class.new(project) } let(:source_branch) { "merge-test" } - let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{source_branch}" } - let(:show_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/#{merge_request.iid}" } + let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{source_branch}" } + let(:show_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/#{merge_request.iid}" } let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } let(:deleted_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 #{Gitlab::Git::BLANK_SHA} refs/heads/#{source_branch}" } let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } @@ -119,7 +119,7 @@ describe MergeRequests::GetUrlsService do let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch" } let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/markdown" } let(:changes) { "#{new_branch_changes}\n#{existing_branch_changes}" } - let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch" } + let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch" } it 'returns 2 urls for both creating new and showing merge request' do result = service.execute(changes) diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb index 6efece64092..a864da0a6fb 100644 --- a/spec/services/merge_requests/mergeability_check_service_spec.rb +++ b/spec/services/merge_requests/mergeability_check_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe MergeRequests::MergeabilityCheckService do +describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shared_state do shared_examples_for 'unmergeable merge request' do it 'updates or keeps merge status as cannot_be_merged' do subject @@ -11,7 +11,11 @@ describe MergeRequests::MergeabilityCheckService do end it 'does not change the merge ref HEAD' do - expect { subject }.not_to change(merge_request, :merge_ref_head) + merge_ref_head = merge_request.merge_ref_head + + subject + + expect(merge_request.reload.merge_ref_head).to eq merge_ref_head end it 'returns ServiceResponse.error' do @@ -56,24 +60,69 @@ describe MergeRequests::MergeabilityCheckService do subject { described_class.new(merge_request).execute } + def execute_within_threads(amount:, retry_lease: true) + threads = [] + + amount.times do + # Let's use a different object for each thread to get closer + # to a real world scenario. + mr = MergeRequest.find(merge_request.id) + + threads << Thread.new do + described_class.new(mr).execute(retry_lease: retry_lease) + end + end + + threads.each(&:join) + threads + end + before do project.add_developer(merge_request.author) end it_behaves_like 'mergeable merge request' - context 'when multiple calls to the service' do - it 'returns success' do - subject - result = subject + context 'when lock is disabled' do + before do + stub_feature_flags(merge_ref_auto_sync_lock: false) + end - expect(result).to be_a(ServiceResponse) - expect(result.success?).to be(true) + it_behaves_like 'mergeable merge request' + end + + context 'when concurrent calls' do + it 'waits first lock and returns "cached" result in subsequent calls' do + threads = execute_within_threads(amount: 3) + results = threads.map { |t| t.value.status } + + expect(results).to contain_exactly(:success, :success, :success) end - it 'second call does not change the merge-ref' do - expect { subject }.to change(merge_request, :merge_ref_head).from(nil) - expect { subject }.not_to change(merge_request, :merge_ref_head) + it 'writes the merge-ref once' do + service = instance_double(MergeRequests::MergeToRefService) + + expect(MergeRequests::MergeToRefService).to receive(:new).once { service } + expect(service).to receive(:execute).once.and_return(success: true) + + execute_within_threads(amount: 3) + end + + it 'resets one merge request upon execution' do + expect_any_instance_of(MergeRequest).to receive(:reset).once + + execute_within_threads(amount: 2) + end + + context 'when retry_lease flag is false' do + it 'the first call succeeds, subsequent concurrent calls get a lock error response' do + threads = execute_within_threads(amount: 3, retry_lease: false) + results = threads.map { |t| [t.value.status, t.value.message] } + + expect(results).to contain_exactly([:error, 'Failed to obtain a lock'], + [:error, 'Failed to obtain a lock'], + [:success, nil]) + end end end @@ -98,8 +147,7 @@ describe MergeRequests::MergeabilityCheckService do context 'when broken' do before do - allow(merge_request).to receive(:broken?) { true } - allow(project.repository).to receive(:can_be_merged?) { false } + expect(merge_request).to receive(:broken?) { true } end it_behaves_like 'unmergeable merge request' @@ -113,10 +161,13 @@ describe MergeRequests::MergeabilityCheckService do end end - context 'when it has conflicts' do - before do - allow(merge_request).to receive(:broken?) { false } - allow(project.repository).to receive(:can_be_merged?) { false } + context 'when it cannot be merged on git' do + let(:merge_request) do + create(:merge_request, + merge_status: :unchecked, + source_branch: 'conflict-resolvable', + source_project: project, + target_branch: 'conflict-start') end it_behaves_like 'unmergeable merge request' diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index 54b9c6dae38..a27fea0c90f 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -11,6 +11,8 @@ describe MergeRequests::PushOptionsHandlerService do let(:service) { described_class.new(project, user, changes, push_options) } let(:source_branch) { 'fix' } let(:target_branch) { 'feature' } + let(:title) { 'my title' } + let(:description) { 'my description' } let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } let(:deleted_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 #{Gitlab::Git::BLANK_SHA} refs/heads/#{source_branch}" } @@ -73,6 +75,26 @@ describe MergeRequests::PushOptionsHandlerService do end end + shared_examples_for 'a service that can set the title of a merge request' do + subject(:last_mr) { MergeRequest.last } + + it 'sets the title' do + service.execute + + expect(last_mr.title).to eq(title) + end + end + + shared_examples_for 'a service that can set the description of a merge request' do + subject(:last_mr) { MergeRequest.last } + + it 'sets the description' do + service.execute + + expect(last_mr.description).to eq(description) + end + end + shared_examples_for 'a service that can set the merge request to merge when pipeline succeeds' do subject(:last_mr) { MergeRequest.last } @@ -90,6 +112,16 @@ describe MergeRequests::PushOptionsHandlerService do end end + shared_examples_for 'a service that can remove the source branch when it is merged' do + subject(:last_mr) { MergeRequest.last } + + it 'returns true to force_remove_source_branch?' do + service.execute + + expect(last_mr.force_remove_source_branch?).to eq(true) + end + end + shared_examples_for 'a service that does not create a merge request' do it do expect { service.execute }.not_to change { MergeRequest.count } @@ -208,6 +240,72 @@ describe MergeRequests::PushOptionsHandlerService do end end + describe '`remove_source_branch` push option' do + let(:push_options) { { remove_source_branch: true } } + + context 'with a new branch' do + let(:changes) { new_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" + + service.execute + + expect(service.errors).to include(error) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, remove_source_branch: true } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can remove the source branch when it is merged' + end + end + + context 'with an existing branch but no open MR' do + let(:changes) { existing_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" + + service.execute + + expect(service.errors).to include(error) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, remove_source_branch: true } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can remove the source branch when it is merged' + end + end + + context 'with an existing branch that has a merge request open' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can remove the source branch when it is merged' + end + + context 'with a deleted branch' do + let(:changes) { deleted_branch_changes } + + it_behaves_like 'a service that does nothing' + end + + context 'with the project default branch' do + let(:changes) { default_branch_changes } + + it_behaves_like 'a service that does nothing' + end + end + describe '`target` push option' do let(:push_options) { { target: target_branch } } @@ -274,6 +372,138 @@ describe MergeRequests::PushOptionsHandlerService do end end + describe '`title` push option' do + let(:push_options) { { title: title } } + + context 'with a new branch' do + let(:changes) { new_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" + + service.execute + + expect(service.errors).to include(error) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, title: title } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the title of a merge request' + end + end + + context 'with an existing branch but no open MR' do + let(:changes) { existing_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" + + service.execute + + expect(service.errors).to include(error) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, title: title } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the title of a merge request' + end + end + + context 'with an existing branch that has a merge request open' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can set the title of a merge request' + end + + context 'with a deleted branch' do + let(:changes) { deleted_branch_changes } + + it_behaves_like 'a service that does nothing' + end + + context 'with the project default branch' do + let(:changes) { default_branch_changes } + + it_behaves_like 'a service that does nothing' + end + end + + describe '`description` push option' do + let(:push_options) { { description: description } } + + context 'with a new branch' do + let(:changes) { new_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" + + service.execute + + expect(service.errors).to include(error) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, description: description } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the description of a merge request' + end + end + + context 'with an existing branch but no open MR' do + let(:changes) { existing_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" + + service.execute + + expect(service.errors).to include(error) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, description: description } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the description of a merge request' + end + end + + context 'with an existing branch that has a merge request open' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can set the description of a merge request' + end + + context 'with a deleted branch' do + let(:changes) { deleted_branch_changes } + + it_behaves_like 'a service that does nothing' + end + + context 'with the project default branch' do + let(:changes) { default_branch_changes } + + it_behaves_like 'a service that does nothing' + end + end + describe 'multiple pushed branches' do let(:push_options) { { create: true } } let(:changes) do diff --git a/spec/services/merge_requests/rebase_service_spec.rb b/spec/services/merge_requests/rebase_service_spec.rb index ee9caaf2f47..7b8c94c86fe 100644 --- a/spec/services/merge_requests/rebase_service_spec.rb +++ b/spec/services/merge_requests/rebase_service_spec.rb @@ -25,7 +25,7 @@ describe MergeRequests::RebaseService do describe '#execute' do context 'when another rebase is already in progress' do before do - allow(merge_request).to receive(:gitaly_rebase_in_progress?).and_return(true) + allow(repository).to receive(:rebase_in_progress?).with(merge_request.id).and_return(true) end it 'saves the error message' do diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index f566d235787..9688e02d6ac 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -91,7 +91,8 @@ describe MergeRequests::UpdateService, :mailer do labels: [], mentioned_users: [user2], assignees: [user3], - total_time_spent: 0 + total_time_spent: 0, + description: "FYI #{user2.to_reference}" } ) end @@ -99,7 +100,7 @@ describe MergeRequests::UpdateService, :mailer do it 'sends email to user2 about assign of new merge request and email to user3 about merge request unassignment' do deliveries = ActionMailer::Base.deliveries email = deliveries.last - recipients = deliveries.last(2).map(&:to).flatten + recipients = deliveries.last(2).flat_map(&:to) expect(recipients).to include(user2.email, user3.email) expect(email.subject).to include(merge_request.title) end @@ -598,7 +599,7 @@ describe MergeRequests::UpdateService, :mailer do feature_visibility_attr = :"#{merge_request.model_name.plural}_access_level" project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE) - expect { update_merge_request(assignee_ids: [assignee]) }.not_to change { merge_request.reload.assignees } + expect { update_merge_request(assignee_ids: [assignee]) }.not_to change(merge_request.assignees, :count) end end end diff --git a/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb b/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb new file mode 100644 index 00000000000..53b7497ae21 --- /dev/null +++ b/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb @@ -0,0 +1,145 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Metrics::Dashboard::CustomMetricEmbedService do + include MetricsDashboardHelpers + + set(:project) { build(:project) } + set(:user) { create(:user) } + set(:environment) { create(:environment, project: project) } + + before do + project.add_maintainer(user) + end + + let(:dashboard_path) { system_dashboard_path } + let(:group) { business_metric_title } + let(:title) { 'title' } + let(:y_label) { 'y_label' } + + describe '.valid_params?' do + let(:valid_params) do + { + embedded: true, + dashboard_path: dashboard_path, + group: group, + title: title, + y_label: y_label + } + end + + subject { described_class.valid_params?(params) } + + let(:params) { valid_params } + + it { is_expected.to be_truthy } + + context 'not embedded' do + let(:params) { valid_params.except(:embedded) } + + it { is_expected.to be_falsey } + end + + context 'non-system dashboard' do + let(:dashboard_path) { '.gitlab/dashboards/test.yml' } + + it { is_expected.to be_falsey } + end + + context 'undefined dashboard' do + let(:params) { valid_params.except(:dashboard_path) } + + it { is_expected.to be_truthy } + end + + context 'non-custom metric group' do + let(:group) { 'Different Group' } + + it { is_expected.to be_falsey } + end + + context 'missing group' do + let(:group) { nil } + + it { is_expected.to be_falsey } + end + + context 'missing title' do + let(:title) { nil } + + it { is_expected.to be_falsey } + end + + context 'undefined y-axis label' do + let(:params) { valid_params.except(:y_label) } + + it { is_expected.to be_falsey } + end + end + + describe '#get_dashboard' do + let(:service_params) do + [ + project, + user, + { + embedded: true, + environment: environment, + dashboard_path: dashboard_path, + group: group, + title: title, + y_label: y_label + } + ] + end + + let(:service_call) { described_class.new(*service_params).get_dashboard } + + it_behaves_like 'misconfigured dashboard service response', :not_found + it_behaves_like 'raises error for users with insufficient permissions' + + context 'the custom metric exists' do + let!(:metric) { create(:prometheus_metric, project: project) } + + it_behaves_like 'valid embedded dashboard service response' + + it 'does not cache the unprocessed dashboard' do + expect(Gitlab::Metrics::Dashboard::Cache).not_to receive(:fetch) + + described_class.new(*service_params).get_dashboard + end + + context 'multiple metrics meet criteria' do + let!(:metric_2) { create(:prometheus_metric, project: project, query: 'avg(metric_2)') } + + it_behaves_like 'valid embedded dashboard service response' + + it 'includes both metrics' do + result = service_call + included_queries = all_queries(result[:dashboard]) + + expect(included_queries).to include('avg(metric_2)', 'avg(metric)') + end + end + end + + context 'when the metric exists in another project' do + let!(:metric) { create(:prometheus_metric, project: create(:project)) } + + it_behaves_like 'misconfigured dashboard service response', :not_found + end + end + + private + + def all_queries(dashboard) + dashboard[:panel_groups].flat_map do |group| + group[:panels].flat_map do |panel| + panel[:metrics].map do |metric| + metric[:query_range] + end + end + end + end +end diff --git a/spec/services/metrics/dashboard/default_embed_service_spec.rb b/spec/services/metrics/dashboard/default_embed_service_spec.rb new file mode 100644 index 00000000000..803b9a93be7 --- /dev/null +++ b/spec/services/metrics/dashboard/default_embed_service_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Metrics::Dashboard::DefaultEmbedService, :use_clean_rails_memory_store_caching do + include MetricsDashboardHelpers + + set(:project) { build(:project) } + set(:user) { create(:user) } + set(:environment) { create(:environment, project: project) } + + before do + project.add_maintainer(user) + end + + describe '#get_dashboard' do + let(:service_params) { [project, user, { environment: environment }] } + let(:service_call) { described_class.new(*service_params).get_dashboard } + + it_behaves_like 'valid embedded dashboard service response' + it_behaves_like 'raises error for users with insufficient permissions' + + it 'caches the unprocessed dashboard for subsequent calls' do + system_service = Metrics::Dashboard::SystemDashboardService + + expect(system_service).to receive(:new).once.and_call_original + + described_class.new(*service_params).get_dashboard + described_class.new(*service_params).get_dashboard + end + + context 'when called with a non-system dashboard' do + let(:dashboard_path) { 'garbage/dashboard/path' } + + it_behaves_like 'valid embedded dashboard service response' + end + end +end diff --git a/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb b/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb new file mode 100644 index 00000000000..a0f7315f750 --- /dev/null +++ b/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb @@ -0,0 +1,151 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Metrics::Dashboard::DynamicEmbedService, :use_clean_rails_memory_store_caching do + include MetricsDashboardHelpers + + set(:project) { build(:project) } + set(:user) { create(:user) } + set(:environment) { create(:environment, project: project) } + + before do + project.add_maintainer(user) + end + + let(:dashboard_path) { '.gitlab/dashboards/test.yml' } + let(:group) { 'Group A' } + let(:title) { 'Super Chart A1' } + let(:y_label) { 'y_label' } + + describe '.valid_params?' do + let(:valid_params) do + { + embedded: true, + dashboard_path: dashboard_path, + group: group, + title: title, + y_label: y_label + } + end + + subject { described_class.valid_params?(params) } + + let(:params) { valid_params } + + it { is_expected.to be_truthy } + + context 'not embedded' do + let(:params) { valid_params.except(:embedded) } + + it { is_expected.to be_falsey } + end + + context 'undefined dashboard' do + let(:params) { valid_params.except(:dashboard_path) } + + it { is_expected.to be_truthy } + end + + context 'missing dashboard' do + let(:dashboard) { '' } + + it { is_expected.to be_truthy } + end + + context 'missing group' do + let(:group) { '' } + + it { is_expected.to be_falsey } + end + + context 'missing title' do + let(:title) { '' } + + it { is_expected.to be_falsey } + end + + context 'undefined y-axis label' do + let(:params) { valid_params.except(:y_label) } + + it { is_expected.to be_falsey } + end + end + + describe '#get_dashboard' do + let(:service_params) do + [ + project, + user, + { + environment: environment, + dashboard_path: dashboard_path, + group: group, + title: title, + y_label: y_label + } + ] + end + + let(:service_call) { described_class.new(*service_params).get_dashboard } + + context 'when the dashboard does not exist' do + it_behaves_like 'misconfigured dashboard service response', :not_found + end + + context 'when the dashboard is exists' do + let(:project) { project_with_dashboard(dashboard_path) } + + it_behaves_like 'valid embedded dashboard service response' + it_behaves_like 'raises error for users with insufficient permissions' + + it 'caches the unprocessed dashboard for subsequent calls' do + expect(YAML).to receive(:safe_load).once.and_call_original + + described_class.new(*service_params).get_dashboard + described_class.new(*service_params).get_dashboard + end + + context 'when the specified group is not present on the dashboard' do + let(:group) { 'Group Not Found' } + + it_behaves_like 'misconfigured dashboard service response', :not_found + end + + context 'when the specified title is not present on the dashboard' do + let(:title) { 'Title Not Found' } + + it_behaves_like 'misconfigured dashboard service response', :not_found + end + + context 'when the specified y-axis label is not present on the dashboard' do + let(:y_label) { 'Y-Axis Not Found' } + + it_behaves_like 'misconfigured dashboard service response', :not_found + end + end + + shared_examples 'uses system dashboard' do + it 'uses the default dashboard' do + expect(Gitlab::Metrics::Dashboard::Finder) + .to receive(:find_raw) + .with(project, dashboard_path: system_dashboard_path) + .once + + service_call + end + end + + context 'when the dashboard is nil' do + let(:dashboard_path) { nil } + + it_behaves_like 'uses system dashboard' + end + + context 'when the dashboard is not present' do + let(:dashboard_path) { '' } + + it_behaves_like 'uses system dashboard' + end + end +end diff --git a/spec/services/metrics/dashboard/project_dashboard_service_spec.rb b/spec/services/metrics/dashboard/project_dashboard_service_spec.rb new file mode 100644 index 00000000000..1357914be2a --- /dev/null +++ b/spec/services/metrics/dashboard/project_dashboard_service_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Metrics::Dashboard::ProjectDashboardService, :use_clean_rails_memory_store_caching do + include MetricsDashboardHelpers + + set(:user) { create(:user) } + set(:project) { create(:project) } + set(:environment) { create(:environment, project: project) } + + before do + project.add_maintainer(user) + end + + describe 'get_dashboard' do + let(:dashboard_path) { '.gitlab/dashboards/test.yml' } + let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] } + let(:service_call) { described_class.new(*service_params).get_dashboard } + + context 'when the dashboard does not exist' do + it_behaves_like 'misconfigured dashboard service response', :not_found + end + + it_behaves_like 'raises error for users with insufficient permissions' + + context 'when the dashboard exists' do + let(:project) { project_with_dashboard(dashboard_path) } + + it_behaves_like 'valid dashboard service response' + + it 'caches the unprocessed dashboard for subsequent calls' do + expect_any_instance_of(described_class) + .to receive(:get_raw_dashboard) + .once + .and_call_original + + described_class.new(*service_params).get_dashboard + described_class.new(*service_params).get_dashboard + end + + context 'and the dashboard is then deleted' do + it 'does not return the previously cached dashboard' do + described_class.new(*service_params).get_dashboard + + delete_project_dashboard(project, user, dashboard_path) + + expect_any_instance_of(described_class) + .to receive(:get_raw_dashboard) + .once + .and_call_original + + described_class.new(*service_params).get_dashboard + end + end + end + + context 'when the dashboard is configured incorrectly' do + let(:project) { project_with_dashboard(dashboard_path, {}) } + + it_behaves_like 'misconfigured dashboard service response', :unprocessable_entity + end + end + + describe '::all_dashboard_paths' do + let(:all_dashboards) { described_class.all_dashboard_paths(project) } + + context 'when there are no project dashboards' do + it 'returns an empty array' do + expect(all_dashboards).to be_empty + end + end + + context 'when there are project dashboards available' do + let(:dashboard_path) { '.gitlab/dashboards/test.yml' } + let(:project) { project_with_dashboard(dashboard_path) } + + it 'returns the dashboard attributes' do + expect(all_dashboards).to eq( + [{ + path: dashboard_path, + display_name: 'test.yml', + default: false + }] + ) + end + end + end +end diff --git a/spec/services/metrics/dashboard/system_dashboard_service_spec.rb b/spec/services/metrics/dashboard/system_dashboard_service_spec.rb new file mode 100644 index 00000000000..8be3e7f6064 --- /dev/null +++ b/spec/services/metrics/dashboard/system_dashboard_service_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Metrics::Dashboard::SystemDashboardService, :use_clean_rails_memory_store_caching do + include MetricsDashboardHelpers + + set(:user) { create(:user) } + set(:project) { create(:project) } + set(:environment) { create(:environment, project: project) } + + before do + project.add_maintainer(user) + end + + describe 'get_dashboard' do + let(:dashboard_path) { described_class::SYSTEM_DASHBOARD_PATH } + let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] } + let(:service_call) { described_class.new(*service_params).get_dashboard } + + it_behaves_like 'valid dashboard service response' + it_behaves_like 'raises error for users with insufficient permissions' + + it 'caches the unprocessed dashboard for subsequent calls' do + expect(YAML).to receive(:safe_load).once.and_call_original + + described_class.new(*service_params).get_dashboard + described_class.new(*service_params).get_dashboard + end + + context 'when called with a non-system dashboard' do + let(:dashboard_path) { 'garbage/dashboard/path' } + + # We want to alwaus return the system dashboard. + it_behaves_like 'valid dashboard service response' + end + end + + describe '::all_dashboard_paths' do + it 'returns the dashboard attributes' do + all_dashboards = described_class.all_dashboard_paths(project) + + expect(all_dashboards).to eq( + [{ + path: described_class::SYSTEM_DASHBOARD_PATH, + display_name: described_class::SYSTEM_DASHBOARD_NAME, + default: true + }] + ) + end + end +end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 46abd8f356a..cd4ea9c401d 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -365,5 +365,43 @@ describe Notes::CreateService do .and change { existing_note.updated_at } end end + + describe "usage counter" do + let(:counter) { Gitlab::UsageDataCounters::NoteCounter } + + context 'snippet note' do + let(:snippet) { create(:project_snippet, project: project) } + let(:opts) { { note: 'reply', noteable_type: 'Snippet', noteable_id: snippet.id, project: project } } + + it 'increments usage counter' do + expect do + note = described_class.new(project, user, opts).execute + + expect(note).to be_valid + end.to change { counter.read(:create, opts[:noteable_type]) }.by 1 + end + + it 'does not increment usage counter when creation fails' do + expect do + note = described_class.new(project, user, { note: '' }).execute + + expect(note).to be_invalid + end.not_to change { counter.read(:create, opts[:noteable_type]) } + end + end + + context 'issue note' do + let(:issue) { create(:issue, project: project) } + let(:opts) { { note: 'reply', noteable_type: 'Issue', noteable_id: issue.id, project: project } } + + it 'does not increment usage counter' do + expect do + note = described_class.new(project, user, opts).execute + + expect(note).to be_valid + end.not_to change { counter.read(:create, opts[:noteable_type]) } + end + end + end end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index f25e2fe5e2b..d925aa2b6c3 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -111,7 +111,7 @@ describe NotificationService, :mailer do should_email(participant) end - context 'for subgroups', :nested_groups do + context 'for subgroups' do before do build_group(project) end @@ -215,13 +215,14 @@ describe NotificationService, :mailer do let(:project) { create(:project, :private) } let(:issue) { create(:issue, project: project, assignees: [assignee]) } let(:mentioned_issue) { create(:issue, assignees: issue.assignees) } - let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@mention referenced, @unsubscribed_mentioned and @outsider also') } + let(:author) { create(:user) } + let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@mention referenced, @unsubscribed_mentioned and @outsider also') } before do - build_team(note.project) + build_team(project) project.add_maintainer(issue.author) project.add_maintainer(assignee) - project.add_maintainer(note.author) + project.add_maintainer(author) @u_custom_off = create_user_with_notification(:custom, 'custom_off') project.add_guest(@u_custom_off) @@ -239,43 +240,50 @@ describe NotificationService, :mailer do end describe '#new_note' do - it do - add_users_with_subscription(note.project, issue) - reset_delivered_emails! + context do + before do + add_users(project) + add_user_subscriptions(issue) + reset_delivered_emails! + end - expect(SentNotification).to receive(:record).with(issue, any_args).exactly(10).times + it do + expect(SentNotification).to receive(:record).with(issue, any_args).exactly(10).times - notification.new_note(note) + notification.new_note(note) - should_email(@u_watcher) - should_email(note.noteable.author) - should_email(note.noteable.assignees.first) - should_email(@u_custom_global) - should_email(@u_mentioned) - should_email(@subscriber) - should_email(@watcher_and_subscriber) - should_email(@subscribed_participant) - should_email(@u_custom_off) - should_email(@unsubscribed_mentioned) - should_not_email(@u_guest_custom) - should_not_email(@u_guest_watcher) - should_not_email(note.author) - should_not_email(@u_participating) - should_not_email(@u_disabled) - should_not_email(@unsubscriber) - should_not_email(@u_outsider_mentioned) - should_not_email(@u_lazy_participant) - end + should_email(@u_watcher) + should_email(note.noteable.author) + should_email(note.noteable.assignees.first) + should_email(@u_custom_global) + should_email(@u_mentioned) + should_email(@subscriber) + should_email(@watcher_and_subscriber) + should_email(@subscribed_participant) + should_email(@u_custom_off) + should_email(@unsubscribed_mentioned) + should_not_email(@u_guest_custom) + should_not_email(@u_guest_watcher) + should_not_email(note.author) + should_not_email(@u_participating) + should_not_email(@u_disabled) + should_not_email(@unsubscriber) + should_not_email(@u_outsider_mentioned) + should_not_email(@u_lazy_participant) + end - it "emails the note author if they've opted into notifications about their activity" do - add_users_with_subscription(note.project, issue) - reset_delivered_emails! + it "emails the note author if they've opted into notifications about their activity" do + note.author.notified_of_own_activity = true - note.author.notified_of_own_activity = true + notification.new_note(note) - notification.new_note(note) + should_email(note.author) + end - should_email(note.author) + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end end it 'filters out "mentioned in" notes' do @@ -334,7 +342,12 @@ describe NotificationService, :mailer do it_behaves_like 'new note notifications' - context 'which is a subgroup', :nested_groups do + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end + + context 'which is a subgroup' do let!(:parent) { create(:group) } let!(:group) { create(:group, parent: parent) } @@ -385,7 +398,7 @@ describe NotificationService, :mailer do should_email(admin) end - context 'on project that belongs to subgroup', :nested_groups do + context 'on project that belongs to subgroup' do let(:group_reporter) { create(:user) } let(:group_guest) { create(:user) } let(:parent_group) { create(:group) } @@ -415,13 +428,15 @@ describe NotificationService, :mailer do let(:project) { create(:project, :public) } let(:issue) { create(:issue, project: project, assignees: [assignee]) } let(:mentioned_issue) { create(:issue, assignees: issue.assignees) } - let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@all mentioned') } + let(:author) { create(:user) } + let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@all mentioned') } before do - build_team(note.project) - build_group(note.project) - note.project.add_maintainer(note.author) - add_users_with_subscription(note.project, issue) + build_team(project) + build_group(project) + add_users(project) + add_user_subscriptions(issue) + project.add_maintainer(author) reset_delivered_emails! end @@ -453,7 +468,7 @@ describe NotificationService, :mailer do should_not_email_nested_group_user(@pg_disabled) end - it 'notifies parent group members with mention level', :nested_groups do + it 'notifies parent group members with mention level' do note = create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: "@#{@pg_mention.username}") notification.new_note(note) @@ -467,23 +482,29 @@ describe NotificationService, :mailer do expect(Notify).not_to receive(:note_issue_email) notification.new_note(mentioned_note) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end end end context 'project snippet note' do let!(:project) { create(:project, :public) } let(:snippet) { create(:project_snippet, project: project, author: create(:user)) } - let(:note) { create(:note_on_project_snippet, noteable: snippet, project_id: project.id, note: '@all mentioned') } + let(:author) { create(:user) } + let(:note) { create(:note_on_project_snippet, author: author, noteable: snippet, project_id: project.id, note: '@all mentioned') } before do build_team(project) build_group(project) + project.add_maintainer(author) # make sure these users can read the project snippet! project.add_guest(@u_guest_watcher) project.add_guest(@u_guest_custom) add_member_for_parent_group(@pg_watcher, project) - note.project.add_maintainer(note.author) reset_delivered_emails! end @@ -613,6 +634,11 @@ describe NotificationService, :mailer do notification.new_note(note) should_not_email(@u_committer) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end end end @@ -639,6 +665,11 @@ describe NotificationService, :mailer do .to contain_exactly(*merge_request.assignees.pluck(:id), merge_request.author.id, @u_watcher.id) expect(SentNotification.last.in_reply_to_discussion_id).to eq(note.discussion_id) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end end end end @@ -708,10 +739,11 @@ describe NotificationService, :mailer do let(:issue) { create :issue, project: project, assignees: [assignee], description: 'cc @participant @unsubscribed_mentioned' } before do - build_team(issue.project) - build_group(issue.project) + build_team(project) + build_group(project) - add_users_with_subscription(issue.project, issue) + add_users(project) + add_user_subscriptions(issue) reset_delivered_emails! update_custom_notification(:new_issue, @u_guest_custom, resource: project) update_custom_notification(:new_issue, @u_custom_global) @@ -812,6 +844,11 @@ describe NotificationService, :mailer do should_email(user_4) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.new_issue(issue, @u_disabled) } + end + context 'confidential issues' do let(:author) { create(:user) } let(:assignee) { create(:user) } @@ -854,6 +891,11 @@ describe NotificationService, :mailer do let(:mentionable) { issue } include_examples 'notifications for new mentions' + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { send_notifications(@u_watcher, @u_participant_mentioned, @u_custom_global, @u_mentioned) } + end end describe '#reassigned_issue' do @@ -962,6 +1004,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled, [assignee]) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled, [assignee]) } + end end describe '#relabeled_issue' do @@ -1021,6 +1068,11 @@ describe NotificationService, :mailer do should_email(subscriber_to_both) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.relabeled_issue(issue, [group_label_2, label_2], @u_disabled) } + end + context 'confidential issues' do let(:author) { create(:user) } let(:assignee) { create(:user) } @@ -1058,12 +1110,19 @@ describe NotificationService, :mailer do end describe '#removed_milestone_issue' do - it_behaves_like 'altered milestone notification on issue' do + context do let(:milestone) { create(:milestone, project: project, issues: [issue]) } let!(:subscriber_to_new_milestone) { create(:user) { |u| issue.toggle_subscription(u, project) } } - before do - notification.removed_milestone_issue(issue, issue.author) + it_behaves_like 'altered milestone notification on issue' do + before do + notification.removed_milestone_issue(issue, issue.author) + end + end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.removed_milestone_issue(issue, issue.author) } end end @@ -1103,12 +1162,19 @@ describe NotificationService, :mailer do end describe '#changed_milestone_issue' do - it_behaves_like 'altered milestone notification on issue' do + context do let(:new_milestone) { create(:milestone, project: project, issues: [issue]) } let!(:subscriber_to_new_milestone) { create(:user) { |u| issue.toggle_subscription(u, project) } } - before do - notification.changed_milestone_issue(issue, new_milestone, issue.author) + it_behaves_like 'altered milestone notification on issue' do + before do + notification.changed_milestone_issue(issue, new_milestone, issue.author) + end + end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.changed_milestone_issue(issue, new_milestone, issue.author) } end end @@ -1176,6 +1242,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.close_issue(issue, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.close_issue(issue, @u_disabled) } + end end describe '#reopen_issue' do @@ -1207,6 +1278,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.reopen_issue(issue, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.reopen_issue(issue, @u_disabled) } + end end describe '#issue_moved' do @@ -1233,6 +1309,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.issue_moved(issue, new_issue, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.issue_moved(issue, new_issue, @u_disabled) } + end end describe '#issue_due' do @@ -1273,6 +1354,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.issue_due(issue) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.issue_due(issue) } + end end end @@ -1281,13 +1367,16 @@ describe NotificationService, :mailer do let(:project) { create(:project, :public, :repository, namespace: group) } let(:another_project) { create(:project, :public, namespace: group) } let(:assignee) { create(:user) } - let(:merge_request) { create :merge_request, source_project: project, assignees: [assignee], description: 'cc @participant' } + let(:assignees) { Array.wrap(assignee) } + let(:author) { create(:user) } + let(:merge_request) { create :merge_request, author: author, source_project: project, assignees: assignees, description: 'cc @participant' } before do - project.add_maintainer(merge_request.author) - merge_request.assignees.each { |assignee| project.add_maintainer(assignee) } - build_team(merge_request.target_project) - add_users_with_subscription(merge_request.target_project, merge_request) + project.add_maintainer(author) + assignees.each { |assignee| project.add_maintainer(assignee) } + build_team(project) + add_users(project) + add_user_subscriptions(merge_request) update_custom_notification(:new_merge_request, @u_guest_custom, resource: project) update_custom_notification(:new_merge_request, @u_custom_global) reset_delivered_emails! @@ -1364,6 +1453,11 @@ describe NotificationService, :mailer do should_email(user_4) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.new_merge_request(merge_request, @u_disabled) } + end + context 'participating' do it_should_behave_like 'participating by assignee notification' do let(:participant) { create(:user, username: 'user-participant')} @@ -1396,6 +1490,11 @@ describe NotificationService, :mailer do let(:mentionable) { merge_request } include_examples 'notifications for new mentions' + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { send_notifications(@u_watcher, @u_participant_mentioned, @u_custom_global, @u_mentioned) } + end end describe '#reassigned_merge_request' do @@ -1439,6 +1538,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.reassigned_merge_request(merge_request, current_user, [assignee]) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.reassigned_merge_request(merge_request, current_user, [assignee]) } + end end describe '#push_to_merge_request' do @@ -1469,6 +1573,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.push_to_merge_request(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.push_to_merge_request(merge_request, @u_disabled) } + end end describe '#relabel_merge_request' do @@ -1502,28 +1611,43 @@ describe NotificationService, :mailer do should_not_email(@u_participating) should_not_email(@u_lazy_participant) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.relabeled_merge_request(merge_request, [group_label_2, label_2], @u_disabled) } + end end describe '#removed_milestone_merge_request' do - it_behaves_like 'altered milestone notification on merge request' do - let(:milestone) { create(:milestone, project: project, merge_requests: [merge_request]) } - let!(:subscriber_to_new_milestone) { create(:user) { |u| merge_request.toggle_subscription(u, project) } } + let(:milestone) { create(:milestone, project: project, merge_requests: [merge_request]) } + let!(:subscriber_to_new_milestone) { create(:user) { |u| merge_request.toggle_subscription(u, project) } } + it_behaves_like 'altered milestone notification on merge request' do before do notification.removed_milestone_merge_request(merge_request, merge_request.author) end end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.removed_milestone_merge_request(merge_request, merge_request.author) } + end end describe '#changed_milestone_merge_request' do - it_behaves_like 'altered milestone notification on merge request' do - let(:new_milestone) { create(:milestone, project: project, merge_requests: [merge_request]) } - let!(:subscriber_to_new_milestone) { create(:user) { |u| merge_request.toggle_subscription(u, project) } } + let(:new_milestone) { create(:milestone, project: project, merge_requests: [merge_request]) } + let!(:subscriber_to_new_milestone) { create(:user) { |u| merge_request.toggle_subscription(u, project) } } + it_behaves_like 'altered milestone notification on merge request' do before do notification.changed_milestone_merge_request(merge_request, new_milestone, merge_request.author) end end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.changed_milestone_merge_request(merge_request, new_milestone, merge_request.author) } + end end describe '#merge_request_unmergeable' do @@ -1534,6 +1658,11 @@ describe NotificationService, :mailer do expect(email_recipients.size).to eq(1) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.merge_request_unmergeable(merge_request) } + end + describe 'when merge_when_pipeline_succeeds is true' do before do merge_request.update( @@ -1580,6 +1709,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.close_mr(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.close_mr(merge_request, @u_disabled) } + end end describe '#merged_merge_request' do @@ -1632,6 +1766,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.merge_mr(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.merge_mr(merge_request, @u_disabled) } + end end describe '#reopen_merge_request' do @@ -1662,6 +1801,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.reopen_mr(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.reopen_mr(merge_request, @u_disabled) } + end end describe "#resolve_all_discussions" do @@ -1685,6 +1829,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.resolve_all_discussions(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.resolve_all_discussions(merge_request, @u_disabled) } + end end end @@ -1709,6 +1858,11 @@ describe NotificationService, :mailer do should_not_email(@u_disabled) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.project_was_moved(project, "gitlab/gitlab") } + end + context 'users not having access to the new location' do it 'does not send email' do old_user = create(:user) @@ -1752,6 +1906,11 @@ describe NotificationService, :mailer do should_only_email(@u_participating) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.project_exported(project, @u_participating) } + end end describe '#project_not_exported' do @@ -1760,6 +1919,11 @@ describe NotificationService, :mailer do should_only_email(@u_participating) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.project_not_exported(project, @u_participating, ['error']) } + end end end end @@ -1790,6 +1954,11 @@ describe NotificationService, :mailer do should_email(maintainer) should_not_email(developer) end + + it_behaves_like 'group emails are disabled' do + let(:notification_target) { group } + let(:notification_trigger) { group.request_access(added_user) } + end end describe '#decline_group_invite' do @@ -1829,12 +1998,17 @@ describe NotificationService, :mailer do should_not_email_anyone end end + + it_behaves_like 'group emails are disabled' do + let(:notification_target) { group } + let(:notification_trigger) { group.add_guest(added_user) } + end end end describe 'ProjectMember' do let(:project) { create(:project) } - set(:added_user) { create(:user) } + let(:added_user) { create(:user) } describe '#new_access_request' do context 'for a project in a user namespace' do @@ -1849,6 +2023,11 @@ describe NotificationService, :mailer do should_only_email(project.owner) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { project.request_access(added_user) } + end end context 'for a project in a group' do @@ -1868,7 +2047,7 @@ describe NotificationService, :mailer do end end - describe '#decline_group_invite' do + describe '#decline_project_invite' do let(:member) { create(:user) } before do @@ -1890,6 +2069,11 @@ describe NotificationService, :mailer do should_only_email(added_user) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { create_member! } + end + context 'when notifications are disabled' do before do create_global_setting_for(added_user, :disabled) @@ -2053,27 +2237,69 @@ describe NotificationService, :mailer do end context 'when the creator has custom notifications enabled' do - before do - pipeline = create_pipeline(u_custom_notification_enabled, :success) - notification.pipeline_finished(pipeline) - end + let(:pipeline) { create_pipeline(u_custom_notification_enabled, :success) } it 'emails only the creator' do + notification.pipeline_finished(pipeline) + should_only_email(u_custom_notification_enabled, kind: :bcc) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { pipeline } + let(:notification_trigger) { notification.pipeline_finished(pipeline) } + end + + context 'when the creator has group notification email set' do + let(:group_notification_email) { 'user+group@example.com' } + + before do + group = create(:group) + + project.update(group: group) + create(:notification_setting, user: u_custom_notification_enabled, source: group, notification_email: group_notification_email) + end + + it 'sends to group notification email' do + notification.pipeline_finished(pipeline) + + expect(email_recipients(kind: :bcc).first).to eq(group_notification_email) + end + end end end context 'with a failed pipeline' do context 'when the creator has no custom notification set' do - before do - pipeline = create_pipeline(u_member, :failed) - notification.pipeline_finished(pipeline) - end + let(:pipeline) { create_pipeline(u_member, :failed) } it 'emails only the creator' do + notification.pipeline_finished(pipeline) + should_only_email(u_member, kind: :bcc) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { pipeline } + let(:notification_trigger) { notification.pipeline_finished(pipeline) } + end + + context 'when the creator has group notification email set' do + let(:group_notification_email) { 'user+group@example.com' } + + before do + group = create(:group) + + project.update(group: group) + create(:notification_setting, user: u_member, source: group, notification_email: group_notification_email) + end + + it 'sends to group notification email' do + notification.pipeline_finished(pipeline) + + expect(email_recipients(kind: :bcc).first).to eq(group_notification_email) + end + end end context 'when the creator has watch set' do @@ -2173,6 +2399,11 @@ describe NotificationService, :mailer do should_only_email(u_maintainer1, u_maintainer2, u_owner) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { domain } + let(:notification_trigger) { notify! } + end + it 'emails nobody if the project is missing' do domain.project = nil @@ -2182,30 +2413,6 @@ describe NotificationService, :mailer do end end end - - describe '#pages_domain_verification_failed' do - it 'emails current watching maintainers' do - notification.pages_domain_verification_failed(domain) - - should_only_email(u_maintainer1, u_maintainer2, u_owner) - end - end - - describe '#pages_domain_enabled' do - it 'emails current watching maintainers' do - notification.pages_domain_enabled(domain) - - should_only_email(u_maintainer1, u_maintainer2, u_owner) - end - end - - describe '#pages_domain_disabled' do - it 'emails current watching maintainers' do - notification.pages_domain_disabled(domain) - - should_only_email(u_maintainer1, u_maintainer2, u_owner) - end - end end context 'Auto DevOps notifications' do @@ -2224,6 +2431,11 @@ describe NotificationService, :mailer do should_email(owner, times: 1) # Once for the disable pipeline. should_email(pipeline_user, times: 2) # Once for the new permission, once for the disable. end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.autodevops_disabled(pipeline, [owner.email, pipeline_user.email]) } + end end end @@ -2237,6 +2449,11 @@ describe NotificationService, :mailer do should_email(user) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.repository_cleanup_success(project, user) } + end end describe '#repository_cleanup_failure' do @@ -2245,6 +2462,11 @@ describe NotificationService, :mailer do should_email(user) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.repository_cleanup_failure(project, user, 'Some error') } + end end end @@ -2278,6 +2500,11 @@ describe NotificationService, :mailer do should_only_email(u_maintainer1, u_maintainer2, u_owner) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.remote_mirror_update_failed(remote_mirror) } + end end end @@ -2368,56 +2595,44 @@ describe NotificationService, :mailer do group end - # Creates a nested group only if supported - # to avoid errors on MySQL def create_nested_group(visibility) - if Group.supports_nested_objects? - parent_group = create(:group, visibility) - child_group = create(:group, visibility, parent: parent_group) + parent_group = create(:group, visibility) + child_group = create(:group, visibility, parent: parent_group) - # Parent group member: global=disabled, parent_group=watch, child_group=global - @pg_watcher ||= create_user_with_notification(:watch, 'parent_group_watcher', parent_group) - @pg_watcher.notification_settings_for(nil).disabled! + # Parent group member: global=disabled, parent_group=watch, child_group=global + @pg_watcher ||= create_user_with_notification(:watch, 'parent_group_watcher', parent_group) + @pg_watcher.notification_settings_for(nil).disabled! - # Parent group member: global=global, parent_group=disabled, child_group=global - @pg_disabled ||= create_user_with_notification(:disabled, 'parent_group_disabled', parent_group) - @pg_disabled.notification_settings_for(nil).global! + # Parent group member: global=global, parent_group=disabled, child_group=global + @pg_disabled ||= create_user_with_notification(:disabled, 'parent_group_disabled', parent_group) + @pg_disabled.notification_settings_for(nil).global! - # Parent group member: global=global, parent_group=mention, child_group=global - @pg_mention ||= create_user_with_notification(:mention, 'parent_group_mention', parent_group) - @pg_mention.notification_settings_for(nil).global! + # Parent group member: global=global, parent_group=mention, child_group=global + @pg_mention ||= create_user_with_notification(:mention, 'parent_group_mention', parent_group) + @pg_mention.notification_settings_for(nil).global! - # Parent group member: global=global, parent_group=participating, child_group=global - @pg_participant ||= create_user_with_notification(:participating, 'parent_group_participant', parent_group) - @pg_mention.notification_settings_for(nil).global! + # Parent group member: global=global, parent_group=participating, child_group=global + @pg_participant ||= create_user_with_notification(:participating, 'parent_group_participant', parent_group) + @pg_mention.notification_settings_for(nil).global! - child_group - else - create(:group, visibility) - end + child_group end def add_member_for_parent_group(user, project) - return unless Group.supports_nested_objects? - project.reload project.group.parent.add_maintainer(user) end def should_email_nested_group_user(user, times: 1, recipients: email_recipients) - return unless Group.supports_nested_objects? - should_email(user, times: 1, recipients: email_recipients) end def should_not_email_nested_group_user(user, recipients: email_recipients) - return unless Group.supports_nested_objects? - should_not_email(user, recipients: email_recipients) end - def add_users_with_subscription(project, issuable) + def add_users(project) @subscriber = create :user @unsubscriber = create :user @unsubscribed_mentioned = create :user, username: 'unsubscribed_mentioned' @@ -2429,7 +2644,9 @@ describe NotificationService, :mailer do project.add_maintainer(@unsubscriber) project.add_maintainer(@watcher_and_subscriber) project.add_maintainer(@unsubscribed_mentioned) + end + def add_user_subscriptions(issuable) issuable.subscriptions.create(user: @unsubscribed_mentioned, project: project, subscribed: false) issuable.subscriptions.create(user: @subscriber, project: project, subscribed: true) issuable.subscriptions.create(user: @subscribed_participant, project: project, subscribed: true) diff --git a/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb b/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb index 8d43ce4f662..af79a42b611 100644 --- a/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb +++ b/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb @@ -12,6 +12,12 @@ describe PagesDomains::ObtainLetsEncryptCertificateService do stub_lets_encrypt_settings end + around do |example| + Sidekiq::Testing.fake! do + example.run + end + end + def expect_to_create_acme_challenge expect(::PagesDomains::CreateAcmeOrderService).to receive(:new).with(pages_domain) .and_wrap_original do |m, *args| diff --git a/spec/services/projects/autocomplete_service_spec.rb b/spec/services/projects/autocomplete_service_spec.rb index 2f70c8ea94d..b625653bc77 100644 --- a/spec/services/projects/autocomplete_service_spec.rb +++ b/spec/services/projects/autocomplete_service_spec.rb @@ -118,7 +118,7 @@ describe Projects::AutocompleteService do expect(milestone_titles).to eq([group_milestone2.title, group_milestone1.title]) end - context 'with nested groups', :nested_groups do + context 'with nested groups' do let(:subgroup) { create(:group, :public, parent: group) } let!(:subgroup_milestone) { create(:milestone, group: subgroup) } diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index b0b74407812..d4fa62fa85d 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -78,6 +78,7 @@ describe Projects::CreateService, '#execute' do expect(project).to be_valid expect(project.owner).to eq(group) expect(project.namespace).to eq(group) + expect(project.team.owners).to include(user) expect(user.authorized_projects).to include(project) end end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 3af7ee3ad50..9a6f64b825a 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -121,7 +121,22 @@ describe Projects::DestroyService do it { expect(Dir.exist?(remove_path)).to be_truthy } end - context 'when flushing caches fail' do + context 'when flushing caches fail due to Git errors' do + before do + allow(project.repository).to receive(:before_delete).and_raise(::Gitlab::Git::CommandError) + allow(Gitlab::GitLogger).to receive(:warn).with( + class: described_class.name, + project_id: project.id, + disk_path: project.disk_path, + message: 'Gitlab::Git::CommandError').and_call_original + + perform_enqueued_jobs { destroy_project(project, user, {}) } + end + + it_behaves_like 'deleting the project' + end + + context 'when flushing caches fail due to Redis' do before do new_user = create(:user) project.team.add_user(new_user, Gitlab::Access::DEVELOPER) @@ -226,6 +241,18 @@ describe Projects::DestroyService do expect(destroy_project(project, user)).to be false end end + + context 'when registry is disabled' do + before do + stub_container_registry_config(enabled: false) + end + + it 'does not attempting to remove any tags' do + expect(Projects::ContainerRepository::DestroyService).not_to receive(:new) + + destroy_project(project, user) + end + end end context 'when there are tags for legacy root repository' do diff --git a/spec/services/projects/download_service_spec.rb b/spec/services/projects/download_service_spec.rb index f25233ceeb1..06efc2ff825 100644 --- a/spec/services/projects/download_service_spec.rb +++ b/spec/services/projects/download_service_spec.rb @@ -20,13 +20,8 @@ describe Projects::DownloadService do context 'for URLs that are on the whitelist' do before do - sham_rack_app = ShamRack.at('mycompany.fogbugz.com').stub - sham_rack_app.register_resource('/rails_sample.jpg', File.read(Rails.root + 'spec/fixtures/rails_sample.jpg'), 'image/jpg') - sham_rack_app.register_resource('/doc_sample.txt', File.read(Rails.root + 'spec/fixtures/doc_sample.txt'), 'text/plain') - end - - after do - ShamRack.unmount_all + stub_request(:get, 'http://mycompany.fogbugz.com/rails_sample.jpg').to_return(body: File.read(Rails.root + 'spec/fixtures/rails_sample.jpg')) + stub_request(:get, 'http://mycompany.fogbugz.com/doc_sample.txt').to_return(body: File.read(Rails.root + 'spec/fixtures/doc_sample.txt')) end context 'an image file' do diff --git a/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb index 80debcd3a7a..dabfd61d3f5 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb @@ -33,7 +33,7 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do before do allow(project).to receive(:lfs_enabled?).and_return(true) - response = instance_double(HTTParty::Response) + response = instance_double(Gitlab::HTTP::Response) allow(response).to receive(:body).and_return(objects_response.to_json) allow(response).to receive(:success?).and_return(true) allow(Gitlab::HTTP).to receive(:post).and_return(response) @@ -95,7 +95,7 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do shared_examples 'JSON parse errors' do |body| it 'raises error' do - response = instance_double(HTTParty::Response) + response = instance_double(Gitlab::HTTP::Response) allow(response).to receive(:body).and_return(body) allow(response).to receive(:success?).and_return(true) allow(Gitlab::HTTP).to receive(:post).and_return(response) diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index 75d534c59bf..970e82e7107 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -17,7 +17,7 @@ describe Projects::LfsPointers::LfsDownloadService do before do ApplicationSetting.create_from_defaults - stub_application_setting(allow_local_requests_from_hooks_and_services: local_request_setting) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: local_request_setting) allow(project).to receive(:lfs_enabled?).and_return(true) end diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb index be2811ab1e7..4396ccab584 100644 --- a/spec/services/projects/update_remote_mirror_service_spec.rb +++ b/spec/services/projects/update_remote_mirror_service_spec.rb @@ -10,49 +10,91 @@ describe Projects::UpdateRemoteMirrorService do subject(:service) { described_class.new(project, project.creator) } - describe "#execute" do + describe '#execute' do + subject(:execute!) { service.execute(remote_mirror, 0) } + before do project.repository.add_branch(project.owner, 'existing-branch', 'master') allow(remote_mirror).to receive(:update_repository).and_return(true) end - it "ensures the remote exists" do + it 'ensures the remote exists' do stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) expect(remote_mirror).to receive(:ensure_remote!) - service.execute(remote_mirror) + execute! end - it "fetches the remote repository" do + it 'fetches the remote repository' do expect(project.repository) .to receive(:fetch_remote) - .with(remote_mirror.remote_name, no_tags: true, ssh_auth: remote_mirror) + .with(remote_mirror.remote_name, no_tags: true, ssh_auth: remote_mirror) - service.execute(remote_mirror) + execute! end - it "returns success when updated succeeds" do + it 'marks the mirror as started when beginning' do + expect(remote_mirror).to receive(:update_start!).and_call_original + + execute! + end + + it 'marks the mirror as successfully finished' do stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) - result = service.execute(remote_mirror) + result = execute! expect(result[:status]).to eq(:success) + expect(remote_mirror).to be_finished + end + + it 'marks the mirror as failed and raises the error when an unexpected error occurs' do + allow(project.repository).to receive(:fetch_remote).and_raise('Badly broken') + + expect { execute! }.to raise_error /Badly broken/ + + expect(remote_mirror).to be_failed + expect(remote_mirror.last_error).to include('Badly broken') + end + + context 'when the update fails because of a `Gitlab::Git::CommandError`' do + before do + allow(project.repository).to receive(:fetch_remote).and_raise(Gitlab::Git::CommandError.new('fetch failed')) + end + + it 'wraps `Gitlab::Git::CommandError`s in a service error' do + expect(execute!).to eq(status: :error, message: 'fetch failed') + end + + it 'marks the mirror as to be retried' do + execute! + + expect(remote_mirror).to be_to_retry + expect(remote_mirror.last_error).to include('fetch failed') + end + + it "marks the mirror as failed after #{described_class::MAX_TRIES} tries" do + service.execute(remote_mirror, described_class::MAX_TRIES) + + expect(remote_mirror).to be_failed + expect(remote_mirror.last_error).to include('fetch failed') + end end context 'when syncing all branches' do - it "push all the branches the first time" do + it 'push all the branches the first time' do stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) expect(remote_mirror).to receive(:update_repository).with({}) - service.execute(remote_mirror) + execute! end end context 'when only syncing protected branches' do - it "sync updated protected branches" do + it 'sync updated protected branches' do stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) protected_branch = create_protected_branch(project) remote_mirror.only_protected_branches = true @@ -61,7 +103,7 @@ describe Projects::UpdateRemoteMirrorService do .to receive(:update_repository) .with(only_branches_matching: [protected_branch.name]) - service.execute(remote_mirror) + execute! end def create_protected_branch(project) diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 1dcfb739eb6..31bd0f0f836 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -186,7 +186,10 @@ describe Projects::UpdateService do expect_any_instance_of(ProjectWiki).to receive(:wiki).and_raise(ProjectWiki::CouldNotCreateWikiError) expect_any_instance_of(described_class).to receive(:log_error).with("Could not create wiki for #{project.full_name}") - expect(Gitlab::Metrics).to receive(:counter) + + counter = double(:counter) + expect(Gitlab::Metrics).to receive(:counter).with(:wiki_can_not_be_created_total, 'Counts the times we failed to create a wiki').and_return(counter) + expect(counter).to receive(:increment) update_project(project, user, project_feature_attributes: { wiki_access_level: ProjectFeature::ENABLED }) end @@ -347,13 +350,13 @@ describe Projects::UpdateService do context 'when updating #pages_access_level' do subject(:call_service) do - update_project(project, admin, project_feature_attributes: { pages_access_level: ProjectFeature::PRIVATE }) + update_project(project, admin, project_feature_attributes: { pages_access_level: ProjectFeature::ENABLED }) end it 'updates the attribute' do expect { call_service } .to change { project.project_feature.pages_access_level } - .to(ProjectFeature::PRIVATE) + .to(ProjectFeature::ENABLED) end it 'calls Projects::UpdatePagesConfigurationService' do @@ -366,9 +369,28 @@ describe Projects::UpdateService do end end + context 'when updating #emails_disabled' do + it 'updates the attribute for the project owner' do + expect { update_project(project, user, emails_disabled: true) } + .to change { project.emails_disabled } + .to(true) + end + + it 'does not update when not project owner' do + maintainer = create(:user) + project.add_user(maintainer, :maintainer) + + expect { update_project(project, maintainer, emails_disabled: true) } + .not_to change { project.emails_disabled } + end + end + context 'with external authorization enabled' do before do enable_external_authorization_service_check + + allow(::Gitlab::ExternalAuthorization) + .to receive(:access_allowed?).with(user, 'default_label', project.full_path).and_call_original end it 'does not save the project with an error if the service denies access' do @@ -399,8 +421,7 @@ describe Projects::UpdateService do end it 'does not check the label when it does not change' do - expect(::Gitlab::ExternalAuthorization) - .not_to receive(:access_allowed?) + expect(::Gitlab::ExternalAuthorization).to receive(:access_allowed?).once update_project(project, user, { name: 'New name' }) end diff --git a/spec/services/prometheus/proxy_service_spec.rb b/spec/services/prometheus/proxy_service_spec.rb index 4bdb20de4c9..03bda94e9c6 100644 --- a/spec/services/prometheus/proxy_service_spec.rb +++ b/spec/services/prometheus/proxy_service_spec.rb @@ -131,7 +131,7 @@ describe Prometheus::ProxyService do allow(environment).to receive(:prometheus_adapter) .and_return(prometheus_adapter) allow(prometheus_adapter).to receive(:can_query?).and_return(true) - allow(prometheus_adapter).to receive(:prometheus_client_wrapper) + allow(prometheus_adapter).to receive(:prometheus_client) .and_return(prometheus_client) end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 95a131e8c86..c9714964fc9 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -28,61 +28,108 @@ describe QuickActions::InterpretService do shared_examples 'reopen command' do it 'returns state_event: "reopen" if content contains /reopen' do issuable.close! - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(state_event: 'reopen') end + + it 'returns the reopen message' do + issuable.close! + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Reopened this #{issuable.to_ability_name.humanize(capitalize: false)}.") + end end shared_examples 'close command' do it 'returns state_event: "close" if content contains /close' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(state_event: 'close') end + + it 'returns the close message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Closed this #{issuable.to_ability_name.humanize(capitalize: false)}.") + end end shared_examples 'title command' do it 'populates title: "A brand new title" if content contains /title A brand new title' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(title: 'A brand new title') end + + it 'returns the title message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq(%{Changed the title to "A brand new title".}) + end end shared_examples 'milestone command' do it 'fetches milestone and populates milestone_id if content contains /milestone' do milestone # populate the milestone - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(milestone_id: milestone.id) end + + it 'returns the milestone message' do + milestone # populate the milestone + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Set the milestone to #{milestone.to_reference}.") + end + + it 'returns empty milestone message when milestone is wrong' do + _, _, message = service.execute('/milestone %wrong-milestone', issuable) + + expect(message).to be_empty + end end shared_examples 'remove_milestone command' do it 'populates milestone_id: nil if content contains /remove_milestone' do issuable.update!(milestone_id: milestone.id) - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(milestone_id: nil) end + + it 'returns removed milestone message' do + issuable.update!(milestone_id: milestone.id) + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Removed #{milestone.to_reference} milestone.") + end end shared_examples 'label command' do it 'fetches label ids and populates add_label_ids if content contains /label' do bug # populate the label inprogress # populate the label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(add_label_ids: [bug.id, inprogress.id]) end + + it 'returns the label message' do + bug # populate the label + inprogress # populate the label + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Added #{bug.to_reference(format: :name)} #{inprogress.to_reference(format: :name)} labels.") + end end shared_examples 'multiple label command' do it 'fetches label ids and populates add_label_ids if content contains multiple /label' do bug # populate the label inprogress # populate the label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(add_label_ids: [inprogress.id, bug.id]) end @@ -91,7 +138,7 @@ describe QuickActions::InterpretService do shared_examples 'multiple label with same argument' do it 'prevents duplicate label ids and populates add_label_ids if content contains multiple /label' do inprogress # populate the label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(add_label_ids: [inprogress.id]) end @@ -120,16 +167,23 @@ describe QuickActions::InterpretService do shared_examples 'unlabel command' do it 'fetches label ids and populates remove_label_ids if content contains /unlabel' do issuable.update!(label_ids: [inprogress.id]) # populate the label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(remove_label_ids: [inprogress.id]) end + + it 'returns the unlabel message' do + issuable.update!(label_ids: [inprogress.id]) # populate the label + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Removed #{inprogress.to_reference(format: :name)} label.") + end end shared_examples 'multiple unlabel command' do it 'fetches label ids and populates remove_label_ids if content contains mutiple /unlabel' do issuable.update!(label_ids: [inprogress.id, bug.id]) # populate the label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(remove_label_ids: [inprogress.id, bug.id]) end @@ -138,7 +192,7 @@ describe QuickActions::InterpretService do shared_examples 'unlabel command with no argument' do it 'populates label_ids: [] if content contains /unlabel with no arguments' do issuable.update!(label_ids: [inprogress.id]) # populate the label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(label_ids: []) end @@ -148,91 +202,161 @@ describe QuickActions::InterpretService do it 'populates label_ids: [] if content contains /relabel' do issuable.update!(label_ids: [bug.id]) # populate the label inprogress # populate the label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(label_ids: [inprogress.id]) end + + it 'returns the relabel message' do + issuable.update!(label_ids: [bug.id]) # populate the label + inprogress # populate the label + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Replaced all labels with #{inprogress.to_reference(format: :name)} label.") + end end shared_examples 'todo command' do it 'populates todo_event: "add" if content contains /todo' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(todo_event: 'add') end + + it 'returns the todo message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Added a To Do.') + end end shared_examples 'done command' do it 'populates todo_event: "done" if content contains /done' do TodoService.new.mark_todo(issuable, developer) - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(todo_event: 'done') end + + it 'returns the done message' do + TodoService.new.mark_todo(issuable, developer) + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Marked To Do as done.') + end end shared_examples 'subscribe command' do it 'populates subscription_event: "subscribe" if content contains /subscribe' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(subscription_event: 'subscribe') end + + it 'returns the subscribe message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Subscribed to this #{issuable.to_ability_name.humanize(capitalize: false)}.") + end end shared_examples 'unsubscribe command' do it 'populates subscription_event: "unsubscribe" if content contains /unsubscribe' do issuable.subscribe(developer, project) - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(subscription_event: 'unsubscribe') end + + it 'returns the unsubscribe message' do + issuable.subscribe(developer, project) + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Unsubscribed from this #{issuable.to_ability_name.humanize(capitalize: false)}.") + end end shared_examples 'due command' do + let(:expected_date) { Date.new(2016, 8, 28) } + it 'populates due_date: Date.new(2016, 8, 28) if content contains /due 2016-08-28' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) + + expect(updates).to eq(due_date: expected_date) + end - expect(updates).to eq(due_date: defined?(expected_date) ? expected_date : Date.new(2016, 8, 28)) + it 'returns due_date message: Date.new(2016, 8, 28) if content contains /due 2016-08-28' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Set the due date to #{expected_date.to_s(:medium)}.") end end shared_examples 'remove_due_date command' do - it 'populates due_date: nil if content contains /remove_due_date' do + before do issuable.update!(due_date: Date.today) - _, updates = service.execute(content, issuable) + end + + it 'populates due_date: nil if content contains /remove_due_date' do + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(due_date: nil) end + + it 'returns Removed the due date' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Removed the due date.') + end end shared_examples 'wip command' do it 'returns wip_event: "wip" if content contains /wip' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(wip_event: 'wip') end + + it 'returns the wip message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Marked this #{issuable.to_ability_name.humanize(capitalize: false)} as Work In Progress.") + end end shared_examples 'unwip command' do it 'returns wip_event: "unwip" if content contains /wip' do issuable.update!(title: issuable.wip_title) - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(wip_event: 'unwip') end + + it 'returns the unwip message' do + issuable.update!(title: issuable.wip_title) + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Unmarked this #{issuable.to_ability_name.humanize(capitalize: false)} as Work In Progress.") + end end shared_examples 'estimate command' do it 'populates time_estimate: 3600 if content contains /estimate 1h' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(time_estimate: 3600) end + + it 'returns the time_estimate formatted message' do + _, _, message = service.execute('/estimate 79d', issuable) + + expect(message).to eq('Set time estimate to 3mo 3w 4d.') + end end shared_examples 'spend command' do it 'populates spend_time: 3600 if content contains /spend 1h' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(spend_time: { duration: 3600, @@ -240,11 +364,17 @@ describe QuickActions::InterpretService do spent_at: DateTime.now.to_date }) end + + it 'returns the spend_time message including the formatted duration and verb' do + _, _, message = service.execute('/spend -120m', issuable) + + expect(message).to eq('Subtracted 2h spent time.') + end end shared_examples 'spend command with negative time' do it 'populates spend_time: -1800 if content contains /spend -30m' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(spend_time: { duration: -1800, @@ -256,7 +386,7 @@ describe QuickActions::InterpretService do shared_examples 'spend command with valid date' do it 'populates spend time: 1800 with date in date type format' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(spend_time: { duration: 1800, @@ -268,7 +398,7 @@ describe QuickActions::InterpretService do shared_examples 'spend command with invalid date' do it 'will not create any note and timelog' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq({}) end @@ -276,7 +406,7 @@ describe QuickActions::InterpretService do shared_examples 'spend command with future date' do it 'will not create any note and timelog' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq({}) end @@ -284,18 +414,30 @@ describe QuickActions::InterpretService do shared_examples 'remove_estimate command' do it 'populates time_estimate: 0 if content contains /remove_estimate' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(time_estimate: 0) end + + it 'returns the remove_estimate message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Removed time estimate.') + end end shared_examples 'remove_time_spent command' do it 'populates spend_time: :reset if content contains /remove_time_spent' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(spend_time: { duration: :reset, user_id: developer.id }) end + + it 'returns the remove_time_spent message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Removed spent time.') + end end shared_examples 'lock command' do @@ -303,10 +445,16 @@ describe QuickActions::InterpretService do let(:merge_request) { create(:merge_request, source_project: project, discussion_locked: false) } it 'returns discussion_locked: true if content contains /lock' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(discussion_locked: true) end + + it 'returns the lock discussion message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Locked the discussion.') + end end shared_examples 'unlock command' do @@ -314,45 +462,79 @@ describe QuickActions::InterpretService do let(:merge_request) { create(:merge_request, source_project: project, discussion_locked: true) } it 'returns discussion_locked: true if content contains /unlock' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(discussion_locked: false) end + + it 'returns the unlock discussion message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Unlocked the discussion.') + end end - shared_examples 'empty command' do + shared_examples 'empty command' do |error_msg| it 'populates {} if content contains an unsupported command' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to be_empty end + + it "returns #{error_msg || 'an empty'} message" do + _, _, message = service.execute(content, issuable) + + if error_msg + expect(message).to eq(error_msg) + else + expect(message).to be_empty + end + end end shared_examples 'merge command' do let(:project) { create(:project, :repository) } it 'runs merge command if content contains /merge' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(merge: merge_request.diff_head_sha) end + + it 'returns them merge message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Scheduled to merge this merge request when the pipeline succeeds.') + end end shared_examples 'award command' do it 'toggle award 100 emoji if content contains /award :100:' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(emoji_award: "100") end + + it 'returns the award message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Toggled :100: emoji award.') + end end shared_examples 'duplicate command' do it 'fetches issue and populates canonical_issue_id if content contains /duplicate issue_reference' do issue_duplicate # populate the issue - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(canonical_issue_id: issue_duplicate.id) end + + it 'returns the duplicate message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Marked this issue as a duplicate of #{issue_duplicate.to_reference(project)}.") + end end shared_examples 'copy_metadata command' do @@ -360,7 +542,7 @@ describe QuickActions::InterpretService do source_issuable # populate the issue todo_label # populate this label inreview_label # populate this label - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates[:add_label_ids]).to match_array([inreview_label.id, todo_label.id]) @@ -370,19 +552,45 @@ describe QuickActions::InterpretService do expect(updates).not_to have_key(:milestone_id) end end + + it 'returns the copy metadata message' do + _, _, message = service.execute("/copy_metadata #{source_issuable.to_reference}", issuable) + + expect(message).to eq("Copied labels and milestone from #{source_issuable.to_reference}.") + end + end + + describe 'move issue command' do + it 'returns the move issue message' do + _, _, message = service.execute("/move #{project.full_path}", issue) + + expect(message).to eq("Moved this issue to #{project.full_path}.") + end + + it 'returns move issue failure message when the referenced issue is not found' do + _, _, message = service.execute('/move invalid', issue) + + expect(message).to eq("Failed to move this issue because target project doesn't exist.") + end end shared_examples 'confidential command' do it 'marks issue as confidential if content contains /confidential' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(confidential: true) end + + it 'returns the confidential message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq('Made this issue confidential.') + end end shared_examples 'shrug command' do it 'appends ¯\_(ツ)_/¯ to the comment' do - new_content, _ = service.execute(content, issuable) + new_content, _, _ = service.execute(content, issuable) expect(new_content).to end_with(described_class::SHRUG) end @@ -390,7 +598,7 @@ describe QuickActions::InterpretService do shared_examples 'tableflip command' do it 'appends (╯°□°)╯︵ ┻━┻ to the comment' do - new_content, _ = service.execute(content, issuable) + new_content, _, _ = service.execute(content, issuable) expect(new_content).to end_with(described_class::TABLEFLIP) end @@ -398,18 +606,34 @@ describe QuickActions::InterpretService do shared_examples 'tag command' do it 'tags a commit' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(tag_name: tag_name, tag_message: tag_message) end + + it 'returns the tag message' do + _, _, message = service.execute(content, issuable) + + if tag_message.present? + expect(message).to eq(%{Tagged this commit to #{tag_name} with "#{tag_message}".}) + else + expect(message).to eq("Tagged this commit to #{tag_name}.") + end + end end shared_examples 'assign command' do it 'assigns to a single user' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(assignee_ids: [developer.id]) end + + it 'returns the assign message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Assigned #{developer.to_reference}.") + end end it_behaves_like 'reopen command' do @@ -463,7 +687,7 @@ describe QuickActions::InterpretService do let(:service) { described_class.new(project, developer, {}) } it 'precheck passes and returns merge command' do - _, updates = service.execute('/merge', merge_request) + _, updates, _ = service.execute('/merge', merge_request) expect(updates).to eq(merge: nil) end @@ -559,7 +783,7 @@ describe QuickActions::InterpretService do end end - it_behaves_like 'empty command' do + it_behaves_like 'empty command', "Failed to assign a user because no user was found." do let(:content) { '/assign @abcd1234' } let(:issuable) { issue } end @@ -574,20 +798,34 @@ describe QuickActions::InterpretService do context 'Issue' do it 'populates assignee_ids: [] if content contains /unassign' do - issue.update(assignee_ids: [developer.id]) - _, updates = service.execute(content, issue) + issue.update!(assignee_ids: [developer.id]) + _, updates, _ = service.execute(content, issue) expect(updates).to eq(assignee_ids: []) end + + it 'returns the unassign message for all the assignee if content contains /unassign' do + issue.update(assignee_ids: [developer.id, developer2.id]) + _, _, message = service.execute(content, issue) + + expect(message).to eq("Removed assignees #{developer.to_reference} and #{developer2.to_reference}.") + end end context 'Merge Request' do it 'populates assignee_ids: [] if content contains /unassign' do - merge_request.update(assignee_ids: [developer.id]) - _, updates = service.execute(content, merge_request) + merge_request.update!(assignee_ids: [developer.id]) + _, updates, _ = service.execute(content, merge_request) expect(updates).to eq(assignee_ids: []) end + + it 'returns the unassign message for all the assignee if content contains /unassign' do + merge_request.update(assignee_ids: [developer.id, developer2.id]) + _, _, message = service.execute(content, merge_request) + + expect(message).to eq("Removed assignees #{developer.to_reference} and #{developer2.to_reference}.") + end end end @@ -979,12 +1217,12 @@ describe QuickActions::InterpretService do let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'empty command', 'Failed to mark this issue as a duplicate because referenced issue was not found.' do let(:content) { "/duplicate imaginary#1234" } let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'empty command', 'Failed to mark this issue as a duplicate because referenced issue was not found.' do let(:other_project) { create(:project, :private) } let(:issue_duplicate) { create(:issue, project: other_project) } @@ -1049,7 +1287,7 @@ describe QuickActions::InterpretService do let(:issuable) { issue } end - it_behaves_like 'empty command' do + it_behaves_like 'empty command', 'Failed to mark this issue as a duplicate because referenced issue was not found.' do let(:content) { '/duplicate #{issue.to_reference}' } let(:issuable) { issue } end @@ -1132,13 +1370,13 @@ describe QuickActions::InterpretService do let(:service) { described_class.new(non_empty_project, developer)} it 'updates target_branch if /target_branch command is executed' do - _, updates = service.execute('/target_branch merge-test', merge_request) + _, updates, _ = service.execute('/target_branch merge-test', merge_request) expect(updates).to eq(target_branch: 'merge-test') end it 'handles blanks around param' do - _, updates = service.execute('/target_branch merge-test ', merge_request) + _, updates, _ = service.execute('/target_branch merge-test ', merge_request) expect(updates).to eq(target_branch: 'merge-test') end @@ -1156,6 +1394,12 @@ describe QuickActions::InterpretService do let(:issuable) { another_merge_request } end end + + it 'returns the target_branch message' do + _, _, message = service.execute('/target_branch merge-test', merge_request) + + expect(message).to eq('Set target branch to merge-test.') + end end context '/board_move command' do @@ -1171,13 +1415,13 @@ describe QuickActions::InterpretService do it 'populates remove_label_ids for all current board columns' do issue.update!(label_ids: [todo.id, inprogress.id]) - _, updates = service.execute(content, issue) + _, updates, _ = service.execute(content, issue) expect(updates[:remove_label_ids]).to match_array([todo.id, inprogress.id]) end it 'populates add_label_ids with the id of the given label' do - _, updates = service.execute(content, issue) + _, updates, _ = service.execute(content, issue) expect(updates[:add_label_ids]).to eq([inreview.id]) end @@ -1185,7 +1429,7 @@ describe QuickActions::InterpretService do it 'does not include the given label id in remove_label_ids' do issue.update!(label_ids: [todo.id, inreview.id]) - _, updates = service.execute(content, issue) + _, updates, _ = service.execute(content, issue) expect(updates[:remove_label_ids]).to match_array([todo.id]) end @@ -1193,11 +1437,19 @@ describe QuickActions::InterpretService do it 'does not remove label ids that are not lists on the board' do issue.update!(label_ids: [todo.id, bug.id]) - _, updates = service.execute(content, issue) + _, updates, _ = service.execute(content, issue) expect(updates[:remove_label_ids]).to match_array([todo.id]) end + it 'returns board_move message' do + issue.update!(label_ids: [todo.id, inprogress.id]) + + _, _, message = service.execute(content, issue) + + expect(message).to eq("Moved issue to ~#{inreview.id} column in the board.") + end + context 'if the project has multiple boards' do let(:issuable) { issue } @@ -1211,19 +1463,19 @@ describe QuickActions::InterpretService do context 'if the given label does not exist' do let(:issuable) { issue } let(:content) { '/board_move ~"Fake Label"' } - it_behaves_like 'empty command' + it_behaves_like 'empty command', 'Failed to move this issue because label was not found.' end context 'if multiple labels are given' do let(:issuable) { issue } let(:content) { %{/board_move ~"#{inreview.title}" ~"#{todo.title}"} } - it_behaves_like 'empty command' + it_behaves_like 'empty command', 'Failed to move this issue because only a single label can be provided.' end context 'if the given label is not a list on the board' do let(:issuable) { issue } let(:content) { %{/board_move ~"#{bug.title}"} } - it_behaves_like 'empty command' + it_behaves_like 'empty command', 'Failed to move this issue because label was not found.' end context 'if issuable is not an Issue' do @@ -1292,10 +1544,16 @@ describe QuickActions::InterpretService do end it 'populates create_merge_request with branch_name and issue iid' do - _, updates = service.execute(content, issuable) + _, updates, _ = service.execute(content, issuable) expect(updates).to eq(create_merge_request: { branch_name: branch_name, issue_iid: issuable.iid }) end + + it 'returns the create_merge_request message' do + _, _, message = service.execute(content, issuable) + + expect(message).to eq("Created branch '#{branch_name}' and a merge request to resolve this issue.") + end end end @@ -1556,7 +1814,13 @@ describe QuickActions::InterpretService do it 'uses the default branch name' do _, explanations = service.explain(content, issue) - expect(explanations).to eq(['Creates a branch and a merge request to resolve this issue']) + expect(explanations).to eq(['Creates a branch and a merge request to resolve this issue.']) + end + + it 'returns the execution message using the default branch name' do + _, _, message = service.execute(content, issue) + + expect(message).to eq('Created a branch and a merge request to resolve this issue.') end end @@ -1566,7 +1830,13 @@ describe QuickActions::InterpretService do it 'uses the given branch name' do _, explanations = service.explain(content, issue) - expect(explanations).to eq(["Creates branch 'foo' and a merge request to resolve this issue"]) + expect(explanations).to eq(["Creates branch 'foo' and a merge request to resolve this issue."]) + end + + it 'returns the execution message using the given branch name' do + _, _, message = service.execute(content, issue) + + expect(message).to eq("Created branch 'foo' and a merge request to resolve this issue.") end end end diff --git a/spec/services/submodules/update_service_spec.rb b/spec/services/submodules/update_service_spec.rb index cf92350c1b2..47b31d4bcbf 100644 --- a/spec/services/submodules/update_service_spec.rb +++ b/spec/services/submodules/update_service_spec.rb @@ -142,7 +142,7 @@ describe Submodules::UpdateService do let(:branch_name) { nil } it_behaves_like 'returns error result' do - let(:error_message) { 'You can only create or edit files when you are on a branch' } + let(:error_message) { 'Invalid parameters' } end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 157cfc46e69..f46f9633c1c 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -513,6 +513,30 @@ describe SystemNoteService do end 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 '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 { described_class.zoom_link_removed(issue, project, author) } + + 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 + describe '.cross_reference' do subject { described_class.cross_reference(noteable, mentioner, author) } diff --git a/spec/services/task_list_toggle_service_spec.rb b/spec/services/task_list_toggle_service_spec.rb index 9adaee6481b..a309951bbcb 100644 --- a/spec/services/task_list_toggle_service_spec.rb +++ b/spec/services/task_list_toggle_service_spec.rb @@ -114,6 +114,23 @@ describe TaskListToggleService do expect(toggler.execute).to be_falsey end + it 'properly handles tasks in a blockquote' do + markdown = + <<-EOT.strip_heredoc + > > * [ ] Task 1 + > * [x] Task 2 + EOT + + markdown_html = Banzai::Pipeline::FullPipeline.call(markdown, project: nil)[:output].to_html + toggler = described_class.new(markdown, markdown_html, + toggle_as_checked: true, + line_source: '> > * [ ] Task 1', line_number: 1) + + expect(toggler.execute).to be_truthy + expect(toggler.updated_markdown.lines[0]).to eq "> > * [x] Task 1\n" + expect(toggler.updated_markdown_html).to include('disabled checked> Task 1') + end + it 'properly handles a GitLab blockquote' do markdown = <<-EOT.strip_heredoc diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb index 2a553e18807..ce809bbf6c5 100644 --- a/spec/services/todos/destroy/entity_leave_service_spec.rb +++ b/spec/services/todos/destroy/entity_leave_service_spec.rb @@ -176,7 +176,7 @@ describe Todos::Destroy::EntityLeaveService do end end - context 'with nested groups', :nested_groups do + context 'with nested groups' do let(:subgroup) { create(:group, :private, parent: group) } let(:subgroup2) { create(:group, :private, parent: group) } let(:subproject) { create(:project, group: subgroup) } diff --git a/spec/services/todos/destroy/group_private_service_spec.rb b/spec/services/todos/destroy/group_private_service_spec.rb index a1798686d7c..7dd495847b3 100644 --- a/spec/services/todos/destroy/group_private_service_spec.rb +++ b/spec/services/todos/destroy/group_private_service_spec.rb @@ -35,7 +35,7 @@ describe Todos::Destroy::GroupPrivateService do expect(project_member.todos).to match_array([todo_project_member]) end - context 'with nested groups', :nested_groups do + context 'with nested groups' do let(:parent_group) { create(:group) } let(:subgroup) { create(:group, :private, parent: group) } let(:subproject) { create(:project, group: subgroup) } diff --git a/spec/services/update_snippet_service_spec.rb b/spec/services/update_snippet_service_spec.rb index 23ea4e003f8..0678f54c195 100644 --- a/spec/services/update_snippet_service_spec.rb +++ b/spec/services/update_snippet_service_spec.rb @@ -40,6 +40,23 @@ describe UpdateSnippetService do end end + describe 'usage counter' do + let(:counter) { Gitlab::UsageDataCounters::SnippetCounter } + let(:snippet) { create_snippet(nil, @user, @opts) } + + it 'increments count' do + expect do + update_snippet(nil, @admin, snippet, @opts) + end.to change { counter.read(:update) }.by 1 + end + + it 'does not increment count if create fails' do + expect do + update_snippet(nil, @admin, snippet, { title: '' }) + end.not_to change { counter.read(:update) } + end + end + def create_snippet(project, user, opts) CreateSnippetService.new(project, user, opts).execute end diff --git a/spec/services/users/refresh_authorized_projects_service_spec.rb b/spec/services/users/refresh_authorized_projects_service_spec.rb index 0287a24808d..f5a914bb482 100644 --- a/spec/services/users/refresh_authorized_projects_service_spec.rb +++ b/spec/services/users/refresh_authorized_projects_service_spec.rb @@ -135,7 +135,7 @@ describe Users::RefreshAuthorizedProjectsService do end end - context 'projects of subgroups of groups the user is a member of', :nested_groups do + context 'projects of subgroups of groups the user is a member of' do let(:group) { create(:group) } let(:nested_group) { create(:group, parent: group) } let!(:other_project) { create(:project, group: nested_group) } @@ -163,7 +163,7 @@ describe Users::RefreshAuthorizedProjectsService do end end - context 'projects shared with subgroups of groups the user is a member of', :nested_groups do + context 'projects shared with subgroups of groups the user is a member of' do let(:group) { create(:group) } let(:nested_group) { create(:group, parent: group) } let(:other_project) { create(:project) } diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 37bafc0c002..2a4368868d5 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -19,47 +19,74 @@ describe WebHookService do let(:service_instance) { described_class.new(project_hook, data, :push_hooks) } describe '#initialize' do - it 'allow_local_requests is true if hook is a SystemHook' do - instance = described_class.new(build(:system_hook), data, :system_hook) - expect(instance.request_options[:allow_local_requests]).to be_truthy + before do + stub_application_setting(setting_name => setting) end - it 'allow_local_requests is false if hook is not a SystemHook' do - %i(project_hook service_hook web_hook_log).each do |hook| - instance = described_class.new(build(hook), data, hook) - expect(instance.request_options[:allow_local_requests]).to be_falsey + shared_examples_for 'respects outbound network setting' do + context 'when local requests are allowed' do + let(:setting) { true } + + it { expect(hook.request_options[:allow_local_requests]).to be_truthy } + end + + context 'when local requests are not allowed' do + let(:setting) { false } + + it { expect(hook.request_options[:allow_local_requests]).to be_falsey } end end + + context 'when SystemHook' do + let(:setting_name) { :allow_local_requests_from_system_hooks } + let(:hook) { described_class.new(build(:system_hook), data, :system_hook) } + + include_examples 'respects outbound network setting' + end + + context 'when ProjectHook' do + let(:setting_name) { :allow_local_requests_from_web_hooks_and_services } + let(:hook) { described_class.new(build(:project_hook), data, :project_hook) } + + include_examples 'respects outbound network setting' + end end describe '#execute' do before do project.hooks << [project_hook] - - WebMock.stub_request(:post, project_hook.url) end context 'when token is defined' do let(:project_hook) { create(:project_hook, :token) } it 'POSTs to the webhook URL' do + stub_full_request(project_hook.url, method: :post) + service_instance.execute - expect(WebMock).to have_requested(:post, project_hook.url).with( + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).with( headers: headers.merge({ 'X-Gitlab-Token' => project_hook.token }) ).once end end it 'POSTs to the webhook URL' do + stub_full_request(project_hook.url, method: :post) + service_instance.execute - expect(WebMock).to have_requested(:post, project_hook.url).with( + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).with( headers: headers ).once end it 'POSTs the data as JSON' do + stub_full_request(project_hook.url, method: :post) + service_instance.execute - expect(WebMock).to have_requested(:post, project_hook.url).with( + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).with( headers: headers ).once end @@ -95,7 +122,7 @@ describe WebHookService do end it 'catches exceptions' do - WebMock.stub_request(:post, project_hook.url).to_raise(StandardError.new('Some error')) + stub_full_request(project_hook.url, method: :post).to_raise(StandardError.new('Some error')) expect { service_instance.execute }.to raise_error(StandardError) end @@ -105,20 +132,20 @@ describe WebHookService do exceptions.each do |exception_class| exception = exception_class.new('Exception message') - WebMock.stub_request(:post, project_hook.url).to_raise(exception) + stub_full_request(project_hook.url, method: :post).to_raise(exception) expect(service_instance.execute).to eq({ status: :error, message: exception.to_s }) expect { service_instance.execute }.not_to raise_error end end it 'handles 200 status code' do - WebMock.stub_request(:post, project_hook.url).to_return(status: 200, body: 'Success') + stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success') expect(service_instance.execute).to include({ status: :success, http_status: 200, message: 'Success' }) end it 'handles 2xx status codes' do - WebMock.stub_request(:post, project_hook.url).to_return(status: 201, body: 'Success') + stub_full_request(project_hook.url, method: :post).to_return(status: 201, body: 'Success') expect(service_instance.execute).to include({ status: :success, http_status: 201, message: 'Success' }) end @@ -128,7 +155,7 @@ describe WebHookService do context 'with success' do before do - WebMock.stub_request(:post, project_hook.url).to_return(status: 200, body: 'Success') + stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success') service_instance.execute end @@ -145,7 +172,7 @@ describe WebHookService do context 'with exception' do before do - WebMock.stub_request(:post, project_hook.url).to_raise(SocketError.new('Some HTTP Post error')) + stub_full_request(project_hook.url, method: :post).to_raise(SocketError.new('Some HTTP Post error')) service_instance.execute end @@ -162,7 +189,7 @@ describe WebHookService do context 'with unsafe response body' do before do - WebMock.stub_request(:post, project_hook.url).to_return(status: 200, body: "\xBB") + stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: "\xBB") service_instance.execute end @@ -182,7 +209,7 @@ describe WebHookService do let(:service_instance) { described_class.new(service_hook, data, 'service_hook') } before do - WebMock.stub_request(:post, service_hook.url).to_return(status: 200, body: 'Success') + stub_full_request(service_hook.url, method: :post).to_return(status: 200, body: 'Success') end it { expect { service_instance.execute }.not_to change(WebHookLog, :count) } diff --git a/spec/services/wiki_pages/base_service_spec.rb b/spec/services/wiki_pages/base_service_spec.rb new file mode 100644 index 00000000000..2e70246c6f2 --- /dev/null +++ b/spec/services/wiki_pages/base_service_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe WikiPages::BaseService do + let(:project) { double('project') } + let(:user) { double('user') } + + subject(:service) { described_class.new(project, user, {}) } + + describe '#increment_usage' do + counter = Gitlab::UsageDataCounters::WikiPageCounter + error = counter::UnknownEvent + + it 'raises an error on unknown events' do + expect { subject.send(:increment_usage, :bad_event) }.to raise_error error + end + + context 'the event is valid' do + counter::KNOWN_EVENTS.each do |e| + it "updates the #{e} counter" do + expect { subject.send(:increment_usage, e) }.to change { counter.read(e) } + end + end + end + end +end diff --git a/spec/services/wiki_pages/create_service_spec.rb b/spec/services/wiki_pages/create_service_spec.rb index 84510dcf700..ef03a2e9788 100644 --- a/spec/services/wiki_pages/create_service_spec.rb +++ b/spec/services/wiki_pages/create_service_spec.rb @@ -14,6 +14,10 @@ describe WikiPages::CreateService do } end + let(:bad_opts) do + { title: '' } + end + subject(:service) { described_class.new(project, user, opts) } before do @@ -36,5 +40,26 @@ describe WikiPages::CreateService do service.execute end + + it 'counts wiki page creation' do + counter = Gitlab::UsageDataCounters::WikiPageCounter + + expect { service.execute }.to change { counter.read(:create) }.by 1 + end + + context 'when the options are bad' do + subject(:service) { described_class.new(project, user, bad_opts) } + + it 'does not count a creation event' do + counter = Gitlab::UsageDataCounters::WikiPageCounter + + expect { service.execute }.not_to change { counter.read(:create) } + end + + it 'reports the error' do + expect(service.execute).to be_invalid + .and have_attributes(errors: be_present) + end + end end end diff --git a/spec/services/wiki_pages/destroy_service_spec.rb b/spec/services/wiki_pages/destroy_service_spec.rb index c74eac4dad6..350a7eb123b 100644 --- a/spec/services/wiki_pages/destroy_service_spec.rb +++ b/spec/services/wiki_pages/destroy_service_spec.rb @@ -20,5 +20,17 @@ describe WikiPages::DestroyService do service.execute(page) end + + it 'increments the delete count' do + counter = Gitlab::UsageDataCounters::WikiPageCounter + + expect { service.execute(page) }.to change { counter.read(:delete) }.by 1 + end + + it 'does not increment the delete count if the deletion failed' do + counter = Gitlab::UsageDataCounters::WikiPageCounter + + expect { service.execute(nil) }.not_to change { counter.read(:delete) } + end end end diff --git a/spec/services/wiki_pages/update_service_spec.rb b/spec/services/wiki_pages/update_service_spec.rb index 19866bd3bfc..d5f46e7b2db 100644 --- a/spec/services/wiki_pages/update_service_spec.rb +++ b/spec/services/wiki_pages/update_service_spec.rb @@ -16,6 +16,10 @@ describe WikiPages::UpdateService do } end + let(:bad_opts) do + { title: '' } + end + subject(:service) { described_class.new(project, user, opts) } before do @@ -39,5 +43,26 @@ describe WikiPages::UpdateService do service.execute(page) end + + it 'counts edit events' do + counter = Gitlab::UsageDataCounters::WikiPageCounter + + expect { service.execute page }.to change { counter.read(:update) }.by 1 + end + + context 'when the options are bad' do + subject(:service) { described_class.new(project, user, bad_opts) } + + it 'does not count an edit event' do + counter = Gitlab::UsageDataCounters::WikiPageCounter + + expect { service.execute page }.not_to change { counter.read(:update) } + end + + it 'reports the error' do + expect(service.execute page).to be_invalid + .and have_attributes(errors: be_present) + end + end end end diff --git a/spec/services/zoom_notes_service_spec.rb b/spec/services/zoom_notes_service_spec.rb new file mode 100644 index 00000000000..419ecf3f374 --- /dev/null +++ b/spec/services/zoom_notes_service_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ZoomNotesService do + describe '#execute' do + let(:issue) { OpenStruct.new(description: description) } + let(:project) { Object.new } + let(:user) { Object.new } + let(:description) { 'an issue description' } + let(:old_description) { nil } + + subject { described_class.new(issue, project, user, old_description: old_description) } + + shared_examples 'no notifications' do + it "doesn't create notifications" do + expect(SystemNoteService).not_to receive(:zoom_link_added) + expect(SystemNoteService).not_to receive(:zoom_link_removed) + + subject.execute + end + end + + it_behaves_like 'no notifications' + + context 'when the zoom link exists in both description and old_description' do + let(:description) { 'a changed issue description https://zoom.us/j/123' } + let(:old_description) { 'an issue description https://zoom.us/j/123' } + + it_behaves_like 'no notifications' + end + + context "when the zoom link doesn't exist in both description and old_description" do + let(:description) { 'a changed issue description' } + let(:old_description) { 'an issue description' } + + it_behaves_like 'no notifications' + end + + context 'when description == old_description' do + let(:old_description) { 'an issue description' } + + it_behaves_like 'no notifications' + end + + context 'when the description contains a zoom link and old_description is nil' do + let(:description) { 'a changed issue description https://zoom.us/j/123' } + + it 'creates a zoom_link_added notification' do + expect(SystemNoteService).to receive(:zoom_link_added).with(issue, project, user) + expect(SystemNoteService).not_to receive(:zoom_link_removed) + + subject.execute + end + end + + context 'when the zoom link has been added to the description' do + let(:description) { 'a changed issue description https://zoom.us/j/123' } + let(:old_description) { 'an issue description' } + + it 'creates a zoom_link_added notification' do + expect(SystemNoteService).to receive(:zoom_link_added).with(issue, project, user) + expect(SystemNoteService).not_to receive(:zoom_link_removed) + + subject.execute + end + end + + context 'when the zoom link has been removed from the description' do + let(:description) { 'a changed issue description' } + let(:old_description) { 'an issue description https://zoom.us/j/123' } + + it 'creates a zoom_link_removed notification' do + expect(SystemNoteService).not_to receive(:zoom_link_added).with(issue, project, user) + expect(SystemNoteService).to receive(:zoom_link_removed) + + subject.execute + end + end + end +end |