diff options
Diffstat (limited to 'spec/services')
119 files changed, 4189 insertions, 1196 deletions
diff --git a/spec/services/admin/propagate_integration_service_spec.rb b/spec/services/admin/propagate_integration_service_spec.rb index 2e879cf06d1..49d974b7154 100644 --- a/spec/services/admin/propagate_integration_service_spec.rb +++ b/spec/services/admin/propagate_integration_service_spec.rb @@ -4,8 +4,15 @@ require 'spec_helper' RSpec.describe Admin::PropagateIntegrationService do describe '.propagate' do - let(:excluded_attributes) { %w[id project_id inherit_from_id instance created_at updated_at default] } + include JiraServiceHelper + + before do + stub_jira_service_test + end + + let(:excluded_attributes) { %w[id project_id group_id inherit_from_id instance created_at updated_at default] } let!(:project) { create(:project) } + let!(:group) { create(:group) } let!(:instance_integration) do JiraService.create!( instance: true, @@ -43,7 +50,7 @@ RSpec.describe Admin::PropagateIntegrationService do ) end - let!(:another_inherited_integration) do + let!(:different_type_inherited_integration) do BambooService.create!( project: create(:project), inherit_from_id: instance_integration.id, @@ -59,7 +66,7 @@ RSpec.describe Admin::PropagateIntegrationService do shared_examples 'inherits settings from integration' do it 'updates the inherited integrations' do - described_class.propagate(integration: instance_integration, overwrite: overwrite) + described_class.propagate(instance_integration) expect(integration.reload.inherit_from_id).to eq(instance_integration.id) expect(integration.attributes.except(*excluded_attributes)) @@ -70,7 +77,7 @@ RSpec.describe Admin::PropagateIntegrationService do let(:excluded_attributes) { %w[id service_id created_at updated_at] } it 'updates the data fields from inherited integrations' do - described_class.propagate(integration: instance_integration, overwrite: overwrite) + described_class.propagate(instance_integration) expect(integration.reload.data_fields.attributes.except(*excluded_attributes)) .to eq(instance_integration.data_fields.attributes.except(*excluded_attributes)) @@ -80,7 +87,7 @@ RSpec.describe Admin::PropagateIntegrationService do shared_examples 'does not inherit settings from integration' do it 'does not update the not inherited integrations' do - described_class.propagate(integration: instance_integration, overwrite: overwrite) + described_class.propagate(instance_integration) expect(integration.reload.attributes.except(*excluded_attributes)) .not_to eq(instance_integration.attributes.except(*excluded_attributes)) @@ -88,8 +95,6 @@ RSpec.describe Admin::PropagateIntegrationService do end context 'update only inherited integrations' do - let(:overwrite) { false } - it_behaves_like 'inherits settings from integration' do let(:integration) { inherited_integration } end @@ -99,36 +104,20 @@ RSpec.describe Admin::PropagateIntegrationService do end it_behaves_like 'does not inherit settings from integration' do - let(:integration) { another_inherited_integration } + let(:integration) { different_type_inherited_integration } end it_behaves_like 'inherits settings from integration' do let(:integration) { project.jira_service } end - end - - context 'update all integrations' do - let(:overwrite) { true } - - it_behaves_like 'inherits settings from integration' do - let(:integration) { inherited_integration } - end it_behaves_like 'inherits settings from integration' do - let(:integration) { not_inherited_integration } - end - - it_behaves_like 'does not inherit settings from integration' do - let(:integration) { another_inherited_integration } - end - - it_behaves_like 'inherits settings from integration' do - let(:integration) { project.jira_service } + let(:integration) { Service.find_by(group_id: group.id) } end end it 'updates project#has_external_issue_tracker for issue tracker services' do - described_class.propagate(integration: instance_integration, overwrite: true) + described_class.propagate(instance_integration) expect(project.reload.has_external_issue_tracker).to eq(true) end @@ -141,7 +130,7 @@ RSpec.describe Admin::PropagateIntegrationService do external_wiki_url: 'http://external-wiki-url.com' ) - described_class.propagate(integration: instance_integration, overwrite: true) + described_class.propagate(instance_integration) expect(project.reload.has_external_wiki).to eq(true) end diff --git a/spec/services/projects/propagate_service_template_spec.rb b/spec/services/admin/propagate_service_template_spec.rb index df69e5a29fb..15654653095 100644 --- a/spec/services/projects/propagate_service_template_spec.rb +++ b/spec/services/admin/propagate_service_template_spec.rb @@ -2,10 +2,10 @@ require 'spec_helper' -RSpec.describe Projects::PropagateServiceTemplate do +RSpec.describe Admin::PropagateServiceTemplate do describe '.propagate' do let!(:service_template) do - PushoverService.create( + PushoverService.create!( template: true, active: true, push_events: false, @@ -31,8 +31,7 @@ RSpec.describe Projects::PropagateServiceTemplate do end it 'creates services for a project that has another service' do - BambooService.create( - template: true, + BambooService.create!( active: true, project: project, properties: { @@ -51,7 +50,7 @@ RSpec.describe Projects::PropagateServiceTemplate do end it 'does not create the service if it exists already' do - other_service = BambooService.create( + other_service = BambooService.create!( template: true, active: true, properties: { @@ -79,7 +78,11 @@ RSpec.describe Projects::PropagateServiceTemplate do end context 'service with data fields' do + include JiraServiceHelper + let(:service_template) do + stub_jira_service_test + JiraService.create!( template: true, active: true, @@ -106,7 +109,7 @@ RSpec.describe Projects::PropagateServiceTemplate do let(:project_total) { 5 } before do - stub_const 'Projects::PropagateServiceTemplate::BATCH_SIZE', 3 + stub_const('Admin::PropagateServiceTemplate::BATCH_SIZE', 3) project_total.times { create(:project) } diff --git a/spec/services/alert_management/create_alert_issue_service_spec.rb b/spec/services/alert_management/create_alert_issue_service_spec.rb index cf24188a738..f2be317a13d 100644 --- a/spec/services/alert_management/create_alert_issue_service_spec.rb +++ b/spec/services/alert_management/create_alert_issue_service_spec.rb @@ -66,7 +66,7 @@ RSpec.describe AlertManagement::CreateAlertIssueService do end it 'sets the issue description' do - expect(created_issue.description).to include(alert_presenter.issue_summary_markdown.strip) + expect(created_issue.description).to include(alert_presenter.send(:issue_summary_markdown).strip) end end @@ -82,6 +82,30 @@ RSpec.describe AlertManagement::CreateAlertIssueService do expect(user).to have_received(:can?).with(:create_issue, project) end + context 'with alert severity' do + using RSpec::Parameterized::TableSyntax + + where(:alert_severity, :incident_severity) do + 'critical' | 'critical' + 'high' | 'high' + 'medium' | 'medium' + 'low' | 'low' + 'info' | 'unknown' + 'unknown' | 'unknown' + end + + with_them do + before do + alert.update!(severity: alert_severity) + execute + end + + it 'sets the correct severity level' do + expect(created_issue.severity).to eq(incident_severity) + end + end + end + context 'when the alert is prometheus alert' do let(:alert) { prometheus_alert } let(:issue) { subject.payload[:issue] } diff --git a/spec/services/alert_management/process_prometheus_alert_service_spec.rb b/spec/services/alert_management/process_prometheus_alert_service_spec.rb index 533e2473cb8..b14cc65506a 100644 --- a/spec/services/alert_management/process_prometheus_alert_service_spec.rb +++ b/spec/services/alert_management/process_prometheus_alert_service_spec.rb @@ -3,17 +3,29 @@ require 'spec_helper' RSpec.describe AlertManagement::ProcessPrometheusAlertService do - let_it_be(:project) { create(:project, :repository) } + let_it_be(:project, reload: true) { create(:project, :repository) } before do allow(ProjectServiceWorker).to receive(:perform_async) end describe '#execute' do - subject(:execute) { described_class.new(project, nil, payload).execute } + let(:service) { described_class.new(project, nil, payload) } + let(:incident_management_setting) { double(auto_close_incident?: auto_close_incident, create_issue?: create_issue) } + let(:auto_close_incident) { true } + let(:create_issue) { true } + + before do + allow(service) + .to receive(:incident_management_setting) + .and_return(incident_management_setting) + end + + subject(:execute) { service.execute } context 'when alert payload is valid' do - let(:parsed_alert) { Gitlab::Alerting::Alert.new(project: project, payload: payload) } + let(:parsed_payload) { Gitlab::AlertManagement::Payload.parse(project, payload, monitoring_tool: 'Prometheus') } + let(:fingerprint) { parsed_payload.gitlab_fingerprint } let(:payload) do { 'status' => status, @@ -39,25 +51,26 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do context 'when Prometheus alert status is firing' do context 'when alert with the same fingerprint already exists' do - let!(:alert) { create(:alert_management_alert, project: project, fingerprint: parsed_alert.gitlab_fingerprint) } + let!(:alert) { create(:alert_management_alert, project: project, fingerprint: fingerprint) } it_behaves_like 'adds an alert management alert event' + it_behaves_like 'processes incident issues' context 'existing alert is resolved' do - let!(:alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: parsed_alert.gitlab_fingerprint) } + let!(:alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: fingerprint) } it_behaves_like 'creates an alert management alert' end context 'existing alert is ignored' do - let!(:alert) { create(:alert_management_alert, :ignored, project: project, fingerprint: parsed_alert.gitlab_fingerprint) } + let!(:alert) { create(:alert_management_alert, :ignored, project: project, fingerprint: fingerprint) } it_behaves_like 'adds an alert management alert event' end context 'two existing alerts, one resolved one open' do - let!(:resolved_alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: parsed_alert.gitlab_fingerprint) } - let!(:alert) { create(:alert_management_alert, project: project, fingerprint: parsed_alert.gitlab_fingerprint) } + let!(:resolved_alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: fingerprint) } + let!(:alert) { create(:alert_management_alert, project: project, fingerprint: fingerprint) } it_behaves_like 'adds an alert management alert event' end @@ -78,46 +91,47 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do execute end end + + context 'when auto-alert creation is disabled' do + let(:create_issue) { false } + + it_behaves_like 'does not process incident issues' + end end context 'when alert does not exist' do context 'when alert can be created' do it_behaves_like 'creates an alert management alert' - it 'processes the incident alert' do - expect(IncidentManagement::ProcessAlertWorker) - .to receive(:perform_async) - .with(nil, nil, kind_of(Integer)) - .once + it 'creates a system note corresponding to alert creation' do + expect { subject }.to change(Note, :count).by(1) + end + + it_behaves_like 'processes incident issues' - expect(subject).to be_success + context 'when auto-alert creation is disabled' do + let(:create_issue) { false } + + it_behaves_like 'does not process incident issues' end end context 'when alert cannot be created' do - let(:errors) { double(messages: { hosts: ['hosts array is over 255 chars'] })} - let(:am_alert) { instance_double(AlertManagement::Alert, save: false, errors: errors) } - before do - allow(AlertManagement::Alert).to receive(:new).and_return(am_alert) + payload['annotations']['title'] = 'description' * 50 end it 'writes a warning to the log' do expect(Gitlab::AppLogger).to receive(:warn).with( message: 'Unable to create AlertManagement::Alert', project_id: project.id, - alert_errors: { hosts: ['hosts array is over 255 chars'] } + alert_errors: { title: ["is too long (maximum is 200 characters)"] } ) execute end - it 'does not create incident issue' do - expect(IncidentManagement::ProcessAlertWorker) - .not_to receive(:perform_async) - - expect(subject).to be_success - end + it_behaves_like 'does not process incident issues' end it { is_expected.to be_success } @@ -126,57 +140,67 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do context 'when Prometheus alert status is resolved' do let(:status) { 'resolved' } - let!(:alert) { create(:alert_management_alert, project: project, fingerprint: parsed_alert.gitlab_fingerprint) } + let!(:alert) { create(:alert_management_alert, project: project, fingerprint: fingerprint) } - context 'when status can be changed' do - it 'resolves an existing alert' do - expect { execute }.to change { alert.reload.resolved? }.to(true) - end + context 'when auto_resolve_incident set to true' do + context 'when status can be changed' do + it 'resolves an existing alert' do + expect { execute }.to change { alert.reload.resolved? }.to(true) + end - [true, false].each do |state_tracking_enabled| - context 'existing issue' do - before do - stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - end + [true, false].each do |state_tracking_enabled| + context 'existing issue' do + before do + stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) + end - let!(:alert) { create(:alert_management_alert, :with_issue, project: project, fingerprint: parsed_alert.gitlab_fingerprint) } + let!(:alert) { create(:alert_management_alert, :with_issue, project: project, fingerprint: fingerprint) } - it 'closes the issue' do - issue = alert.issue + it 'closes the issue' do + issue = alert.issue - expect { execute } - .to change { issue.reload.state } - .from('opened') - .to('closed') - end + expect { execute } + .to change { issue.reload.state } + .from('opened') + .to('closed') + end - if state_tracking_enabled - specify { expect { execute }.to change(ResourceStateEvent, :count).by(1) } - else - specify { expect { execute }.to change(Note, :count).by(1) } + if state_tracking_enabled + specify { expect { execute }.to change(ResourceStateEvent, :count).by(1) } + else + specify { expect { execute }.to change(Note, :count).by(1) } + end end end end - end - context 'when status change did not succeed' do - before do - allow(AlertManagement::Alert).to receive(:for_fingerprint).and_return([alert]) - allow(alert).to receive(:resolve).and_return(false) - end + context 'when status change did not succeed' do + before do + allow(AlertManagement::Alert).to receive(:for_fingerprint).and_return([alert]) + allow(alert).to receive(:resolve).and_return(false) + end - it 'writes a warning to the log' do - expect(Gitlab::AppLogger).to receive(:warn).with( - message: 'Unable to update AlertManagement::Alert status to resolved', - project_id: project.id, - alert_id: alert.id - ) + it 'writes a warning to the log' do + expect(Gitlab::AppLogger).to receive(:warn).with( + message: 'Unable to update AlertManagement::Alert status to resolved', + project_id: project.id, + alert_id: alert.id + ) - execute + execute + end end + + it { is_expected.to be_success } end - it { is_expected.to be_success } + context 'when auto_resolve_incident set to false' do + let(:auto_close_incident) { false } + + it 'does not resolve an existing alert' do + expect { execute }.not_to change { alert.reload.resolved? } + end + end end context 'environment given' do diff --git a/spec/services/audit_event_service_spec.rb b/spec/services/audit_event_service_spec.rb index 530d3469481..93de2a23edc 100644 --- a/spec/services/audit_event_service_spec.rb +++ b/spec/services/audit_event_service_spec.rb @@ -22,7 +22,7 @@ RSpec.describe AuditEventService do entity_type: "Project", action: :destroy) - expect { service.security_event }.to change(SecurityEvent, :count).by(1) + expect { service.security_event }.to change(AuditEvent, :count).by(1) end it 'formats from and to fields' do @@ -44,14 +44,30 @@ RSpec.describe AuditEventService do action: :create, target_id: 1) - expect { service.security_event }.to change(SecurityEvent, :count).by(1) + expect { service.security_event }.to change(AuditEvent, :count).by(1) - details = SecurityEvent.last.details + details = AuditEvent.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 + + context 'authentication event' do + let(:audit_service) { described_class.new(user, user, with: 'standard') } + + it 'creates an authentication event' do + expect(AuthenticationEvent).to receive(:create).with( + user: user, + user_name: user.name, + ip_address: user.current_sign_in_ip, + result: AuthenticationEvent.results[:success], + provider: 'standard' + ) + + audit_service.for_authentication.security_event + end + end end describe '#log_security_event_to_file' do diff --git a/spec/services/authorized_project_update/project_create_service_spec.rb b/spec/services/authorized_project_update/project_create_service_spec.rb index 891800bfb87..a9d0b82acfb 100644 --- a/spec/services/authorized_project_update/project_create_service_spec.rb +++ b/spec/services/authorized_project_update/project_create_service_spec.rb @@ -81,6 +81,7 @@ RSpec.describe AuthorizedProjectUpdate::ProjectCreateService do before do create(:group_member, access_level: Gitlab::Access::REPORTER, group: group, user: group_user) create(:group_member, access_level: Gitlab::Access::MAINTAINER, group: shared_with_group, user: group_user) + create(:group_member, :minimal_access, source: shared_with_group, user: create(:user)) create(:group_group_link, shared_group: group, shared_with_group: shared_with_group, group_access: Gitlab::Access::DEVELOPER) @@ -97,6 +98,11 @@ RSpec.describe AuthorizedProjectUpdate::ProjectCreateService do access_level: Gitlab::Access::DEVELOPER) expect(project_authorization).to exist end + + it 'does not create project authorization for user with minimal access' do + expect { service.execute }.to( + change { ProjectAuthorization.count }.from(0).to(1)) + end end end @@ -118,6 +124,17 @@ RSpec.describe AuthorizedProjectUpdate::ProjectCreateService do end end + context 'member with minimal access' do + before do + create(:group_member, :minimal_access, user: group_user, source: group) + end + + it 'does not create project authorization' do + expect { service.execute }.not_to( + change { ProjectAuthorization.count }.from(0)) + end + end + context 'project has more user than BATCH_SIZE' do let(:batch_size) { 2 } let(:users) { create_list(:user, batch_size + 1 ) } diff --git a/spec/services/authorized_project_update/project_group_link_create_service_spec.rb b/spec/services/authorized_project_update/project_group_link_create_service_spec.rb index 961322a1a21..1fd47f78c24 100644 --- a/spec/services/authorized_project_update/project_group_link_create_service_spec.rb +++ b/spec/services/authorized_project_update/project_group_link_create_service_spec.rb @@ -112,6 +112,17 @@ RSpec.describe AuthorizedProjectUpdate::ProjectGroupLinkCreateService do end end + context 'minimal access member' do + before do + create(:group_member, :minimal_access, user: group_user, source: group) + end + + it 'does not create project authorization' do + expect { service.execute }.not_to( + change { ProjectAuthorization.count }.from(0)) + end + end + context 'project has more users than BATCH_SIZE' do let(:batch_size) { 2 } let(:users) { create_list(:user, batch_size + 1 ) } diff --git a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb index 3bf59f6a2d1..7b428550768 100644 --- a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb @@ -91,18 +91,6 @@ RSpec.describe AutoMerge::MergeWhenPipelineSucceedsService do end end - context 'without feature enabled' do - it 'does not send notification' do - stub_feature_flags(mwps_notification: false) - - allow(merge_request) - .to receive_messages(head_pipeline: pipeline, actual_head_pipeline: pipeline) - expect(MailScheduler::NotificationServiceWorker).not_to receive(:perform_async) - - service.execute(merge_request) - end - end - context 'already approved' do let(:service) { described_class.new(project, user, should_remove_source_branch: true) } let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch) } diff --git a/spec/services/branches/delete_service_spec.rb b/spec/services/branches/delete_service_spec.rb index f1e7c9340b1..291431c1723 100644 --- a/spec/services/branches/delete_service_spec.rb +++ b/spec/services/branches/delete_service_spec.rb @@ -37,6 +37,21 @@ RSpec.describe Branches::DeleteService do end it_behaves_like 'a deleted branch', 'feature' + + context 'when Gitlab::Git::CommandError is raised' do + before do + allow(repository).to receive(:rm_branch) do + raise Gitlab::Git::CommandError.new('Could not update patch') + end + end + + it 'handles and returns error' do + result = service.execute('feature') + + expect(result.status).to eq(:error) + expect(result.message).to eq('Could not update patch') + end + end end context 'when user does not have access to push to repository' do diff --git a/spec/services/ci/cancel_user_pipelines_service_spec.rb b/spec/services/ci/cancel_user_pipelines_service_spec.rb index 12117051b64..8491242dfd5 100644 --- a/spec/services/ci/cancel_user_pipelines_service_spec.rb +++ b/spec/services/ci/cancel_user_pipelines_service_spec.rb @@ -19,5 +19,17 @@ RSpec.describe Ci::CancelUserPipelinesService do expect(build.reload).to be_canceled end end + + context 'when an error ocurrs' do + it 'raises a service level error' do + service = double(execute: ServiceResponse.error(message: 'Error canceling pipeline')) + allow(::Ci::CancelUserPipelinesService).to receive(:new).and_return(service) + + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result).to be_error + end + end end end diff --git a/spec/services/ci/create_cross_project_pipeline_service_spec.rb b/spec/services/ci/create_downstream_pipeline_service_spec.rb index 1aabdb85afd..a6ea30e4703 100644 --- a/spec/services/ci/create_cross_project_pipeline_service_spec.rb +++ b/spec/services/ci/create_downstream_pipeline_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::CreateCrossProjectPipelineService, '#execute' do +RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do let_it_be(:user) { create(:user) } let(:upstream_project) { create(:project, :repository) } let_it_be(:downstream_project) { create(:project, :repository) } @@ -130,7 +130,7 @@ RSpec.describe Ci::CreateCrossProjectPipelineService, '#execute' do expect(Gitlab::ErrorTracking) .to receive(:track_exception) .with( - instance_of(Ci::CreateCrossProjectPipelineService::DuplicateDownstreamPipelineError), + instance_of(described_class::DuplicateDownstreamPipelineError), bridge_id: bridge.id, project_id: bridge.project.id) .and_call_original expect(Ci::CreatePipelineService).not_to receive(:new) @@ -179,7 +179,7 @@ RSpec.describe Ci::CreateCrossProjectPipelineService, '#execute' do end end - context 'when downstream project is the same as the job project' do + context 'when downstream project is the same as the upstream project' do let(:trigger) do { trigger: { project: upstream_project.full_path } } end @@ -311,24 +311,78 @@ RSpec.describe Ci::CreateCrossProjectPipelineService, '#execute' do end end - context 'when upstream pipeline is a child pipeline' do - let!(:pipeline_source) do + context 'when upstream pipeline has a parent pipeline' do + before do create(:ci_sources_pipeline, source_pipeline: create(:ci_pipeline, project: upstream_pipeline.project), pipeline: upstream_pipeline ) end + it 'creates the pipeline' do + expect { service.execute(bridge) } + .to change { Ci::Pipeline.count }.by(1) + + expect(bridge.reload).to be_success + end + + context 'when FF ci_child_of_child_pipeline is disabled' do + before do + stub_feature_flags(ci_child_of_child_pipeline: false) + end + + it 'does not create a further child pipeline' do + expect { service.execute(bridge) } + .not_to change { Ci::Pipeline.count } + + expect(bridge.reload).to be_failed + expect(bridge.failure_reason).to eq 'bridge_pipeline_is_child_pipeline' + end + end + end + + context 'when upstream pipeline has a parent pipeline, which has a parent pipeline' do before do - upstream_pipeline.update!(source: :parent_pipeline) + parent_of_upstream_pipeline = create(:ci_pipeline, project: upstream_pipeline.project) + + create(:ci_sources_pipeline, + source_pipeline: create(:ci_pipeline, project: upstream_pipeline.project), + pipeline: parent_of_upstream_pipeline + ) + + create(:ci_sources_pipeline, + source_pipeline: parent_of_upstream_pipeline, + pipeline: upstream_pipeline + ) end - it 'does not create a further child pipeline' do + it 'does not create a second descendant pipeline' do expect { service.execute(bridge) } .not_to change { Ci::Pipeline.count } expect(bridge.reload).to be_failed - expect(bridge.failure_reason).to eq 'bridge_pipeline_is_child_pipeline' + expect(bridge.failure_reason).to eq 'reached_max_descendant_pipelines_depth' + end + end + + context 'when upstream pipeline has two level upstream pipelines from different projects' do + before do + upstream_of_upstream_of_upstream_pipeline = create(:ci_pipeline) + upstream_of_upstream_pipeline = create(:ci_pipeline) + + create(:ci_sources_pipeline, + source_pipeline: upstream_of_upstream_of_upstream_pipeline, + pipeline: upstream_of_upstream_pipeline + ) + + create(:ci_sources_pipeline, + source_pipeline: upstream_of_upstream_pipeline, + pipeline: upstream_pipeline + ) + end + + it 'create the pipeline' do + expect { service.execute(bridge) }.to change { Ci::Pipeline.count }.by(1) end end end @@ -397,7 +451,7 @@ RSpec.describe Ci::CreateCrossProjectPipelineService, '#execute' do context 'when pipeline variables are defined' do before do - upstream_pipeline.variables.create(key: 'PIPELINE_VARIABLE', value: 'my-value') + upstream_pipeline.variables.create!(key: 'PIPELINE_VARIABLE', value: 'my-value') end it 'does not pass pipeline variables directly downstream' do diff --git a/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb b/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb index 3be5ac1f739..b5b3832ac00 100644 --- a/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb +++ b/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb @@ -101,7 +101,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'contains only errors' do - error_message = 'root config contains unknown keys: invalid' + error_message = 'jobs invalid config should implement a script: or a trigger: keyword' expect(pipeline.yaml_errors).to eq(error_message) expect(pipeline.error_messages.map(&:content)).to contain_exactly(error_message) expect(pipeline.errors.full_messages).to contain_exactly(error_message) diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index db4c2f5a047..e0893ed6de3 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -223,7 +223,7 @@ RSpec.describe Ci::CreatePipelineService do context 'auto-cancel enabled' do before do - project.update(auto_cancel_pending_pipelines: 'enabled') + project.update!(auto_cancel_pending_pipelines: 'enabled') end it 'does not cancel HEAD pipeline' do @@ -248,7 +248,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'cancel created outdated pipelines', :sidekiq_might_not_need_inline do - pipeline_on_previous_commit.update(status: 'created') + pipeline_on_previous_commit.update!(status: 'created') pipeline expect(pipeline_on_previous_commit.reload).to have_attributes(status: 'canceled', auto_canceled_by_id: pipeline.id) @@ -439,7 +439,7 @@ RSpec.describe Ci::CreatePipelineService do context 'auto-cancel disabled' do before do - project.update(auto_cancel_pending_pipelines: 'disabled') + project.update!(auto_cancel_pending_pipelines: 'disabled') end it 'does not auto cancel pending non-HEAD pipelines' do @@ -513,7 +513,7 @@ RSpec.describe Ci::CreatePipelineService do it 'pull it from Auto-DevOps' do pipeline = execute_service expect(pipeline).to be_auto_devops_source - expect(pipeline.builds.map(&:name)).to match_array(%w[build code_quality eslint-sast secret_detection_default_branch secrets-sast test]) + expect(pipeline.builds.map(&:name)).to match_array(%w[build code_quality eslint-sast secret_detection_default_branch test]) end end diff --git a/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb b/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb index 79443f16276..1c96be42a2f 100644 --- a/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb +++ b/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb @@ -11,6 +11,10 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared let(:service) { described_class.new } let!(:artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) } + before do + artifact.job.pipeline.unlocked! + end + context 'when artifact is expired' do context 'when artifact is not locked' do before do @@ -88,6 +92,8 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared before do stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 1) stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1) + + second_artifact.job.pipeline.unlocked! end let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) } @@ -102,7 +108,9 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared end context 'when there are no artifacts' do - let!(:artifact) { } + before do + artifact.destroy! + end it 'does not raise error' do expect { subject }.not_to raise_error @@ -112,6 +120,8 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared context 'when there are artifacts more than batch sizes' do before do stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1) + + second_artifact.job.pipeline.unlocked! end let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) } @@ -120,5 +130,45 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared expect { subject }.to change { Ci::JobArtifact.count }.by(-2) end end + + context 'when artifact is a pipeline artifact' do + context 'when artifacts are expired' do + let!(:pipeline_artifact_1) { create(:ci_pipeline_artifact, expire_at: 1.week.ago) } + let!(:pipeline_artifact_2) { create(:ci_pipeline_artifact, expire_at: 1.week.ago) } + + before do + [pipeline_artifact_1, pipeline_artifact_2].each { |pipeline_artifact| pipeline_artifact.pipeline.unlocked! } + end + + it 'destroys pipeline artifacts' do + expect { subject }.to change { Ci::PipelineArtifact.count }.by(-2) + end + end + + context 'when artifacts are not expired' do + let!(:pipeline_artifact_1) { create(:ci_pipeline_artifact, expire_at: 2.days.from_now) } + let!(:pipeline_artifact_2) { create(:ci_pipeline_artifact, expire_at: 2.days.from_now) } + + before do + [pipeline_artifact_1, pipeline_artifact_2].each { |pipeline_artifact| pipeline_artifact.pipeline.unlocked! } + end + + it 'does not destroy pipeline artifacts' do + expect { subject }.not_to change { Ci::PipelineArtifact.count } + end + end + end + + context 'when some artifacts are locked' do + before do + pipeline = create(:ci_pipeline, locked: :artifacts_locked) + job = create(:ci_build, pipeline: pipeline) + create(:ci_job_artifact, expire_at: 1.day.ago, job: job) + end + + it 'destroys only unlocked artifacts' do + expect { subject }.to change { Ci::JobArtifact.count }.by(-1) + end + end end end diff --git a/spec/services/ci/destroy_pipeline_service_spec.rb b/spec/services/ci/destroy_pipeline_service_spec.rb index 23cbe683d2f..6977c99e335 100644 --- a/spec/services/ci/destroy_pipeline_service_spec.rb +++ b/spec/services/ci/destroy_pipeline_service_spec.rb @@ -29,7 +29,7 @@ RSpec.describe ::Ci::DestroyPipelineService do end it 'does not log an audit event' do - expect { subject }.not_to change { SecurityEvent.count } + expect { subject }.not_to change { AuditEvent.count } end context 'when the pipeline has jobs' do diff --git a/spec/services/ci/generate_coverage_reports_service_spec.rb b/spec/services/ci/generate_coverage_reports_service_spec.rb index a3ed2eec713..d39053adebc 100644 --- a/spec/services/ci/generate_coverage_reports_service_spec.rb +++ b/spec/services/ci/generate_coverage_reports_service_spec.rb @@ -15,21 +15,25 @@ RSpec.describe Ci::GenerateCoverageReportsService do let!(:head_pipeline) { merge_request.head_pipeline } let!(:base_pipeline) { nil } - it 'returns status and data' do + it 'returns status and data', :aggregate_failures do + expect_any_instance_of(Ci::PipelineArtifact) do |instance| + expect(instance).to receive(:present) + expect(instance).to receive(:for_files).with(merge_request.new_paths).and_call_original + end + expect(subject[:status]).to eq(:parsed) expect(subject[:data]).to eq(files: {}) end end - context 'when head pipeline has corrupted coverage reports' do + context 'when head pipeline does not have a coverage report artifact' do let!(:merge_request) { create(:merge_request, :with_coverage_reports, source_project: project) } let!(:service) { described_class.new(project, nil, id: merge_request.id) } let!(:head_pipeline) { merge_request.head_pipeline } let!(:base_pipeline) { nil } before do - build = create(:ci_build, pipeline: head_pipeline, project: head_pipeline.project) - create(:ci_job_artifact, :coverage_with_corrupted_data, job: build, project: project) + head_pipeline.pipeline_artifacts.destroy_all # rubocop: disable Cop/DestroyAll end it 'returns status and error message' do diff --git a/spec/services/ci/parse_dotenv_artifact_service_spec.rb b/spec/services/ci/parse_dotenv_artifact_service_spec.rb index a5f01187a83..91b81af9fd1 100644 --- a/spec/services/ci/parse_dotenv_artifact_service_spec.rb +++ b/spec/services/ci/parse_dotenv_artifact_service_spec.rb @@ -66,12 +66,13 @@ RSpec.describe Ci::ParseDotenvArtifactService do end context 'when multiple key/value pairs exist in one line' do - let(:blob) { 'KEY1=VAR1KEY2=VAR1' } + let(:blob) { 'KEY=VARCONTAINING=EQLS' } - it 'returns error' do - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq("Validation failed: Key can contain only letters, digits and '_'.") - expect(subject[:http_status]).to eq(:bad_request) + it 'parses the dotenv data' do + subject + + expect(build.job_variables.as_json).to contain_exactly( + hash_including('key' => 'KEY', 'value' => 'VARCONTAINING=EQLS')) end end diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_other_build_succeeds_deploy_needs_one_build_and_test.yml b/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_other_build_succeeds_deploy_needs_one_build_and_test.yml index a133023b12d..ef4ddff9b64 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_other_build_succeeds_deploy_needs_one_build_and_test.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_other_build_succeeds_deploy_needs_one_build_and_test.yml @@ -47,16 +47,13 @@ transitions: - event: drop jobs: [build_2] expect: - pipeline: running + pipeline: failed stages: build: failed test: skipped - deploy: pending + deploy: skipped jobs: build_1: success build_2: failed test: skipped - deploy: pending - -# TODO: should we run deploy? -# Further discussions: https://gitlab.com/gitlab-org/gitlab/-/issues/213080 + deploy: skipped diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_builds_succeed_test_on_failure_deploy_needs_one_build_and_test.yml b/spec/services/ci/pipeline_processing/test_cases/dag_builds_succeed_test_on_failure_deploy_needs_one_build_and_test.yml index f324525bd56..29c1562389c 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_builds_succeed_test_on_failure_deploy_needs_one_build_and_test.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_builds_succeed_test_on_failure_deploy_needs_one_build_and_test.yml @@ -34,30 +34,13 @@ transitions: - event: success jobs: [build_1, build_2] expect: - pipeline: running - stages: - build: success - test: skipped - deploy: pending - jobs: - build_1: success - build_2: success - test: skipped - deploy: pending - - - event: success - jobs: [deploy] - expect: pipeline: success stages: build: success test: skipped - deploy: success + deploy: skipped jobs: build_1: success build_2: success test: skipped - deploy: success - -# TODO: should we run deploy? -# Further discussions: https://gitlab.com/gitlab-org/gitlab/-/issues/213080 + deploy: skipped diff --git a/spec/services/ci/pipelines/create_artifact_service_spec.rb b/spec/services/ci/pipelines/create_artifact_service_spec.rb new file mode 100644 index 00000000000..d5e9cf83a6d --- /dev/null +++ b/spec/services/ci/pipelines/create_artifact_service_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::Pipelines::CreateArtifactService do + describe '#execute' do + subject { described_class.new.execute(pipeline) } + + context 'when pipeline has coverage reports' do + let(:pipeline) { create(:ci_pipeline, :with_coverage_reports) } + + context 'when pipeline is finished' do + it 'creates a pipeline artifact' do + subject + + expect(Ci::PipelineArtifact.count).to eq(1) + end + + it 'persists the default file name' do + subject + + file = Ci::PipelineArtifact.first.file + + expect(file.filename).to eq('code_coverage.json') + end + + it 'sets expire_at to 1 week' do + freeze_time do + subject + + pipeline_artifact = Ci::PipelineArtifact.first + + expect(pipeline_artifact.expire_at).to eq(1.week.from_now) + end + end + end + + context 'when feature is disabled' do + it 'does not create a pipeline artifact' do + stub_feature_flags(coverage_report_view: false) + + subject + + expect(Ci::PipelineArtifact.count).to eq(0) + end + end + + context 'when pipeline artifact has already been created' do + it 'do not raise an error and do not persist the same artifact twice' do + expect { 2.times { described_class.new.execute(pipeline) } }.not_to raise_error(ActiveRecord::RecordNotUnique) + + expect(Ci::PipelineArtifact.count).to eq(1) + end + end + end + + context 'when pipeline is running and coverage report does not exist' do + let(:pipeline) { create(:ci_pipeline, :running) } + + it 'does not persist data' do + subject + + expect(Ci::PipelineArtifact.count).to eq(0) + end + end + end +end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 921f5ba4c7e..0cdc8d2c870 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -15,14 +15,14 @@ module Ci describe '#execute' do context 'runner follow tag list' do it "picks build with the same tag" do - pending_job.update(tag_list: ["linux"]) - specific_runner.update(tag_list: ["linux"]) + pending_job.update!(tag_list: ["linux"]) + specific_runner.update!(tag_list: ["linux"]) expect(execute(specific_runner)).to eq(pending_job) end it "does not pick build with different tag" do - pending_job.update(tag_list: ["linux"]) - specific_runner.update(tag_list: ["win32"]) + pending_job.update!(tag_list: ["linux"]) + specific_runner.update!(tag_list: ["win32"]) expect(execute(specific_runner)).to be_falsey end @@ -31,24 +31,24 @@ module Ci end it "does not pick build with tag" do - pending_job.update(tag_list: ["linux"]) + pending_job.update!(tag_list: ["linux"]) expect(execute(specific_runner)).to be_falsey end it "pick build without tag" do - specific_runner.update(tag_list: ["win32"]) + specific_runner.update!(tag_list: ["win32"]) expect(execute(specific_runner)).to eq(pending_job) end end context 'deleted projects' do before do - project.update(pending_delete: true) + project.update!(pending_delete: true) end context 'for shared runners' do before do - project.update(shared_runners_enabled: true) + project.update!(shared_runners_enabled: true) end it 'does not pick a build' do @@ -65,7 +65,7 @@ module Ci context 'allow shared runners' do before do - project.update(shared_runners_enabled: true) + project.update!(shared_runners_enabled: true) end context 'for multiple builds' do @@ -131,7 +131,7 @@ module Ci context 'disallow shared runners' do before do - project.update(shared_runners_enabled: false) + project.update!(shared_runners_enabled: false) end context 'shared runner' do @@ -152,7 +152,7 @@ module Ci context 'disallow when builds are disabled' do before do - project.update(shared_runners_enabled: true, group_runners_enabled: true) + project.update!(shared_runners_enabled: true, group_runners_enabled: true) project.project_feature.update_attribute(:builds_access_level, ProjectFeature::DISABLED) end @@ -591,8 +591,8 @@ module Ci .with(:job_queue_duration_seconds, anything, anything, anything) .and_return(job_queue_duration_seconds) - project.update(shared_runners_enabled: true) - pending_job.update(created_at: current_time - 3600, queued_at: current_time - 1800) + project.update!(shared_runners_enabled: true) + pending_job.update!(created_at: current_time - 3600, queued_at: current_time - 1800) end shared_examples 'attempt counter collector' do @@ -661,7 +661,7 @@ module Ci context 'when pending job with queued_at=nil is used' do before do - pending_job.update(queued_at: nil) + pending_job.update!(queued_at: nil) end it_behaves_like 'attempt counter collector' diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 5a245415b32..51741440075 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -22,7 +22,7 @@ RSpec.describe Ci::RetryBuildService do described_class.new(project, user) end - clone_accessors = described_class::CLONE_ACCESSORS + clone_accessors = described_class.clone_accessors reject_accessors = %i[id status user token token_encrypted coverage trace runner @@ -50,7 +50,7 @@ RSpec.describe Ci::RetryBuildService do metadata runner_session trace_chunks upstream_pipeline_id artifacts_file artifacts_metadata artifacts_size commands resource resource_group_id processed security_scans author - pipeline_id report_results].freeze + pipeline_id report_results pending_state pages_deployments].freeze shared_examples 'build duplication' do let(:another_pipeline) { create(:ci_empty_pipeline, project: project) } @@ -70,7 +70,7 @@ RSpec.describe Ci::RetryBuildService do # Make sure that build has both `stage_id` and `stage` because FactoryBot # can reset one of the fields when assigning another. We plan to deprecate # and remove legacy `stage` column in the future. - build.update(stage: 'test', stage_id: stage.id) + build.update!(stage: 'test', stage_id: stage.id) # Make sure we have one instance for every possible job_artifact_X # associations to check they are correctly rejected on build duplication. @@ -143,6 +143,8 @@ RSpec.describe Ci::RetryBuildService do Ci::Build.reflect_on_all_associations.map(&:name) + [:tag_list, :needs_attributes] + current_accessors << :secrets if Gitlab.ee? + current_accessors.uniq! expect(current_accessors).to include(*processed_accessors) @@ -181,17 +183,24 @@ RSpec.describe Ci::RetryBuildService do service.execute(build) end - context 'when there are subsequent builds that are skipped' do + context 'when there are subsequent processables that are skipped' do let!(:subsequent_build) do create(:ci_build, :skipped, stage_idx: 2, pipeline: pipeline, stage: 'deploy') end - it 'resumes pipeline processing in a subsequent stage' do + let!(:subsequent_bridge) do + create(:ci_bridge, :skipped, stage_idx: 2, + pipeline: pipeline, + stage: 'deploy') + end + + it 'resumes pipeline processing in the subsequent stage' do service.execute(build) expect(subsequent_build.reload).to be_created + expect(subsequent_bridge.reload).to be_created end end @@ -223,6 +232,19 @@ RSpec.describe Ci::RetryBuildService do end end end + + context 'when the pipeline is a child pipeline and the bridge is depended' do + let!(:parent_pipeline) { create(:ci_pipeline, project: project) } + let!(:pipeline) { create(:ci_pipeline, project: project) } + let!(:bridge) { create(:ci_bridge, :strategy_depend, pipeline: parent_pipeline, status: 'success') } + let!(:source_pipeline) { create(:ci_sources_pipeline, pipeline: pipeline, source_job: bridge) } + + it 'marks source bridge as pending' do + service.execute(build) + + expect(bridge.reload).to be_pending + end + end end context 'when user does not have ability to execute build' do diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb index 212c8f99865..526c2f39b46 100644 --- a/spec/services/ci/retry_pipeline_service_spec.rb +++ b/spec/services/ci/retry_pipeline_service_spec.rb @@ -280,6 +280,20 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do expect(build3.reload.scheduling_type).to eq('dag') end end + + context 'when the pipeline is a downstream pipeline and the bridge is depended' do + let!(:bridge) { create(:ci_bridge, :strategy_depend, status: 'success') } + + before do + create(:ci_sources_pipeline, pipeline: pipeline, source_job: bridge) + end + + it 'marks source bridge as pending' do + service.execute(pipeline) + + expect(bridge.reload).to be_pending + end + end end context 'when user is not allowed to retry pipeline' do diff --git a/spec/services/ci/update_build_state_service_spec.rb b/spec/services/ci/update_build_state_service_spec.rb new file mode 100644 index 00000000000..f5ad732bf7e --- /dev/null +++ b/spec/services/ci/update_build_state_service_spec.rb @@ -0,0 +1,238 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::UpdateBuildStateService do + let(:project) { create(:project) } + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:build) { create(:ci_build, :running, pipeline: pipeline) } + let(:metrics) { spy('metrics') } + + subject { described_class.new(build, params) } + + before do + stub_feature_flags(ci_enable_live_trace: true) + end + + context 'when build does not have checksum' do + context 'when state has changed' do + let(:params) { { state: 'success' } } + + it 'updates a state of a running build' do + subject.execute + + expect(build).to be_success + end + + it 'returns 200 OK status' do + result = subject.execute + + expect(result.status).to eq 200 + end + + it 'does not increment finalized trace metric' do + execute_with_stubbed_metrics! + + expect(metrics) + .not_to have_received(:increment_trace_operation) + .with(operation: :finalized) + end + end + + context 'when it is a heartbeat request' do + let(:params) { { state: 'success' } } + + it 'updates a build timestamp' do + expect { subject.execute }.to change { build.updated_at } + end + end + + context 'when request payload carries a trace' do + let(:params) { { state: 'success', trace: 'overwritten' } } + + it 'overwrites a trace' do + result = subject.execute + + expect(build.trace.raw).to eq 'overwritten' + expect(result.status).to eq 200 + end + + it 'updates overwrite operation metric' do + execute_with_stubbed_metrics! + + expect(metrics) + .to have_received(:increment_trace_operation) + .with(operation: :overwrite) + end + end + + context 'when state is unknown' do + let(:params) { { state: 'unknown' } } + + it 'responds with 400 bad request' do + result = subject.execute + + expect(result.status).to eq 400 + expect(build).to be_running + end + end + end + + context 'when build has a checksum' do + let(:params) do + { checksum: 'crc32:12345678', state: 'failed', failure_reason: 'script_failure' } + end + + context 'when build trace has been migrated' do + before do + create(:ci_build_trace_chunk, :database_with_data, build: build) + end + + it 'updates a build state' do + subject.execute + + expect(build).to be_failed + end + + it 'responds with 200 OK status' do + result = subject.execute + + expect(result.status).to eq 200 + end + + it 'increments trace finalized operation metric' do + execute_with_stubbed_metrics! + + expect(metrics) + .to have_received(:increment_trace_operation) + .with(operation: :finalized) + end + end + + context 'when build trace has not been migrated yet' do + before do + create(:ci_build_trace_chunk, :redis_with_data, build: build) + end + + it 'does not update a build state' do + subject.execute + + expect(build).to be_running + end + + it 'responds with 202 accepted' do + result = subject.execute + + expect(result.status).to eq 202 + end + + it 'schedules live chunks for migration' do + expect(Ci::BuildTraceChunkFlushWorker) + .to receive(:perform_async) + .with(build.trace_chunks.first.id) + + subject.execute + end + + it 'increments trace accepted operation metric' do + execute_with_stubbed_metrics! + + expect(metrics) + .to have_received(:increment_trace_operation) + .with(operation: :accepted) + end + + it 'creates a pending state record' do + subject.execute + + build.pending_state.then do |status| + expect(status).to be_present + expect(status.state).to eq 'failed' + expect(status.trace_checksum).to eq 'crc32:12345678' + expect(status.failure_reason).to eq 'script_failure' + end + end + + context 'when build pending state is outdated' do + before do + build.create_pending_state( + state: 'failed', + trace_checksum: 'crc32:12345678', + failure_reason: 'script_failure', + created_at: 10.minutes.ago + ) + end + + it 'responds with 200 OK' do + result = subject.execute + + expect(result.status).to eq 200 + end + + it 'updates build state' do + subject.execute + + expect(build.reload).to be_failed + expect(build.failure_reason).to eq 'script_failure' + end + + it 'increments discarded traces metric' do + execute_with_stubbed_metrics! + + expect(metrics) + .to have_received(:increment_trace_operation) + .with(operation: :discarded) + end + + it 'does not increment finalized trace metric' do + execute_with_stubbed_metrics! + + expect(metrics) + .not_to have_received(:increment_trace_operation) + .with(operation: :finalized) + end + end + + context 'when build pending state has changes' do + before do + build.create_pending_state( + state: 'success', + created_at: 10.minutes.ago + ) + end + + it 'uses stored state and responds with 200 OK' do + result = subject.execute + + expect(result.status).to eq 200 + end + + it 'increments conflict trace metric' do + execute_with_stubbed_metrics! + + expect(metrics) + .to have_received(:increment_trace_operation) + .with(operation: :conflict) + end + end + + context 'when live traces are disabled' do + before do + stub_feature_flags(ci_enable_live_trace: false) + end + + it 'responds with 200 OK' do + result = subject.execute + + expect(result.status).to eq 200 + end + end + end + end + + def execute_with_stubbed_metrics! + described_class + .new(build, params, metrics) + .execute + end +end diff --git a/spec/services/ci/update_runner_service_spec.rb b/spec/services/ci/update_runner_service_spec.rb index cad9e893335..1c875b2f54a 100644 --- a/spec/services/ci/update_runner_service_spec.rb +++ b/spec/services/ci/update_runner_service_spec.rb @@ -50,7 +50,7 @@ RSpec.describe Ci::UpdateRunnerService do end def update - described_class.new(runner).update(params) + described_class.new(runner).update(params) # rubocop: disable Rails/SaveBang end end end diff --git a/spec/services/clusters/applications/schedule_update_service_spec.rb b/spec/services/clusters/applications/schedule_update_service_spec.rb index f559fb1b7aa..01a75a334e6 100644 --- a/spec/services/clusters/applications/schedule_update_service_spec.rb +++ b/spec/services/clusters/applications/schedule_update_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Clusters::Applications::ScheduleUpdateService do let(:project) { create(:project) } around do |example| - Timecop.freeze { example.run } + freeze_time { example.run } end context 'when application is able to be updated' do diff --git a/spec/services/clusters/aws/provision_service_spec.rb b/spec/services/clusters/aws/provision_service_spec.rb index 529e1d26575..52612e5ac40 100644 --- a/spec/services/clusters/aws/provision_service_spec.rb +++ b/spec/services/clusters/aws/provision_service_spec.rb @@ -22,6 +22,7 @@ RSpec.describe Clusters::Aws::ProvisionService do [ { parameter_key: 'ClusterName', parameter_value: provider.cluster.name }, { parameter_key: 'ClusterRole', parameter_value: provider.role_arn }, + { parameter_key: 'KubernetesVersion', parameter_value: provider.kubernetes_version }, { parameter_key: 'ClusterControlPlaneSecurityGroup', parameter_value: provider.security_group_id }, { parameter_key: 'VpcId', parameter_value: provider.vpc_id }, { parameter_key: 'Subnets', parameter_value: provider.subnet_ids.join(',') }, diff --git a/spec/services/deployments/after_create_service_spec.rb b/spec/services/deployments/after_create_service_spec.rb index 3287eed03b7..6cdb4c88191 100644 --- a/spec/services/deployments/after_create_service_spec.rb +++ b/spec/services/deployments/after_create_service_spec.rb @@ -122,7 +122,7 @@ RSpec.describe Deployments::AfterCreateService do end it 'renews auto stop at' do - Timecop.freeze do + freeze_time do environment.update!(auto_stop_at: nil) expect { subject.execute } diff --git a/spec/services/design_management/move_designs_service_spec.rb b/spec/services/design_management/move_designs_service_spec.rb index a05518dc28d..a43f0a2f805 100644 --- a/spec/services/design_management/move_designs_service_spec.rb +++ b/spec/services/design_management/move_designs_service_spec.rb @@ -32,30 +32,6 @@ RSpec.describe DesignManagement::MoveDesignsService do describe '#execute' do subject { service.execute } - context 'the feature is unavailable' do - let(:current_design) { designs.first } - let(:previous_design) { designs.second } - let(:next_design) { designs.third } - - before do - stub_feature_flags(reorder_designs: false) - end - - it 'raises cannot_move' do - expect(subject).to be_error.and(have_attributes(message: :cannot_move)) - end - - context 'but it is available on the current project' do - before do - stub_feature_flags(reorder_designs: issue.project) - end - - it 'is successful' do - expect(subject).to be_success - end - end - end - context 'the user cannot move designs' do let(:current_design) { designs.first } let(:current_user) { build_stubbed(:user) } @@ -124,7 +100,7 @@ RSpec.describe DesignManagement::MoveDesignsService do expect(subject).to be_success - expect(issue.designs.ordered(issue.project)).to eq([ + expect(issue.designs.ordered).to eq([ # Existing designs which already had a relative_position set. # These should stay at the beginning, in the same order. other_design1, diff --git a/spec/services/error_tracking/list_projects_service_spec.rb b/spec/services/error_tracking/list_projects_service_spec.rb index 8bc632349fa..ce391bd1ca0 100644 --- a/spec/services/error_tracking/list_projects_service_spec.rb +++ b/spec/services/error_tracking/list_projects_service_spec.rb @@ -121,7 +121,7 @@ RSpec.describe ErrorTracking::ListProjectsService do end context 'error_tracking_setting is nil' do - let(:error_tracking_setting) { build(:project_error_tracking_setting) } + let(:error_tracking_setting) { build(:project_error_tracking_setting, project: project) } let(:new_api_url) { new_api_host + 'api/0/projects/org/proj/' } before do diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index a91519a710f..3c67c15f10a 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -8,6 +8,16 @@ RSpec.describe EventCreateService do let_it_be(:user, reload: true) { create :user } let_it_be(:project) { create(:project) } + shared_examples 'it records the event in the event counter' do + specify do + tracking_params = { event_action: event_action, date_from: Date.yesterday, date_to: Date.today } + + expect { subject } + .to change { Gitlab::UsageDataCounters::TrackUniqueEvents.count_unique_events(tracking_params) } + .by(1) + end + end + describe 'Issues' do describe '#open_issue' do let(:issue) { create(:issue) } @@ -40,34 +50,52 @@ RSpec.describe EventCreateService do end end - describe 'Merge Requests' do + describe 'Merge Requests', :clean_gitlab_redis_shared_state do describe '#open_mr' do + subject(:open_mr) { service.open_mr(merge_request, merge_request.author) } + let(:merge_request) { create(:merge_request) } - it { expect(service.open_mr(merge_request, merge_request.author)).to be_truthy } + it { expect(open_mr).to be_truthy } it "creates new event" do - expect { service.open_mr(merge_request, merge_request.author) }.to change { Event.count } + expect { open_mr }.to change { Event.count } + end + + it_behaves_like "it records the event in the event counter" do + let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION } end end describe '#close_mr' do + subject(:close_mr) { service.close_mr(merge_request, merge_request.author) } + let(:merge_request) { create(:merge_request) } - it { expect(service.close_mr(merge_request, merge_request.author)).to be_truthy } + it { expect(close_mr).to be_truthy } it "creates new event" do - expect { service.close_mr(merge_request, merge_request.author) }.to change { Event.count } + expect { close_mr }.to change { Event.count } + end + + it_behaves_like "it records the event in the event counter" do + let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION } end end describe '#merge_mr' do + subject(:merge_mr) { service.merge_mr(merge_request, merge_request.author) } + let(:merge_request) { create(:merge_request) } - it { expect(service.merge_mr(merge_request, merge_request.author)).to be_truthy } + it { expect(merge_mr).to be_truthy } it "creates new event" do - expect { service.merge_mr(merge_request, merge_request.author) }.to change { Event.count } + expect { merge_mr }.to change { Event.count } + end + + it_behaves_like "it records the event in the event counter" do + let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION } end end @@ -180,6 +208,8 @@ RSpec.describe EventCreateService do where(:action) { Event::WIKI_ACTIONS.map { |action| [action] } } with_them do + subject { create_event } + it 'creates the event' do expect(create_event).to have_attributes( wiki_page?: true, @@ -201,13 +231,8 @@ RSpec.describe EventCreateService do expect(duplicate).to eq(event) end - it 'records the event in the event counter' do - counter_class = Gitlab::UsageDataCounters::TrackUniqueActions - tracking_params = { event_action: counter_class::WIKI_ACTION, date_from: Date.yesterday, date_to: Date.today } - - expect { create_event } - .to change { counter_class.count_unique(tracking_params) } - .by(1) + it_behaves_like "it records the event in the event counter" do + let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::WIKI_ACTION } end end @@ -242,13 +267,8 @@ RSpec.describe EventCreateService do it_behaves_like 'service for creating a push event', PushEventPayloadService - it 'records the event in the event counter' do - counter_class = Gitlab::UsageDataCounters::TrackUniqueActions - tracking_params = { event_action: counter_class::PUSH_ACTION, date_from: Date.yesterday, date_to: Date.today } - - expect { subject } - .to change { counter_class.count_unique(tracking_params) } - .from(0).to(1) + it_behaves_like "it records the event in the event counter" do + let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::PUSH_ACTION } end end @@ -265,13 +285,8 @@ RSpec.describe EventCreateService do it_behaves_like 'service for creating a push event', BulkPushEventPayloadService - it 'records the event in the event counter' do - counter_class = Gitlab::UsageDataCounters::TrackUniqueActions - tracking_params = { event_action: counter_class::PUSH_ACTION, date_from: Date.yesterday, date_to: Date.today } - - expect { subject } - .to change { counter_class.count_unique(tracking_params) } - .from(0).to(1) + it_behaves_like "it records the event in the event counter" do + let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::PUSH_ACTION } end end @@ -299,7 +314,7 @@ RSpec.describe EventCreateService do let_it_be(:updated) { create_list(:design, 5) } let_it_be(:created) { create_list(:design, 3) } - let(:result) { service.save_designs(author, create: created, update: updated) } + subject(:result) { service.save_designs(author, create: created, update: updated) } specify { expect { result }.to change { Event.count }.by(8) } @@ -319,13 +334,8 @@ RSpec.describe EventCreateService do expect(events.map(&:design)).to match_array(updated) end - it 'records the event in the event counter' do - counter_class = Gitlab::UsageDataCounters::TrackUniqueActions - tracking_params = { event_action: counter_class::DESIGN_ACTION, date_from: Date.yesterday, date_to: Date.today } - - expect { result } - .to change { counter_class.count_unique(tracking_params) } - .from(0).to(1) + it_behaves_like "it records the event in the event counter" do + let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::DESIGN_ACTION } end end @@ -333,7 +343,7 @@ RSpec.describe EventCreateService do let_it_be(:designs) { create_list(:design, 5) } let_it_be(:author) { create(:user) } - let(:result) { service.destroy_designs(designs, author) } + subject(:result) { service.destroy_designs(designs, author) } specify { expect { result }.to change { Event.count }.by(5) } @@ -346,13 +356,37 @@ RSpec.describe EventCreateService do expect(events.map(&:design)).to match_array(designs) end - it 'records the event in the event counter' do - counter_class = Gitlab::UsageDataCounters::TrackUniqueActions - tracking_params = { event_action: counter_class::DESIGN_ACTION, date_from: Date.yesterday, date_to: Date.today } + it_behaves_like "it records the event in the event counter" do + let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::DESIGN_ACTION } + end + end + end + + describe '#leave_note' do + subject(:leave_note) { service.leave_note(note, author) } + + let(:note) { create(:note) } + let(:author) { create(:user) } + let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION } + + it { expect(leave_note).to be_truthy } + + it "creates new event" do + expect { leave_note }.to change { Event.count }.by(1) + end + + context 'when it is a diff note' do + it_behaves_like "it records the event in the event counter" do + let(:note) { create(:diff_note_on_merge_request) } + end + end + + context 'when it is not a diff note' do + it 'does not change the unique action counter' do + counter_class = Gitlab::UsageDataCounters::TrackUniqueEvents + tracking_params = { event_action: event_action, date_from: Date.yesterday, date_to: Date.today } - expect { result } - .to change { counter_class.count_unique(tracking_params) } - .from(0).to(1) + expect { subject }.not_to change { counter_class.count_unique_events(tracking_params) } end end end diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb index 7f22af8bfc6..db25bb766c9 100644 --- a/spec/services/git/branch_hooks_service_spec.rb +++ b/spec/services/git/branch_hooks_service_spec.rb @@ -348,7 +348,7 @@ RSpec.describe Git::BranchHooksService do context 'when the project is forked', :sidekiq_might_not_need_inline do let(:upstream_project) { project } - let(:forked_project) { fork_project(upstream_project, user, repository: true) } + let(:forked_project) { fork_project(upstream_project, user, repository: true, using_service: true) } let!(:forked_service) do described_class.new(forked_project, user, change: { oldrev: oldrev, newrev: newrev, ref: ref }) diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index 6ccf2d03e4a..5d73794c1ec 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -416,6 +416,7 @@ RSpec.describe Git::BranchPushService, services: true do before do # project.create_jira_service doesn't seem to invalidate the cache here project.has_external_issue_tracker = true + stub_jira_service_test jira_service_settings stub_jira_urls("JIRA-1") @@ -703,4 +704,68 @@ RSpec.describe Git::BranchPushService, services: true do service.execute service end + + context 'Jira Connect hooks' do + let_it_be(:project) { create(:project, :repository) } + let(:branch_to_sync) { nil } + let(:commits_to_sync) { [] } + let(:params) do + { change: { oldrev: oldrev, newrev: newrev, ref: ref } } + end + + subject do + described_class.new(project, user, params) + end + + shared_examples 'enqueues Jira sync worker' do + specify do + Sidekiq::Testing.fake! do + expect(JiraConnect::SyncBranchWorker).to receive(:perform_async) + .with(project.id, branch_to_sync, commits_to_sync) + .and_call_original + + expect { subject.execute }.to change(JiraConnect::SyncBranchWorker.jobs, :size).by(1) + end + end + end + + shared_examples 'does not enqueue Jira sync worker' do + specify do + Sidekiq::Testing.fake! do + expect { subject.execute }.not_to change(JiraConnect::SyncBranchWorker.jobs, :size) + end + end + end + + context 'with a Jira subscription' do + before do + create(:jira_connect_subscription, namespace: project.namespace) + end + + context 'branch name contains Jira issue key' do + let(:branch_to_sync) { 'branch-JIRA-123' } + let(:ref) { "refs/heads/#{branch_to_sync}" } + + it_behaves_like 'enqueues Jira sync worker' + end + + context 'commit message contains Jira issue key' do + let(:commits_to_sync) { [newrev] } + + before do + allow_any_instance_of(Commit).to receive(:safe_message).and_return('Commit with key JIRA-123') + end + + it_behaves_like 'enqueues Jira sync worker' + end + + context 'branch name and commit message does not contain Jira issue key' do + it_behaves_like 'does not enqueue Jira sync worker' + end + end + + context 'without a Jira subscription' do + it_behaves_like 'does not enqueue Jira sync worker' + end + end end diff --git a/spec/services/git/wiki_push_service_spec.rb b/spec/services/git/wiki_push_service_spec.rb index 7f709be8593..816f20f0bc3 100644 --- a/spec/services/git/wiki_push_service_spec.rb +++ b/spec/services/git/wiki_push_service_spec.rb @@ -6,12 +6,20 @@ RSpec.describe Git::WikiPushService, services: true do include RepoHelpers let_it_be(:key_id) { create(:key, user: current_user).shell_id } - let_it_be(:project) { create(:project, :wiki_repo) } - let_it_be(:current_user) { create(:user) } - let_it_be(:git_wiki) { project.wiki.wiki } - let_it_be(:repository) { git_wiki.repository } + let_it_be(:wiki) { create(:project_wiki) } + let_it_be(:current_user) { wiki.container.default_owner } + let_it_be(:git_wiki) { wiki.wiki } + let_it_be(:repository) { wiki.repository } describe '#execute' do + it 'executes model-specific callbacks' do + expect(wiki).to receive(:after_post_receive) + + create_service(current_sha).execute + end + end + + describe '#process_changes' do context 'the push contains more than the permitted number of changes' do def run_service process_changes { described_class::MAX_CHANGES.succ.times { write_new_page } } @@ -37,8 +45,8 @@ RSpec.describe Git::WikiPushService, services: true do let(:count) { Event::WIKI_ACTIONS.size } def run_service - wiki_page_a = create(:wiki_page, project: project) - wiki_page_b = create(:wiki_page, project: project) + wiki_page_a = create(:wiki_page, wiki: wiki) + wiki_page_b = create(:wiki_page, wiki: wiki) process_changes do write_new_page @@ -135,7 +143,7 @@ RSpec.describe Git::WikiPushService, services: true do end context 'when a page we already know about has been updated' do - let(:wiki_page) { create(:wiki_page, project: project) } + let(:wiki_page) { create(:wiki_page, wiki: wiki) } before do create(:wiki_page_meta, :for_wiki_page, wiki_page: wiki_page) @@ -165,7 +173,7 @@ RSpec.describe Git::WikiPushService, services: true do context 'when a page we do not know about has been updated' do def run_service - wiki_page = create(:wiki_page, project: project) + wiki_page = create(:wiki_page, wiki: wiki) process_changes { update_page(wiki_page.title) } end @@ -189,7 +197,7 @@ RSpec.describe Git::WikiPushService, services: true do context 'when a page we do not know about has been deleted' do def run_service - wiki_page = create(:wiki_page, project: project) + wiki_page = create(:wiki_page, wiki: wiki) process_changes { delete_page(wiki_page.page.path) } end @@ -254,9 +262,9 @@ RSpec.describe Git::WikiPushService, services: true do it_behaves_like 'a no-op push' - context 'but is enabled for a given project' do + context 'but is enabled for a given container' do before do - stub_feature_flags(wiki_events_on_git_push: project) + stub_feature_flags(wiki_events_on_git_push: wiki.container) end it 'creates events' do @@ -280,19 +288,19 @@ RSpec.describe Git::WikiPushService, services: true do def create_service(base, refs = ['refs/heads/master']) changes = post_received(base, refs).changes - described_class.new(project, current_user, changes: changes) + described_class.new(wiki, current_user, changes: changes) end def post_received(base, refs) change_str = refs.map { |ref| +"#{base} #{current_sha} #{ref}" }.join("\n") - post_received = ::Gitlab::GitPostReceive.new(project, key_id, change_str, {}) + post_received = ::Gitlab::GitPostReceive.new(wiki.container, key_id, change_str, {}) allow(post_received).to receive(:identify).with(key_id).and_return(current_user) post_received end def current_sha - repository.gitaly_ref_client.find_branch('master')&.dereferenced_target&.id || Gitlab::Git::BLANK_SHA + repository.commit('master')&.id || Gitlab::Git::BLANK_SHA end # It is important not to re-use the WikiPage services here, since they create @@ -312,7 +320,7 @@ RSpec.describe Git::WikiPushService, services: true do file_content: 'some stuff', branch_name: 'master' } - ::Wikis::CreateAttachmentService.new(container: project, current_user: project.owner, params: params).execute + ::Wikis::CreateAttachmentService.new(container: wiki.container, current_user: current_user, params: params).execute end def update_page(title) diff --git a/spec/services/ci/web_ide_config_service_spec.rb b/spec/services/ide/base_config_service_spec.rb index 437b468cec8..debdc6e5809 100644 --- a/spec/services/ci/web_ide_config_service_spec.rb +++ b/spec/services/ide/base_config_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::WebIdeConfigService do +RSpec.describe Ide::BaseConfigService do let_it_be(:project) { create(:project, :repository) } let_it_be(:user) { create(:user) } let(:sha) { 'sha' } @@ -47,44 +47,6 @@ RSpec.describe Ci::WebIdeConfigService do message: "Invalid configuration format") end end - - context 'content is valid, but terminal not defined' do - let(:config_content) { '{}' } - - it 'returns success' do - is_expected.to include( - status: :success, - terminal: nil) - end - end - - context 'content is valid, with enabled terminal' do - let(:config_content) { 'terminal: {}' } - - it 'returns success' do - is_expected.to include( - status: :success, - terminal: { - tag_list: [], - yaml_variables: [], - options: { script: ["sleep 60"] } - }) - end - end - - context 'content is valid, with custom terminal' do - let(:config_content) { 'terminal: { before_script: [ls] }' } - - it 'returns success' do - is_expected.to include( - status: :success, - terminal: { - tag_list: [], - yaml_variables: [], - options: { before_script: ["ls"], script: ["sleep 60"] } - }) - end - end end end end diff --git a/spec/services/ide/schemas_config_service_spec.rb b/spec/services/ide/schemas_config_service_spec.rb new file mode 100644 index 00000000000..19e5ca9e87d --- /dev/null +++ b/spec/services/ide/schemas_config_service_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ide::SchemasConfigService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let(:filename) { 'sample.yml' } + let(:schema_content) { double(body: '{"title":"Sample schema"}') } + + describe '#execute' do + before do + project.add_developer(user) + + allow(Gitlab::HTTP).to receive(:get).with(anything) do + schema_content + end + end + + subject { described_class.new(project, user, filename: filename).execute } + + context 'feature flag schema_linting is enabled', unless: Gitlab.ee? do + before do + stub_feature_flags(schema_linting: true) + end + + context 'when no predefined schema exists for the given filename' do + it 'returns an empty object' do + is_expected.to include( + status: :success, + schema: {}) + end + end + + context 'when a predefined schema exists for the given filename' do + let(:filename) { '.gitlab-ci.yml' } + + it 'uses predefined schema matches' do + expect(Gitlab::HTTP).to receive(:get).with('https://json.schemastore.org/gitlab-ci') + expect(subject[:schema]['title']).to eq "Sample schema" + end + end + end + + context 'feature flag schema_linting is disabled', unless: Gitlab.ee? do + it 'returns an empty object' do + is_expected.to include( + status: :success, + schema: {}) + end + end + end +end diff --git a/spec/services/ide/terminal_config_service_spec.rb b/spec/services/ide/terminal_config_service_spec.rb new file mode 100644 index 00000000000..d6c4f7a2a69 --- /dev/null +++ b/spec/services/ide/terminal_config_service_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ide::TerminalConfigService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let(:sha) { 'sha' } + + describe '#execute' do + subject { described_class.new(project, user, sha: sha).execute } + + before do + project.add_developer(user) + + allow(project.repository).to receive(:blob_data_at).with('sha', anything) do + config_content + end + end + + context 'content is not valid' do + let(:config_content) { 'invalid content' } + + it 'returns an error' do + is_expected.to include( + status: :error, + message: "Invalid configuration format") + end + end + + context 'terminal not defined' do + let(:config_content) { '{}' } + + it 'returns success' do + is_expected.to include( + status: :success, + terminal: nil) + end + end + + context 'terminal enabled' do + let(:config_content) { 'terminal: {}' } + + it 'returns success' do + is_expected.to include( + status: :success, + terminal: { + tag_list: [], + yaml_variables: [], + options: { script: ["sleep 60"] } + }) + end + end + + context 'custom terminal enabled' do + let(:config_content) { 'terminal: { before_script: [ls] }' } + + it 'returns success' do + is_expected.to include( + status: :success, + terminal: { + tag_list: [], + yaml_variables: [], + options: { before_script: ["ls"], script: ["sleep 60"] } + }) + end + end + end +end diff --git a/spec/services/incident_management/create_incident_label_service_spec.rb b/spec/services/incident_management/create_incident_label_service_spec.rb index 18a7c019497..4771dfc9e64 100644 --- a/spec/services/incident_management/create_incident_label_service_spec.rb +++ b/spec/services/incident_management/create_incident_label_service_spec.rb @@ -10,9 +10,10 @@ RSpec.describe IncidentManagement::CreateIncidentLabelService do subject(:execute) { service.execute } describe 'execute' do - let(:title) { described_class::LABEL_PROPERTIES[:title] } - let(:color) { described_class::LABEL_PROPERTIES[:color] } - let(:description) { described_class::LABEL_PROPERTIES[:description] } + let(:incident_label_attributes) { attributes_for(:label, :incident) } + let(:title) { incident_label_attributes[:title] } + let(:color) { incident_label_attributes[:color] } + let(:description) { incident_label_attributes[:description] } shared_examples 'existing label' do it 'returns the existing label' do diff --git a/spec/services/incident_management/incidents/create_service_spec.rb b/spec/services/incident_management/incidents/create_service_spec.rb index 404c428cd94..1330f3ae033 100644 --- a/spec/services/incident_management/incidents/create_service_spec.rb +++ b/spec/services/incident_management/incidents/create_service_spec.rb @@ -13,7 +13,7 @@ RSpec.describe IncidentManagement::Incidents::CreateService do context 'when incident has title and description' do let(:title) { 'Incident title' } let(:new_issue) { Issue.last! } - let(:label_title) { IncidentManagement::CreateIncidentLabelService::LABEL_PROPERTIES[:title] } + let(:label_title) { attributes_for(:label, :incident)[:title] } it 'responds with success' do expect(create_incident).to be_success @@ -23,14 +23,47 @@ RSpec.describe IncidentManagement::Incidents::CreateService do expect { create_incident }.to change(Issue, :count).by(1) end - it 'created issue has correct attributes' do + it 'created issue has correct attributes', :aggregate_failures do create_incident - aggregate_failures do - expect(new_issue.title).to eq(title) - expect(new_issue.description).to eq(description) - expect(new_issue.author).to eq(user) - expect(new_issue.issue_type).to eq('incident') - expect(new_issue.labels.map(&:title)).to eq([label_title]) + + expect(new_issue.title).to eq(title) + expect(new_issue.description).to eq(description) + expect(new_issue.author).to eq(user) + end + + it_behaves_like 'incident issue' do + before do + create_incident + end + + let(:issue) { new_issue } + end + + context 'with default severity' do + it 'sets the correct severity level to "unknown"' do + create_incident + expect(new_issue.severity).to eq(IssuableSeverity::DEFAULT) + end + end + + context 'with severity' do + using RSpec::Parameterized::TableSyntax + + subject(:create_incident) { described_class.new(project, user, title: title, description: description, severity: severity).execute } + + where(:severity, :incident_severity) do + 'critical' | 'critical' + 'high' | 'high' + 'medium' | 'medium' + 'low' | 'low' + 'unknown' | 'unknown' + end + + with_them do + it 'sets the correct severity level' do + create_incident + expect(new_issue.severity).to eq(incident_severity) + end end end diff --git a/spec/services/issuable/common_system_notes_service_spec.rb b/spec/services/issuable/common_system_notes_service_spec.rb index daf4f68208e..217550542bb 100644 --- a/spec/services/issuable/common_system_notes_service_spec.rb +++ b/spec/services/issuable/common_system_notes_service_spec.rb @@ -32,17 +32,6 @@ RSpec.describe Issuable::CommonSystemNotesService do end end - context 'when new milestone is assigned' do - before do - milestone = create(:milestone, project: project) - issuable.milestone_id = milestone.id - - stub_feature_flags(track_resource_milestone_change_events: false) - end - - it_behaves_like 'system note creation', {}, 'changed milestone' - end - context 'with merge requests Draft note' do context 'adding Draft note' do let(:issuable) { create(:merge_request, title: "merge request") } @@ -100,32 +89,10 @@ RSpec.describe Issuable::CommonSystemNotesService do expect(event.user_id).to eq user.id end - context 'when milestone change event tracking is disabled' do - before do - stub_feature_flags(track_resource_milestone_change_events: false) - - issuable.milestone = create(:milestone, project: project) - issuable.save - end - - it 'creates a system note for milestone set' do - expect { subject }.to change { issuable.notes.count }.from(0).to(1) - expect(issuable.notes.last.note).to match('changed milestone') - end - - it 'does not create a milestone change event' do - expect { subject }.not_to change { ResourceMilestoneEvent.count } - end - end - - context 'when milestone change event tracking is enabled' do + context 'when changing milestones' do let_it_be(:milestone) { create(:milestone, project: project) } let_it_be(:issuable) { create(:issue, project: project, milestone: milestone) } - before do - stub_feature_flags(track_resource_milestone_change_events: true) - end - it 'does not create a system note for milestone set' do expect { subject }.not_to change { issuable.notes.count } end diff --git a/spec/services/issue_links/create_service_spec.rb b/spec/services/issue_links/create_service_spec.rb new file mode 100644 index 00000000000..873890d25cf --- /dev/null +++ b/spec/services/issue_links/create_service_spec.rb @@ -0,0 +1,184 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IssueLinks::CreateService do + describe '#execute' do + let(:namespace) { create :namespace } + let(:project) { create :project, namespace: namespace } + let(:issue) { create :issue, project: project } + let(:user) { create :user } + let(:params) do + {} + end + + before do + project.add_developer(user) + end + + subject { described_class.new(issue, user, params).execute } + + context 'when the reference list is empty' do + let(:params) do + { issuable_references: [] } + end + + it 'returns error' do + is_expected.to eq(message: 'No Issue found for given params', status: :error, http_status: 404) + end + end + + context 'when Issue not found' do + let(:params) do + { issuable_references: ["##{non_existing_record_iid}"] } + end + + it 'returns error' do + is_expected.to eq(message: 'No Issue found for given params', status: :error, http_status: 404) + end + + it 'no relationship is created' do + expect { subject }.not_to change(IssueLink, :count) + end + end + + context 'when user has no permission to target project Issue' do + let(:target_issuable) { create :issue } + + let(:params) do + { issuable_references: [target_issuable.to_reference(project)] } + end + + it 'returns error' do + target_issuable.project.add_guest(user) + + is_expected.to eq(message: 'No Issue found for given params', status: :error, http_status: 404) + end + + it 'no relationship is created' do + expect { subject }.not_to change(IssueLink, :count) + end + end + + context 'source and target are the same issue' do + let(:params) do + { issuable_references: [issue.to_reference] } + end + + it 'does not create notes' do + expect(SystemNoteService).not_to receive(:relate_issue) + + subject + end + + it 'no relationship is created' do + expect { subject }.not_to change(IssueLink, :count) + end + end + + context 'when there is an issue to relate' do + let(:issue_a) { create :issue, project: project } + let(:another_project) { create :project, namespace: project.namespace } + let(:another_project_issue) { create :issue, project: another_project } + + let(:issue_a_ref) { issue_a.to_reference } + let(:another_project_issue_ref) { another_project_issue.to_reference(project) } + + let(:params) do + { issuable_references: [issue_a_ref, another_project_issue_ref] } + end + + before do + another_project.add_developer(user) + end + + it 'creates relationships' do + expect { subject }.to change(IssueLink, :count).from(0).to(2) + + expect(IssueLink.find_by!(target: issue_a)).to have_attributes(source: issue, link_type: 'relates_to') + expect(IssueLink.find_by!(target: another_project_issue)).to have_attributes(source: issue, link_type: 'relates_to') + end + + it 'returns success status' do + is_expected.to eq(status: :success) + end + + it 'creates notes' do + # First two-way relation notes + expect(SystemNoteService).to receive(:relate_issue) + .with(issue, issue_a, user) + expect(SystemNoteService).to receive(:relate_issue) + .with(issue_a, issue, user) + + # Second two-way relation notes + expect(SystemNoteService).to receive(:relate_issue) + .with(issue, another_project_issue, user) + expect(SystemNoteService).to receive(:relate_issue) + .with(another_project_issue, issue, user) + + subject + end + + context 'issue is an incident' do + let(:issue) { create(:incident, project: project) } + + it_behaves_like 'an incident management tracked event', :incident_management_incident_relate do + let(:current_user) { user } + end + end + end + + context 'when reference of any already related issue is present' do + let(:issue_a) { create :issue, project: project } + let(:issue_b) { create :issue, project: project } + let(:issue_c) { create :issue, project: project } + + before do + create :issue_link, source: issue, target: issue_b, link_type: IssueLink::TYPE_RELATES_TO + create :issue_link, source: issue, target: issue_c, link_type: IssueLink::TYPE_RELATES_TO + end + + let(:params) do + { + issuable_references: [ + issue_a.to_reference, + issue_b.to_reference, + issue_c.to_reference + ], + link_type: IssueLink::TYPE_RELATES_TO + } + end + + it 'creates notes only for new relations' do + expect(SystemNoteService).to receive(:relate_issue).with(issue, issue_a, anything) + expect(SystemNoteService).to receive(:relate_issue).with(issue_a, issue, anything) + expect(SystemNoteService).not_to receive(:relate_issue).with(issue, issue_b, anything) + expect(SystemNoteService).not_to receive(:relate_issue).with(issue_b, issue, anything) + expect(SystemNoteService).not_to receive(:relate_issue).with(issue, issue_c, anything) + expect(SystemNoteService).not_to receive(:relate_issue).with(issue_c, issue, anything) + + subject + end + end + + context 'when there are invalid references' do + let(:issue_a) { create :issue, project: project } + + let(:params) do + { issuable_references: [issue.to_reference, issue_a.to_reference] } + end + + it 'creates links only for valid references' do + expect { subject }.to change { IssueLink.count }.by(1) + end + + it 'returns error status' do + expect(subject).to eq( + status: :error, + http_status: 422, + message: "#{issue.to_reference} cannot be added: cannot be related to itself" + ) + end + end + end +end diff --git a/spec/services/issue_links/destroy_service_spec.rb b/spec/services/issue_links/destroy_service_spec.rb new file mode 100644 index 00000000000..f441629f892 --- /dev/null +++ b/spec/services/issue_links/destroy_service_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IssueLinks::DestroyService do + describe '#execute' do + let(:project) { create(:project_empty_repo) } + let(:user) { create(:user) } + + subject { described_class.new(issue_link, user).execute } + + context 'when successfully removes an issue link' do + let(:issue_a) { create(:issue, project: project) } + let(:issue_b) { create(:issue, project: project) } + + let!(:issue_link) { create(:issue_link, source: issue_a, target: issue_b) } + + before do + project.add_reporter(user) + end + + it 'removes related issue' do + expect { subject }.to change(IssueLink, :count).from(1).to(0) + end + + it 'creates notes' do + # Two-way notes creation + expect(SystemNoteService).to receive(:unrelate_issue) + .with(issue_link.source, issue_link.target, user) + expect(SystemNoteService).to receive(:unrelate_issue) + .with(issue_link.target, issue_link.source, user) + + subject + end + + it 'returns success message' do + is_expected.to eq(message: 'Relation was removed', status: :success) + end + + context 'target is an incident' do + let(:issue_b) { create(:incident, project: project) } + + it_behaves_like 'an incident management tracked event', :incident_management_incident_unrelate do + let(:current_user) { user } + end + end + end + + context 'when failing to remove an issue link' do + let(:unauthorized_project) { create(:project) } + let(:issue_a) { create(:issue, project: project) } + let(:issue_b) { create(:issue, project: unauthorized_project) } + + let!(:issue_link) { create(:issue_link, source: issue_a, target: issue_b) } + + it 'does not remove relation' do + expect { subject }.not_to change(IssueLink, :count).from(1) + end + + it 'does not create notes' do + expect(SystemNoteService).not_to receive(:unrelate_issue) + end + + it 'returns error message' do + is_expected.to eq(message: 'No Issue Link found', status: :error, http_status: 404) + end + end + end +end diff --git a/spec/services/issue_links/list_service_spec.rb b/spec/services/issue_links/list_service_spec.rb new file mode 100644 index 00000000000..7a3ba845c7c --- /dev/null +++ b/spec/services/issue_links/list_service_spec.rb @@ -0,0 +1,194 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IssueLinks::ListService do + let(:user) { create :user } + let(:project) { create(:project_empty_repo, :private) } + let(:issue) { create :issue, project: project } + let(:user_role) { :developer } + + before do + project.add_role(user, user_role) + end + + describe '#execute' do + subject { described_class.new(issue, user).execute } + + context 'user can see all issues' do + let(:issue_b) { create :issue, project: project } + let(:issue_c) { create :issue, project: project } + let(:issue_d) { create :issue, project: project } + + let!(:issue_link_c) do + create(:issue_link, source: issue_d, + target: issue) + end + + let!(:issue_link_b) do + create(:issue_link, source: issue, + target: issue_c) + end + + let!(:issue_link_a) do + create(:issue_link, source: issue, + target: issue_b) + end + + it 'ensures no N+1 queries are made' do + control_count = ActiveRecord::QueryRecorder.new { subject }.count + + project = create :project, :public + milestone = create :milestone, project: project + issue_x = create :issue, project: project, milestone: milestone + issue_y = create :issue, project: project, assignees: [user] + issue_z = create :issue, project: project + create :issue_link, source: issue_x, target: issue_y + create :issue_link, source: issue_x, target: issue_z + create :issue_link, source: issue_y, target: issue_z + + expect { subject }.not_to exceed_query_limit(control_count) + end + + it 'returns related issues JSON' do + expect(subject.size).to eq(3) + + expect(subject).to include(include(id: issue_b.id, + title: issue_b.title, + state: issue_b.state, + reference: issue_b.to_reference(project), + path: "/#{project.full_path}/-/issues/#{issue_b.iid}", + relation_path: "/#{project.full_path}/-/issues/#{issue.iid}/links/#{issue_link_a.id}")) + + expect(subject).to include(include(id: issue_c.id, + title: issue_c.title, + state: issue_c.state, + reference: issue_c.to_reference(project), + path: "/#{project.full_path}/-/issues/#{issue_c.iid}", + relation_path: "/#{project.full_path}/-/issues/#{issue.iid}/links/#{issue_link_b.id}")) + + expect(subject).to include(include(id: issue_d.id, + title: issue_d.title, + state: issue_d.state, + reference: issue_d.to_reference(project), + path: "/#{project.full_path}/-/issues/#{issue_d.iid}", + relation_path: "/#{project.full_path}/-/issues/#{issue.iid}/links/#{issue_link_c.id}")) + end + end + + context 'referencing a public project issue' do + let(:public_project) { create :project, :public } + let(:issue_b) { create :issue, project: public_project } + + let!(:issue_link) do + create(:issue_link, source: issue, target: issue_b) + end + + it 'presents issue' do + expect(subject.size).to eq(1) + end + end + + context 'referencing issue with removed relationships' do + context 'when referenced a deleted issue' do + let(:issue_b) { create :issue, project: project } + let!(:issue_link) do + create(:issue_link, source: issue, target: issue_b) + end + + it 'ignores issue' do + issue_b.destroy! + + is_expected.to eq([]) + end + end + + context 'when referenced an issue with deleted project' do + let(:issue_b) { create :issue, project: project } + let!(:issue_link) do + create(:issue_link, source: issue, target: issue_b) + end + + it 'ignores issue' do + project.destroy! + + is_expected.to eq([]) + end + end + + context 'when referenced an issue with deleted namespace' do + let(:issue_b) { create :issue, project: project } + let!(:issue_link) do + create(:issue_link, source: issue, target: issue_b) + end + + it 'ignores issue' do + project.namespace.destroy! + + is_expected.to eq([]) + end + end + end + + context 'user cannot see relations' do + context 'when user cannot see the referenced issue' do + let!(:issue_link) do + create(:issue_link, source: issue) + end + + it 'returns an empty list' do + is_expected.to eq([]) + end + end + + context 'when user cannot see the issue that referenced' do + let!(:issue_link) do + create(:issue_link, target: issue) + end + + it 'returns an empty list' do + is_expected.to eq([]) + end + end + end + + context 'remove relations' do + let!(:issue_link) do + create(:issue_link, source: issue, target: referenced_issue) + end + + context 'user can admin related issues just on target project' do + let(:user_role) { :guest } + let(:target_project) { create :project } + let(:referenced_issue) { create :issue, project: target_project } + + it 'returns no destroy relation path' do + target_project.add_developer(user) + + expect(subject.first[:relation_path]).to be_nil + end + end + + context 'user can admin related issues just on source project' do + let(:user_role) { :developer } + let(:target_project) { create :project } + let(:referenced_issue) { create :issue, project: target_project } + + it 'returns no destroy relation path' do + target_project.add_guest(user) + + expect(subject.first[:relation_path]).to be_nil + end + end + + context 'when user can admin related issues on both projects' do + let(:referenced_issue) { create :issue, project: project } + + it 'returns related issue destroy relation path' do + expect(subject.first[:relation_path]) + .to eq("/#{project.full_path}/-/issues/#{issue.iid}/links/#{issue_link.id}") + end + end + end + end +end diff --git a/spec/services/issue_rebalancing_service_spec.rb b/spec/services/issue_rebalancing_service_spec.rb new file mode 100644 index 00000000000..94f594c8083 --- /dev/null +++ b/spec/services/issue_rebalancing_service_spec.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IssueRebalancingService do + let_it_be(:project) { create(:project) } + let_it_be(:user) { project.creator } + let_it_be(:start) { RelativePositioning::START_POSITION } + let_it_be(:max_pos) { RelativePositioning::MAX_POSITION } + let_it_be(:min_pos) { RelativePositioning::MIN_POSITION } + let_it_be(:clump_size) { 300 } + + let_it_be(:unclumped) do + (0..clump_size).to_a.map do |i| + create(:issue, project: project, author: user, relative_position: start + (1024 * i)) + end + end + + let_it_be(:end_clump) do + (0..clump_size).to_a.map do |i| + create(:issue, project: project, author: user, relative_position: max_pos - i) + end + end + + let_it_be(:start_clump) do + (0..clump_size).to_a.map do |i| + create(:issue, project: project, author: user, relative_position: min_pos + i) + end + end + + def issues_in_position_order + project.reload.issues.reorder(relative_position: :asc).to_a + end + + it 'rebalances a set of issues with clumps at the end and start' do + all_issues = start_clump + unclumped + end_clump.reverse + service = described_class.new(project.issues.first) + + expect { service.execute }.not_to change { issues_in_position_order.map(&:id) } + + all_issues.each(&:reset) + + gaps = all_issues.take(all_issues.count - 1).zip(all_issues.drop(1)).map do |a, b| + b.relative_position - a.relative_position + end + + expect(gaps).to all(be > RelativePositioning::MIN_GAP) + expect(all_issues.first.relative_position).to be > (RelativePositioning::MIN_POSITION * 0.9999) + expect(all_issues.last.relative_position).to be < (RelativePositioning::MAX_POSITION * 0.9999) + end + + it 'is idempotent' do + service = described_class.new(project.issues.first) + + expect do + service.execute + service.execute + end.not_to change { issues_in_position_order.map(&:id) } + end + + it 'does nothing if the feature flag is disabled' do + stub_feature_flags(rebalance_issues: false) + issue = project.issues.first + issue.project + issue.project.group + old_pos = issue.relative_position + + service = described_class.new(issue) + + expect { service.execute }.not_to exceed_query_limit(0) + expect(old_pos).to eq(issue.reload.relative_position) + end + + it 'acts if the flag is enabled for the project' do + issue = create(:issue, project: project, author: user, relative_position: max_pos) + stub_feature_flags(rebalance_issues: issue.project) + + service = described_class.new(issue) + + expect { service.execute }.to change { issue.reload.relative_position } + end + + it 'acts if the flag is enabled for the group' do + issue = create(:issue, project: project, author: user, relative_position: max_pos) + project.update!(group: create(:group)) + stub_feature_flags(rebalance_issues: issue.project.group) + + service = described_class.new(issue) + + expect { service.execute }.to change { issue.reload.relative_position } + end + + it 'aborts if there are too many issues' do + issue = project.issues.first + base = double(count: 10_001) + + allow(Issue).to receive(:relative_positioning_query_base).with(issue).and_return(base) + + expect { described_class.new(issue).execute }.to raise_error(described_class::TooManyIssues) + end +end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 7ca7d3be99c..4db6e5cac12 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -67,6 +67,15 @@ RSpec.describe Issues::CloseService do service.execute(issue) end + + context 'issue is incident type' do + let(:issue) { create(:incident, project: project) } + let(:current_user) { user } + + subject { service.execute(issue) } + + it_behaves_like 'an incident management tracked event', :incident_management_incident_closed + end end describe '#close_issue' do @@ -288,7 +297,7 @@ RSpec.describe Issues::CloseService do end it 'deletes milestone issue counters cache' do - issue.update(milestone: create(:milestone, project: project)) + issue.update!(milestone: create(:milestone, project: project)) expect_next_instance_of(Milestones::ClosedIssuesCountService, issue.milestone) do |service| expect(service).to receive(:delete_cache).and_call_original diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index fdf2326b75e..e09a7faece5 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -3,18 +3,18 @@ require 'spec_helper' RSpec.describe Issues::CreateService do - let(:project) { create(:project) } - let(:user) { create(:user) } + let_it_be_with_reload(:project) { create(:project) } + let_it_be(:user) { create(:user) } describe '#execute' do + let_it_be(:assignee) { create(:user) } + let_it_be(:milestone) { create(:milestone, project: project) } let(:issue) { described_class.new(project, user, opts).execute } - let(:assignee) { create(:user) } - let(:milestone) { create(:milestone, project: project) } context 'when params are valid' do - let(:labels) { create_pair(:label, project: project) } + let_it_be(:labels) { create_pair(:label, project: project) } - before do + before_all do project.add_maintainer(user) project.add_maintainer(assignee) end @@ -29,6 +29,8 @@ RSpec.describe Issues::CreateService do end it 'creates the issue with the given params' do + expect(Issuable::CommonSystemNotesService).to receive_message_chain(:new, :execute) + expect(issue).to be_persisted expect(issue.title).to eq('Awesome issue') expect(issue.assignees).to eq [assignee] @@ -37,14 +39,55 @@ RSpec.describe Issues::CreateService do expect(issue.due_date).to eq Date.tomorrow end + context 'when skip_system_notes is true' do + let(:issue) { described_class.new(project, user, opts).execute(skip_system_notes: true) } + + it 'does not call Issuable::CommonSystemNotesService' do + expect(Issuable::CommonSystemNotesService).not_to receive(:new) + + issue + end + end + + it_behaves_like 'not an incident issue' + + context 'issue is incident type' do + before do + opts.merge!(issue_type: 'incident') + end + + let(:current_user) { user } + let(:incident_label_attributes) { attributes_for(:label, :incident) } + + subject { issue } + + it_behaves_like 'incident issue' + it_behaves_like 'an incident management tracked event', :incident_management_incident_created + + it 'does create an incident label' do + expect { subject } + .to change { Label.where(incident_label_attributes).count }.by(1) + end + + context 'when invalid' do + before do + opts.merge!(title: '') + end + + it 'does not create an incident label prematurely' do + expect { subject }.not_to change(Label, :count) + end + end + end + it 'refreshes the number of open issues', :use_clean_rails_memory_store_caching do expect { issue }.to change { project.open_issues_count }.from(0).to(1) end context 'when current user cannot admin issues in the project' do - let(:guest) { create(:user) } + let_it_be(:guest) { create(:user) } - before do + before_all do project.add_guest(guest) end @@ -75,6 +118,12 @@ RSpec.describe Issues::CreateService do expect(Todo.where(attributes).count).to eq 1 end + it 'moves the issue to the end, in an asynchronous worker' do + expect(IssuePlacementWorker).to receive(:perform_async).with(be_nil, Integer) + + described_class.new(project, user, opts).execute + end + context 'when label belongs to project group' do let(:group) { create(:group) } let(:group_labels) { create_pair(:group_label, group: group) } @@ -88,7 +137,7 @@ RSpec.describe Issues::CreateService do end before do - project.update(group: group) + project.update!(group: group) end it 'assigns group labels' do @@ -233,7 +282,7 @@ RSpec.describe Issues::CreateService do context 'issue create service' do context 'assignees' do - before do + before_all do project.add_maintainer(user) end @@ -264,7 +313,7 @@ RSpec.describe Issues::CreateService do context "when issuable feature is private" do before do - project.project_feature.update(issues_access_level: ProjectFeature::PRIVATE, + project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE, merge_requests_access_level: ProjectFeature::PRIVATE) end @@ -272,7 +321,7 @@ RSpec.describe Issues::CreateService do levels.each do |level| it "removes not authorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do - project.update(visibility_level: level) + project.update!(visibility_level: level) opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } issue = described_class.new(project, user, opts).execute @@ -299,7 +348,7 @@ RSpec.describe Issues::CreateService do } end - before do + before_all do project.add_maintainer(user) project.add_maintainer(assignee) end @@ -313,11 +362,11 @@ RSpec.describe Issues::CreateService do end context 'resolving discussions' do - let(:discussion) { create(:diff_note_on_merge_request).to_discussion } - let(:merge_request) { discussion.noteable } - let(:project) { merge_request.source_project } + let_it_be(:discussion) { create(:diff_note_on_merge_request).to_discussion } + let_it_be(:merge_request) { discussion.noteable } + let_it_be(:project) { merge_request.source_project } - before do + before_all do project.add_maintainer(user) end diff --git a/spec/services/issues/duplicate_service_spec.rb b/spec/services/issues/duplicate_service_spec.rb index 78e030e6ac7..0b5bc3f32ef 100644 --- a/spec/services/issues/duplicate_service_spec.rb +++ b/spec/services/issues/duplicate_service_spec.rb @@ -83,6 +83,17 @@ RSpec.describe Issues::DuplicateService do expect(duplicate_issue.reload.duplicated_to).to eq(canonical_issue) end + + it 'relates the duplicate issues' do + canonical_project.add_reporter(user) + duplicate_project.add_reporter(user) + + subject.execute(duplicate_issue, canonical_issue) + + issue_link = IssueLink.last + expect(issue_link.source).to eq(duplicate_issue) + expect(issue_link.target).to eq(canonical_issue) + end end end end diff --git a/spec/services/issues/export_csv_service_spec.rb b/spec/services/issues/export_csv_service_spec.rb index 76381fe525b..8072b7a478e 100644 --- a/spec/services/issues/export_csv_service_spec.rb +++ b/spec/services/issues/export_csv_service_spec.rb @@ -38,8 +38,8 @@ RSpec.describe Issues::ExportCsvService do before do # Creating a timelog touches the updated_at timestamp of issue, # so create these first. - issue.timelogs.create(time_spent: 360, user: user) - issue.timelogs.create(time_spent: 200, user: user) + issue.timelogs.create!(time_spent: 360, user: user) + issue.timelogs.create!(time_spent: 200, user: user) issue.update!(milestone: milestone, assignees: [user], description: 'Issue with details', diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index 5f944d1213b..c2989dc86cf 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -101,6 +101,41 @@ RSpec.describe Issues::MoveService do end end + context 'issue with milestone' do + let(:milestone) { create(:milestone, group: sub_group_1) } + let(:new_project) { create(:project, namespace: sub_group_1) } + + let(:old_issue) do + create(:issue, title: title, description: description, project: old_project, author: author, milestone: milestone) + end + + before do + create(:resource_milestone_event, issue: old_issue, milestone: milestone, action: :add) + end + + it 'does not create extra milestone events' do + new_issue = move_service.execute(old_issue, new_project) + + expect(new_issue.resource_milestone_events.count).to eq(old_issue.resource_milestone_events.count) + end + end + + context 'issue with due date' do + let(:old_issue) do + create(:issue, title: title, description: description, project: old_project, author: author, due_date: '2020-01-10') + end + + before do + SystemNoteService.change_due_date(old_issue, old_project, author, old_issue.due_date) + end + + it 'does not create extra system notes' do + new_issue = move_service.execute(old_issue, new_project) + + expect(new_issue.notes.count).to eq(old_issue.notes.count) + end + end + context 'issue with assignee' do let_it_be(:assignee) { create(:user) } @@ -223,6 +258,45 @@ RSpec.describe Issues::MoveService do end end + describe '#rewrite_related_issues' do + include_context 'user can move issue' + + let(:admin) { create(:admin) } + let(:authorized_project) { create(:project) } + let(:authorized_project2) { create(:project) } + let(:unauthorized_project) { create(:project) } + + let(:authorized_issue_b) { create(:issue, project: authorized_project) } + let(:authorized_issue_c) { create(:issue, project: authorized_project2) } + let(:authorized_issue_d) { create(:issue, project: authorized_project2) } + let(:unauthorized_issue) { create(:issue, project: unauthorized_project) } + + let!(:issue_link_a) { create(:issue_link, source: old_issue, target: authorized_issue_b) } + let!(:issue_link_b) { create(:issue_link, source: old_issue, target: unauthorized_issue) } + let!(:issue_link_c) { create(:issue_link, source: old_issue, target: authorized_issue_c) } + let!(:issue_link_d) { create(:issue_link, source: authorized_issue_d, target: old_issue) } + + before do + authorized_project.add_developer(user) + authorized_project2.add_developer(user) + end + + context 'multiple related issues' do + it 'moves all related issues and retains permissions' do + new_issue = move_service.execute(old_issue, new_project) + + expect(new_issue.related_issues(admin)) + .to match_array([authorized_issue_b, authorized_issue_c, authorized_issue_d, unauthorized_issue]) + + expect(new_issue.related_issues(user)) + .to match_array([authorized_issue_b, authorized_issue_c, authorized_issue_d]) + + expect(authorized_issue_d.related_issues(user)) + .to match_array([new_issue]) + end + end + end + context 'updating sent notifications' do let!(:old_issue_notification_1) { create(:sent_notification, project: old_issue.project, noteable: old_issue) } let!(:old_issue_notification_2) { create(:sent_notification, project: old_issue.project, noteable: old_issue) } diff --git a/spec/services/issues/related_branches_service_spec.rb b/spec/services/issues/related_branches_service_spec.rb index d79132d98db..1780023803a 100644 --- a/spec/services/issues/related_branches_service_spec.rb +++ b/spec/services/issues/related_branches_service_spec.rb @@ -57,7 +57,7 @@ RSpec.describe Issues::RelatedBranchesService do unreadable_branch_name => unreadable_pipeline }.each do |name, pipeline| allow(repo).to receive(:find_branch).with(name).and_return(make_branch) - allow(project).to receive(:pipeline_for).with(name, sha).and_return(pipeline) + allow(project).to receive(:latest_pipeline).with(name, sha).and_return(pipeline) end allow(repo).to receive(:find_branch).with(missing_branch).and_return(nil) diff --git a/spec/services/issues/reopen_service_spec.rb b/spec/services/issues/reopen_service_spec.rb index f7416203259..ffe74cca9cf 100644 --- a/spec/services/issues/reopen_service_spec.rb +++ b/spec/services/issues/reopen_service_spec.rb @@ -44,7 +44,7 @@ RSpec.describe Issues::ReopenService do end it 'deletes milestone issue counters cache' do - issue.update(milestone: create(:milestone, project: project)) + issue.update!(milestone: create(:milestone, project: project)) expect_next_instance_of(Milestones::ClosedIssuesCountService, issue.milestone) do |service| expect(service).to receive(:delete_cache).and_call_original @@ -53,6 +53,15 @@ RSpec.describe Issues::ReopenService do described_class.new(project, user).execute(issue) end + context 'issue is incident type' do + let(:issue) { create(:incident, :closed, project: project) } + let(:current_user) { user } + + subject { described_class.new(project, user).execute(issue) } + + it_behaves_like 'an incident management tracked event', :incident_management_incident_reopened + end + context 'when issue is not confidential' do it 'executes issue hooks' do expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) diff --git a/spec/services/issues/reorder_service_spec.rb b/spec/services/issues/reorder_service_spec.rb index b6ad488a48c..78b937a1caf 100644 --- a/spec/services/issues/reorder_service_spec.rb +++ b/spec/services/issues/reorder_service_spec.rb @@ -3,9 +3,9 @@ require 'spec_helper' RSpec.describe Issues::ReorderService do - let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project) } + let_it_be(:user) { create_default(:user) } let_it_be(:group) { create(:group) } + let_it_be(:project, reload: true) { create(:project, namespace: group) } shared_examples 'issues reorder service' do context 'when reordering issues' do @@ -14,7 +14,7 @@ RSpec.describe Issues::ReorderService do end it 'returns false with both invalid params' do - params = { move_after_id: nil, move_before_id: 1 } + params = { move_after_id: nil, move_before_id: non_existing_record_id } expect(service(params).execute(issue1)).to be_falsey end @@ -27,27 +27,39 @@ RSpec.describe Issues::ReorderService do expect(issue1.relative_position) .to be_between(issue2.relative_position, issue3.relative_position) end + + it 'sorts issues if only given one neighbour, on the left' do + params = { move_before_id: issue3.id } + + service(params).execute(issue1) + + expect(issue1.relative_position).to be > issue3.relative_position + end + + it 'sorts issues if only given one neighbour, on the right' do + params = { move_after_id: issue1.id } + + service(params).execute(issue3) + + expect(issue3.relative_position).to be < issue1.relative_position + end end end describe '#execute' do - let(:issue1) { create(:issue, project: project, relative_position: 10) } - let(:issue2) { create(:issue, project: project, relative_position: 20) } - let(:issue3) { create(:issue, project: project, relative_position: 30) } + let_it_be(:issue1, reload: true) { create(:issue, project: project, relative_position: 10) } + let_it_be(:issue2) { create(:issue, project: project, relative_position: 20) } + let_it_be(:issue3, reload: true) { create(:issue, project: project, relative_position: 30) } context 'when ordering issues in a project' do - let(:parent) { project } - before do - parent.add_developer(user) + project.add_developer(user) end it_behaves_like 'issues reorder service' end context 'when ordering issues in a group' do - let(:project) { create(:project, namespace: group) } - before do group.add_developer(user) end diff --git a/spec/services/issues/resolve_discussions_spec.rb b/spec/services/issues/resolve_discussions_spec.rb index a541d92feb2..9fbc9cbcca6 100644 --- a/spec/services/issues/resolve_discussions_spec.rb +++ b/spec/services/issues/resolve_discussions_spec.rb @@ -79,7 +79,7 @@ RSpec.describe Issues::ResolveDiscussions do noteable: merge_request, project: merge_request.target_project, line_number: 15 - )]) + )]) service = DummyService.new( project, user, diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 42452e95f6b..f0092c35fda 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -52,7 +52,8 @@ RSpec.describe Issues::UpdateService, :mailer do state_event: 'close', label_ids: [label.id], due_date: Date.tomorrow, - discussion_locked: true + discussion_locked: true, + severity: 'low' } end @@ -71,6 +72,51 @@ RSpec.describe Issues::UpdateService, :mailer do expect(issue.discussion_locked).to be_truthy end + context 'when issue type is not incident' do + it 'returns default severity' do + update_issue(opts) + + expect(issue.severity).to eq(IssuableSeverity::DEFAULT) + end + + it_behaves_like 'not an incident issue' do + before do + update_issue(opts) + end + end + end + + context 'when issue type is incident' do + let(:issue) { create(:incident, project: project) } + + it 'changes updates the severity' do + update_issue(opts) + + expect(issue.severity).to eq('low') + end + + it_behaves_like 'incident issue' do + before do + update_issue(opts) + end + end + + context 'with existing incident label' do + let_it_be(:incident_label) { create(:label, :incident, project: project) } + + before do + opts.delete(:label_ids) # don't override but retain existing labels + issue.labels << incident_label + end + + it_behaves_like 'incident issue' do + before do + update_issue(opts) + end + end + end + end + it 'refreshes the number of open issues when the issue is made confidential', :use_clean_rails_memory_store_caching do issue # make sure the issue is created first so our counts are correct. @@ -93,6 +139,40 @@ RSpec.describe Issues::UpdateService, :mailer do update_issue(confidential: false) end + context 'issue in incident type' do + let(:current_user) { user } + let(:incident_label_attributes) { attributes_for(:label, :incident) } + + before do + opts.merge!(issue_type: 'incident', confidential: true) + end + + subject { update_issue(opts) } + + it_behaves_like 'an incident management tracked event', :incident_management_incident_change_confidential + + it_behaves_like 'incident issue' do + before do + subject + end + end + + it 'does create an incident label' do + expect { subject } + .to change { Label.where(incident_label_attributes).count }.by(1) + end + + context 'when invalid' do + before do + opts.merge!(title: '') + end + + it 'does not create an incident label prematurely' do + expect { subject }.not_to change(Label, :count) + end + end + end + it 'updates open issue counter for assignees when issue is reassigned' do update_issue(assignee_ids: [user2.id]) @@ -106,7 +186,7 @@ RSpec.describe Issues::UpdateService, :mailer do [issue, issue1, issue2].each do |issue| issue.move_to_end - issue.save + issue.save! end opts[:move_between_ids] = [issue1.id, issue2.id] @@ -116,6 +196,66 @@ RSpec.describe Issues::UpdateService, :mailer do expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position) end + it 'does not rebalance even if needed if the flag is disabled' do + stub_feature_flags(rebalance_issues: false) + + range = described_class::NO_REBALANCING_NEEDED + issue1 = create(:issue, project: project, relative_position: range.first - 100) + issue2 = create(:issue, project: project, relative_position: range.first) + issue.update!(relative_position: RelativePositioning::START_POSITION) + + opts[:move_between_ids] = [issue1.id, issue2.id] + + expect(IssueRebalancingWorker).not_to receive(:perform_async) + + update_issue(opts) + expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position) + end + + it 'rebalances if needed if the flag is enabled for the project' do + stub_feature_flags(rebalance_issues: project) + + range = described_class::NO_REBALANCING_NEEDED + issue1 = create(:issue, project: project, relative_position: range.first - 100) + issue2 = create(:issue, project: project, relative_position: range.first) + issue.update!(relative_position: RelativePositioning::START_POSITION) + + opts[:move_between_ids] = [issue1.id, issue2.id] + + expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id) + + update_issue(opts) + expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position) + end + + it 'rebalances if needed on the left' do + range = described_class::NO_REBALANCING_NEEDED + issue1 = create(:issue, project: project, relative_position: range.first - 100) + issue2 = create(:issue, project: project, relative_position: range.first) + issue.update!(relative_position: RelativePositioning::START_POSITION) + + opts[:move_between_ids] = [issue1.id, issue2.id] + + expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id) + + update_issue(opts) + expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position) + end + + it 'rebalances if needed on the right' do + range = described_class::NO_REBALANCING_NEEDED + issue1 = create(:issue, project: project, relative_position: range.last) + issue2 = create(:issue, project: project, relative_position: range.last + 100) + issue.update!(relative_position: RelativePositioning::START_POSITION) + + opts[:move_between_ids] = [issue1.id, issue2.id] + + expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id) + + update_issue(opts) + expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position) + end + context 'when moving issue between issues from different projects' do let(:group) { create(:group) } let(:subgroup) { create(:group, parent: group) } @@ -294,7 +434,7 @@ RSpec.describe Issues::UpdateService, :mailer do end it 'does not update assignee_id with unauthorized users' do - project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) update_issue(confidential: true) non_member = create(:user) original_assignees = issue.assignees @@ -382,13 +522,16 @@ RSpec.describe Issues::UpdateService, :mailer do expect(Todo.where(attributes).count).to eq(1) end - end - context 'when the milestone is removed' do - before do - stub_feature_flags(track_resource_milestone_change_events: false) + context 'issue is incident type' do + let(:issue) { create(:incident, project: project) } + let(:current_user) { user } + + it_behaves_like 'an incident management tracked event', :incident_management_incident_assigned end + end + context 'when the milestone is removed' do let!(:non_subscriber) { create(:user) } let!(:subscriber) do @@ -398,12 +541,10 @@ RSpec.describe Issues::UpdateService, :mailer do end end - it_behaves_like 'system notes for milestones' - it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do issue.milestone = create(:milestone, project: project) - issue.save + issue.save! perform_enqueued_jobs do update_issue(milestone_id: "") @@ -416,7 +557,7 @@ RSpec.describe Issues::UpdateService, :mailer do it 'clears milestone issue counters cache' do issue.milestone = create(:milestone, project: project) - issue.save + issue.save! expect_next_instance_of(Milestones::IssuesCountService, issue.milestone) do |service| expect(service).to receive(:delete_cache).and_call_original @@ -430,10 +571,6 @@ RSpec.describe Issues::UpdateService, :mailer do end context 'when the milestone is assigned' do - before do - stub_feature_flags(track_resource_milestone_change_events: false) - end - let!(:non_subscriber) { create(:user) } let!(:subscriber) do @@ -449,8 +586,6 @@ RSpec.describe Issues::UpdateService, :mailer do expect(todo.reload.done?).to eq true end - it_behaves_like 'system notes for milestones' - it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do perform_enqueued_jobs do update_issue(milestone: create(:milestone, project: project)) @@ -670,7 +805,7 @@ RSpec.describe Issues::UpdateService, :mailer do let(:params) { { label_ids: [label.id], add_label_ids: [label3.id] } } before do - issue.update(labels: [label2]) + issue.update!(labels: [label2]) end it 'replaces the labels with the ones in label_ids and adds those in add_label_ids' do @@ -682,7 +817,7 @@ RSpec.describe Issues::UpdateService, :mailer do let(:params) { { label_ids: [label.id, label2.id, label3.id], remove_label_ids: [label.id] } } before do - issue.update(labels: [label, label3]) + issue.update!(labels: [label, label3]) end it 'replaces the labels with the ones in label_ids and removes those in remove_label_ids' do @@ -694,7 +829,7 @@ RSpec.describe Issues::UpdateService, :mailer do let(:params) { { add_label_ids: [label3.id], remove_label_ids: [label.id] } } before do - issue.update(labels: [label]) + issue.update!(labels: [label]) end it 'adds the passed labels' do @@ -711,7 +846,7 @@ RSpec.describe Issues::UpdateService, :mailer do context 'for a label assigned to an issue' do it 'removes the label' do - issue.update(labels: [label]) + issue.update!(labels: [label]) expect(result.label_ids).to be_empty end @@ -760,7 +895,7 @@ RSpec.describe Issues::UpdateService, :mailer do levels.each do |level| it "does not update with unauthorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do assignee = create(:user) - project.update(visibility_level: level) + project.update!(visibility_level: level) feature_visibility_attr = :"#{issue.model_name.plural}_access_level" project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE) diff --git a/spec/services/issues/zoom_link_service_spec.rb b/spec/services/issues/zoom_link_service_spec.rb index 56aec4fe564..b095cb24212 100644 --- a/spec/services/issues/zoom_link_service_spec.rb +++ b/spec/services/issues/zoom_link_service_spec.rb @@ -82,6 +82,13 @@ RSpec.describe Issues::ZoomLinkService do include_examples 'can add meeting' + context 'issue is incident type' do + let(:issue) { create(:incident) } + let(:current_user) { user } + + it_behaves_like 'an incident management tracked event', :incident_management_incident_zoom_meeting + end + context 'with insufficient issue update permissions' do include_context 'insufficient issue update permissions' include_examples 'cannot add meeting' diff --git a/spec/services/jira/requests/projects/list_service_spec.rb b/spec/services/jira/requests/projects/list_service_spec.rb index b4db77f8104..415dd42c795 100644 --- a/spec/services/jira/requests/projects/list_service_spec.rb +++ b/spec/services/jira/requests/projects/list_service_spec.rb @@ -69,7 +69,7 @@ RSpec.describe Jira::Requests::Projects::ListService do expect(client).to receive(:get).and_return([{ 'key' => 'pr1', 'name' => 'First Project' }, { 'key' => 'pr2', 'name' => 'Second Project' }]) end - it 'returns a paylod with Jira projets' do + it 'returns a paylod with Jira projects' do payload = subject.payload expect(subject.success?).to be_truthy @@ -80,7 +80,7 @@ RSpec.describe Jira::Requests::Projects::ListService do context 'when filtering projects by name' do let(:params) { { query: 'first' } } - it 'returns a paylod with Jira projets' do + it 'returns a paylod with Jira procjets' do payload = subject.payload expect(subject.success?).to be_truthy diff --git a/spec/services/jira_connect/sync_service_spec.rb b/spec/services/jira_connect/sync_service_spec.rb new file mode 100644 index 00000000000..e26ca30d0e1 --- /dev/null +++ b/spec/services/jira_connect/sync_service_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe JiraConnect::SyncService do + describe '#execute' do + let_it_be(:project) { create(:project, :repository) } + let(:branches) { [project.repository.find_branch('master')] } + let(:commits) { project.commits_by(oids: %w[b83d6e3 5a62481]) } + let(:merge_requests) { [create(:merge_request, source_project: project, target_project: project)] } + + subject do + described_class.new(project).execute(commits: commits, branches: branches, merge_requests: merge_requests) + end + + before do + create(:jira_connect_subscription, namespace: project.namespace) + end + + def expect_jira_client_call(return_value = { 'status': 'success' }) + expect_next_instance_of(Atlassian::JiraConnect::Client) do |instance| + expect(instance).to receive(:store_dev_info).with( + project: project, + commits: commits, + branches: [instance_of(Gitlab::Git::Branch)], + merge_requests: merge_requests + ).and_return(return_value) + end + end + + def expect_log(type, message) + expect(Gitlab::ProjectServiceLogger) + .to receive(type).with( + message: 'response from jira dev_info api', + integration: 'JiraConnect', + project_id: project.id, + project_path: project.full_path, + jira_response: message&.to_json + ) + end + + it 'calls Atlassian::JiraConnect::Client#store_dev_info and logs the response' do + expect_jira_client_call + + expect_log(:info, { 'status': 'success' }) + + subject + end + + context 'when request returns an error' do + it 'logs the response as an error' do + expect_jira_client_call({ + 'errorMessages' => ['some error message'] + }) + + expect_log(:error, { 'errorMessages' => ['some error message'] }) + + subject + end + end + end +end diff --git a/spec/services/jira_connect_subscriptions/create_service_spec.rb b/spec/services/jira_connect_subscriptions/create_service_spec.rb new file mode 100644 index 00000000000..77e758cf6fe --- /dev/null +++ b/spec/services/jira_connect_subscriptions/create_service_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe JiraConnectSubscriptions::CreateService do + let(:installation) { create(:jira_connect_installation) } + let(:current_user) { create(:user) } + let(:group) { create(:group) } + let(:path) { group.full_path } + + subject { described_class.new(installation, current_user, namespace_path: path).execute } + + before do + group.add_maintainer(current_user) + end + + shared_examples 'a failed execution' do + it 'does not create a subscription' do + expect { subject }.not_to change { installation.subscriptions.count } + end + + it 'returns an error status' do + expect(subject[:status]).to eq(:error) + end + end + + context 'when user does have access' do + it 'creates a subscription' do + expect { subject }.to change { installation.subscriptions.count }.from(0).to(1) + end + + it 'returns success' do + expect(subject[:status]).to eq(:success) + end + end + + context 'when path is invalid' do + let(:path) { 'some_invalid_namespace_path' } + + it_behaves_like 'a failed execution' + end + + context 'when user does not have access' do + subject { described_class.new(installation, create(:user), namespace_path: path).execute } + + it_behaves_like 'a failed execution' + end +end diff --git a/spec/services/lfs/push_service_spec.rb b/spec/services/lfs/push_service_spec.rb new file mode 100644 index 00000000000..8e5b98fdc9c --- /dev/null +++ b/spec/services/lfs/push_service_spec.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Lfs::PushService do + let(:logger) { service.send(:logger) } + let(:lfs_client) { service.send(:lfs_client) } + + let_it_be(:project) { create(:forked_project_with_submodules) } + let_it_be(:remote_mirror) { create(:remote_mirror, project: project, enabled: true) } + let_it_be(:lfs_object) { create_linked_lfs_object(project, :project) } + + let(:params) { { url: remote_mirror.bare_url, credentials: remote_mirror.credentials } } + + subject(:service) { described_class.new(project, nil, params) } + + describe "#execute" do + it 'uploads the object when upload is requested' do + stub_lfs_batch(lfs_object) + + expect(lfs_client) + .to receive(:upload) + .with(lfs_object, upload_action_spec(lfs_object), authenticated: true) + + expect(service.execute).to eq(status: :success) + end + + it 'does nothing if there are no LFS objects' do + lfs_object.destroy! + + expect(lfs_client).not_to receive(:upload) + + expect(service.execute).to eq(status: :success) + end + + it 'does not upload the object when upload is not requested' do + stub_lfs_batch(lfs_object, upload: false) + + expect(lfs_client).not_to receive(:upload) + + expect(service.execute).to eq(status: :success) + end + + it 'returns a failure when submitting a batch fails' do + expect(lfs_client).to receive(:batch) { raise 'failed' } + + expect(service.execute).to eq(status: :error, message: 'failed') + end + + it 'returns a failure when submitting an upload fails' do + stub_lfs_batch(lfs_object) + expect(lfs_client).to receive(:upload) { raise 'failed' } + + expect(service.execute).to eq(status: :error, message: 'failed') + end + + context 'non-project-repository LFS objects' do + let_it_be(:nil_lfs_object) { create_linked_lfs_object(project, nil) } + let_it_be(:wiki_lfs_object) { create_linked_lfs_object(project, :wiki) } + let_it_be(:design_lfs_object) { create_linked_lfs_object(project, :design) } + + it 'only tries to upload the project-repository LFS object' do + stub_lfs_batch(nil_lfs_object, lfs_object, upload: false) + + expect(service.execute).to eq(status: :success) + end + end + end + + def create_linked_lfs_object(project, type) + create(:lfs_objects_project, project: project, repository_type: type).lfs_object + end + + def stub_lfs_batch(*objects, upload: true) + expect(lfs_client) + .to receive(:batch).with('upload', containing_exactly(*objects)) + .and_return('transfer' => 'basic', 'objects' => objects.map { |o| object_spec(o, upload: upload) }) + end + + def batch_spec(*objects, upload: true) + { 'transfer' => 'basic', 'objects' => objects.map {|o| object_spec(o, upload: upload) } } + end + + def object_spec(object, upload: true) + { 'oid' => object.oid, 'size' => object.size, 'authenticated' => true }.tap do |spec| + spec['actions'] = { 'upload' => upload_action_spec(object) } if upload + end + end + + def upload_action_spec(object) + { 'href' => "https://example.com/#{object.oid}/#{object.size}", 'header' => { 'Key' => 'value' } } + end +end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 5c90f1f54ea..3b3f2f3b95a 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -192,8 +192,8 @@ RSpec.describe Members::DestroyService do context 'with an access requester' do before do - group_project.update(request_access_enabled: true) - group.update(request_access_enabled: true) + group_project.update!(request_access_enabled: true) + group.update!(request_access_enabled: true) group_project.request_access(member_user) group.request_access(member_user) end diff --git a/spec/services/merge_requests/base_service_spec.rb b/spec/services/merge_requests/base_service_spec.rb new file mode 100644 index 00000000000..bb7b70f1ba2 --- /dev/null +++ b/spec/services/merge_requests/base_service_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::BaseService do + include ProjectForksHelper + + let_it_be(:project) { create(:project, :repository) } + let(:title) { 'Awesome merge_request' } + let(:params) do + { + title: title, + description: 'please fix', + source_branch: 'feature', + target_branch: 'master' + } + end + + subject { MergeRequests::CreateService.new(project, project.owner, params) } + + describe '#execute_hooks' do + shared_examples 'enqueues Jira sync worker' do + it do + Sidekiq::Testing.fake! do + expect { subject.execute }.to change(JiraConnect::SyncMergeRequestWorker.jobs, :size).by(1) + end + end + end + + shared_examples 'does not enqueue Jira sync worker' do + it do + Sidekiq::Testing.fake! do + expect { subject.execute }.not_to change(JiraConnect::SyncMergeRequestWorker.jobs, :size) + end + end + end + + context 'with a Jira subscription' do + before do + create(:jira_connect_subscription, namespace: project.namespace) + end + + context 'MR contains Jira issue key' do + let(:title) { 'Awesome merge_request with issue JIRA-123' } + + it_behaves_like 'enqueues Jira sync worker' + end + + context 'MR does not contain Jira issue key' do + it_behaves_like 'does not enqueue Jira sync worker' + end + end + + context 'without a Jira subscription' do + it_behaves_like 'does not enqueue Jira sync worker' + end + end +end diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index f99be26927d..f83b8d98ce8 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -88,6 +88,10 @@ RSpec.describe MergeRequests::BuildService do let(:source_project) { fork_project(project, user) } let(:merge_request) { described_class.new(project, user, mr_params).execute } + before do + project.add_reporter(user) + end + it 'assigns force_remove_source_branch' do expect(merge_request.force_remove_source_branch?).to be_truthy end @@ -510,7 +514,7 @@ RSpec.describe MergeRequests::BuildService do let(:target_project) { create(:project, :public, :repository) } before do - target_project.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + target_project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) end it 'sets the target_project correctly' do diff --git a/spec/services/merge_requests/cleanup_refs_service_spec.rb b/spec/services/merge_requests/cleanup_refs_service_spec.rb new file mode 100644 index 00000000000..b38ccee4aa0 --- /dev/null +++ b/spec/services/merge_requests/cleanup_refs_service_spec.rb @@ -0,0 +1,146 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::CleanupRefsService do + describe '.schedule' do + let(:merge_request) { build(:merge_request) } + + it 'schedules MergeRequestCleanupRefsWorker' do + expect(MergeRequestCleanupRefsWorker) + .to receive(:perform_in) + .with(described_class::TIME_THRESHOLD, merge_request.id) + + described_class.schedule(merge_request) + end + end + + describe '#execute' do + before do + # Need to re-enable this as it's being stubbed in spec_helper for + # performance reasons but is needed to run for this test. + allow(Gitlab::Git::KeepAround).to receive(:execute).and_call_original + end + + subject(:result) { described_class.new(merge_request).execute } + + shared_examples_for 'service that cleans up merge request refs' do + it 'creates keep around ref and deletes merge request refs' do + old_ref_head = ref_head + + aggregate_failures do + expect(result[:status]).to eq(:success) + expect(kept_around?(old_ref_head)).to be_truthy + expect(ref_head).to be_nil + end + end + + context 'when merge request has merge ref' do + before do + MergeRequests::MergeToRefService + .new(merge_request.project, merge_request.author) + .execute(merge_request) + end + + it 'caches merge ref sha and deletes merge ref' do + old_merge_ref_head = merge_request.merge_ref_head + + aggregate_failures do + expect(result[:status]).to eq(:success) + expect(kept_around?(old_merge_ref_head)).to be_truthy + expect(merge_request.reload.merge_ref_sha).to eq(old_merge_ref_head.id) + expect(ref_exists?(merge_request.merge_ref_path)).to be_falsy + end + end + + context 'when merge ref sha cannot be cached' do + before do + allow(merge_request) + .to receive(:update_column) + .with(:merge_ref_sha, merge_request.merge_ref_head.id) + .and_return(false) + end + + it_behaves_like 'service that does not clean up merge request refs' + end + end + + context 'when keep around ref cannot be created' do + before do + allow_next_instance_of(Gitlab::Git::KeepAround) do |keep_around| + expect(keep_around).to receive(:kept_around?).and_return(false) + end + end + + it_behaves_like 'service that does not clean up merge request refs' + end + end + + shared_examples_for 'service that does not clean up merge request refs' do + it 'does not delete merge request refs' do + aggregate_failures do + expect(result[:status]).to eq(:error) + expect(ref_head).to be_present + end + end + end + + context 'when merge request is closed' do + let(:merge_request) { create(:merge_request, :closed) } + + context "when closed #{described_class::TIME_THRESHOLD.inspect} ago" do + before do + merge_request.metrics.update!(latest_closed_at: described_class::TIME_THRESHOLD.ago) + end + + it_behaves_like 'service that cleans up merge request refs' + end + + context "when closed later than #{described_class::TIME_THRESHOLD.inspect} ago" do + before do + merge_request.metrics.update!(latest_closed_at: (described_class::TIME_THRESHOLD - 1.day).ago) + end + + it_behaves_like 'service that does not clean up merge request refs' + end + end + + context 'when merge request is merged' do + let(:merge_request) { create(:merge_request, :merged) } + + context "when merged #{described_class::TIME_THRESHOLD.inspect} ago" do + before do + merge_request.metrics.update!(merged_at: described_class::TIME_THRESHOLD.ago) + end + + it_behaves_like 'service that cleans up merge request refs' + end + + context "when merged later than #{described_class::TIME_THRESHOLD.inspect} ago" do + before do + merge_request.metrics.update!(merged_at: (described_class::TIME_THRESHOLD - 1.day).ago) + end + + it_behaves_like 'service that does not clean up merge request refs' + end + end + + context 'when merge request is not closed nor merged' do + let(:merge_request) { create(:merge_request, :opened) } + + it_behaves_like 'service that does not clean up merge request refs' + end + end + + def kept_around?(commit) + Gitlab::Git::KeepAround.new(merge_request.project.repository).kept_around?(commit.id) + end + + def ref_head + merge_request.project.repository.commit(merge_request.ref_path) + end + + def ref_exists?(ref) + merge_request.project.repository.ref_exists?(ref) + end +end diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index e518e439a84..e7ac286f48b 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -99,6 +99,12 @@ RSpec.describe MergeRequests::CloseService do described_class.new(project, user).execute(merge_request) end + it 'schedules CleanupRefsService' do + expect(MergeRequests::CleanupRefsService).to receive(:schedule).with(merge_request) + + described_class.new(project, user).execute(merge_request) + end + context 'current user is not authorized to close merge request' do before do perform_enqueued_jobs do diff --git a/spec/services/merge_requests/conflicts/list_service_spec.rb b/spec/services/merge_requests/conflicts/list_service_spec.rb index 14133731e37..5132eac0158 100644 --- a/spec/services/merge_requests/conflicts/list_service_spec.rb +++ b/spec/services/merge_requests/conflicts/list_service_spec.rb @@ -30,14 +30,13 @@ RSpec.describe MergeRequests::Conflicts::ListService do it 'returns a falsey value when one of the MR branches is missing' do merge_request = create_merge_request('conflict-resolvable') merge_request.project.repository.rm_branch(merge_request.author, 'conflict-resolvable') - merge_request.clear_memoized_source_branch_exists expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey end it 'returns a falsey value when the MR does not support new diff notes' do merge_request = create_merge_request('conflict-resolvable') - merge_request.merge_request_diff.update(start_commit_sha: nil) + merge_request.merge_request_diff.update!(start_commit_sha: nil) expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey end diff --git a/spec/services/merge_requests/create_pipeline_service_spec.rb b/spec/services/merge_requests/create_pipeline_service_spec.rb index db46bd37eea..4dd70627977 100644 --- a/spec/services/merge_requests/create_pipeline_service_spec.rb +++ b/spec/services/merge_requests/create_pipeline_service_spec.rb @@ -5,13 +5,14 @@ require 'spec_helper' RSpec.describe MergeRequests::CreatePipelineService do include ProjectForksHelper - let_it_be(:project) { create(:project, :repository) } + let_it_be(:project, reload: true) { create(:project, :repository) } let_it_be(:user) { create(:user) } let(:service) { described_class.new(project, actor, params) } let(:actor) { user } let(:params) { {} } before do + stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false) project.add_developer(user) end @@ -58,9 +59,27 @@ RSpec.describe MergeRequests::CreatePipelineService do expect(subject.project).to eq(project) end - context 'when ci_allow_to_create_merge_request_pipelines_in_target_project feature flag is disabled' do + context 'when source branch is protected' do + context 'when actor does not have permission to update the protected branch in target project' do + let!(:protected_branch) { create(:protected_branch, name: '*', project: project) } + + it 'creates a pipeline in the source project' do + expect(subject.project).to eq(source_project) + end + end + + context 'when actor has permission to update the protected branch in target project' do + let!(:protected_branch) { create(:protected_branch, :developers_can_merge, name: '*', project: project) } + + it 'creates a pipeline in the target project' do + expect(subject.project).to eq(project) + end + end + end + + context 'when ci_disallow_to_create_merge_request_pipelines_in_target_project feature flag is enabled' do before do - stub_feature_flags(ci_allow_to_create_merge_request_pipelines_in_target_project: false) + stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: true) end it 'creates a pipeline in the source project' do diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index bb62e594e7a..d042b318d02 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do let(:project) { create(:project, :repository) } let(:user) { create(:user) } - let(:assignee) { create(:user) } + let(:user2) { create(:user) } describe '#execute' do context 'valid params' do @@ -26,7 +26,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do before do project.add_maintainer(user) - project.add_developer(assignee) + project.add_developer(user2) allow(service).to receive(:execute_hooks) end @@ -75,7 +75,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do description: "well this is not done yet\n/wip", source_branch: 'feature', target_branch: 'master', - assignees: [assignee] + assignees: [user2] } end @@ -91,7 +91,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do description: "well this is not done yet\n/wip", source_branch: 'feature', target_branch: 'master', - assignees: [assignee] + assignees: [user2] } end @@ -108,17 +108,17 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do description: 'please fix', source_branch: 'feature', target_branch: 'master', - assignees: [assignee] + assignees: [user2] } end - it { expect(merge_request.assignees).to eq([assignee]) } + it { expect(merge_request.assignees).to eq([user2]) } it 'creates a todo for new assignee' do attributes = { project: project, author: user, - user: assignee, + user: user2, target_id: merge_request.id, target_type: merge_request.class.name, action: Todo::ASSIGNED, @@ -129,6 +129,34 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end end + context 'when reviewer is assigned' do + let(:opts) do + { + title: 'Awesome merge_request', + description: 'please fix', + source_branch: 'feature', + target_branch: 'master', + reviewers: [user2] + } + end + + it { expect(merge_request.reviewers).to eq([user2]) } + + it 'creates a todo for new reviewer' do + attributes = { + project: project, + author: user, + user: user2, + target_id: merge_request.id, + target_type: merge_request.class.name, + action: Todo::REVIEW_REQUESTED, + state: :pending + } + + expect(Todo.where(attributes).count).to eq 1 + end + end + context 'when head pipelines already exist for merge request source branch', :sidekiq_inline do let(:shas) { project.repository.commits(opts[:source_branch], limit: 2).map(&:id) } let!(:pipeline_1) { create(:ci_pipeline, project: project, ref: opts[:source_branch], project_id: project.id, sha: shas[1]) } @@ -212,7 +240,8 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end before do - target_project.add_developer(assignee) + stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false) + target_project.add_developer(user2) target_project.add_maintainer(user) end @@ -338,6 +367,10 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end end end + + it_behaves_like 'reviewer_ids filter' do + let(:execute) { service.execute } + end end it_behaves_like 'issuable record that supports quick actions' do @@ -361,7 +394,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do assignee_ids: create(:user).id, milestone_id: 1, title: 'Title', - description: %(/assign @#{assignee.username}\n/milestone %"#{milestone.name}"), + description: %(/assign @#{user2.username}\n/milestone %"#{milestone.name}"), source_branch: 'feature', target_branch: 'master' } @@ -369,12 +402,12 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do before do project.add_maintainer(user) - project.add_maintainer(assignee) + project.add_maintainer(user2) end it 'assigns and sets milestone to issuable from command' do expect(merge_request).to be_persisted - expect(merge_request.assignees).to eq([assignee]) + expect(merge_request.assignees).to eq([user2]) expect(merge_request.milestone).to eq(milestone) end end @@ -382,7 +415,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do context 'merge request create service' do context 'asssignee_id' do - let(:assignee) { create(:user) } + let(:user2) { create(:user) } before do project.add_maintainer(user) @@ -405,12 +438,12 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end it 'saves assignee when user id is valid' do - project.add_maintainer(assignee) - opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } + project.add_maintainer(user2) + opts = { title: 'Title', description: 'Description', assignee_ids: [user2.id] } merge_request = described_class.new(project, user, opts).execute - expect(merge_request.assignees).to eq([assignee]) + expect(merge_request.assignees).to eq([user2]) end context 'when assignee is set' do @@ -418,24 +451,24 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do { title: 'Title', description: 'Description', - assignee_ids: [assignee.id], + assignee_ids: [user2.id], source_branch: 'feature', target_branch: 'master' } end it 'invalidates open merge request counter for assignees when merge request is assigned' do - project.add_maintainer(assignee) + project.add_maintainer(user2) described_class.new(project, user, opts).execute - expect(assignee.assigned_open_merge_requests_count).to eq 1 + expect(user2.assigned_open_merge_requests_count).to eq 1 end end context "when issuable feature is private" do before do - project.project_feature.update(issues_access_level: ProjectFeature::PRIVATE, + project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE, merge_requests_access_level: ProjectFeature::PRIVATE) end @@ -443,8 +476,8 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do levels.each do |level| it "removes not authorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do - project.update(visibility_level: level) - opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } + project.update!(visibility_level: level) + opts = { title: 'Title', description: 'Description', assignee_ids: [user2.id] } merge_request = described_class.new(project, user, opts).execute @@ -470,7 +503,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do before do project.add_maintainer(user) - project.add_developer(assignee) + project.add_developer(user2) end it 'creates a `MergeRequestsClosingIssues` record for each issue' do @@ -498,7 +531,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do context 'when user can not access source project' do before do - target_project.add_developer(assignee) + target_project.add_developer(user2) target_project.add_maintainer(user) end @@ -510,7 +543,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do context 'when user can not access target project' do before do - target_project.add_developer(assignee) + target_project.add_developer(user2) target_project.add_maintainer(user) end @@ -562,7 +595,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end before do - project.add_developer(assignee) + project.add_developer(user2) project.add_maintainer(user) end diff --git a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb index 377615bbc6f..cdaacaf5fca 100644 --- a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb +++ b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb @@ -19,7 +19,7 @@ RSpec.describe MergeRequests::DeleteNonLatestDiffsService, :clean_gitlab_redis_s expect(diffs.count).to eq(4) - Timecop.freeze do + freeze_time do expect(DeleteDiffFilesWorker) .to receive(:bulk_perform_in) .with(5.minutes, [[diffs.first.id], [diffs.second.id]]) diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 11e341994f7..8328f461029 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -152,6 +152,7 @@ RSpec.describe MergeRequests::MergeService do let(:commit) { double('commit', safe_message: "Fixes #{jira_issue.to_reference}") } before do + stub_jira_service_test project.update!(has_external_issue_tracker: true) jira_service_settings stub_jira_urls(jira_issue.id) @@ -175,7 +176,7 @@ RSpec.describe MergeRequests::MergeService do end it 'does not close issue' do - jira_tracker.update(jira_issue_transition_id: nil) + jira_tracker.update!(jira_issue_transition_id: nil) expect_any_instance_of(JiraService).not_to receive(:transition_issue) @@ -388,7 +389,7 @@ RSpec.describe MergeRequests::MergeService do error_message = 'Failed to squash. Should be done manually' allow_any_instance_of(MergeRequests::SquashService).to receive(:squash!).and_return(nil) - merge_request.update(squash: true) + merge_request.update!(squash: true) service.execute(merge_request) @@ -402,7 +403,7 @@ RSpec.describe MergeRequests::MergeService do error_message = 'another squash is already in progress' allow_any_instance_of(MergeRequest).to receive(:squash_in_progress?).and_return(true) - merge_request.update(squash: true) + merge_request.update!(squash: true) service.execute(merge_request) @@ -420,7 +421,7 @@ RSpec.describe MergeRequests::MergeService do %w(semi-linear ff).each do |merge_method| it "logs and saves error if merge is #{merge_method} only" do merge_method = 'rebase_merge' if merge_method == 'semi-linear' - merge_request.project.update(merge_method: merge_method) + merge_request.project.update!(merge_method: merge_method) error_message = 'Only fast-forward merge is allowed for your project. Please update your source branch' allow(service).to receive(:execute_hooks) @@ -434,6 +435,43 @@ RSpec.describe MergeRequests::MergeService do end end end + + context 'when not mergeable' do + let!(:error_message) { 'Merge request is not mergeable' } + + context 'with failing CI' do + before do + allow(merge_request).to receive(:mergeable_ci_state?) { false } + end + + it 'logs and saves error' do + service.execute(merge_request) + + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + end + end + + context 'with unresolved discussions' do + before do + allow(merge_request).to receive(:mergeable_discussions_state?) { false } + end + + it 'logs and saves error' do + service.execute(merge_request) + + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + end + + context 'when passing `skip_discussions_check: true` as `options` parameter' do + it 'merges the merge request' do + service.execute(merge_request, skip_discussions_check: true) + + expect(merge_request).to be_valid + expect(merge_request).to be_merged + end + end + end + end end end end diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index a51a896ca96..402f753c0af 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -50,7 +50,7 @@ RSpec.describe MergeRequests::PostMergeService do end it 'marks MR as merged regardless of errors when closing issues' do - merge_request.update(target_branch: 'foo') + merge_request.update!(target_branch: 'foo') allow(project).to receive(:default_branch).and_return('foo') issue = create(:issue, project: project) @@ -72,6 +72,12 @@ RSpec.describe MergeRequests::PostMergeService do subject end + it 'schedules CleanupRefsService' do + expect(MergeRequests::CleanupRefsService).to receive(:schedule).with(merge_request) + + subject + end + context 'when the merge request has review apps' do it 'cancels all review app deployments' do pipeline = create(:ci_pipeline, diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 0696e8a247f..cace1e0bf09 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -225,6 +225,10 @@ RSpec.describe MergeRequests::RefreshService do context 'when service runs on forked project' do let(:project) { @fork_project } + before do + stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false) + end + it 'creates detached merge request pipeline for fork merge request', :sidekiq_inline do expect { subject } .to change { @fork_merge_request.pipelines_for_merge_request.count }.by(1) @@ -617,7 +621,7 @@ RSpec.describe MergeRequests::RefreshService do before do stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - @fork_project.destroy + @fork_project.destroy! service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') reload_mrs end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index c3433c8c9d2..6b7463d4996 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -52,6 +52,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do title: 'New title', description: 'Also please fix', assignee_ids: [user.id], + reviewer_ids: [user.id], state_event: 'close', label_ids: [label.id], target_branch: 'target', @@ -75,6 +76,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(@merge_request).to be_valid expect(@merge_request.title).to eq('New title') expect(@merge_request.assignees).to match_array([user]) + expect(@merge_request.reviewers).to match_array([user]) expect(@merge_request).to be_closed expect(@merge_request.labels.count).to eq(1) expect(@merge_request.labels.first.title).to eq(label.name) @@ -161,6 +163,29 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(@merge_request.merge_params["force_remove_source_branch"]).to eq("1") end end + + it_behaves_like 'reviewer_ids filter' do + let(:opts) { {} } + let(:execute) { update_merge_request(opts) } + end + + context 'with an existing reviewer' do + let(:merge_request) do + create(:merge_request, :simple, source_project: project, reviewer_ids: [user2.id]) + end + + context 'when merge_request_reviewer feature is enabled' do + before do + stub_feature_flags(merge_request_reviewer: true) + end + + let(:opts) { { reviewer_ids: [IssuableFinder::Params::NONE] } } + + it 'removes reviewers' do + expect(update_merge_request(opts).reviewers).to eq [] + end + end + end end context 'after_save callback to store_mentions' do @@ -379,11 +404,31 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end end - context 'when the milestone is removed' do + context 'when reviewers gets changed' do before do - stub_feature_flags(track_resource_milestone_change_events: false) + update_merge_request({ reviewer_ids: [user2.id] }) + end + + it 'marks pending todo as done' do + expect(pending_todo.reload).to be_done + end + + it 'creates a pending todo for new review request' do + attributes = { + project: project, + author: user, + user: user2, + target_id: merge_request.id, + target_type: merge_request.class.name, + action: Todo::REVIEW_REQUESTED, + state: :pending + } + + expect(Todo.where(attributes).count).to eq 1 end + end + context 'when the milestone is removed' do let!(:non_subscriber) { create(:user) } let!(:subscriber) do @@ -393,12 +438,10 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end end - it_behaves_like 'system notes for milestones' - it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do merge_request.milestone = create(:milestone, project: project) - merge_request.save + merge_request.save! perform_enqueued_jobs do update_merge_request(milestone_id: "") @@ -410,10 +453,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end context 'when the milestone is changed' do - before do - stub_feature_flags(track_resource_milestone_change_events: false) - end - let!(:non_subscriber) { create(:user) } let!(:subscriber) do @@ -429,8 +468,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(pending_todo.reload).to be_done end - it_behaves_like 'system notes for milestones' - it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do perform_enqueued_jobs do update_merge_request(milestone: create(:milestone, project: project)) @@ -628,7 +665,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do context 'updating asssignee_ids' do it 'does not update assignee when assignee_id is invalid' do - merge_request.update(assignee_ids: [user.id]) + merge_request.update!(assignee_ids: [user.id]) update_merge_request(assignee_ids: [-1]) @@ -636,7 +673,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end it 'unassigns assignee when user id is 0' do - merge_request.update(assignee_ids: [user.id]) + merge_request.update!(assignee_ids: [user.id]) update_merge_request(assignee_ids: [0]) @@ -664,7 +701,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do levels.each do |level| it "does not update with unauthorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do assignee = create(:user) - project.update(visibility_level: level) + project.update!(visibility_level: level) feature_visibility_attr = :"#{merge_request.model_name.plural}_access_level" project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE) diff --git a/spec/services/metrics/dashboard/gitlab_alert_embed_service_spec.rb b/spec/services/metrics/dashboard/gitlab_alert_embed_service_spec.rb index dd9d498e307..d5928b1b5af 100644 --- a/spec/services/metrics/dashboard/gitlab_alert_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/gitlab_alert_embed_service_spec.rb @@ -10,9 +10,8 @@ RSpec.describe Metrics::Dashboard::GitlabAlertEmbedService do let_it_be(:user) { create(:user) } let(:alert_id) { alert.id } - before do + before_all do project.add_maintainer(user) - project.clear_memoization(:licensed_feature_available) end describe '.valid_params?' do diff --git a/spec/services/note_summary_spec.rb b/spec/services/note_summary_spec.rb index 38174748b19..ad244f62292 100644 --- a/spec/services/note_summary_spec.rb +++ b/spec/services/note_summary_spec.rb @@ -23,7 +23,7 @@ RSpec.describe NoteSummary do describe '#note' do it 'returns note hash' do - Timecop.freeze do + freeze_time do expect(create_note_summary.note).to eq(noteable: noteable, project: project, author: user, note: 'note', created_at: Time.current) end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index f087f72ca46..7c0d4b756bd 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -41,7 +41,7 @@ RSpec.describe Notes::CreateService do end it 'TodoService#new_note is called' do - note = build(:note, project: project) + note = build(:note, project: project, noteable: issue) allow(Note).to receive(:new).with(opts) { note } expect_any_instance_of(TodoService).to receive(:new_note).with(note, user) @@ -50,13 +50,23 @@ RSpec.describe Notes::CreateService do end it 'enqueues NewNoteWorker' do - note = build(:note, id: non_existing_record_id, project: project) + note = build(:note, id: non_existing_record_id, project: project, noteable: issue) allow(Note).to receive(:new).with(opts) { note } expect(NewNoteWorker).to receive(:perform_async).with(note.id) described_class.new(project, user, opts).execute end + + context 'issue is an incident' do + subject { described_class.new(project, user, opts).execute } + + let(:issue) { create(:incident, project: project) } + + it_behaves_like 'an incident management tracked event', :incident_management_incident_comment do + let(:current_user) { user } + end + end end context 'noteable highlight cache clearing' do diff --git a/spec/services/notes/quick_actions_service_spec.rb b/spec/services/notes/quick_actions_service_spec.rb index e9decd44730..64aa845841b 100644 --- a/spec/services/notes/quick_actions_service_spec.rb +++ b/spec/services/notes/quick_actions_service_spec.rb @@ -4,9 +4,9 @@ require 'spec_helper' RSpec.describe Notes::QuickActionsService do shared_context 'note on noteable' do - let(:project) { create(:project, :repository) } - let(:maintainer) { create(:user).tap { |u| project.add_maintainer(u) } } - let(:assignee) { create(:user) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:maintainer) { create(:user).tap { |u| project.add_maintainer(u) } } + let_it_be(:assignee) { create(:user) } before do project.add_maintainer(assignee) @@ -30,10 +30,9 @@ RSpec.describe Notes::QuickActionsService do end it 'closes noteable, sets labels, assigns, and sets milestone to noteable, and leave no note' do - content, update_params = service.execute(note) - service.apply_updates(update_params, note) + content = execute(note) - expect(content).to eq '' + expect(content).to be_empty expect(note.noteable).to be_closed expect(note.noteable.labels).to match_array(labels) expect(note.noteable.assignees).to eq([assignee]) @@ -41,6 +40,30 @@ RSpec.describe Notes::QuickActionsService do end end + context '/relate' do + let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:other_issue) { create(:issue, project: project) } + let(:note_text) { "/relate #{other_issue.to_reference}" } + let(:note) { create(:note_on_issue, noteable: issue, project: project, note: note_text) } + + context 'user cannot relate issues' do + before do + project.team.find_member(maintainer.id).destroy! + project.update!(visibility: Gitlab::VisibilityLevel::PUBLIC) + end + + it 'does not create issue relation' do + expect { execute(note) }.not_to change { IssueLink.count } + end + end + + context 'user is allowed to relate issues' do + it 'creates issue relation' do + expect { execute(note) }.to change { IssueLink.count }.by(1) + end + end + end + describe '/reopen' do before do note.noteable.close! @@ -49,10 +72,9 @@ RSpec.describe Notes::QuickActionsService do let(:note_text) { '/reopen' } it 'opens the noteable, and leave no note' do - content, update_params = service.execute(note) - service.apply_updates(update_params, note) + content = execute(note) - expect(content).to eq '' + expect(content).to be_empty expect(note.noteable).to be_open end end @@ -62,10 +84,9 @@ RSpec.describe Notes::QuickActionsService do let(:note_text) { '/spend 1h' } it 'adds time to noteable, adds timelog with nil note_id and has no content' do - content, update_params = service.execute(note) - service.apply_updates(update_params, note) + content = execute(note) - expect(content).to eq '' + expect(content).to be_empty expect(note.noteable.time_spent).to eq(3600) expect(Timelog.last.note_id).to be_nil end @@ -92,8 +113,7 @@ RSpec.describe Notes::QuickActionsService do end it 'closes noteable, sets labels, assigns, and sets milestone to noteable' do - content, update_params = service.execute(note) - service.apply_updates(update_params, note) + content = execute(note) expect(content).to eq "HELLO\nWORLD" expect(note.noteable).to be_closed @@ -111,14 +131,87 @@ RSpec.describe Notes::QuickActionsService do let(:note_text) { "HELLO\n/reopen\nWORLD" } it 'opens the noteable' do - content, update_params = service.execute(note) - service.apply_updates(update_params, note) + content = execute(note) expect(content).to eq "HELLO\nWORLD" expect(note.noteable).to be_open end end end + + describe '/milestone' do + let(:issue) { create(:issue, project: project) } + let(:note_text) { %(/milestone %"#{milestone.name}") } + let(:note) { create(:note_on_issue, noteable: issue, project: project, note: note_text) } + + context 'on an incident' do + before do + issue.update!(issue_type: :incident) + end + + it 'leaves the note empty' do + expect(execute(note)).to be_empty + end + + it 'does not assign the milestone' do + expect { execute(note) }.not_to change { issue.reload.milestone } + end + end + + context 'on a merge request' do + let(:note_mr) { create(:note_on_merge_request, project: project, note: note_text) } + + it 'leaves the note empty' do + expect(execute(note_mr)).to be_empty + end + + it 'assigns the milestone' do + expect { execute(note) }.to change { issue.reload.milestone }.from(nil).to(milestone) + end + end + end + + describe '/remove_milestone' do + let(:issue) { create(:issue, project: project, milestone: milestone) } + let(:note_text) { '/remove_milestone' } + let(:note) { create(:note_on_issue, noteable: issue, project: project, note: note_text) } + + context 'on an issue' do + it 'leaves the note empty' do + expect(execute(note)).to be_empty + end + + it 'removes the milestone' do + expect { execute(note) }.to change { issue.reload.milestone }.from(milestone).to(nil) + end + end + + context 'on an incident' do + before do + issue.update!(issue_type: :incident) + end + + it 'leaves the note empty' do + expect(execute(note)).to be_empty + end + + it 'does not remove the milestone' do + expect { execute(note) }.not_to change { issue.reload.milestone } + end + end + + context 'on a merge request' do + let(:note_mr) { create(:note_on_merge_request, project: project, note: note_text) } + + it 'leaves the note empty' do + expect(execute(note_mr)).to be_empty + end + + it 'removes the milestone' do + expect { execute(note) }.to change { issue.reload.milestone }.from(milestone).to(nil) + end + end + end end describe '.noteable_update_service' do @@ -180,11 +273,13 @@ RSpec.describe Notes::QuickActionsService do let(:service) { described_class.new(project, maintainer) } it_behaves_like 'note on noteable that supports quick actions' do - let(:note) { build(:note_on_issue, project: project) } + let_it_be(:issue, reload: true) { create(:issue, project: project) } + let(:note) { build(:note_on_issue, project: project, noteable: issue) } end it_behaves_like 'note on noteable that supports quick actions' do - let(:note) { build(:note_on_merge_request, project: project) } + let(:merge_request) { create(:merge_request, source_project: project) } + let(:note) { build(:note_on_merge_request, project: project, noteable: merge_request) } end end @@ -207,11 +302,17 @@ RSpec.describe Notes::QuickActionsService do end it 'adds only one assignee from the list' do - _, update_params = service.execute(note) - service.apply_updates(update_params, note) + execute(note) expect(note.noteable.assignees.count).to eq(1) end end end + + def execute(note) + content, update_params = service.execute(note) + service.apply_updates(update_params, note) + + content + end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 8186bc40bc0..03e24524f9f 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -7,8 +7,10 @@ RSpec.describe NotificationService, :mailer do include ExternalAuthorizationServiceHelpers include NotificationHelpers + let_it_be(:project, reload: true) { create(:project, :public) } + let_it_be_with_refind(:assignee) { create(:user) } + let(:notification) { described_class.new } - let(:assignee) { create(:user) } around(:example, :deliver_mails_inline) do |example| # This is a temporary `around` hook until all the examples check the @@ -149,9 +151,9 @@ RSpec.describe NotificationService, :mailer do end shared_examples_for 'participating notifications' do - it_should_behave_like 'participating by note notification' - it_should_behave_like 'participating by author notification' - it_should_behave_like 'participating by assignee notification' + it_behaves_like 'participating by note notification' + it_behaves_like 'participating by author notification' + it_behaves_like 'participating by assignee notification' end describe '#async' do @@ -272,97 +274,26 @@ RSpec.describe NotificationService, :mailer do end end + describe '#disabled_two_factor' do + let_it_be(:user) { create(:user) } + + subject { notification.disabled_two_factor(user) } + + it 'sends email to the user' do + expect { subject }.to have_enqueued_email(user, mail: 'disabled_two_factor_email') + end + end + describe 'Notes' do context 'issue note' do - let(:project) { create(:project, :private) } - let(:issue) { create(:issue, project: project, assignees: [assignee]) } - let(:mentioned_issue) { create(:issue, assignees: issue.assignees) } - let(:author) { create(:user) } + let_it_be(:project) { create(:project, :private) } + let_it_be(:issue) { create(:issue, project: project, assignees: [assignee]) } + let_it_be(:mentioned_issue) { create(:issue, assignees: issue.assignees) } + let_it_be_with_reload(: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') } subject { notification.new_note(note) } - before do - build_team(project) - project.add_maintainer(issue.author) - project.add_maintainer(assignee) - project.add_maintainer(author) - - @u_custom_off = create_user_with_notification(:custom, 'custom_off') - project.add_guest(@u_custom_off) - - create( - :note_on_issue, - author: @u_custom_off, - noteable: issue, - project_id: issue.project_id, - note: 'i think @subscribed_participant should see this' - ) - - update_custom_notification(:new_note, @u_guest_custom, resource: project) - update_custom_notification(:new_note, @u_custom_global) - end - - describe '#new_note' do - context do - before do - add_users(project) - add_user_subscriptions(issue) - reset_delivered_emails! - end - - it 'sends emails to recipients' do - subject - - expect_delivery_jobs_count(10) - expect_enqueud_email(@u_watcher.id, note.id, nil, mail: "note_issue_email") - expect_enqueud_email(note.noteable.author.id, note.id, nil, mail: "note_issue_email") - expect_enqueud_email(note.noteable.assignees.first.id, note.id, nil, mail: "note_issue_email") - expect_enqueud_email(@u_custom_global.id, note.id, nil, mail: "note_issue_email") - expect_enqueud_email(@u_mentioned.id, note.id, "mentioned", mail: "note_issue_email") - expect_enqueud_email(@subscriber.id, note.id, "subscribed", mail: "note_issue_email") - expect_enqueud_email(@watcher_and_subscriber.id, note.id, "subscribed", mail: "note_issue_email") - expect_enqueud_email(@subscribed_participant.id, note.id, "subscribed", mail: "note_issue_email") - expect_enqueud_email(@u_custom_off.id, note.id, nil, mail: "note_issue_email") - expect_enqueud_email(@unsubscribed_mentioned.id, note.id, "mentioned", mail: "note_issue_email") - end - - it "emails the note author if they've opted into notifications about their activity", :deliver_mails_inline do - note.author.notified_of_own_activity = true - - notification.new_note(note) - - should_email(note.author) - expect(find_email_for(note.author)).to have_header('X-GitLab-NotificationReason', 'own_activity') - end - - it_behaves_like 'project emails are disabled', check_delivery_jobs_queue: true do - let(:notification_target) { note } - let(:notification_trigger) { notification.new_note(note) } - end - end - - it 'filters out "mentioned in" notes' do - mentioned_note = SystemNoteService.cross_reference(mentioned_issue, issue, issue.author) - reset_delivered_emails! - - notification.new_note(mentioned_note) - - expect_no_delivery_jobs - end - - context 'participating' do - context 'by note' do - before do - note.author = @u_lazy_participant - note.save - end - - it { expect { subject }.not_to have_enqueued_email(@u_lazy_participant.id, note.id, mail: "note_issue_email") } - end - end - end - context 'on service desk issue' do before do allow(Notify).to receive(:service_desk_new_note_email) @@ -436,73 +367,158 @@ RSpec.describe NotificationService, :mailer do end end - describe 'new note on issue in project that belongs to a group' do - before do - note.project.namespace_id = group.id - group.add_user(@u_watcher, GroupMember::MAINTAINER) - group.add_user(@u_custom_global, GroupMember::MAINTAINER) - note.project.save + describe '#new_note' do + before_all do + build_team(project) + project.add_maintainer(issue.author) + project.add_maintainer(assignee) + project.add_maintainer(author) + + @u_custom_off = create_user_with_notification(:custom, 'custom_off') + project.add_guest(@u_custom_off) + + create( + :note_on_issue, + author: @u_custom_off, + noteable: issue, + project_id: issue.project_id, + note: 'i think @subscribed_participant should see this' + ) - @u_watcher.notification_settings_for(note.project).participating! - @u_watcher.notification_settings_for(group).global! + update_custom_notification(:new_note, @u_guest_custom, resource: project) update_custom_notification(:new_note, @u_custom_global) - reset_delivered_emails! end - shared_examples 'new note notifications' do - it 'sends notifications', :deliver_mails_inline do + context 'with users' do + before_all do + add_users(project) + add_user_subscriptions(issue) + end + + before do + reset_delivered_emails! + end + + it 'sends emails to recipients', :aggregate_failures do + subject + + expect_delivery_jobs_count(10) + expect_enqueud_email(@u_watcher.id, note.id, nil, mail: "note_issue_email") + expect_enqueud_email(note.noteable.author.id, note.id, nil, mail: "note_issue_email") + expect_enqueud_email(note.noteable.assignees.first.id, note.id, nil, mail: "note_issue_email") + expect_enqueud_email(@u_custom_global.id, note.id, nil, mail: "note_issue_email") + expect_enqueud_email(@u_mentioned.id, note.id, "mentioned", mail: "note_issue_email") + expect_enqueud_email(@subscriber.id, note.id, "subscribed", mail: "note_issue_email") + expect_enqueud_email(@watcher_and_subscriber.id, note.id, "subscribed", mail: "note_issue_email") + expect_enqueud_email(@subscribed_participant.id, note.id, "subscribed", mail: "note_issue_email") + expect_enqueud_email(@u_custom_off.id, note.id, nil, mail: "note_issue_email") + expect_enqueud_email(@unsubscribed_mentioned.id, note.id, "mentioned", mail: "note_issue_email") + end + + it "emails the note author if they've opted into notifications about their activity", :deliver_mails_inline do + note.author.notified_of_own_activity = true + notification.new_note(note) - should_email(note.noteable.author) - should_email(note.noteable.assignees.first) - should_email(@u_mentioned) - should_email(@u_custom_global) - should_not_email(@u_guest_custom) - should_not_email(@u_guest_watcher) - should_not_email(@u_watcher) - should_not_email(note.author) - should_not_email(@u_participating) - should_not_email(@u_disabled) - should_not_email(@u_lazy_participant) + should_email(note.author) + expect(find_email_for(note.author)).to have_header('X-GitLab-NotificationReason', 'own_activity') + end - expect(find_email_for(@u_mentioned)).to have_header('X-GitLab-NotificationReason', 'mentioned') - expect(find_email_for(@u_custom_global)).to have_header('X-GitLab-NotificationReason', '') + it_behaves_like 'project emails are disabled', check_delivery_jobs_queue: true do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } end end - let(:group) { create(:group) } + it 'filters out "mentioned in" notes' do + mentioned_note = SystemNoteService.cross_reference(mentioned_issue, issue, issue.author) + reset_delivered_emails! - it_behaves_like 'new note notifications' + notification.new_note(mentioned_note) - it_behaves_like 'project emails are disabled', check_delivery_jobs_queue: true do - let(:notification_target) { note } - let(:notification_trigger) { notification.new_note(note) } + expect_no_delivery_jobs end - context 'which is a subgroup' do - let!(:parent) { create(:group) } - let!(:group) { create(:group, parent: parent) } + context 'participating' do + context 'by note' do + before do + note.author = @u_lazy_participant + note.save + end - it_behaves_like 'new note notifications' + it { expect { subject }.not_to have_enqueued_email(@u_lazy_participant.id, note.id, mail: "note_issue_email") } + end + end + + context 'in project that belongs to a group' do + let_it_be(:parent_group) { create(:group) } - it 'overrides child objects with global level' do - user = create(:user) - parent.add_developer(user) - user.notification_settings_for(parent).watch! + before do + note.project.namespace_id = group.id + group.add_user(@u_watcher, GroupMember::MAINTAINER) + group.add_user(@u_custom_global, GroupMember::MAINTAINER) + note.project.save + + @u_watcher.notification_settings_for(note.project).participating! + @u_watcher.notification_settings_for(group).global! + update_custom_notification(:new_note, @u_custom_global) reset_delivered_emails! + end - notification.new_note(note) + shared_examples 'new note notifications' do + it 'sends notifications', :deliver_mails_inline do + notification.new_note(note) + + should_email(note.noteable.author) + should_email(note.noteable.assignees.first) + should_email(@u_mentioned) + should_email(@u_custom_global) + should_not_email(@u_guest_custom) + should_not_email(@u_guest_watcher) + should_not_email(@u_watcher) + should_not_email(note.author) + should_not_email(@u_participating) + should_not_email(@u_disabled) + should_not_email(@u_lazy_participant) + + expect(find_email_for(@u_mentioned)).to have_header('X-GitLab-NotificationReason', 'mentioned') + expect(find_email_for(@u_custom_global)).to have_header('X-GitLab-NotificationReason', '') + end + end + + context 'which is a top-level group' do + let!(:group) { parent_group } - expect_enqueud_email(user.id, note.id, nil, mail: "note_issue_email") + it_behaves_like 'new note notifications' + + it_behaves_like 'project emails are disabled', check_delivery_jobs_queue: true do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end + end + + context 'which is a subgroup' do + let!(:group) { create(:group, parent: parent_group) } + + it_behaves_like 'new note notifications' + + it 'overrides child objects with global level' do + user = create(:user) + parent_group.add_developer(user) + user.notification_settings_for(parent_group).watch! + reset_delivered_emails! + + notification.new_note(note) + + expect_enqueud_email(user.id, note.id, nil, mail: "note_issue_email") + end end end end end context 'confidential issue note' do - let(:project) { create(:project, :public) } let(:author) { create(:user) } - let(:assignee) { create(:user) } let(:non_member) { create(:user) } let(:member) { create(:user) } let(:guest) { create(:user) } @@ -556,18 +572,20 @@ RSpec.describe NotificationService, :mailer do end context 'issue note mention', :deliver_mails_inline do - let(:project) { create(:project, :public) } - let(:issue) { create(:issue, project: project, assignees: [assignee]) } - let(:mentioned_issue) { create(:issue, assignees: issue.assignees) } - let(:author) { create(:user) } + let_it_be(:issue) { create(:issue, project: project, assignees: [assignee]) } + let_it_be(:mentioned_issue) { create(:issue, assignees: issue.assignees) } + let_it_be(:author) { create(:user) } let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@all mentioned') } - before do + before_all do build_team(project) build_group(project) add_users(project) add_user_subscriptions(issue) project.add_maintainer(author) + end + + before do reset_delivered_emails! end @@ -622,7 +640,6 @@ RSpec.describe NotificationService, :mailer do end context 'project snippet note', :deliver_mails_inline do - let!(:project) { create(:project, :public) } let(:snippet) { create(:project_snippet, project: project, author: create(:user)) } let(:author) { create(:user) } let(:note) { create(:note_on_project_snippet, author: author, noteable: snippet, project_id: project.id, note: '@all mentioned') } @@ -715,18 +732,21 @@ RSpec.describe NotificationService, :mailer do end context 'commit note', :deliver_mails_inline do - let(:project) { create(:project, :public, :repository) } - let(:note) { create(:note_on_commit, project: project) } + let_it_be(:project) { create(:project, :public, :repository) } + let_it_be(:note) { create(:note_on_commit, project: project) } - before do - build_team(note.project) + before_all do + build_team(project) build_group(project) - reset_delivered_emails! - allow(note.noteable).to receive(:author).and_return(@u_committer) update_custom_notification(:new_note, @u_guest_custom, resource: project) update_custom_notification(:new_note, @u_custom_global) end + before do + reset_delivered_emails! + allow(note.noteable).to receive(:author).and_return(@u_committer) + end + describe '#new_note, #perform_enqueued_jobs' do it do notification.new_note(note) @@ -774,12 +794,12 @@ RSpec.describe NotificationService, :mailer do end context "merge request diff note", :deliver_mails_inline do - let(:project) { create(:project, :repository) } - let(:user) { create(:user) } - let(:merge_request) { create(:merge_request, source_project: project, assignees: [user], author: create(:user)) } - let(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, assignees: [user], author: create(:user)) } + let_it_be(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request) } - before do + before_all do build_team(note.project) project.add_maintainer(merge_request.author) merge_request.assignees.each { |assignee| project.add_maintainer(assignee) } @@ -878,11 +898,11 @@ RSpec.describe NotificationService, :mailer do end describe 'Participating project notification settings have priority over group and global settings if available', :deliver_mails_inline do - let!(:group) { create(:group) } - let!(:maintainer) { group.add_owner(create(:user, username: 'maintainer')).user } - let!(:user1) { group.add_developer(create(:user, username: 'user_with_project_and_custom_setting')).user } + let_it_be(:group) { create(:group) } + let_it_be(:maintainer) { group.add_owner(create(:user, username: 'maintainer')).user } + let_it_be(:user1) { group.add_developer(create(:user, username: 'user_with_project_and_custom_setting')).user } + let_it_be(:project) { create(:project, :public, namespace: group) } - let(:project) { create(:project, :public, namespace: group) } let(:issue) { create :issue, project: project, assignees: [assignee], description: '' } before do @@ -936,20 +956,25 @@ RSpec.describe NotificationService, :mailer do end describe 'Issues', :deliver_mails_inline do - let(:group) { create(:group) } - let(:project) { create(:project, :public, namespace: group) } let(:another_project) { create(:project, :public, namespace: group) } let(:issue) { create :issue, project: project, assignees: [assignee], description: 'cc @participant @unsubscribed_mentioned' } - before do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :public, namespace: group) } + + before_all do build_team(project) build_group(project) - add_users(project) + end + + before do 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) + + issue.author.notified_of_own_activity = false end describe '#new_issue' do @@ -1066,7 +1091,6 @@ RSpec.describe NotificationService, :mailer do context 'confidential issues' do let(:author) { create(:user) } - let(:assignee) { create(:user) } let(:non_member) { create(:user) } let(:member) { create(:user) } let(:guest) { create(:user) } @@ -1272,7 +1296,6 @@ RSpec.describe NotificationService, :mailer do context 'confidential issues' do let(:author) { create(:user) } - let(:assignee) { create(:user) } let(:non_member) { create(:user) } let(:member) { create(:user) } let(:guest) { create(:user) } @@ -1325,7 +1348,6 @@ RSpec.describe NotificationService, :mailer do context 'confidential issues' do let(:author) { create(:user) } - let(:assignee) { create(:user) } let(:non_member) { create(:user) } let(:member) { create(:user) } let(:guest) { create(:user) } @@ -1377,7 +1399,6 @@ RSpec.describe NotificationService, :mailer do context 'confidential issues' do let(:author) { create(:user) } - let(:assignee) { create(:user) } let(:non_member) { create(:user) } let(:member) { create(:user) } let(:guest) { create(:user) } @@ -1571,19 +1592,23 @@ RSpec.describe NotificationService, :mailer do end describe 'Merge Requests', :deliver_mails_inline do - let(:group) { create(:group) } - let(:project) { create(:project, :public, :repository, namespace: group) } let(:another_project) { create(:project, :public, namespace: group) } - let(:assignee) { create(:user) } 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(author) - assignees.each { |assignee| project.add_maintainer(assignee) } + let_it_be_with_reload(:author) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :public, :repository, namespace: group) } + + before_all do build_team(project) add_users(project) + + project.add_maintainer(author) + project.add_maintainer(assignee) + end + + before do 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) @@ -1667,13 +1692,13 @@ RSpec.describe NotificationService, :mailer do end context 'participating' do - it_should_behave_like 'participating by assignee notification' do + it_behaves_like 'participating by assignee notification' do let(:participant) { create(:user, username: 'user-participant')} let(:issuable) { merge_request } let(:notification_trigger) { notification.new_merge_request(merge_request, @u_disabled) } end - it_should_behave_like 'participating by note notification' do + it_behaves_like 'participating by note notification' do let(:participant) { create(:user, username: 'user-participant')} let(:issuable) { merge_request } let(:notification_trigger) { notification.new_merge_request(merge_request, @u_disabled) } @@ -2066,9 +2091,7 @@ RSpec.describe NotificationService, :mailer do end describe 'Projects', :deliver_mails_inline do - let(:project) { create(:project) } - - before do + before_all do build_team(project) reset_delivered_emails! end @@ -2293,7 +2316,6 @@ RSpec.describe NotificationService, :mailer do end describe 'ProjectMember', :deliver_mails_inline do - let(:project) { create(:project) } let(:added_user) { create(:user) } describe '#new_access_request' do @@ -2327,7 +2349,6 @@ RSpec.describe NotificationService, :mailer do end it_behaves_like 'sends notification only to a maximum of ten, most recently active project maintainers' do - let(:project) { create(:project, :public) } let(:notification_trigger) { project.request_access(added_user) } end end @@ -2457,7 +2478,6 @@ RSpec.describe NotificationService, :mailer do let(:private_project) { create(:project, :private) } let(:guest) { create(:user) } let(:developer) { create(:user) } - let(:assignee) { create(:user) } let(:merge_request) { create(:merge_request, source_project: private_project, assignees: [assignee]) } let(:merge_request1) { create(:merge_request, source_project: private_project, assignees: [assignee], description: "cc @#{guest.username}") } let(:note) { create(:note, noteable: merge_request, project: private_project) } @@ -2510,15 +2530,15 @@ RSpec.describe NotificationService, :mailer do describe 'Pipelines', :deliver_mails_inline do describe '#pipeline_finished' do - let(:project) { create(:project, :public, :repository) } - let(:u_member) { create(:user) } - let(:u_watcher) { create_user_with_notification(:watch, 'watcher') } + let_it_be(:project) { create(:project, :public, :repository) } + let_it_be(:u_member) { create(:user) } + let_it_be(:u_watcher) { create_user_with_notification(:watch, 'watcher') } - let(:u_custom_notification_unset) do + let_it_be(:u_custom_notification_unset) do create_user_with_notification(:custom, 'custom_unset') end - let(:u_custom_notification_enabled) do + let_it_be(:u_custom_notification_enabled) do user = create_user_with_notification(:custom, 'custom_enabled') update_custom_notification(:success_pipeline, user, resource: project) update_custom_notification(:failed_pipeline, user, resource: project) @@ -2526,7 +2546,7 @@ RSpec.describe NotificationService, :mailer do user end - let(:u_custom_notification_disabled) do + let_it_be(:u_custom_notification_disabled) do user = create_user_with_notification(:custom, 'custom_disabled') update_custom_notification(:success_pipeline, user, resource: project, value: false) update_custom_notification(:failed_pipeline, user, resource: project, value: false) @@ -2545,13 +2565,15 @@ RSpec.describe NotificationService, :mailer do before_sha: '00000000') end - before do + before_all do project.add_maintainer(u_member) project.add_maintainer(u_watcher) project.add_maintainer(u_custom_notification_unset) project.add_maintainer(u_custom_notification_enabled) project.add_maintainer(u_custom_notification_disabled) + end + before do reset_delivered_emails! end @@ -2889,7 +2911,6 @@ RSpec.describe NotificationService, :mailer do describe 'Repository cleanup', :deliver_mails_inline do let(:user) { create(:user) } - let(:project) { create(:project) } describe '#repository_cleanup_success' do it 'emails the specified user only' do @@ -2920,7 +2941,6 @@ RSpec.describe NotificationService, :mailer do context 'Remote mirror notifications', :deliver_mails_inline do describe '#remote_mirror_update_failed' do - let(:project) { create(:project) } let(:remote_mirror) { create(:remote_mirror, project: project) } let(:u_blocked) { create(:user, :blocked) } let(:u_silence) { create_user_with_notification(:disabled, 'silent-maintainer', project) } @@ -3159,11 +3179,11 @@ RSpec.describe NotificationService, :mailer do end def should_email_nested_group_user(user, times: 1, recipients: email_recipients) - should_email(user, times: 1, recipients: email_recipients) + should_email(user, times: times, recipients: recipients) end def should_not_email_nested_group_user(user, recipients: email_recipients) - should_not_email(user, recipients: email_recipients) + should_not_email(user, recipients: recipients) end def add_users(project) diff --git a/spec/services/packages/composer/create_package_service_spec.rb b/spec/services/packages/composer/create_package_service_spec.rb index 3f9da31cf6e..a1fe9a1b918 100644 --- a/spec/services/packages/composer/create_package_service_spec.rb +++ b/spec/services/packages/composer/create_package_service_spec.rb @@ -37,12 +37,16 @@ RSpec.describe Packages::Composer::CreatePackageService do expect(created_package.composer_metadatum.target_sha).to eq branch.target expect(created_package.composer_metadatum.composer_json.to_json).to eq json end + + it_behaves_like 'assigns the package creator' do + let(:package) { created_package } + end end context 'with a tag' do let(:tag) { project.repository.find_tag('v1.2.3') } - before do + before(:all) do project.repository.add_tag(user, 'v1.2.3', 'master') end @@ -54,6 +58,10 @@ RSpec.describe Packages::Composer::CreatePackageService do expect(created_package.name).to eq package_name expect(created_package.version).to eq '1.2.3' end + + it_behaves_like 'assigns the package creator' do + let(:package) { created_package } + end end end diff --git a/spec/services/packages/conan/create_package_service_spec.rb b/spec/services/packages/conan/create_package_service_spec.rb index f8068f6e57b..b217e570aba 100644 --- a/spec/services/packages/conan/create_package_service_spec.rb +++ b/spec/services/packages/conan/create_package_service_spec.rb @@ -5,9 +5,11 @@ RSpec.describe Packages::Conan::CreatePackageService do let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } - subject { described_class.new(project, user, params) } + subject(:service) { described_class.new(project, user, params) } describe '#execute' do + subject(:package) { service.execute } + context 'valid params' do let(:params) do { @@ -19,8 +21,6 @@ RSpec.describe Packages::Conan::CreatePackageService do end it 'creates a new package' do - package = subject.execute - expect(package).to be_valid expect(package.name).to eq(params[:package_name]) expect(package.version).to eq(params[:package_version]) @@ -28,6 +28,8 @@ RSpec.describe Packages::Conan::CreatePackageService do expect(package.conan_metadatum.package_username).to eq(params[:package_username]) expect(package.conan_metadatum.package_channel).to eq(params[:package_channel]) end + + it_behaves_like 'assigns the package creator' end context 'invalid params' do @@ -41,7 +43,7 @@ RSpec.describe Packages::Conan::CreatePackageService do end it 'fails' do - expect { subject.execute }.to raise_exception(ActiveRecord::RecordInvalid) + expect { package }.to raise_exception(ActiveRecord::RecordInvalid) end end end diff --git a/spec/services/packages/maven/create_package_service_spec.rb b/spec/services/packages/maven/create_package_service_spec.rb index bfdf62008ba..7ec368aa00f 100644 --- a/spec/services/packages/maven/create_package_service_spec.rb +++ b/spec/services/packages/maven/create_package_service_spec.rb @@ -34,6 +34,8 @@ RSpec.describe Packages::Maven::CreatePackageService do end it_behaves_like 'assigns build to package' + + it_behaves_like 'assigns the package creator' end context 'without version' do @@ -57,6 +59,8 @@ RSpec.describe Packages::Maven::CreatePackageService do expect(package.maven_metadatum.app_name).to eq(app_name) expect(package.maven_metadatum.app_version).to be nil end + + it_behaves_like 'assigns the package creator' end context 'path is missing' do diff --git a/spec/services/packages/npm/create_package_service_spec.rb b/spec/services/packages/npm/create_package_service_spec.rb index c1391746f52..c8431c640da 100644 --- a/spec/services/packages/npm/create_package_service_spec.rb +++ b/spec/services/packages/npm/create_package_service_spec.rb @@ -27,6 +27,10 @@ RSpec.describe Packages::Npm::CreatePackageService do .and change { Packages::Tag.count }.by(1) end + it_behaves_like 'assigns the package creator' do + let(:package) { subject } + end + it { is_expected.to be_valid } it 'creates a package with name and version' do @@ -61,6 +65,15 @@ RSpec.describe Packages::Npm::CreatePackageService do it { expect(subject[:message]).to be 'Package already exists.' } end + context 'file size above maximum limit' do + before do + params['_attachments']["#{package_name}-#{version}.tgz"]['length'] = project.actual_limits.npm_max_file_size + 1 + end + + it { expect(subject[:http_status]).to eq 400 } + it { expect(subject[:message]).to be 'File is too large.' } + end + context 'with incorrect namespace' do let(:package_name) { '@my_other_namespace/my-app' } diff --git a/spec/services/packages/nuget/create_package_service_spec.rb b/spec/services/packages/nuget/create_package_service_spec.rb index 1579b42d9ad..e51bc03f311 100644 --- a/spec/services/packages/nuget/create_package_service_spec.rb +++ b/spec/services/packages/nuget/create_package_service_spec.rb @@ -9,9 +9,10 @@ RSpec.describe Packages::Nuget::CreatePackageService do describe '#execute' do subject { described_class.new(project, user, params).execute } + let(:package) { Packages::Package.last } + it 'creates the package' do expect { subject }.to change { Packages::Package.count }.by(1) - package = Packages::Package.last expect(package).to be_valid expect(package.name).to eq(Packages::Nuget::CreatePackageService::TEMPORARY_PACKAGE_NAME) @@ -23,12 +24,12 @@ RSpec.describe Packages::Nuget::CreatePackageService do expect { subject }.to change { Packages::Package.count }.by(1) expect { described_class.new(project, user, params).execute }.to change { Packages::Package.count }.by(1) - package = Packages::Package.last - expect(package).to be_valid expect(package.name).to eq(Packages::Nuget::CreatePackageService::TEMPORARY_PACKAGE_NAME) expect(package.version).to start_with(Packages::Nuget::CreatePackageService::PACKAGE_VERSION) expect(package.package_type).to eq('nuget') end + + it_behaves_like 'assigns the package creator' end end diff --git a/spec/services/packages/pypi/create_package_service_spec.rb b/spec/services/packages/pypi/create_package_service_spec.rb index bfecb32f9ef..c985c1e54ea 100644 --- a/spec/services/packages/pypi/create_package_service_spec.rb +++ b/spec/services/packages/pypi/create_package_service_spec.rb @@ -6,12 +6,14 @@ RSpec.describe Packages::Pypi::CreatePackageService do let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } - let_it_be(:params) do + + let(:requires_python) { '>=2.7' } + let(:params) do { name: 'foo', version: '1.0', content: temp_file('foo.tgz'), - requires_python: '>=2.7', + requires_python: requires_python, sha256_digest: '123', md5_digest: '567' } @@ -37,6 +39,18 @@ RSpec.describe Packages::Pypi::CreatePackageService do end end + context 'with an invalid metadata' do + let(:requires_python) { 'x' * 256 } + + it 'raises an error' do + expect { subject }.to raise_error(ActiveRecord::RecordInvalid) + end + end + + it_behaves_like 'assigns the package creator' do + let(:package) { created_package } + end + context 'with an existing package' do before do described_class.new(project, user, params).execute diff --git a/spec/services/pages/delete_services_spec.rb b/spec/services/pages/delete_services_spec.rb index f6d4694b4dd..440549020a2 100644 --- a/spec/services/pages/delete_services_spec.rb +++ b/spec/services/pages/delete_services_spec.rb @@ -3,25 +3,35 @@ require 'spec_helper' RSpec.describe Pages::DeleteService do - let_it_be(:project) { create(:project, path: "my.project")} - let_it_be(:admin) { create(:admin) } - let_it_be(:domain) { create(:pages_domain, project: project) } - let_it_be(:service) { described_class.new(project, admin)} + shared_examples 'remove pages' do + let_it_be(:project) { create(:project, path: "my.project")} + let_it_be(:admin) { create(:admin) } + let_it_be(:domain) { create(:pages_domain, project: project) } + let_it_be(:service) { described_class.new(project, admin)} - it 'deletes published pages' do - expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true - expect(PagesWorker).to receive(:perform_in).with(5.minutes, :remove, project.namespace.full_path, anything) + it 'deletes published pages' do + expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true + expect(PagesWorker).to receive(:perform_in).with(5.minutes, :remove, project.namespace.full_path, anything) - service.execute + Sidekiq::Testing.inline! { service.execute } - expect(project.reload.pages_metadatum.deployed?).to be(false) - end + expect(project.reload.pages_metadatum.deployed?).to be(false) + end + + it 'deletes all domains' do + expect(project.pages_domains.count).to be 1 - it 'deletes all domains' do - expect(project.pages_domains.count).to be 1 + Sidekiq::Testing.inline! { service.execute } + + expect(project.reload.pages_domains.count).to be 0 + end + end - service.execute + context 'with feature flag enabled' do + before do + expect(PagesRemoveWorker).to receive(:perform_async).and_call_original + end - expect(project.reload.pages_domains.count).to be 0 + it_behaves_like 'remove pages' end end diff --git a/spec/services/product_analytics/build_activity_graph_service_spec.rb b/spec/services/product_analytics/build_activity_graph_service_spec.rb new file mode 100644 index 00000000000..e303656da34 --- /dev/null +++ b/spec/services/product_analytics/build_activity_graph_service_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ProductAnalytics::BuildActivityGraphService do + let_it_be(:project) { create(:project) } + let_it_be(:time_now) { Time.zone.now } + let_it_be(:time_ago) { Time.zone.now - 5.days } + + let_it_be(:events) do + [ + create(:product_analytics_event, project: project, collector_tstamp: time_now), + create(:product_analytics_event, project: project, collector_tstamp: time_now), + create(:product_analytics_event, project: project, collector_tstamp: time_now), + create(:product_analytics_event, project: project, collector_tstamp: time_ago), + create(:product_analytics_event, project: project, collector_tstamp: time_ago) + ] + end + + let(:params) { { timerange: 7 } } + + subject { described_class.new(project, params).execute } + + it 'returns a valid graph hash' do + expected_hash = { + id: 'collector_tstamp', + keys: [time_ago.to_date, time_now.to_date], + values: [2, 3] + } + + expect(subject).to eq(expected_hash) + end +end diff --git a/spec/services/projects/after_rename_service_spec.rb b/spec/services/projects/after_rename_service_spec.rb index 52136b37c66..f03e1ed0e22 100644 --- a/spec/services/projects/after_rename_service_spec.rb +++ b/spec/services/projects/after_rename_service_spec.rb @@ -22,7 +22,6 @@ RSpec.describe Projects::AfterRenameService do # call. This makes testing a bit easier. allow(project).to receive(:gitlab_shell).and_return(gitlab_shell) - stub_feature_flags(skip_hashed_storage_upgrade: false) stub_application_setting(hashed_storage_enabled: false) end @@ -62,13 +61,28 @@ RSpec.describe Projects::AfterRenameService do context 'gitlab pages' do before do - expect(project_storage).to receive(:rename_repo) { true } + allow(project_storage).to receive(:rename_repo) { true } end - it 'moves pages folder to new location' do - expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project) + context 'when the project has pages deployed' do + it 'schedules a move of the pages directory' do + allow(project).to receive(:pages_deployed?).and_return(true) - service_execute + expect(PagesTransferWorker).to receive(:perform_async).with('rename_project', anything) + + service_execute + end + end + + context 'when the project does not have pages deployed' do + it 'does nothing with the pages directory' do + allow(project).to receive(:pages_deployed?).and_return(false) + + expect(PagesTransferWorker).not_to receive(:perform_async) + expect(Gitlab::PagesTransfer).not_to receive(:new) + + service_execute + end end end @@ -126,7 +140,6 @@ RSpec.describe Projects::AfterRenameService do # call. This makes testing a bit easier. allow(project).to receive(:gitlab_shell).and_return(gitlab_shell) - stub_feature_flags(skip_hashed_storage_upgrade: false) stub_application_setting(hashed_storage_enabled: true) end @@ -160,10 +173,25 @@ RSpec.describe Projects::AfterRenameService do end context 'gitlab pages' do - it 'moves pages folder to new location' do - expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project) + context 'when the project has pages deployed' do + it 'schedules a move of the pages directory' do + allow(project).to receive(:pages_deployed?).and_return(true) - service_execute + expect(PagesTransferWorker).to receive(:perform_async).with('rename_project', anything) + + service_execute + end + end + + context 'when the project does not have pages deployed' do + it 'does nothing with the pages directory' do + allow(project).to receive(:pages_deployed?).and_return(false) + + expect(PagesTransferWorker).not_to receive(:perform_async) + expect(Gitlab::PagesTransfer).not_to receive(:new) + + service_execute + end end end diff --git a/spec/services/projects/alerting/notify_service_spec.rb b/spec/services/projects/alerting/notify_service_spec.rb index 3e74a15c3c0..77a0e330109 100644 --- a/spec/services/projects/alerting/notify_service_spec.rb +++ b/spec/services/projects/alerting/notify_service_spec.rb @@ -3,67 +3,32 @@ require 'spec_helper' RSpec.describe Projects::Alerting::NotifyService do - let_it_be(:project, reload: true) { create(:project) } + let_it_be_with_reload(:project) { create(:project, :repository) } before do - # We use `let_it_be(:project)` so we make sure to clear caches - project.clear_memoization(:licensed_feature_available) allow(ProjectServiceWorker).to receive(:perform_async) end - shared_examples 'processes incident issues' do - let(:create_incident_service) { spy } - - before do - allow_any_instance_of(AlertManagement::Alert).to receive(:execute_services) - end - - it 'processes issues' do - expect(IncidentManagement::ProcessAlertWorker) - .to receive(:perform_async) - .with(nil, nil, kind_of(Integer)) - .once - - Sidekiq::Testing.inline! do - expect(subject).to be_success - end - end - end - - shared_examples 'does not process incident issues' do - it 'does not process issues' do - expect(IncidentManagement::ProcessAlertWorker) - .not_to receive(:perform_async) - - expect(subject).to be_success - end - end - - shared_examples 'does not process incident issues due to error' do |http_status:| - it 'does not process issues' do - expect(IncidentManagement::ProcessAlertWorker) - .not_to receive(:perform_async) - - expect(subject).to be_error - expect(subject.http_status).to eq(http_status) - end - end - describe '#execute' do let(:token) { 'invalid-token' } let(:starts_at) { Time.current.change(usec: 0) } let(:fingerprint) { 'testing' } let(:service) { described_class.new(project, nil, payload) } + let_it_be(:environment) { create(:environment, project: project) } + let(:environment) { create(:environment, project: project) } + let(:ended_at) { nil } let(:payload_raw) do { title: 'alert title', start_time: starts_at.rfc3339, + end_time: ended_at&.rfc3339, severity: 'low', monitoring_tool: 'GitLab RSpec', service: 'GitLab Test Suite', description: 'Very detailed description', hosts: ['1.1.1.1', '2.2.2.2'], - fingerprint: fingerprint + fingerprint: fingerprint, + gitlab_environment_name: environment.name }.with_indifferent_access end @@ -72,13 +37,14 @@ RSpec.describe Projects::Alerting::NotifyService do subject { service.execute(token) } context 'with activated Alerts Service' do - let!(:alerts_service) { create(:alerts_service, project: project) } + let_it_be_with_reload(:alerts_service) { create(:alerts_service, project: project) } context 'with valid token' do let(:token) { alerts_service.token } - let(:incident_management_setting) { double(send_email?: email_enabled, create_issue?: issue_enabled) } + let(:incident_management_setting) { double(send_email?: email_enabled, create_issue?: issue_enabled, auto_close_incident?: auto_close_enabled) } let(:email_enabled) { false } let(:issue_enabled) { false } + let(:auto_close_enabled) { false } before do allow(service) @@ -105,9 +71,9 @@ RSpec.describe Projects::Alerting::NotifyService do monitoring_tool: payload_raw.fetch(:monitoring_tool), service: payload_raw.fetch(:service), fingerprint: Digest::SHA1.hexdigest(fingerprint), + environment_id: environment.id, ended_at: nil, - prometheus_alert_id: nil, - environment_id: nil + prometheus_alert_id: nil ) end end @@ -121,12 +87,67 @@ RSpec.describe Projects::Alerting::NotifyService do it_behaves_like 'creates an alert management alert' it_behaves_like 'assigns the alert properties' + it 'creates a system note corresponding to alert creation' do + expect { subject }.to change(Note, :count).by(1) + end + context 'existing alert with same fingerprint' do let(:fingerprint_sha) { Digest::SHA1.hexdigest(fingerprint) } let!(:alert) { create(:alert_management_alert, project: project, fingerprint: fingerprint_sha) } it_behaves_like 'adds an alert management alert event' + context 'end time given' do + let(:ended_at) { Time.current.change(nsec: 0) } + + it 'does not resolve the alert' do + expect { subject }.not_to change { alert.reload.status } + end + + it 'does not set the ended at' do + subject + + expect(alert.reload.ended_at).to be_nil + end + + it_behaves_like 'does not an create alert management alert' + + context 'auto_close_enabled setting enabled' do + let(:auto_close_enabled) { true } + + it 'resolves the alert and sets the end time', :aggregate_failures do + subject + alert.reload + + expect(alert.resolved?).to eq(true) + expect(alert.ended_at).to eql(ended_at) + end + + context 'related issue exists' do + let(:alert) { create(:alert_management_alert, :with_issue, project: project, fingerprint: fingerprint_sha) } + let(:issue) { alert.issue } + + context 'state_tracking is enabled' do + before do + stub_feature_flags(track_resource_state_change_events: true) + end + + it { expect { subject }.to change { issue.reload.state }.from('opened').to('closed') } + it { expect { subject }.to change(ResourceStateEvent, :count).by(1) } + end + + context 'state_tracking is disabled' do + before do + stub_feature_flags(track_resource_state_change_events: false) + end + + it { expect { subject }.to change { issue.reload.state }.from('opened').to('closed') } + it { expect { subject }.to change(Note, :count).by(1) } + end + end + end + end + context 'existing alert is resolved' do let!(:alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: fingerprint_sha) } @@ -148,6 +169,13 @@ RSpec.describe Projects::Alerting::NotifyService do end end + context 'end time given' do + let(:ended_at) { Time.current } + + it_behaves_like 'creates an alert management alert' + it_behaves_like 'assigns the alert properties' + end + context 'with a minimal payload' do let(:payload_raw) do { @@ -183,6 +211,18 @@ RSpec.describe Projects::Alerting::NotifyService do end end + context 'with overlong payload' do + let(:payload_raw) do + { + title: 'a' * Gitlab::Utils::DeepSize::DEFAULT_MAX_SIZE, + start_time: starts_at.rfc3339 + } + end + + it_behaves_like 'does not process incident issues due to error', http_status: :bad_request + it_behaves_like 'does not an create alert management alert' + end + it_behaves_like 'does not process incident issues' context 'issue enabled' do @@ -230,7 +270,9 @@ RSpec.describe Projects::Alerting::NotifyService do end context 'with deactivated Alerts Service' do - let!(:alerts_service) { create(:alerts_service, :inactive, project: project) } + before do + alerts_service.update!(active: false) + end it_behaves_like 'does not process incident issues due to error', http_status: :forbidden it_behaves_like 'does not an create alert management alert' diff --git a/spec/services/projects/container_repository/delete_tags_service_spec.rb b/spec/services/projects/container_repository/delete_tags_service_spec.rb index 3014ccbd7ba..5116427dad2 100644 --- a/spec/services/projects/container_repository/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/delete_tags_service_spec.rb @@ -90,6 +90,10 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do subject { service.execute(repository) } + before do + stub_feature_flags(container_registry_expiration_policies_throttling: false) + end + context 'without permissions' do it { is_expected.to include(status: :error) } end @@ -119,6 +123,18 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do it_behaves_like 'logging a success response' end + + context 'with a timeout error' do + before do + expect_next_instance_of(::Projects::ContainerRepository::Gitlab::DeleteTagsService) do |delete_service| + expect(delete_service).to receive(:delete_tags).and_raise(::Projects::ContainerRepository::Gitlab::DeleteTagsService::TimeoutError) + end + end + + it { is_expected.to include(status: :error, message: 'timeout while deleting tags') } + + it_behaves_like 'logging an error response', message: 'timeout while deleting tags' + end end context 'and the feature is disabled' do diff --git a/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb b/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb index 68c232e5d83..3bbcec8775e 100644 --- a/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb @@ -12,13 +12,21 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService do subject { service.execute } - context 'with tags to delete' do + before do + stub_feature_flags(container_registry_expiration_policies_throttling: false) + end + + RSpec.shared_examples 'deleting tags' do it 'deletes the tags by name' do stub_delete_reference_requests(tags) expect_delete_tag_by_names(tags) is_expected.to eq(status: :success, deleted: tags) end + end + + context 'with tags to delete' do + it_behaves_like 'deleting tags' it 'succeeds when tag delete returns 404' do stub_delete_reference_requests('A' => 200, 'Ba' => 404) @@ -41,6 +49,47 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService do it { is_expected.to eq(status: :error, message: 'could not delete tags') } end end + + context 'with throttling enabled' do + let(:timeout) { 10 } + + before do + stub_feature_flags(container_registry_expiration_policies_throttling: true) + stub_application_setting(container_registry_delete_tags_service_timeout: timeout) + end + + it_behaves_like 'deleting tags' + + context 'with timeout' do + context 'set to a valid value' do + before do + allow(Time.zone).to receive(:now).and_return(10, 15, 25) # third call to Time.zone.now will be triggering the timeout + stub_delete_reference_requests('A' => 200) + end + + it { is_expected.to include(status: :error, message: 'timeout while deleting tags') } + + it 'tracks the exception' do + expect(::Gitlab::ErrorTracking) + .to receive(:track_exception).with(::Projects::ContainerRepository::Gitlab::DeleteTagsService::TimeoutError, tags_count: tags.size, container_repository_id: repository.id) + + subject + end + end + + context 'set to 0' do + let(:timeout) { 0 } + + it_behaves_like 'deleting tags' + end + + context 'set to nil' do + let(:timeout) { nil } + + it_behaves_like 'deleting tags' + end + end + end end context 'with empty tags' do diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 56b19c33ece..a3711c9e17f 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::DestroyService do +RSpec.describe Projects::DestroyService, :aggregate_failures do include ProjectForksHelper let_it_be(:user) { create(:user) } @@ -60,317 +60,353 @@ RSpec.describe Projects::DestroyService do end end - it_behaves_like 'deleting the project' - - it 'invalidates personal_project_count cache' do - expect(user).to receive(:invalidate_personal_projects_count) - - destroy_project(project, user, {}) - end - - context 'when project has remote mirrors' do - let!(:project) do - create(:project, :repository, namespace: user.namespace).tap do |project| - project.remote_mirrors.create(url: 'http://test.com') - end - end + shared_examples 'project destroy' do + it_behaves_like 'deleting the project' - it 'destroys them' do - expect(RemoteMirror.count).to eq(1) + it 'invalidates personal_project_count cache' do + expect(user).to receive(:invalidate_personal_projects_count) destroy_project(project, user, {}) - - expect(RemoteMirror.count).to eq(0) end - end - context 'when project has exports' do - let!(:project_with_export) do - create(:project, :repository, namespace: user.namespace).tap do |project| - create(:import_export_upload, - project: project, - export_file: fixture_file_upload('spec/fixtures/project_export.tar.gz')) + context 'when project has remote mirrors' do + let!(:project) do + create(:project, :repository, namespace: user.namespace).tap do |project| + project.remote_mirrors.create(url: 'http://test.com') + end end - end - it 'destroys project and export' do - expect do - destroy_project(project_with_export, user, {}) - end.to change(ImportExportUpload, :count).by(-1) + it 'destroys them' do + expect(RemoteMirror.count).to eq(1) - expect(Project.all).not_to include(project_with_export) - end - end + destroy_project(project, user, {}) - context 'Sidekiq fake' do - before do - # Dont run sidekiq to check if renamed repository exists - Sidekiq::Testing.fake! { destroy_project(project, user, {}) } + expect(RemoteMirror.count).to eq(0) + end end - it { expect(Project.all).not_to include(project) } - - it do - expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey - end + context 'when project has exports' do + let!(:project_with_export) do + create(:project, :repository, namespace: user.namespace).tap do |project| + create(:import_export_upload, + project: project, + export_file: fixture_file_upload('spec/fixtures/project_export.tar.gz')) + end + end - it do - expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_truthy - end - end + it 'destroys project and export' do + expect do + destroy_project(project_with_export, user, {}) + end.to change(ImportExportUpload, :count).by(-1) - 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: Repositories::DestroyService.name, - container_id: project.id, - disk_path: project.disk_path, - message: 'Gitlab::Git::CommandError').and_call_original + expect(Project.all).not_to include(project_with_export) + end end - it_behaves_like 'deleting the project' - end + context 'Sidekiq fake' do + before do + # Dont run sidekiq to check if renamed repository exists + Sidekiq::Testing.fake! { destroy_project(project, user, {}) } + 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) - allow_any_instance_of(described_class).to receive(:flush_caches).and_raise(::Redis::CannotConnectError) - end + it { expect(Project.all).not_to include(project) } - it 'keeps project team intact upon an error' do - perform_enqueued_jobs do - destroy_project(project, user, {}) - rescue ::Redis::CannotConnectError + it do + expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey end - expect(project.team.members.count).to eq 2 + it do + expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_truthy + end end - end - context 'with async_execute', :sidekiq_inline do - let(:async) { true } - - context 'async delete of project with private issue visibility' do + context 'when flushing caches fail due to Git errors' do before do - project.project_feature.update_attribute("issues_access_level", ProjectFeature::PRIVATE) + allow(project.repository).to receive(:before_delete).and_raise(::Gitlab::Git::CommandError) + allow(Gitlab::GitLogger).to receive(:warn).with( + class: Repositories::DestroyService.name, + container_id: project.id, + disk_path: project.disk_path, + message: 'Gitlab::Git::CommandError').and_call_original end it_behaves_like 'deleting the project' end - it_behaves_like 'deleting the project with pipeline and build' + context 'when flushing caches fail due to Redis' do + before do + new_user = create(:user) + project.team.add_user(new_user, Gitlab::Access::DEVELOPER) + allow_any_instance_of(described_class).to receive(:flush_caches).and_raise(::Redis::CannotConnectError) + end - context 'errors' do - context 'when `remove_legacy_registry_tags` fails' do - before do - expect_any_instance_of(described_class) - .to receive(:remove_legacy_registry_tags).and_return(false) + it 'keeps project team intact upon an error' do + perform_enqueued_jobs do + destroy_project(project, user, {}) + rescue ::Redis::CannotConnectError end - it_behaves_like 'handles errors thrown during async destroy', "Failed to remove some tags" + expect(project.team.members.count).to eq 2 end + end + + context 'with async_execute', :sidekiq_inline do + let(:async) { true } - context 'when `remove_repository` fails' do + context 'async delete of project with private issue visibility' do before do - expect_any_instance_of(described_class) - .to receive(:remove_repository).and_return(false) + project.project_feature.update_attribute("issues_access_level", ProjectFeature::PRIVATE) end - it_behaves_like 'handles errors thrown during async destroy', "Failed to remove project repository" + it_behaves_like 'deleting the project' end - context 'when `execute` raises expected error' do - before do - expect_any_instance_of(Project) - .to receive(:destroy!).and_raise(StandardError.new("Other error message")) + it_behaves_like 'deleting the project with pipeline and build' + + context 'errors' do + context 'when `remove_legacy_registry_tags` fails' do + before do + expect_any_instance_of(described_class) + .to receive(:remove_legacy_registry_tags).and_return(false) + end + + it_behaves_like 'handles errors thrown during async destroy', "Failed to remove some tags" end - it_behaves_like 'handles errors thrown during async destroy', "Other error message" - end + context 'when `remove_repository` fails' do + before do + expect_any_instance_of(described_class) + .to receive(:remove_repository).and_return(false) + end - context 'when `execute` raises unexpected error' do - before do - expect_any_instance_of(Project) - .to receive(:destroy!).and_raise(Exception.new('Other error message')) + it_behaves_like 'handles errors thrown during async destroy', "Failed to remove project repository" end - it 'allows error to bubble up and rolls back project deletion' do - expect do - destroy_project(project, user, {}) - end.to raise_error(Exception, 'Other error message') + context 'when `execute` raises expected error' do + before do + expect_any_instance_of(Project) + .to receive(:destroy!).and_raise(StandardError.new("Other error message")) + end - expect(project.reload.pending_delete).to be(false) - expect(project.delete_error).to include("Other error message") + it_behaves_like 'handles errors thrown during async destroy', "Other error message" end - end - end - end - describe 'container registry' do - context 'when there are regular container repositories' do - let(:container_repository) { create(:container_repository) } + context 'when `execute` raises unexpected error' do + before do + expect_any_instance_of(Project) + .to receive(:destroy!).and_raise(Exception.new('Other error message')) + end - before do - stub_container_registry_tags(repository: project.full_path + '/image', - tags: ['tag']) - project.container_repositories << container_repository + it 'allows error to bubble up and rolls back project deletion' do + expect do + destroy_project(project, user, {}) + end.to raise_error(Exception, 'Other error message') + + expect(project.reload.pending_delete).to be(false) + expect(project.delete_error).to include("Other error message") + end + end end + end - context 'when image repository deletion succeeds' do - it 'removes tags' do - expect_any_instance_of(ContainerRepository) - .to receive(:delete_tags!).and_return(true) + describe 'container registry' do + context 'when there are regular container repositories' do + let(:container_repository) { create(:container_repository) } - destroy_project(project, user) + before do + stub_container_registry_tags(repository: project.full_path + '/image', + tags: ['tag']) + project.container_repositories << container_repository end - end - context 'when image repository deletion fails' do - it 'raises an exception' do - expect_any_instance_of(ContainerRepository) - .to receive(:delete_tags!).and_raise(RuntimeError) + context 'when image repository deletion succeeds' do + it 'removes tags' do + expect_any_instance_of(ContainerRepository) + .to receive(:delete_tags!).and_return(true) - expect(destroy_project(project, user)).to be false + destroy_project(project, user) + end end - end - context 'when registry is disabled' do - before do - stub_container_registry_config(enabled: false) + context 'when image repository deletion fails' do + it 'raises an exception' do + expect_any_instance_of(ContainerRepository) + .to receive(:delete_tags!).and_raise(RuntimeError) + + expect(destroy_project(project, user)).to be false + end end - it 'does not attempting to remove any tags' do - expect(Projects::ContainerRepository::DestroyService).not_to receive(:new) + context 'when registry is disabled' do + before do + stub_container_registry_config(enabled: false) + end - destroy_project(project, user) + it 'does not attempting to remove any tags' do + expect(Projects::ContainerRepository::DestroyService).not_to receive(:new) + + destroy_project(project, user) + end end end - end - context 'when there are tags for legacy root repository' do - before do - stub_container_registry_tags(repository: project.full_path, - tags: ['tag']) - end + context 'when there are tags for legacy root repository' do + before do + stub_container_registry_tags(repository: project.full_path, + tags: ['tag']) + end - context 'when image repository tags deletion succeeds' do - it 'removes tags' do - expect_any_instance_of(ContainerRepository) - .to receive(:delete_tags!).and_return(true) + context 'when image repository tags deletion succeeds' do + it 'removes tags' do + expect_any_instance_of(ContainerRepository) + .to receive(:delete_tags!).and_return(true) - destroy_project(project, user) + destroy_project(project, user) + end end - end - context 'when image repository tags deletion fails' do - it 'raises an exception' do - expect_any_instance_of(ContainerRepository) - .to receive(:delete_tags!).and_return(false) + context 'when image repository tags deletion fails' do + it 'raises an exception' do + expect_any_instance_of(ContainerRepository) + .to receive(:delete_tags!).and_return(false) - expect(destroy_project(project, user)).to be false + expect(destroy_project(project, user)).to be false + end end end end - end - context 'for a forked project with LFS objects' do - let(:forked_project) { fork_project(project, user) } + context 'for a forked project with LFS objects' do + let(:forked_project) { fork_project(project, user) } - before do - project.lfs_objects << create(:lfs_object) - forked_project.reload - end + before do + project.lfs_objects << create(:lfs_object) + forked_project.reload + end - it 'destroys the fork' do - expect { destroy_project(forked_project, user) } - .not_to raise_error + it 'destroys the fork' do + expect { destroy_project(forked_project, user) } + .not_to raise_error + end end - end - context 'as the root of a fork network' do - let!(:fork_1) { fork_project(project, user) } - let!(:fork_2) { fork_project(project, user) } + context 'as the root of a fork network' do + let!(:fork_1) { fork_project(project, user) } + let!(:fork_2) { fork_project(project, user) } - it 'updates the fork network with the project name' do - fork_network = project.fork_network + it 'updates the fork network with the project name' do + fork_network = project.fork_network - destroy_project(project, user) + destroy_project(project, user) - fork_network.reload + fork_network.reload - expect(fork_network.deleted_root_project_name).to eq(project.full_name) - expect(fork_network.root_project).to be_nil + expect(fork_network.deleted_root_project_name).to eq(project.full_name) + expect(fork_network.root_project).to be_nil + end end - end - context 'repository +deleted path removal' do - context 'regular phase' do - it 'schedules +deleted removal of existing repos' do - service = described_class.new(project, user, {}) - allow(service).to receive(:schedule_stale_repos_removal) + context 'repository +deleted path removal' do + context 'regular phase' do + it 'schedules +deleted removal of existing repos' do + service = described_class.new(project, user, {}) + allow(service).to receive(:schedule_stale_repos_removal) - expect(Repositories::ShellDestroyService).to receive(:new).and_call_original - expect(GitlabShellWorker).to receive(:perform_in) - .with(5.minutes, :remove_repository, project.repository_storage, removal_path(project.disk_path)) + expect(Repositories::ShellDestroyService).to receive(:new).and_call_original + expect(GitlabShellWorker).to receive(:perform_in) + .with(5.minutes, :remove_repository, project.repository_storage, removal_path(project.disk_path)) - service.execute + service.execute + end end - end - context 'stale cleanup' do - let(:async) { true } + context 'stale cleanup' do + let(:async) { true } - it 'schedules +deleted wiki and repo removal' do - allow(ProjectDestroyWorker).to receive(:perform_async) + it 'schedules +deleted wiki and repo removal' do + allow(ProjectDestroyWorker).to receive(:perform_async) - expect(Repositories::ShellDestroyService).to receive(:new).with(project.repository).and_call_original - expect(GitlabShellWorker).to receive(:perform_in) - .with(10.minutes, :remove_repository, project.repository_storage, removal_path(project.disk_path)) + expect(Repositories::ShellDestroyService).to receive(:new).with(project.repository).and_call_original + expect(GitlabShellWorker).to receive(:perform_in) + .with(10.minutes, :remove_repository, project.repository_storage, removal_path(project.disk_path)) - expect(Repositories::ShellDestroyService).to receive(:new).with(project.wiki.repository).and_call_original - expect(GitlabShellWorker).to receive(:perform_in) - .with(10.minutes, :remove_repository, project.repository_storage, removal_path(project.wiki.disk_path)) + expect(Repositories::ShellDestroyService).to receive(:new).with(project.wiki.repository).and_call_original + expect(GitlabShellWorker).to receive(:perform_in) + .with(10.minutes, :remove_repository, project.repository_storage, removal_path(project.wiki.disk_path)) - destroy_project(project, user, {}) + destroy_project(project, user, {}) + end end end - end - context 'snippets' do - let!(:snippet1) { create(:project_snippet, project: project, author: user) } - let!(:snippet2) { create(:project_snippet, project: project, author: user) } + context 'snippets' do + let!(:snippet1) { create(:project_snippet, project: project, author: user) } + let!(:snippet2) { create(:project_snippet, project: project, author: user) } - it 'does not include snippets when deleting in batches' do - expect(project).to receive(:destroy_dependent_associations_in_batches).with({ exclude: [:container_repositories, :snippets] }) + it 'does not include snippets when deleting in batches' do + expect(project).to receive(:destroy_dependent_associations_in_batches).with({ exclude: [:container_repositories, :snippets] }) - destroy_project(project, user) - end + destroy_project(project, user) + end - it 'calls the bulk snippet destroy service' do - expect(project.snippets.count).to eq 2 + it 'calls the bulk snippet destroy service' do + expect(project.snippets.count).to eq 2 - expect(Snippets::BulkDestroyService).to receive(:new) - .with(user, project.snippets).and_call_original + expect(Snippets::BulkDestroyService).to receive(:new) + .with(user, project.snippets).and_call_original - expect do - destroy_project(project, user) - end.to change(Snippet, :count).by(-2) - end + expect do + destroy_project(project, user) + end.to change(Snippet, :count).by(-2) + end - context 'when an error is raised deleting snippets' do - it 'does not delete project' do - allow_next_instance_of(Snippets::BulkDestroyService) do |instance| - allow(instance).to receive(:execute).and_return(ServiceResponse.error(message: 'foo')) + context 'when an error is raised deleting snippets' do + it 'does not delete project' do + allow_next_instance_of(Snippets::BulkDestroyService) do |instance| + allow(instance).to receive(:execute).and_return(ServiceResponse.error(message: 'foo')) + end + + expect(destroy_project(project, user)).to be_falsey + expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_truthy end + end + end - expect(destroy_project(project, user)).to be_falsey - expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_truthy + context 'error while destroying', :sidekiq_inline do + let!(:pipeline) { create(:ci_pipeline, project: project) } + let!(:builds) { create_list(:ci_build, 2, :artifacts, pipeline: pipeline) } + let!(:build_trace) { create(:ci_build_trace_chunk, build: builds[0]) } + + it 'deletes on retry' do + # We can expect this to timeout for very large projects + # TODO: remove allow_next_instance_of: https://gitlab.com/gitlab-org/gitlab/-/issues/220440 + allow_any_instance_of(Ci::Build).to receive(:destroy).and_raise('boom') + destroy_project(project, user, {}) + + allow_any_instance_of(Ci::Build).to receive(:destroy).and_call_original + destroy_project(project, user, {}) + + expect(Project.unscoped.all).not_to include(project) + expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey + expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey + expect(project.all_pipelines).to be_empty + expect(project.builds).to be_empty end end end + context 'when project_transactionless_destroy enabled' do + it_behaves_like 'project destroy' + end + + context 'when project_transactionless_destroy disabled', :sidekiq_inline do + before do + stub_feature_flags(project_transactionless_destroy: false) + end + + it_behaves_like 'project destroy' + end + def destroy_project(project, user, params = {}) described_class.new(project, user, params).public_send(async ? :async_execute : :execute) end diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index 925c2ff5d88..166a2dae55b 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Projects::ForkService do it 'flushes the forks count cache of the source project', :clean_gitlab_redis_cache do expect(from_project.forks_count).to be_zero - fork_project(from_project, to_user) + fork_project(from_project, to_user, using_service: true) BatchLoader::Executor.clear_current expect(from_project.forks_count).to eq(1) @@ -40,7 +40,7 @@ RSpec.describe Projects::ForkService do @guest = create(:user) @from_project.add_user(@guest, :guest) end - subject { fork_project(@from_project, @guest) } + subject { fork_project(@from_project, @guest, using_service: true) } it { is_expected.not_to be_persisted } it { expect(subject.errors[:forked_from_project_id]).to eq(['is forbidden']) } @@ -56,7 +56,7 @@ RSpec.describe Projects::ForkService do end describe "successfully creates project in the user namespace" do - let(:to_project) { fork_project(@from_project, @to_user, namespace: @to_user.namespace) } + let(:to_project) { fork_project(@from_project, @to_user, namespace: @to_user.namespace, using_service: true) } it { expect(to_project).to be_persisted } it { expect(to_project.errors).to be_empty } @@ -92,21 +92,21 @@ RSpec.describe Projects::ForkService do end it 'imports the repository of the forked project', :sidekiq_might_not_need_inline do - to_project = fork_project(@from_project, @to_user, repository: true) + to_project = fork_project(@from_project, @to_user, repository: true, using_service: true) expect(to_project.empty_repo?).to be_falsy end end context 'creating a fork of a fork' do - let(:from_forked_project) { fork_project(@from_project, @to_user) } + let(:from_forked_project) { fork_project(@from_project, @to_user, using_service: true) } let(:other_namespace) do group = create(:group) group.add_owner(@to_user) group end - let(:to_project) { fork_project(from_forked_project, @to_user, namespace: other_namespace) } + let(:to_project) { fork_project(from_forked_project, @to_user, namespace: other_namespace, using_service: true) } it 'sets the root of the network to the root project' do expect(to_project.fork_network.root_project).to eq(@from_project) @@ -126,7 +126,7 @@ RSpec.describe Projects::ForkService do context 'project already exists' do it "fails due to validation, not transaction failure" do @existing_project = create(:project, :repository, creator_id: @to_user.id, name: @from_project.name, namespace: @to_namespace) - @to_project = fork_project(@from_project, @to_user, namespace: @to_namespace) + @to_project = fork_project(@from_project, @to_user, namespace: @to_namespace, using_service: true) expect(@existing_project).to be_persisted expect(@to_project).not_to be_persisted @@ -137,7 +137,7 @@ RSpec.describe Projects::ForkService do context 'repository in legacy storage already exists' do let(:fake_repo_path) { File.join(TestEnv.repos_path, @to_user.namespace.full_path, "#{@from_project.path}.git") } - let(:params) { { namespace: @to_user.namespace } } + let(:params) { { namespace: @to_user.namespace, using_service: true } } before do stub_application_setting(hashed_storage_enabled: false) @@ -169,13 +169,13 @@ RSpec.describe Projects::ForkService do context 'GitLab CI is enabled' do it "forks and enables CI for fork" do @from_project.enable_ci - @to_project = fork_project(@from_project, @to_user) + @to_project = fork_project(@from_project, @to_user, using_service: true) expect(@to_project.builds_enabled?).to be_truthy end end context "CI/CD settings" do - let(:to_project) { fork_project(@from_project, @to_user) } + let(:to_project) { fork_project(@from_project, @to_user, using_service: true) } context "when origin has git depth specified" do before do @@ -206,7 +206,7 @@ RSpec.describe Projects::ForkService do end it "creates fork with lowest level" do - forked_project = fork_project(@from_project, @to_user) + forked_project = fork_project(@from_project, @to_user, using_service: true) expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE) end @@ -218,7 +218,7 @@ RSpec.describe Projects::ForkService do end it "creates fork with private visibility levels" do - forked_project = fork_project(@from_project, @to_user) + forked_project = fork_project(@from_project, @to_user, using_service: true) expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE) end @@ -232,7 +232,7 @@ RSpec.describe Projects::ForkService do end it 'fails' do - to_project = fork_project(@from_project, @to_user, namespace: @to_user.namespace) + to_project = fork_project(@from_project, @to_user, namespace: @to_user.namespace, using_service: true) expect(to_project.errors[:forked_from_project_id]).to eq(['is forbidden']) end @@ -253,7 +253,7 @@ RSpec.describe Projects::ForkService do @group.add_user(@developer, GroupMember::DEVELOPER) @project.add_user(@developer, :developer) @project.add_user(@group_owner, :developer) - @opts = { namespace: @group } + @opts = { namespace: @group, using_service: true } end context 'fork project for group' do @@ -299,7 +299,7 @@ RSpec.describe Projects::ForkService do group_owner = create(:user) private_group.add_owner(group_owner) - forked_project = fork_project(public_project, group_owner, namespace: private_group) + forked_project = fork_project(public_project, group_owner, namespace: private_group, using_service: true) expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE) end @@ -310,7 +310,7 @@ RSpec.describe Projects::ForkService do context 'when a project is already forked' do it 'creates a new poolresository after the project is moved to a new shard' do project = create(:project, :public, :repository) - fork_before_move = fork_project(project) + fork_before_move = fork_project(project, nil, using_service: true) # Stub everything required to move a project to a Gitaly shard that does not exist allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original @@ -329,7 +329,7 @@ RSpec.describe Projects::ForkService do destination_storage_name: 'test_second_storage' ) Projects::UpdateRepositoryStorageService.new(storage_move).execute - fork_after_move = fork_project(project.reload) + fork_after_move = fork_project(project.reload, nil, using_service: true) pool_repository_before_move = PoolRepository.joins(:shard) .find_by(source_project: project, shards: { name: 'default' }) pool_repository_after_move = PoolRepository.joins(:shard) @@ -350,7 +350,7 @@ RSpec.describe Projects::ForkService do context 'when no pool exists' do it 'creates a new object pool' do - forked_project = fork_project(fork_from_project, forker) + forked_project = fork_project(fork_from_project, forker, using_service: true) expect(forked_project.pool_repository).to eq(fork_from_project.pool_repository) end @@ -360,7 +360,7 @@ RSpec.describe Projects::ForkService do let!(:pool_repository) { create(:pool_repository, source_project: fork_from_project) } it 'joins the object pool' do - forked_project = fork_project(fork_from_project, forker) + forked_project = fork_project(fork_from_project, forker, using_service: true) expect(forked_project.pool_repository).to eq(fork_from_project.pool_repository) end diff --git a/spec/services/projects/hashed_storage/base_attachment_service_spec.rb b/spec/services/projects/hashed_storage/base_attachment_service_spec.rb index 5e1b6f2e404..969381b8748 100644 --- a/spec/services/projects/hashed_storage/base_attachment_service_spec.rb +++ b/spec/services/projects/hashed_storage/base_attachment_service_spec.rb @@ -30,7 +30,7 @@ RSpec.describe Projects::HashedStorage::BaseAttachmentService do target_path = Dir.mktmpdir expect(Dir.exist?(target_path)).to be_truthy - Timecop.freeze do + freeze_time do suffix = Time.current.utc.to_i subject.send(:discard_path!, target_path) 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 a606371099d..cfe8e863223 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -4,7 +4,8 @@ require 'spec_helper' RSpec.describe Projects::LfsPointers::LfsDownloadService do include StubRequests - let(:project) { create(:project) } + let_it_be(:project) { create(:project) } + let(:lfs_content) { SecureRandom.random_bytes(10) } let(:oid) { Digest::SHA256.hexdigest(lfs_content) } let(:download_link) { "http://gitlab.com/#{oid}" } @@ -14,9 +15,11 @@ RSpec.describe Projects::LfsPointers::LfsDownloadService do subject { described_class.new(project, lfs_object) } - before do + before_all do ApplicationSetting.create_from_defaults + end + before do stub_application_setting(allow_local_requests_from_web_hooks_and_services: local_request_setting) allow(project).to receive(:lfs_enabled?).and_return(true) end @@ -226,5 +229,55 @@ RSpec.describe Projects::LfsPointers::LfsDownloadService do subject.execute end end + + context 'when a large lfs object with the same oid already exists' do + let!(:existing_lfs_object) { create(:lfs_object, :with_file, :correct_oid) } + + before do + stub_const("#{described_class}::LARGE_FILE_SIZE", 500) + stub_full_request(download_link).to_return(body: lfs_content) + end + + context 'and first fragments are the same' do + let(:lfs_content) { existing_lfs_object.file.read } + + context 'when lfs_link_existing_object feature flag disabled' do + before do + stub_feature_flags(lfs_link_existing_object: false) + end + + it 'does not call link_existing_lfs_object!' do + expect(subject).not_to receive(:link_existing_lfs_object!) + + subject.execute + end + end + + it 'returns success' do + expect(subject.execute).to eq({ status: :success }) + end + + it 'links existing lfs object to the project' do + expect { subject.execute } + .to change { project.lfs_objects.include?(existing_lfs_object) }.from(false).to(true) + end + end + + context 'and first fragments diverges' do + let(:lfs_content) { SecureRandom.random_bytes(1000) } + let(:oid) { existing_lfs_object.oid } + + it 'raises oid mismatch error' do + expect(subject.execute).to eq({ + status: :error, + message: "LFS file with oid #{oid} cannot be linked with an existing LFS object" + }) + end + + it 'does not change lfs objects' do + expect { subject.execute }.not_to change { project.lfs_objects } + end + end + end end end diff --git a/spec/services/projects/lfs_pointers/lfs_link_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_link_service_spec.rb index d59f5dbae19..0e7d16f18e8 100644 --- a/spec/services/projects/lfs_pointers/lfs_link_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_link_service_spec.rb @@ -24,11 +24,11 @@ RSpec.describe Projects::LfsPointers::LfsLinkService do end it 'links existing lfs objects to the project' do - expect(project.all_lfs_objects.count).to eq 2 + expect(project.lfs_objects.count).to eq 2 linked = subject.execute(new_oid_list.keys) - expect(project.all_lfs_objects.count).to eq 3 + expect(project.lfs_objects.count).to eq 3 expect(linked.size).to eq 3 end @@ -52,7 +52,7 @@ RSpec.describe Projects::LfsPointers::LfsLinkService do lfs_objects = create_list(:lfs_object, 7) linked = subject.execute(lfs_objects.pluck(:oid)) - expect(project.all_lfs_objects.count).to eq 9 + expect(project.lfs_objects.count).to eq 9 expect(linked.size).to eq 7 end diff --git a/spec/services/projects/open_issues_count_service_spec.rb b/spec/services/projects/open_issues_count_service_spec.rb index c739fea5ecf..294c9adcc92 100644 --- a/spec/services/projects/open_issues_count_service_spec.rb +++ b/spec/services/projects/open_issues_count_service_spec.rb @@ -10,6 +10,14 @@ RSpec.describe Projects::OpenIssuesCountService, :use_clean_rails_memory_store_c it_behaves_like 'a counter caching service' describe '#count' do + it 'does not count test cases' do + create(:issue, :opened, project: project) + create(:incident, :opened, project: project) + create(:quality_test_case, :opened, project: project) + + expect(described_class.new(project).count).to eq(2) + end + context 'when user is nil' do it 'does not include confidential issues in the issue count' do create(:issue, :opened, project: project) diff --git a/spec/services/projects/overwrite_project_service_spec.rb b/spec/services/projects/overwrite_project_service_spec.rb index e4495da9807..a03746d0271 100644 --- a/spec/services/projects/overwrite_project_service_spec.rb +++ b/spec/services/projects/overwrite_project_service_spec.rb @@ -16,6 +16,8 @@ RSpec.describe Projects::OverwriteProjectService do subject { described_class.new(project_to, user) } before do + project_to.project_feature.reload + allow(project_to).to receive(:import_data).and_return(double(data: { 'original_path' => project_from.path })) end diff --git a/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/spec/services/projects/prometheus/alerts/notify_service_spec.rb index efe8e8b9243..0e5ac7c69e3 100644 --- a/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -16,11 +16,6 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do let(:subject) { service.execute(token_input) } - before do - # We use `let_it_be(:project)` so we make sure to clear caches - project.clear_memoization(:licensed_feature_available) - end - context 'with valid payload' do let_it_be(:alert_firing) { create(:prometheus_alert, project: project) } let_it_be(:alert_resolved) { create(:prometheus_alert, project: project) } diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 3362b333c6e..a0e83fb4a21 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -5,8 +5,8 @@ require 'spec_helper' RSpec.describe Projects::TransferService do include GitHelpers - let(:user) { create(:user) } - let(:group) { create(:group) } + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } let(:project) { create(:project, :repository, :legacy_storage, namespace: user.namespace) } subject(:execute_transfer) { described_class.new(project, user).execute(group) } @@ -489,6 +489,29 @@ RSpec.describe Projects::TransferService do end end + context 'moving pages' do + let_it_be(:project) { create(:project, namespace: user.namespace) } + + before do + group.add_owner(user) + end + + it 'schedules a job when pages are deployed' do + project.mark_pages_as_deployed + + expect(PagesTransferWorker).to receive(:perform_async) + .with("move_project", [project.path, user.namespace.full_path, group.full_path]) + + execute_transfer + end + + it 'does not schedule a job when no pages are deployed' do + expect(PagesTransferWorker).not_to receive(:perform_async) + + execute_transfer + end + end + def rugged_config rugged_repo(project.repository).config end diff --git a/spec/services/projects/unlink_fork_service_spec.rb b/spec/services/projects/unlink_fork_service_spec.rb index 6a2c55a5e55..073e2e09397 100644 --- a/spec/services/projects/unlink_fork_service_spec.rb +++ b/spec/services/projects/unlink_fork_service_spec.rb @@ -58,26 +58,6 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin expect(source.forks_count).to be_zero end - context 'when the source has LFS objects' do - let(:lfs_object) { create(:lfs_object) } - - before do - lfs_object.projects << project - end - - it 'links the fork to the lfs object before unlinking' do - subject.execute - - expect(lfs_object.projects).to include(forked_project) - end - - it 'does not fail if the lfs objects were already linked' do - lfs_object.projects << forked_project - - expect { subject.execute }.not_to raise_error - end - end - context 'when the original project was deleted' do it 'does not fail when the original project is deleted' do source = forked_project.forked_from_project @@ -152,24 +132,6 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin expect(project.forks_count).to be_zero end - context 'when given project is a fork of an unlinked parent' do - let!(:fork_of_fork) { fork_project(forked_project, user) } - let(:lfs_object) { create(:lfs_object) } - - before do - lfs_object.projects << project - end - - it 'saves lfs objects to the root project' do - # Remove parent from network - described_class.new(forked_project, user).execute - - described_class.new(fork_of_fork, user).execute - - expect(lfs_object.projects).to include(fork_of_fork) - end - end - context 'and is node with a parent' do subject { described_class.new(forked_project, user) } diff --git a/spec/services/projects/update_pages_configuration_service_spec.rb b/spec/services/projects/update_pages_configuration_service_spec.rb index 9f7ebd40df6..294de813e02 100644 --- a/spec/services/projects/update_pages_configuration_service_spec.rb +++ b/spec/services/projects/update_pages_configuration_service_spec.rb @@ -48,15 +48,6 @@ RSpec.describe Projects::UpdatePagesConfigurationService do expect(subject).to include(status: :success) end end - - context 'when an error occurs' do - it 'returns an error object' do - e = StandardError.new("Failure") - allow(service).to receive(:reload_daemon).and_raise(e) - - expect(subject).to eq(status: :error, message: "Failure", exception: e) - end - end end context 'when pages are not deployed' do diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 374ce4f4ce2..bfb3cbb0131 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -29,8 +29,9 @@ RSpec.describe Projects::UpdatePagesService do context 'for new artifacts' do context "for a valid job" do + let!(:artifacts_archive) { create(:ci_job_artifact, file: file, job: build) } + before do - create(:ci_job_artifact, file: file, job: build) create(:ci_job_artifact, file_type: :metadata, file_format: :gzip, file: metadata, job: build) build.reload @@ -49,6 +50,7 @@ RSpec.describe Projects::UpdatePagesService do expect(project.pages_deployed?).to be_falsey expect(execute).to eq(:success) expect(project.pages_metadatum).to be_deployed + expect(project.pages_metadatum.artifacts_archive).to eq(artifacts_archive) expect(project.pages_deployed?).to be_truthy # Check that all expected files are extracted diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb index 6785b71fcc0..1de04888e0a 100644 --- a/spec/services/projects/update_remote_mirror_service_spec.rb +++ b/spec/services/projects/update_remote_mirror_service_spec.rb @@ -3,9 +3,10 @@ require 'spec_helper' RSpec.describe Projects::UpdateRemoteMirrorService do - let(:project) { create(:project, :repository) } - let(:remote_project) { create(:forked_project_with_submodules) } - let(:remote_mirror) { create(:remote_mirror, project: project, enabled: true) } + let_it_be(:project) { create(:project, :repository, lfs_enabled: true) } + let_it_be(:remote_project) { create(:forked_project_with_submodules) } + let_it_be(:remote_mirror) { create(:remote_mirror, project: project, enabled: true) } + let(:remote_name) { remote_mirror.remote_name } subject(:service) { described_class.new(project, project.creator) } @@ -79,7 +80,6 @@ RSpec.describe Projects::UpdateRemoteMirrorService do with_them do before do allow(remote_mirror).to receive(:url).and_return(url) - allow(service).to receive(:update_mirror).with(remote_mirror).and_return(true) end it "returns expected status" do @@ -128,5 +128,63 @@ RSpec.describe Projects::UpdateRemoteMirrorService do expect(remote_mirror.last_error).to include("refs/heads/develop") end end + + context "sending lfs objects" do + let_it_be(:lfs_pointer) { create(:lfs_objects_project, project: project) } + + before do + stub_lfs_setting(enabled: true) + end + + context 'feature flag enabled' do + before do + stub_feature_flags(push_mirror_syncs_lfs: true) + end + + it 'pushes LFS objects to a HTTP repository' do + expect_next_instance_of(Lfs::PushService) do |service| + expect(service).to receive(:execute) + end + + execute! + end + + it 'does nothing to an SSH repository' do + remote_mirror.update!(url: 'ssh://example.com') + + expect_any_instance_of(Lfs::PushService).not_to receive(:execute) + + execute! + end + + it 'does nothing if LFS is disabled' do + expect(project).to receive(:lfs_enabled?) { false } + + expect_any_instance_of(Lfs::PushService).not_to receive(:execute) + + execute! + end + + it 'does nothing if non-password auth is specified' do + remote_mirror.update!(auth_method: 'ssh_public_key') + + expect_any_instance_of(Lfs::PushService).not_to receive(:execute) + + execute! + end + end + + context 'feature flag disabled' do + before do + stub_feature_flags(push_mirror_syncs_lfs: false) + end + + it 'does nothing' do + expect_any_instance_of(Lfs::PushService).not_to receive(:execute) + + execute! + end + end + end end end diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 4a613f42556..7832d727220 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -272,7 +272,7 @@ RSpec.describe Projects::UpdateService do result = update_project(project, user, project_feature_attributes: { issues_access_level: ProjectFeature::PRIVATE } - ) + ) expect(result).to eq({ status: :success }) expect(project.project_feature.issues_access_level).to be(ProjectFeature::PRIVATE) @@ -325,20 +325,9 @@ RSpec.describe Projects::UpdateService do expect(project.errors.messages[:base]).to include('There is already a repository with that name on disk') end - it 'renames the project without upgrading it' do - result = update_project(project, admin, path: 'new-path') - - expect(result).not_to include(status: :error) - expect(project).to be_valid - expect(project.errors).to be_empty - expect(project.disk_path).to include('new-path') - expect(project.reload.hashed_storage?(:repository)).to be_falsey - end - context 'when hashed storage is enabled' do before do stub_application_setting(hashed_storage_enabled: true) - stub_feature_flags(skip_hashed_storage_upgrade: false) end it 'migrates project to a hashed storage instead of renaming the repo to another legacy name' do @@ -349,22 +338,6 @@ RSpec.describe Projects::UpdateService do expect(project.errors).to be_empty expect(project.reload.hashed_storage?(:repository)).to be_truthy end - - context 'when skip_hashed_storage_upgrade feature flag is enabled' do - before do - stub_feature_flags(skip_hashed_storage_upgrade: true) - end - - it 'renames the project without upgrading it' do - result = update_project(project, admin, path: 'new-path') - - expect(result).not_to include(status: :error) - expect(project).to be_valid - expect(project.errors).to be_empty - expect(project.disk_path).to include('new-path') - expect(project.reload.hashed_storage?(:repository)).to be_falsey - end - end end end @@ -412,32 +385,6 @@ RSpec.describe Projects::UpdateService do subject end - - context 'when `async_update_pages_config` is disabled' do - before do - stub_feature_flags(async_update_pages_config: false) - end - - it 'calls Projects::UpdatePagesConfigurationService when pages are deployed' do - project.mark_pages_as_deployed - - expect(Projects::UpdatePagesConfigurationService) - .to receive(:new) - .with(project) - .and_call_original - - subject - end - - it "does not update pages config when pages aren't deployed" do - project.mark_pages_as_not_deployed - - expect(Projects::UpdatePagesConfigurationService) - .not_to receive(:new) - - subject - end - end end context 'when updating #pages_https_only', :https_pages_enabled do @@ -532,14 +479,14 @@ RSpec.describe Projects::UpdateService do attributes_for(:prometheus_service, project: project, properties: { api_url: "http://new.prometheus.com", manual_configuration: "0" } - ) + ) end let!(:prometheus_service) do create(:prometheus_service, project: project, properties: { api_url: "http://old.prometheus.com", manual_configuration: "0" } - ) + ) end it 'updates existing record' do @@ -556,7 +503,7 @@ RSpec.describe Projects::UpdateService do attributes_for(:prometheus_service, project: project, properties: { api_url: "http://example.prometheus.com", manual_configuration: "0" } - ) + ) end it 'creates new record' do @@ -572,7 +519,7 @@ RSpec.describe Projects::UpdateService do attributes_for(:prometheus_service, project: project, properties: { api_url: nil, manual_configuration: "1" } - ) + ) end it 'does not create new record' do diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 57e32b1aea9..b970a48051f 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -1644,6 +1644,103 @@ RSpec.describe QuickActions::InterpretService do end end end + + context 'relate command' do + let_it_be_with_refind(:group) { create(:group) } + + shared_examples 'relate command' do + it 'relates issues' do + service.execute(content, issue) + + expect(IssueLink.where(source: issue).map(&:target)).to match_array(issues_related) + end + end + + context 'user is member of group' do + before do + group.add_developer(developer) + end + + context 'relate a single issue' do + let(:other_issue) { create(:issue, project: project) } + let(:issues_related) { [other_issue] } + let(:content) { "/relate #{other_issue.to_reference}" } + + it_behaves_like 'relate command' + end + + context 'relate multiple issues at once' do + let(:second_issue) { create(:issue, project: project) } + let(:third_issue) { create(:issue, project: project) } + let(:issues_related) { [second_issue, third_issue] } + let(:content) { "/relate #{second_issue.to_reference} #{third_issue.to_reference}" } + + it_behaves_like 'relate command' + end + + context 'empty relate command' do + let(:issues_related) { [] } + let(:content) { '/relate' } + + it_behaves_like 'relate command' + end + + context 'already having related issues' do + let(:second_issue) { create(:issue, project: project) } + let(:third_issue) { create(:issue, project: project) } + let(:issues_related) { [second_issue, third_issue] } + let(:content) { "/relate #{third_issue.to_reference(project)}" } + + before do + create(:issue_link, source: issue, target: second_issue) + end + + it_behaves_like 'relate command' + end + + context 'cross project' do + let(:another_group) { create(:group, :public) } + let(:other_project) { create(:project, group: another_group) } + + before do + another_group.add_developer(developer) + end + + context 'relate a cross project issue' do + let(:other_issue) { create(:issue, project: other_project) } + let(:issues_related) { [other_issue] } + let(:content) { "/relate #{other_issue.to_reference(project)}" } + + it_behaves_like 'relate command' + end + + context 'relate multiple cross projects issues at once' do + let(:second_issue) { create(:issue, project: other_project) } + let(:third_issue) { create(:issue, project: other_project) } + let(:issues_related) { [second_issue, third_issue] } + let(:content) { "/relate #{second_issue.to_reference(project)} #{third_issue.to_reference(project)}" } + + it_behaves_like 'relate command' + end + + context 'relate a non-existing issue' do + let(:issues_related) { [] } + let(:content) { "/relate imaginary##{non_existing_record_iid}" } + + it_behaves_like 'relate command' + end + + context 'relate a private issue' do + let(:private_project) { create(:project, :private) } + let(:other_issue) { create(:issue, project: private_project) } + let(:issues_related) { [] } + let(:content) { "/relate #{other_issue.to_reference(project)}" } + + it_behaves_like 'relate command' + end + end + end + end end describe '#explain' do diff --git a/spec/services/releases/create_service_spec.rb b/spec/services/releases/create_service_spec.rb index ad4696b0074..90648340b66 100644 --- a/spec/services/releases/create_service_spec.rb +++ b/spec/services/releases/create_service_spec.rb @@ -202,7 +202,7 @@ RSpec.describe Releases::CreateService do let(:last_release) { project.releases.last } around do |example| - Timecop.freeze { example.run } + freeze_time { example.run } end subject { service.execute } diff --git a/spec/services/resource_events/synthetic_milestone_notes_builder_service_spec.rb b/spec/services/resource_events/synthetic_milestone_notes_builder_service_spec.rb index 5e3afeabee7..1b35e224e98 100644 --- a/spec/services/resource_events/synthetic_milestone_notes_builder_service_spec.rb +++ b/spec/services/resource_events/synthetic_milestone_notes_builder_service_spec.rb @@ -6,20 +6,23 @@ RSpec.describe ResourceEvents::SyntheticMilestoneNotesBuilderService do describe '#execute' do let_it_be(:user) { create(:user) } let_it_be(:issue) { create(:issue, author: user) } + let_it_be(:milestone) { create(:milestone, project: issue.project) } - before do - create_list(:resource_milestone_event, 3, issue: issue) - - stub_feature_flags(track_resource_milestone_change_events: false) + let_it_be(:events) do + [ + create(:resource_milestone_event, issue: issue, milestone: milestone, action: :add, created_at: '2020-01-01 04:00'), + create(:resource_milestone_event, issue: issue, milestone: milestone, action: :remove, created_at: '2020-01-02 08:00') + ] end - context 'when resource milestone events are disabled' do - # https://gitlab.com/gitlab-org/gitlab/-/issues/212985 - it 'still builds notes for existing resource milestone events' do - notes = described_class.new(issue, user).execute + it 'builds milestone notes for resource milestone events' do + notes = described_class.new(issue, user).execute - expect(notes.size).to eq(3) - end + expect(notes.map(&:created_at)).to eq(events.map(&:created_at)) + expect(notes.map(&:note)).to eq([ + "changed milestone to %#{milestone.iid}", + 'removed milestone' + ]) end end end diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index 2106a9c2045..b7fb5a98d06 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -313,6 +313,7 @@ RSpec.describe Snippets::CreateService do it_behaves_like 'creates repository and files' it_behaves_like 'after_save callback to store_mentions', ProjectSnippet it_behaves_like 'when snippet_actions param is present' + it_behaves_like 'invalid params error response' context 'when uploaded files are passed to the service' do let(:extra_opts) { { files: ['foo'] } } @@ -340,6 +341,7 @@ RSpec.describe Snippets::CreateService do it_behaves_like 'creates repository and files' it_behaves_like 'after_save callback to store_mentions', PersonalSnippet it_behaves_like 'when snippet_actions param is present' + it_behaves_like 'invalid params error response' context 'when the snippet description contains files' do include FileMoverHelpers diff --git a/spec/services/snippets/update_service_spec.rb b/spec/services/snippets/update_service_spec.rb index 638fe1948fd..641fc56294a 100644 --- a/spec/services/snippets/update_service_spec.rb +++ b/spec/services/snippets/update_service_spec.rb @@ -479,6 +479,22 @@ RSpec.describe Snippets::UpdateService do expect(blob.data).to eq content end end + + context 'when the file_path is not present' do + let(:snippet_actions) { [{ action: :move, previous_path: file_path }] } + + it 'generates the name for the renamed file' do + old_blob = blob(file_path) + + expect(blob('snippetfile1.txt')).to be_nil + expect(subject).to be_success + + new_blob = blob('snippetfile1.txt') + + expect(new_blob).to be_present + expect(new_blob.data).to eq old_blob.data + end + end end context 'delete action' do @@ -682,6 +698,7 @@ RSpec.describe Snippets::UpdateService do it_behaves_like 'when snippet_actions param is present' it_behaves_like 'only file_name is present' it_behaves_like 'only content is present' + it_behaves_like 'invalid params error response' it_behaves_like 'snippets spam check is performed' do before do subject @@ -709,6 +726,7 @@ RSpec.describe Snippets::UpdateService do it_behaves_like 'when snippet_actions param is present' it_behaves_like 'only file_name is present' it_behaves_like 'only content is present' + it_behaves_like 'invalid params error response' it_behaves_like 'snippets spam check is performed' do before do subject diff --git a/spec/services/static_site_editor/config_service_spec.rb b/spec/services/static_site_editor/config_service_spec.rb new file mode 100644 index 00000000000..5fff4e0af53 --- /dev/null +++ b/spec/services/static_site_editor/config_service_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe StaticSiteEditor::ConfigService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + + # params + let(:ref) { double(:ref) } + let(:path) { double(:path) } + let(:return_url) { double(:return_url) } + + # stub data + let(:generated_data) { { generated: true } } + let(:file_data) { { file: true } } + + describe '#execute' do + subject(:execute) do + described_class.new( + container: project, + current_user: user, + params: { + ref: ref, + path: path, + return_url: return_url + } + ).execute + end + + context 'when insufficient permission' do + it 'returns an error' do + expect(execute).to be_error + expect(execute.message).to eq('Insufficient permissions to read configuration') + end + end + + context 'for developer' do + before do + project.add_developer(user) + + allow_next_instance_of(Gitlab::StaticSiteEditor::Config::GeneratedConfig) do |config| + allow(config).to receive(:data) { generated_data } + end + + allow_next_instance_of(Gitlab::StaticSiteEditor::Config::FileConfig) do |config| + allow(config).to receive(:data) { file_data } + end + end + + it 'returns merged generated data and config file data' do + expect(execute).to be_success + expect(execute.payload).to eq(generated: true, file: true) + end + + it 'returns an error if any keys would be overwritten by the merge' do + generated_data[:duplicate_key] = true + file_data[:duplicate_key] = true + expect(execute).to be_error + expect(execute.message).to match(/duplicate key.*duplicate_key.*found/i) + end + end + end +end diff --git a/spec/services/submit_usage_ping_service_spec.rb b/spec/services/submit_usage_ping_service_spec.rb index 450af68d383..2082a163b29 100644 --- a/spec/services/submit_usage_ping_service_spec.rb +++ b/spec/services/submit_usage_ping_service_spec.rb @@ -68,15 +68,15 @@ RSpec.describe SubmitUsagePingService do end end - shared_examples 'saves DevOps score data from the response' do + shared_examples 'saves DevOps report data from the response' do it do expect { subject.execute } - .to change { DevOpsScore::Metric.count } + .to change { DevOpsReport::Metric.count } .by(1) - expect(DevOpsScore::Metric.last.leader_issues).to eq 10.2 - expect(DevOpsScore::Metric.last.instance_issues).to eq 3.2 - expect(DevOpsScore::Metric.last.percentage_issues).to eq 31.37 + expect(DevOpsReport::Metric.last.leader_issues).to eq 10.2 + expect(DevOpsReport::Metric.last.instance_issues).to eq 3.2 + expect(DevOpsReport::Metric.last.percentage_issues).to eq 31.37 end end @@ -123,15 +123,15 @@ RSpec.describe SubmitUsagePingService do stub_response(body: with_conv_index_params) end - it_behaves_like 'saves DevOps score data from the response' + it_behaves_like 'saves DevOps report data from the response' end - context 'when DevOps score data is passed' do + context 'when DevOps report data is passed' do before do stub_response(body: with_dev_ops_score_params) end - it_behaves_like 'saves DevOps score data from the response' + it_behaves_like 'saves DevOps report data from the response' end context 'with save_raw_usage_data feature enabled' do diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 969e5955609..47b8621b5c9 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -74,15 +74,37 @@ RSpec.describe SystemNoteService do end end - describe '.change_milestone' do - let(:milestone) { double } + describe '.relate_issue' do + let(:noteable_ref) { double } + let(:noteable) { double } + + before do + allow(noteable).to receive(:project).and_return(double) + end it 'calls IssuableService' do expect_next_instance_of(::SystemNotes::IssuablesService) do |service| - expect(service).to receive(:change_milestone).with(milestone) + expect(service).to receive(:relate_issue).with(noteable_ref) end - described_class.change_milestone(noteable, project, author, milestone) + described_class.relate_issue(noteable, noteable_ref, double) + end + end + + describe '.unrelate_issue' do + let(:noteable_ref) { double } + let(:noteable) { double } + + before do + allow(noteable).to receive(:project).and_return(double) + end + + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:unrelate_issue).with(noteable_ref) + end + + described_class.unrelate_issue(noteable, noteable_ref, double) end end @@ -313,6 +335,7 @@ RSpec.describe SystemNoteService do let(:success_message) { "SUCCESS: Successfully posted to http://jira.example.net." } before do + stub_jira_service_test stub_jira_urls(jira_issue.id) jira_service_settings end @@ -705,4 +728,17 @@ RSpec.describe SystemNoteService do described_class.new_alert_issue(alert, alert.issue, author) end end + + describe '.create_new_alert' do + let(:alert) { build(:alert_management_alert) } + let(:monitoring_tool) { 'Prometheus' } + + it 'calls AlertManagementService' do + expect_next_instance_of(SystemNotes::AlertManagementService) do |service| + expect(service).to receive(:create_new_alert).with(monitoring_tool) + end + + described_class.create_new_alert(alert, monitoring_tool) + end + end end diff --git a/spec/services/system_notes/alert_management_service_spec.rb b/spec/services/system_notes/alert_management_service_spec.rb index 943d7f55af4..4ebaa54534c 100644 --- a/spec/services/system_notes/alert_management_service_spec.rb +++ b/spec/services/system_notes/alert_management_service_spec.rb @@ -7,6 +7,19 @@ RSpec.describe ::SystemNotes::AlertManagementService do let_it_be(:project) { create(:project, :repository) } let_it_be(:noteable) { create(:alert_management_alert, :with_issue, :acknowledged, project: project) } + describe '#create_new_alert' do + subject { described_class.new(noteable: noteable, project: project).create_new_alert('Some Service') } + + it_behaves_like 'a system note' do + let(:author) { User.alert_bot } + let(:action) { 'new_alert_added' } + end + + it 'has the appropriate message' do + expect(subject.note).to eq('logged an alert from **Some Service**') + end + end + describe '#change_alert_status' do subject { described_class.new(noteable: noteable, project: project, author: author).change_alert_status(noteable) } diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index 1b5b26d90da..fec2a711dc2 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -13,6 +13,38 @@ RSpec.describe ::SystemNotes::IssuablesService do let(:service) { described_class.new(noteable: noteable, project: project, author: author) } + describe '#relate_issue' do + let(:noteable_ref) { create(:issue) } + + subject { service.relate_issue(noteable_ref) } + + it_behaves_like 'a system note' do + let(:action) { 'relate' } + end + + context 'when issue marks another as related' do + it 'sets the note text' do + expect(subject.note).to eq "marked this issue as related to #{noteable_ref.to_reference(project)}" + end + end + end + + describe '#unrelate_issue' do + let(:noteable_ref) { create(:issue) } + + subject { service.unrelate_issue(noteable_ref) } + + it_behaves_like 'a system note' do + let(:action) { 'unrelate' } + end + + context 'when issue relation is removed' do + it 'sets the note text' do + expect(subject.note).to eq "removed the relation with #{noteable_ref.to_reference(project)}" + end + end + end + describe '#change_assignee' do subject { service.change_assignee(assignee) } @@ -96,64 +128,6 @@ RSpec.describe ::SystemNotes::IssuablesService do end end - describe '#change_milestone' do - subject { service.change_milestone(milestone) } - - context 'for a project milestone' do - let(:milestone) { create(:milestone, project: project) } - - it_behaves_like 'a system note' do - let(:action) { 'milestone' } - end - - context 'when milestone added' do - it 'sets the note text' do - reference = milestone.to_reference(format: :iid) - - expect(subject.note).to eq "changed milestone to #{reference}" - end - - it_behaves_like 'a note with overridable created_at' - end - - context 'when milestone removed' do - let(:milestone) { nil } - - it 'sets the note text' do - expect(subject.note).to eq 'removed milestone' - end - - it_behaves_like 'a note with overridable created_at' - end - end - - context 'for a group milestone' do - let(:milestone) { create(:milestone, group: group) } - - it_behaves_like 'a system note' do - let(:action) { 'milestone' } - end - - context 'when milestone added' do - it 'sets the note text to use the milestone name' do - expect(subject.note).to eq "changed milestone to #{milestone.to_reference(format: :name)}" - end - - it_behaves_like 'a note with overridable created_at' - end - - context 'when milestone removed' do - let(:milestone) { nil } - - it 'sets the note text' do - expect(subject.note).to eq 'removed milestone' - end - - it_behaves_like 'a note with overridable created_at' - end - end - end - describe '#change_status' do subject { service.change_status(status, source) } diff --git a/spec/services/task_list_toggle_service_spec.rb b/spec/services/task_list_toggle_service_spec.rb index 276f2ae435e..81f80ee926a 100644 --- a/spec/services/task_list_toggle_service_spec.rb +++ b/spec/services/task_list_toggle_service_spec.rb @@ -119,7 +119,7 @@ RSpec.describe TaskListToggleService do <<-EOT.strip_heredoc > > * [ ] Task 1 > * [x] Task 2 - EOT + EOT markdown_html = parse_markdown(markdown) toggler = described_class.new(markdown, markdown_html, @@ -140,7 +140,7 @@ RSpec.describe TaskListToggleService do * [ ] Task 1 * [x] Task 2 - EOT + EOT markdown_html = parse_markdown(markdown) toggler = described_class.new(markdown, markdown_html, @@ -158,7 +158,7 @@ RSpec.describe TaskListToggleService do <<-EOT.strip_heredoc - - [ ] Task 1 - [x] Task 2 - EOT + EOT markdown_html = parse_markdown(markdown) toggler = described_class.new(markdown, markdown_html, @@ -175,7 +175,7 @@ RSpec.describe TaskListToggleService do <<-EOT.strip_heredoc 1. - [ ] Task 1 - [x] Task 2 - EOT + EOT markdown_html = parse_markdown(markdown) toggler = described_class.new(markdown, markdown_html, diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 94d4b61933d..60903f8f367 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -65,6 +65,40 @@ RSpec.describe TodoService do end end + shared_examples 'reassigned reviewable target' do + context 'with no existing reviewers' do + let(:assigned_reviewers) { [] } + + it 'creates a pending todo for new reviewer' do + target.reviewers = [john_doe] + service.send(described_method, target, author) + + should_create_todo(user: john_doe, target: target, action: Todo::REVIEW_REQUESTED) + end + end + + context 'with an existing reviewer' do + let(:assigned_reviewers) { [john_doe] } + + it 'does not create a todo if unassigned' do + target.reviewers = [] + + should_not_create_any_todo { service.send(described_method, target, author) } + end + + it 'creates a todo if new reviewer is the current user' do + target.reviewers = [john_doe] + service.send(described_method, target, john_doe) + + should_create_todo(user: john_doe, target: target, author: john_doe, action: Todo::REVIEW_REQUESTED) + end + + it 'does not create a todo if already assigned' do + should_not_create_any_todo { service.send(described_method, target, author, [john_doe]) } + end + end + end + describe 'Issues' do let(:issue) { create(:issue, project: project, assignees: [john_doe], author: author, description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } let(:addressed_issue) { create(:issue, project: project, assignees: [john_doe], author: author, description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } @@ -160,6 +194,19 @@ RSpec.describe TodoService do should_create_todo(user: john_doe, target: issue) end end + + context 'issue is an incident' do + let(:issue) { create(:incident, project: project, assignees: [john_doe], author: author) } + + subject do + service.new_issue(issue, author) + should_create_todo(user: john_doe, target: issue, action: Todo::ASSIGNED) + end + + it_behaves_like 'an incident management tracked event', :incident_management_incident_todo do + let(:current_user) { john_doe} + end + end end describe '#update_issue' do @@ -605,6 +652,17 @@ RSpec.describe TodoService do end end + describe '#reassigned_reviewable' do + let(:described_method) { :reassigned_reviewable } + + context 'reviewable is a merge request' do + it_behaves_like 'reassigned reviewable target' do + let(:assigned_reviewers) { [] } + let(:target) { create(:merge_request, source_project: project, author: author, reviewers: assigned_reviewers) } + end + end + end + describe 'Merge Requests' do let(:mr_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } let(:addressed_mr_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } diff --git a/spec/services/two_factor/destroy_service_spec.rb b/spec/services/two_factor/destroy_service_spec.rb new file mode 100644 index 00000000000..3df4d1593c6 --- /dev/null +++ b/spec/services/two_factor/destroy_service_spec.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe TwoFactor::DestroyService do + let_it_be(:current_user) { create(:user) } + + subject { described_class.new(current_user, user: user).execute } + + context 'disabling two-factor authentication' do + shared_examples_for 'does not send notification email' do + context 'notification', :mailer do + it 'does not send a notification' do + perform_enqueued_jobs do + subject + end + + should_not_email(user) + end + end + end + + context 'when the user does not have two-factor authentication enabled' do + let(:user) { current_user } + + it 'returns error' do + expect(subject).to eq( + { + status: :error, + message: 'Two-factor authentication is not enabled for this user' + } + ) + end + + it_behaves_like 'does not send notification email' + end + + context 'when the user has two-factor authentication enabled' do + context 'when the executor is not authorized to disable two-factor authentication' do + context 'disabling the two-factor authentication of another user' do + let(:user) { create(:user, :two_factor) } + + it 'returns error' do + expect(subject).to eq( + { + status: :error, + message: 'You are not authorized to perform this action' + } + ) + end + + it 'does not disable two-factor authentication' do + expect { subject }.not_to change { user.reload.two_factor_enabled? }.from(true) + end + + it_behaves_like 'does not send notification email' + end + end + + context 'when the executor is authorized to disable two-factor authentication' do + shared_examples_for 'disables two-factor authentication' do + it 'returns success' do + expect(subject).to eq({ status: :success }) + end + + it 'disables the two-factor authentication of the user' do + expect { subject }.to change { user.reload.two_factor_enabled? }.from(true).to(false) + end + + context 'notification', :mailer do + it 'sends a notification' do + perform_enqueued_jobs do + subject + end + + should_email(user) + end + end + end + + context 'disabling their own two-factor authentication' do + let(:current_user) { create(:user, :two_factor) } + let(:user) { current_user } + + it_behaves_like 'disables two-factor authentication' + end + + context 'admin disables the two-factor authentication of another user' do + let(:current_user) { create(:admin) } + let(:user) { create(:user, :two_factor) } + + it_behaves_like 'disables two-factor authentication' + end + end + end + end +end diff --git a/spec/services/users/signup_service_spec.rb b/spec/services/users/signup_service_spec.rb index cc234309817..7169401ab34 100644 --- a/spec/services/users/signup_service_spec.rb +++ b/spec/services/users/signup_service_spec.rb @@ -48,12 +48,27 @@ RSpec.describe Users::SignupService do expect(user.reload.setup_for_company).to be(false) end - it 'returns an error result when setup_for_company is missing' do - result = update_user(user, setup_for_company: '') + context 'when on .com' do + before do + allow(Gitlab).to receive(:com?).and_return(true) + end - expect(user.reload.setup_for_company).not_to be_blank - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq("Setup for company can't be blank") + it 'returns an error result when setup_for_company is missing' do + result = update_user(user, setup_for_company: '') + + expect(user.reload.setup_for_company).not_to be_blank + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq("Setup for company can't be blank") + end + end + + context 'when not on .com' do + it 'returns success when setup_for_company is blank' do + result = update_user(user, setup_for_company: '') + + expect(result).to eq(status: :success) + expect(user.reload.setup_for_company).to be(nil) + end end end diff --git a/spec/services/webauthn/authenticate_service_spec.rb b/spec/services/webauthn/authenticate_service_spec.rb new file mode 100644 index 00000000000..61f64f24f5e --- /dev/null +++ b/spec/services/webauthn/authenticate_service_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'webauthn/fake_client' + +RSpec.describe Webauthn::AuthenticateService do + let(:client) { WebAuthn::FakeClient.new(origin) } + let(:user) { create(:user) } + let(:challenge) { Base64.strict_encode64(SecureRandom.random_bytes(32)) } + + let(:origin) { 'http://localhost' } + + before do + create_result = client.create(challenge: challenge) # rubocop:disable Rails/SaveBang + + webauthn_credential = WebAuthn::Credential.from_create(create_result) + + registration = WebauthnRegistration.new(credential_xid: Base64.strict_encode64(webauthn_credential.raw_id), + public_key: webauthn_credential.public_key, + counter: 0, + name: 'name', + user_id: user.id) + registration.save! + end + + describe '#execute' do + it 'returns true if the response is valid and a matching stored credential is present' do + get_result = client.get(challenge: challenge) + + get_result['clientExtensionResults'] = {} + service = Webauthn::AuthenticateService.new(user, get_result.to_json, challenge) + + expect(service.execute).to be_truthy + end + + it 'returns false if the response is valid but no matching stored credential is present' do + other_client = WebAuthn::FakeClient.new(origin) + other_client.create(challenge: challenge) # rubocop:disable Rails/SaveBang + + get_result = other_client.get(challenge: challenge) + + get_result['clientExtensionResults'] = {} + service = Webauthn::AuthenticateService.new(user, get_result.to_json, challenge) + + expect(service.execute).to be_falsey + end + end +end diff --git a/spec/services/webauthn/register_service_spec.rb b/spec/services/webauthn/register_service_spec.rb new file mode 100644 index 00000000000..bb9fa2080d2 --- /dev/null +++ b/spec/services/webauthn/register_service_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'webauthn/fake_client' + +RSpec.describe Webauthn::RegisterService do + let(:client) { WebAuthn::FakeClient.new(origin) } + let(:user) { create(:user) } + let(:challenge) { Base64.strict_encode64(SecureRandom.random_bytes(32)) } + + let(:origin) { 'http://localhost' } + + describe '#execute' do + it 'returns a registration if challenge matches' do + create_result = client.create(challenge: challenge) # rubocop:disable Rails/SaveBang + webauthn_credential = WebAuthn::Credential.from_create(create_result) + + params = { device_response: create_result.to_json, name: 'abc' } + service = Webauthn::RegisterService.new(user, params, challenge) + + registration = service.execute + expect(registration.credential_xid).to eq(Base64.strict_encode64(webauthn_credential.raw_id)) + expect(registration.errors.size).to eq(0) + end + + it 'returns an error if challenge does not match' do + create_result = client.create(challenge: Base64.strict_encode64(SecureRandom.random_bytes(16))) # rubocop:disable Rails/SaveBang + + params = { device_response: create_result.to_json, name: 'abc' } + service = Webauthn::RegisterService.new(user, params, challenge) + + registration = service.execute + expect(registration.errors.size).to eq(1) + end + end +end |