diff options
Diffstat (limited to 'spec/services')
127 files changed, 3409 insertions, 1082 deletions
diff --git a/spec/services/admin/propagate_integration_service_spec.rb b/spec/services/admin/propagate_integration_service_spec.rb index 5df4d9db8b1..13320528e4f 100644 --- a/spec/services/admin/propagate_integration_service_spec.rb +++ b/spec/services/admin/propagate_integration_service_spec.rb @@ -18,6 +18,7 @@ RSpec.describe Admin::PropagateIntegrationService do let_it_be(:inherited_integration) do create(:jira_service, project: create(:project), inherit_from_id: instance_integration.id) end + let_it_be(:different_type_inherited_integration) do create(:redmine_service, project: project, inherit_from_id: instance_integration.id) end @@ -67,7 +68,7 @@ RSpec.describe Admin::PropagateIntegrationService do end end - context 'with a group without integration' do + context 'with a subgroup without integration' do let(:subgroup) { create(:group, parent: group) } it 'calls to PropagateIntegrationGroupWorker' do @@ -77,6 +78,18 @@ RSpec.describe Admin::PropagateIntegrationService do described_class.propagate(group_integration) end end + + context 'with a subgroup with integration' do + let(:subgroup) { create(:group, parent: group) } + let(:subgroup_integration) { create(:jira_service, group: subgroup, project: nil, inherit_from_id: group_integration.id) } + + it 'calls to PropagateIntegrationInheritDescendantWorker' do + expect(PropagateIntegrationInheritDescendantWorker).to receive(:perform_async) + .with(group_integration.id, subgroup_integration.id, subgroup_integration.id) + + described_class.propagate(group_integration) + end + end end end end diff --git a/spec/services/alert_management/create_alert_issue_service_spec.rb b/spec/services/alert_management/create_alert_issue_service_spec.rb index f2be317a13d..2834322be7b 100644 --- a/spec/services/alert_management/create_alert_issue_service_spec.rb +++ b/spec/services/alert_management/create_alert_issue_service_spec.rb @@ -12,6 +12,7 @@ RSpec.describe AlertManagement::CreateAlertIssueService do 'generatorURL' => 'http://8d467bd4607a:9090/graph?g0.expr=vector%281%29&g0.tab=1' } end + let_it_be(:generic_alert, reload: true) { create(:alert_management_alert, :triggered, project: project, payload: payload) } let_it_be(:prometheus_alert, reload: true) { create(:alert_management_alert, :triggered, :prometheus, project: project, payload: payload) } let(:alert) { generic_alert } diff --git a/spec/services/alert_management/http_integrations/create_service_spec.rb b/spec/services/alert_management/http_integrations/create_service_spec.rb new file mode 100644 index 00000000000..ac5c62caf84 --- /dev/null +++ b/spec/services/alert_management/http_integrations/create_service_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AlertManagement::HttpIntegrations::CreateService do + let_it_be(:user_with_permissions) { create(:user) } + let_it_be(:user_without_permissions) { create(:user) } + let_it_be_with_reload(:project) { create(:project) } + + let(:current_user) { user_with_permissions } + let(:params) { {} } + + let(:service) { described_class.new(project, current_user, params) } + + before_all do + project.add_maintainer(user_with_permissions) + end + + describe '#execute' do + shared_examples 'error response' do |message| + it 'has an informative message' do + expect(response).to be_error + expect(response.message).to eq(message) + end + end + + subject(:response) { service.execute } + + context 'when the current_user is anonymous' do + let(:current_user) { nil } + + it_behaves_like 'error response', 'You have insufficient permissions to create an HTTP integration for this project' + end + + context 'when current_user does not have permission to create integrations' do + let(:current_user) { user_without_permissions } + + it_behaves_like 'error response', 'You have insufficient permissions to create an HTTP integration for this project' + end + + context 'when an integration already exists' do + let_it_be(:existing_integration) { create(:alert_management_http_integration, project: project) } + + it_behaves_like 'error response', 'Multiple HTTP integrations are not supported for this project' + end + + context 'when an error occurs during update' do + it_behaves_like 'error response', "Name can't be blank" + end + + context 'with valid params' do + let(:params) { { name: 'New HTTP Integration', active: true } } + + it 'successfully creates an integration' do + expect(response).to be_success + + integration = response.payload[:integration] + expect(integration).to be_a(::AlertManagement::HttpIntegration) + expect(integration.name).to eq('New HTTP Integration') + expect(integration).to be_active + expect(integration.token).to be_present + expect(integration.endpoint_identifier).to be_present + end + end + end +end diff --git a/spec/services/alert_management/http_integrations/destroy_service_spec.rb b/spec/services/alert_management/http_integrations/destroy_service_spec.rb new file mode 100644 index 00000000000..cd949d728de --- /dev/null +++ b/spec/services/alert_management/http_integrations/destroy_service_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AlertManagement::HttpIntegrations::DestroyService do + let_it_be(:user_with_permissions) { create(:user) } + let_it_be(:user_without_permissions) { create(:user) } + let_it_be(:project) { create(:project) } + + let!(:integration) { create(:alert_management_http_integration, project: project) } + let(:current_user) { user_with_permissions } + let(:params) { {} } + let(:service) { described_class.new(integration, current_user) } + + before_all do + project.add_maintainer(user_with_permissions) + end + + describe '#execute' do + shared_examples 'error response' do |message| + it 'has an informative message' do + expect(response).to be_error + expect(response.message).to eq(message) + end + end + + subject(:response) { service.execute } + + context 'when the current_user is anonymous' do + let(:current_user) { nil } + + it_behaves_like 'error response', 'You have insufficient permissions to remove this HTTP integration' + end + + context 'when current_user does not have permission to create integrations' do + let(:current_user) { user_without_permissions } + + it_behaves_like 'error response', 'You have insufficient permissions to remove this HTTP integration' + end + + context 'when an error occurs during removal' do + before do + allow(integration).to receive(:destroy).and_return(false) + integration.errors.add(:name, 'cannot be removed') + end + + it_behaves_like 'error response', 'Name cannot be removed' + end + + it 'successfully returns the integration' do + expect(response).to be_success + + integration_result = response.payload[:integration] + expect(integration_result).to be_a(::AlertManagement::HttpIntegration) + expect(integration_result.name).to eq(integration.name) + expect(integration_result.active).to eq(integration.active) + expect(integration_result.token).to eq(integration.token) + expect(integration_result.endpoint_identifier).to eq(integration.endpoint_identifier) + + expect { integration.reload }.to raise_error ActiveRecord::RecordNotFound + end + end +end diff --git a/spec/services/alert_management/http_integrations/update_service_spec.rb b/spec/services/alert_management/http_integrations/update_service_spec.rb new file mode 100644 index 00000000000..94c34d9a29c --- /dev/null +++ b/spec/services/alert_management/http_integrations/update_service_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AlertManagement::HttpIntegrations::UpdateService do + let_it_be(:user_with_permissions) { create(:user) } + let_it_be(:user_without_permissions) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be_with_reload(:integration) { create(:alert_management_http_integration, :inactive, project: project, name: 'Old Name') } + + let(:current_user) { user_with_permissions } + let(:params) { {} } + + let(:service) { described_class.new(integration, current_user, params) } + + before_all do + project.add_maintainer(user_with_permissions) + end + + describe '#execute' do + shared_examples 'error response' do |message| + it 'has an informative message' do + expect(response).to be_error + expect(response.message).to eq(message) + end + end + + subject(:response) { service.execute } + + context 'when the current_user is anonymous' do + let(:current_user) { nil } + + it_behaves_like 'error response', 'You have insufficient permissions to update this HTTP integration' + end + + context 'when current_user does not have permission to create integrations' do + let(:current_user) { user_without_permissions } + + it_behaves_like 'error response', 'You have insufficient permissions to update this HTTP integration' + end + + context 'when an error occurs during update' do + let(:params) { { name: '' } } + + it_behaves_like 'error response', "Name can't be blank" + end + + context 'with name param' do + let(:params) { { name: 'New Name' } } + + it 'successfully updates the integration' do + expect(response).to be_success + expect(response.payload[:integration].name).to eq('New Name') + end + end + + context 'with active param' do + let(:params) { { active: true } } + + it 'successfully updates the integration' do + expect(response).to be_success + expect(response.payload[:integration]).to be_active + end + end + + context 'with regenerate_token flag' do + let(:params) { { regenerate_token: true } } + + it 'successfully updates the integration' do + previous_token = integration.token + + expect(response).to be_success + expect(response.payload[:integration].token).not_to eq(previous_token) + end + end + end +end 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 ae0b8d6d7ac..2f920de7fc7 100644 --- a/spec/services/alert_management/process_prometheus_alert_service_spec.rb +++ b/spec/services/alert_management/process_prometheus_alert_service_spec.rb @@ -11,9 +11,16 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do describe '#execute' do 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 } + let(:send_email) { true } + let(:incident_management_setting) do + double( + auto_close_incident?: auto_close_incident, + create_issue?: create_issue, + send_email?: send_email + ) + end before do allow(service) @@ -55,6 +62,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do it_behaves_like 'adds an alert management alert event' it_behaves_like 'processes incident issues' + it_behaves_like 'Alert Notification Service sends notification email' context 'existing alert is resolved' do let!(:alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: fingerprint) } @@ -92,28 +100,48 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do end end - context 'when auto-alert creation is disabled' do + context 'when auto-creation of issues is disabled' do let(:create_issue) { false } it_behaves_like 'does not process incident issues' end + + context 'when emails are disabled' do + let(:send_email) { false } + + it 'does not send notification' do + expect(NotificationService).not_to receive(:new) + + expect(subject).to be_success + end + 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_behaves_like 'Alert Notification Service sends notification email' + it_behaves_like 'processes incident issues' 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' - context 'when auto-alert creation is disabled' do let(:create_issue) { false } it_behaves_like 'does not process incident issues' end + + context 'when emails are disabled' do + let(:send_email) { false } + + it 'does not send notification' do + expect(NotificationService).not_to receive(:new) + + expect(subject).to be_success + end + end end context 'when alert cannot be created' do @@ -125,6 +153,9 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do allow(service).to receive_message_chain(:alert, :errors).and_return(errors) end + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :bad_request + it_behaves_like 'does not process incident issues due to error', http_status: :bad_request + it 'writes a warning to the log' do expect(Gitlab::AppLogger).to receive(:warn).with( message: 'Unable to create AlertManagement::Alert', @@ -134,8 +165,6 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do execute end - - it_behaves_like 'does not process incident issues' end it { is_expected.to be_success } @@ -148,6 +177,9 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do context 'when auto_resolve_incident set to true' do context 'when status can be changed' do + it_behaves_like 'Alert Notification Service sends notification email' + it_behaves_like 'does not process incident issues' + it 'resolves an existing alert' do expect { execute }.to change { alert.reload.resolved? }.to(true) end @@ -185,6 +217,8 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do execute end + + it_behaves_like 'Alert Notification Service sends notification email' end it { is_expected.to be_success } @@ -197,6 +231,16 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do expect { execute }.not_to change { alert.reload.resolved? } end end + + context 'when emails are disabled' do + let(:send_email) { false } + + it 'does not send notification' do + expect(NotificationService).not_to receive(:new) + + expect(subject).to be_success + end + end end context 'environment given' do diff --git a/spec/services/alert_management/sync_alert_service_data_service_spec.rb b/spec/services/alert_management/sync_alert_service_data_service_spec.rb new file mode 100644 index 00000000000..ecec60011db --- /dev/null +++ b/spec/services/alert_management/sync_alert_service_data_service_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AlertManagement::SyncAlertServiceDataService do + let_it_be(:alerts_service) do + AlertsService.skip_callback(:save, :after, :update_http_integration) + service = create(:alerts_service, :active) + AlertsService.set_callback(:save, :after, :update_http_integration) + + service + end + + describe '#execute' do + subject(:execute) { described_class.new(alerts_service).execute } + + context 'without http integration' do + it 'creates the integration' do + expect { execute } + .to change { AlertManagement::HttpIntegration.count }.by(1) + end + + it 'returns a success' do + expect(subject.success?).to eq(true) + end + end + + context 'existing legacy http integration' do + let_it_be(:integration) { create(:alert_management_http_integration, :legacy, project: alerts_service.project) } + + it 'updates the integration' do + expect { execute } + .to change { integration.reload.encrypted_token }.to(alerts_service.data.encrypted_token) + .and change { integration.encrypted_token_iv }.to(alerts_service.data.encrypted_token_iv) + end + + it 'returns a success' do + expect(subject.success?).to eq(true) + end + end + + context 'existing other http integration' do + let_it_be(:integration) { create(:alert_management_http_integration, project: alerts_service.project) } + + it 'creates the integration' do + expect { execute } + .to change { AlertManagement::HttpIntegration.count }.by(1) + end + + it 'returns a success' do + expect(subject.success?).to eq(true) + end + end + end +end diff --git a/spec/services/application_settings/update_service_spec.rb b/spec/services/application_settings/update_service_spec.rb index e5060fa2eeb..03e55b3ec46 100644 --- a/spec/services/application_settings/update_service_spec.rb +++ b/spec/services/application_settings/update_service_spec.rb @@ -66,7 +66,7 @@ RSpec.describe ApplicationSettings::UpdateService do context 'when params is blank' do let(:params) { {} } - it 'does not add to whitelist' do + it 'does not add to allowlist' do expect { subject.execute }.not_to change { application_settings.outbound_local_requests_whitelist } @@ -80,7 +80,7 @@ RSpec.describe ApplicationSettings::UpdateService do let(:params) { { add_to_outbound_local_requests_whitelist: ['example.com', ''] } } - it 'adds to whitelist' do + it 'adds to allowlist' do expect { subject.execute }.to change { application_settings.outbound_local_requests_whitelist } @@ -91,14 +91,14 @@ RSpec.describe ApplicationSettings::UpdateService do end end - context 'when param outbound_local_requests_whitelist_raw is passed' do + context 'when param outbound_local_requests_allowlist_raw is passed' do before do application_settings.outbound_local_requests_whitelist = ['127.0.0.1'] end - let(:params) { { outbound_local_requests_whitelist_raw: 'example.com;gitlab.com' } } + let(:params) { { outbound_local_requests_allowlist_raw: 'example.com;gitlab.com' } } - it 'overwrites the existing whitelist' do + it 'overwrites the existing allowlist' do expect { subject.execute }.to change { application_settings.outbound_local_requests_whitelist } diff --git a/spec/services/audit_event_service_spec.rb b/spec/services/audit_event_service_spec.rb index 3317fcf8444..997f506c269 100644 --- a/spec/services/audit_event_service_spec.rb +++ b/spec/services/audit_event_service_spec.rb @@ -3,17 +3,13 @@ require 'spec_helper' RSpec.describe AuditEventService do - let(:project) { create(:project) } - let(:user) { create(:user, :with_sign_ins) } - let(:project_member) { create(:project_member, user: user) } + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user, :with_sign_ins) } + let_it_be(:project_member) { create(:project_member, user: user) } let(:service) { described_class.new(user, project, { action: :destroy }) } let(:logger) { instance_double(Gitlab::AuditJsonLogger) } describe '#security_event' do - before do - stub_licensed_features(admin_audit_log: false) - end - it 'creates an event and logs to a file' do expect(service).to receive(:file_logger).and_return(logger) expect(logger).to receive(:info).with(author_id: user.id, @@ -78,6 +74,31 @@ RSpec.describe AuditEventService do audit_service.for_authentication.security_event end + + context 'with IP address', :request_store do + using RSpec::Parameterized::TableSyntax + + where(:from_caller, :from_context, :from_author_sign_in, :output) do + '192.168.0.1' | '192.168.0.2' | '192.168.0.3' | '192.168.0.1' + nil | '192.168.0.2' | '192.168.0.3' | '192.168.0.2' + nil | nil | '192.168.0.3' | '192.168.0.3' + end + + with_them do + let(:user) { create(:user, current_sign_in_ip: from_author_sign_in) } + let(:audit_service) { described_class.new(user, user, with: 'standard', ip_address: from_caller) } + + before do + allow(Gitlab::RequestContext.instance).to receive(:client_ip).and_return(from_context) + end + + specify do + expect(AuthenticationEvent).to receive(:new).with(hash_including(ip_address: output)) + + audit_service.for_authentication.security_event + end + end + end end end diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb index bc85f4f0087..90ef32f1c5c 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Auth::ContainerRegistryAuthenticationService do + include AdminModeHelper + let(:current_project) { nil } let(:current_user) { nil } let(:current_params) { {} } @@ -135,7 +137,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end describe '#full_access_token' do - let(:project) { create(:project) } + let_it_be(:project) { create(:project) } let(:token) { described_class.full_access_token(project.full_path) } subject { { token: token } } @@ -148,7 +150,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end describe '#pull_access_token' do - let(:project) { create(:project) } + let_it_be(:project) { create(:project) } let(:token) { described_class.pull_access_token(project.full_path) } subject { { token: token } } @@ -161,7 +163,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'user authorization' do - let(:current_user) { create(:user) } + let_it_be(:current_user) { create(:user) } context 'for registry catalog' do let(:current_params) do @@ -175,14 +177,14 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for private project' do - let(:project) { create(:project) } + let_it_be(:project) { create(:project) } context 'allow to use scope-less authentication' do it_behaves_like 'a valid token' end context 'allow developer to push images' do - before do + before_all do project.add_developer(current_user) end @@ -195,7 +197,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'disallow developer to delete images' do - before do + before_all do project.add_developer(current_user) end @@ -222,7 +224,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'disallow developer to delete images since registry 2.7' do - before do + before_all do project.add_developer(current_user) end @@ -235,7 +237,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'allow reporter to pull images' do - before do + before_all do project.add_reporter(current_user) end @@ -250,7 +252,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'disallow reporter to delete images' do - before do + before_all do project.add_reporter(current_user) end @@ -263,7 +265,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'disallow reporter to delete images since registry 2.7' do - before do + before_all do project.add_reporter(current_user) end @@ -276,7 +278,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'return a least of privileges' do - before do + before_all do project.add_reporter(current_user) end @@ -289,7 +291,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'disallow guest to pull or push images' do - before do + before_all do project.add_guest(current_user) end @@ -302,7 +304,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'disallow guest to delete images' do - before do + before_all do project.add_guest(current_user) end @@ -315,7 +317,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'disallow guest to delete images since registry 2.7' do - before do + before_all do project.add_guest(current_user) end @@ -329,7 +331,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for public project' do - let(:project) { create(:project, :public) } + let_it_be(:project) { create(:project, :public) } context 'allow anyone to pull images' do let(:current_params) do @@ -378,7 +380,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for internal project' do - let(:project) { create(:project, :internal) } + let_it_be(:project) { create(:project, :internal) } context 'for internal user' do context 'allow anyone to pull images' do @@ -420,7 +422,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do context 'for external user' do context 'disallow anyone to pull or push images' do - let(:current_user) { create(:user, external: true) } + let_it_be(:current_user) { create(:user, external: true) } let(:current_params) do { scopes: ["repository:#{project.full_path}:pull,push"] } end @@ -430,7 +432,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'disallow anyone to delete images' do - let(:current_user) { create(:user, external: true) } + let_it_be(:current_user) { create(:user, external: true) } let(:current_params) do { scopes: ["repository:#{project.full_path}:*"] } end @@ -440,7 +442,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'disallow anyone to delete images since registry 2.7' do - let(:current_user) { create(:user, external: true) } + let_it_be(:current_user) { create(:user, external: true) } let(:current_params) do { scopes: ["repository:#{project.full_path}:delete"] } end @@ -453,14 +455,14 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'delete authorized as maintainer' do - let(:current_project) { create(:project) } - let(:current_user) { create(:user) } + let_it_be(:current_project) { create(:project) } + let_it_be(:current_user) { create(:user) } let(:authentication_abilities) do [:admin_container_image] end - before do + before_all do current_project.add_maintainer(current_user) end @@ -488,14 +490,14 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'build authorized as user' do - let(:current_project) { create(:project) } - let(:current_user) { create(:user) } + let_it_be(:current_project) { create(:project) } + let_it_be(:current_user) { create(:user) } let(:authentication_abilities) do [:build_read_container_image, :build_create_container_image, :build_destroy_container_image] end - before do + before_all do current_project.add_developer(current_user) end @@ -550,7 +552,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'allow for public' do - let(:project) { create(:project, :public) } + let_it_be(:project) { create(:project, :public) } it_behaves_like 'a pullable' it_behaves_like 'not a container repository factory' @@ -563,7 +565,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'when you are member' do - before do + before_all do project.add_developer(current_user) end @@ -572,7 +574,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'when you are owner' do - let(:project) { create(:project, namespace: current_user.namespace) } + let_it_be(:project) { create(:project, namespace: current_user.namespace) } it_behaves_like 'a pullable' it_behaves_like 'not a container repository factory' @@ -580,12 +582,12 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for private' do - let(:project) { create(:project, :private) } + let_it_be(:project) { create(:project, :private) } it_behaves_like 'pullable for being team member' context 'when you are admin' do - let(:current_user) { create(:admin) } + let_it_be(:current_user) { create(:admin) } context 'when you are not member' do it_behaves_like 'an inaccessible' @@ -593,7 +595,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'when you are member' do - before do + before_all do project.add_developer(current_user) end @@ -602,7 +604,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'when you are owner' do - let(:project) { create(:project, namespace: current_user.namespace) } + let_it_be(:project) { create(:project, namespace: current_user.namespace) } it_behaves_like 'a pullable' it_behaves_like 'not a container repository factory' @@ -618,9 +620,9 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do context 'disallow for all' do context 'when you are member' do - let(:project) { create(:project, :public) } + let_it_be(:project) { create(:project, :public) } - before do + before_all do project.add_developer(current_user) end @@ -629,7 +631,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'when you are owner' do - let(:project) { create(:project, :public, namespace: current_user.namespace) } + let_it_be(:project) { create(:project, :public, namespace: current_user.namespace) } it_behaves_like 'an inaccessible' it_behaves_like 'not a container repository factory' @@ -639,10 +641,10 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for project without container registry' do - let(:project) { create(:project, :public, container_registry_enabled: false) } + let_it_be(:project) { create(:project, :public, container_registry_enabled: false) } before do - project.update(container_registry_enabled: false) + project.update!(container_registry_enabled: false) end context 'disallow when pulling' do @@ -656,7 +658,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for project that disables repository' do - let(:project) { create(:project, :public, :repository_disabled) } + let_it_be(:project) { create(:project, :public, :repository_disabled) } context 'disallow when pulling' do let(:current_params) do @@ -670,8 +672,8 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'registry catalog browsing authorized as admin' do - let(:current_user) { create(:user, :admin) } - let(:project) { create(:project, :public) } + let_it_be(:current_user) { create(:user, :admin) } + let_it_be(:project) { create(:project, :public) } let(:current_params) do { scopes: ["registry:catalog:*"] } @@ -681,8 +683,8 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'support for multiple scopes' do - let(:internal_project) { create(:project, :internal) } - let(:private_project) { create(:project, :private) } + let_it_be(:internal_project) { create(:project, :internal) } + let_it_be(:private_project) { create(:project, :private) } let(:current_params) do { @@ -694,7 +696,11 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'user has access to all projects' do - let(:current_user) { create(:user, :admin) } + let_it_be(:current_user) { create(:user, :admin) } + + before do + enable_admin_mode!(current_user) + end it_behaves_like 'a browsable' do let(:access) do @@ -711,7 +717,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'user only has access to internal project' do - let(:current_user) { create(:user) } + let_it_be(:current_user) { create(:user) } it_behaves_like 'a browsable' do let(:access) do @@ -747,7 +753,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for private project' do - let(:project) { create(:project, :private) } + let_it_be(:project) { create(:project, :private) } let(:current_params) do { scopes: ["repository:#{project.full_path}:pull"] } @@ -757,7 +763,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for public project' do - let(:project) { create(:project, :public) } + let_it_be(:project) { create(:project, :public) } context 'when pulling and pushing' do let(:current_params) do @@ -806,7 +812,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for public project' do - let(:project) { create(:project, :public) } + let_it_be(:project) { create(:project, :public) } context 'when pulling' do it_behaves_like 'a pullable' @@ -824,7 +830,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for internal project' do - let(:project) { create(:project, :internal) } + let_it_be(:project) { create(:project, :internal) } context 'when pulling' do it_behaves_like 'a pullable' @@ -842,7 +848,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for private project' do - let(:project) { create(:project, :private) } + let_it_be(:project) { create(:project, :private) } context 'when pulling' do it_behaves_like 'a pullable' @@ -880,7 +886,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for public project' do - let(:project) { create(:project, :public) } + let_it_be(:project) { create(:project, :public) } context 'when pulling' do it_behaves_like 'a pullable' @@ -890,7 +896,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for internal project' do - let(:project) { create(:project, :internal) } + let_it_be(:project) { create(:project, :internal) } context 'when pulling' do it_behaves_like 'an inaccessible' @@ -900,7 +906,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for private project' do - let(:project) { create(:project, :internal) } + let_it_be(:project) { create(:project, :internal) } context 'when pulling' do it_behaves_like 'an inaccessible' @@ -918,10 +924,10 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'when deploy token is not related to the project' do - let(:current_user) { create(:deploy_token, read_registry: false) } + let_it_be(:current_user) { create(:deploy_token, read_registry: false) } context 'for public project' do - let(:project) { create(:project, :public) } + let_it_be(:project) { create(:project, :public) } context 'when pulling' do it_behaves_like 'a pullable' @@ -929,7 +935,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for internal project' do - let(:project) { create(:project, :internal) } + let_it_be(:project) { create(:project, :internal) } context 'when pulling' do it_behaves_like 'an inaccessible' @@ -937,7 +943,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'for private project' do - let(:project) { create(:project, :internal) } + let_it_be(:project) { create(:project, :internal) } context 'when pulling' do it_behaves_like 'an inaccessible' @@ -949,19 +955,19 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do let(:current_user) { create(:deploy_token, :revoked, projects: [project]) } context 'for public project' do - let(:project) { create(:project, :public) } + let_it_be(:project) { create(:project, :public) } it_behaves_like 'a pullable' end context 'for internal project' do - let(:project) { create(:project, :internal) } + let_it_be(:project) { create(:project, :internal) } it_behaves_like 'an inaccessible' end context 'for private project' do - let(:project) { create(:project, :internal) } + let_it_be(:project) { create(:project, :internal) } it_behaves_like 'an inaccessible' end @@ -969,14 +975,13 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do end context 'user authorization' do - let(:current_user) { create(:user) } + let_it_be(:current_user) { create(:user) } context 'with multiple scopes' do - let(:project) { create(:project) } - let(:project2) { create } + let_it_be(:project) { create(:project) } context 'allow developer to push images' do - before do + before_all do project.add_developer(current_user) end diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb index 98fa6012089..1d33dc15838 100644 --- a/spec/services/auto_merge/base_service_spec.rb +++ b/spec/services/auto_merge/base_service_spec.rb @@ -131,7 +131,7 @@ RSpec.describe AutoMerge::BaseService do end describe '#update' do - subject { service.update(merge_request) } + subject { service.update(merge_request) } # rubocop:disable Rails/SaveBang let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } diff --git a/spec/services/auto_merge_service_spec.rb b/spec/services/auto_merge_service_spec.rb index eab95973e1b..3f7a26aa00e 100644 --- a/spec/services/auto_merge_service_spec.rb +++ b/spec/services/auto_merge_service_spec.rb @@ -148,7 +148,7 @@ RSpec.describe AutoMergeService do end describe '#update' do - subject { service.update(merge_request) } + subject { service.update(merge_request) } # rubocop:disable Rails/SaveBang context 'when auto merge is enabled' do let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } diff --git a/spec/services/bulk_create_integration_service_spec.rb b/spec/services/bulk_create_integration_service_spec.rb index 5d896f78b35..674382ee14f 100644 --- a/spec/services/bulk_create_integration_service_spec.rb +++ b/spec/services/bulk_create_integration_service_spec.rb @@ -5,13 +5,15 @@ require 'spec_helper' RSpec.describe BulkCreateIntegrationService do include JiraServiceHelper - before do + before_all do stub_jira_service_test end + let_it_be(:excluded_group) { create(:group) } + let_it_be(:excluded_project) { create(:project, group: excluded_group) } + let(:instance_integration) { create(:jira_service, :instance) } + let(:template_integration) { create(:jira_service, :template) } let(:excluded_attributes) { %w[id project_id group_id inherit_from_id instance template created_at updated_at] } - let!(:instance_integration) { create(:jira_service, :instance) } - let!(:template_integration) { create(:jira_service, :template) } shared_examples 'creates integration from batch ids' do it 'updates the inherited integrations' do @@ -37,71 +39,125 @@ RSpec.describe BulkCreateIntegrationService do it 'updates inherit_from_id attributes' do described_class.new(integration, batch, association).execute - expect(created_integration.reload.inherit_from_id).to eq(integration.id) + expect(created_integration.reload.inherit_from_id).to eq(inherit_from_id) end end - shared_examples 'runs project callbacks' do + shared_examples 'updates project callbacks' do it 'updates projects#has_external_issue_tracker for issue tracker services' do described_class.new(integration, batch, association).execute expect(project.reload.has_external_issue_tracker).to eq(true) + expect(excluded_project.reload.has_external_issue_tracker).to eq(false) end context 'with an external wiki integration' do - let(:integration) do - ExternalWikiService.create!( - instance: true, - active: true, - push_events: false, - external_wiki_url: 'http://external-wiki-url.com' - ) + before do + integration.update!(category: 'common', type: 'ExternalWikiService') end it 'updates projects#has_external_wiki for external wiki services' do described_class.new(integration, batch, association).execute expect(project.reload.has_external_wiki).to eq(true) + expect(excluded_project.reload.has_external_wiki).to eq(false) end end end - context 'with an instance-level integration' do + shared_examples 'does not update project callbacks' do + it 'does not update projects#has_external_issue_tracker for issue tracker services' do + described_class.new(integration, batch, association).execute + + expect(project.reload.has_external_issue_tracker).to eq(false) + end + + context 'with an inactive external wiki integration' do + let(:integration) { create(:external_wiki_service, :instance, active: false) } + + it 'does not update projects#has_external_wiki for external wiki services' do + described_class.new(integration, batch, association).execute + + expect(project.reload.has_external_wiki).to eq(false) + end + end + end + + context 'passing an instance-level integration' do let(:integration) { instance_integration } + let(:inherit_from_id) { integration.id } context 'with a project association' do let!(:project) { create(:project) } let(:created_integration) { project.jira_service } - let(:batch) { Project.all } + let(:batch) { Project.where(id: project.id) } let(:association) { 'project' } it_behaves_like 'creates integration from batch ids' it_behaves_like 'updates inherit_from_id' - it_behaves_like 'runs project callbacks' + it_behaves_like 'updates project callbacks' + + context 'when integration is not active' do + before do + integration.update!(active: false) + end + + it_behaves_like 'does not update project callbacks' + end end context 'with a group association' do let!(:group) { create(:group) } let(:created_integration) { Service.find_by(group: group) } - let(:batch) { Group.all } + let(:batch) { Group.where(id: group.id) } + let(:association) { 'group' } + + it_behaves_like 'creates integration from batch ids' + it_behaves_like 'updates inherit_from_id' + end + end + + context 'passing a group integration' do + let_it_be(:group) { create(:group) } + + context 'with a project association' do + let!(:project) { create(:project, group: group) } + let(:integration) { create(:jira_service, group: group, project: nil) } + let(:created_integration) { project.jira_service } + let(:batch) { Project.where(id: Project.minimum(:id)..Project.maximum(:id)).without_integration(integration).in_namespace(integration.group.self_and_descendants) } + let(:association) { 'project' } + let(:inherit_from_id) { integration.id } + + it_behaves_like 'creates integration from batch ids' + it_behaves_like 'updates inherit_from_id' + it_behaves_like 'updates project callbacks' + end + + context 'with a group association' do + let!(:subgroup) { create(:group, parent: group) } + let(:integration) { create(:jira_service, group: group, project: nil, inherit_from_id: instance_integration.id) } + let(:created_integration) { Service.find_by(group: subgroup) } + let(:batch) { Group.where(id: subgroup.id) } let(:association) { 'group' } + let(:inherit_from_id) { instance_integration.id } it_behaves_like 'creates integration from batch ids' it_behaves_like 'updates inherit_from_id' end end - context 'with a template integration' do + context 'passing a template integration' do let(:integration) { template_integration } context 'with a project association' do let!(:project) { create(:project) } let(:created_integration) { project.jira_service } - let(:batch) { Project.all } + let(:batch) { Project.where(id: project.id) } let(:association) { 'project' } + let(:inherit_from_id) { integration.id } it_behaves_like 'creates integration from batch ids' - it_behaves_like 'runs project callbacks' + it_behaves_like 'updates project callbacks' end end end diff --git a/spec/services/bulk_import_service_spec.rb b/spec/services/bulk_import_service_spec.rb new file mode 100644 index 00000000000..e4a50b9d523 --- /dev/null +++ b/spec/services/bulk_import_service_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe BulkImportService do + let(:user) { create(:user) } + let(:credentials) { { url: 'http://gitlab.example', access_token: 'token' } } + let(:params) do + [ + { + source_type: 'group_entity', + source_full_path: 'full/path/to/group1', + destination_name: 'destination group 1', + destination_namespace: 'full/path/to/destination1' + }, + { + source_type: 'group_entity', + source_full_path: 'full/path/to/group2', + destination_name: 'destination group 2', + destination_namespace: 'full/path/to/destination2' + }, + { + source_type: 'project_entity', + source_full_path: 'full/path/to/project1', + destination_name: 'destination project 1', + destination_namespace: 'full/path/to/destination1' + } + ] + end + + subject { described_class.new(user, params, credentials) } + + describe '#execute' do + it 'creates bulk import' do + expect { subject.execute }.to change { BulkImport.count }.by(1) + end + + it 'creates bulk import entities' do + expect { subject.execute }.to change { BulkImports::Entity.count }.by(3) + end + + it 'creates bulk import configuration' do + expect { subject.execute }.to change { BulkImports::Configuration.count }.by(1) + end + + it 'enqueues BulkImportWorker' do + expect(BulkImportWorker).to receive(:perform_async) + + subject.execute + end + end +end diff --git a/spec/services/bulk_update_integration_service_spec.rb b/spec/services/bulk_update_integration_service_spec.rb index 2f0bfd31600..e7944f07bb7 100644 --- a/spec/services/bulk_update_integration_service_spec.rb +++ b/spec/services/bulk_update_integration_service_spec.rb @@ -5,52 +5,74 @@ require 'spec_helper' RSpec.describe BulkUpdateIntegrationService do include JiraServiceHelper - before do + before_all do stub_jira_service_test end let(:excluded_attributes) { %w[id project_id group_id inherit_from_id instance template created_at updated_at] } - let!(:instance_integration) do + let(:batch) do + Service.inherited_descendants_from_self_or_ancestors_from(subgroup_integration).where(id: group_integration.id..integration.id) + end + + let_it_be(:group) { create(:group) } + let_it_be(:subgroup) { create(:group, parent: group) } + let_it_be(:group_integration) do + JiraService.create!( + group: group, + url: 'http://group.jira.com' + ) + end + + let_it_be(:subgroup_integration) do JiraService.create!( - instance: true, - active: true, - push_events: true, - url: 'http://update-jira.instance.com', - username: 'user', - password: 'secret' + inherit_from_id: group_integration.id, + group: subgroup, + url: 'http://subgroup.jira.com', + push_events: true ) end - let!(:integration) do + let_it_be(:excluded_integration) do JiraService.create!( - project: create(:project), - inherit_from_id: instance_integration.id, - instance: false, - active: true, - push_events: false, - url: 'http://jira.instance.com', - username: 'user', - password: 'secret' + group: create(:group), + url: 'http://another.jira.com', + push_events: false + ) + end + + let_it_be(:integration) do + JiraService.create!( + project: create(:project, group: subgroup), + inherit_from_id: subgroup_integration.id, + url: 'http://project.jira.com', + push_events: false ) end context 'with inherited integration' do - it 'updates the integration' do - described_class.new(instance_integration, Service.inherit_from_id(instance_integration.id)).execute + it 'updates the integration', :aggregate_failures do + described_class.new(subgroup_integration, batch).execute + + expect(integration.reload.inherit_from_id).to eq(group_integration.id) + expect(integration.reload.attributes.except(*excluded_attributes)) + .to eq(subgroup_integration.attributes.except(*excluded_attributes)) - expect(integration.reload.inherit_from_id).to eq(instance_integration.id) - expect(integration.attributes.except(*excluded_attributes)) - .to eq(instance_integration.attributes.except(*excluded_attributes)) + expect(excluded_integration.reload.inherit_from_id).not_to eq(group_integration.id) + expect(excluded_integration.reload.attributes.except(*excluded_attributes)) + .not_to eq(subgroup_integration.attributes.except(*excluded_attributes)) end context 'with integration with data fields' do let(:excluded_attributes) { %w[id service_id created_at updated_at] } - it 'updates the data fields from the integration' do - described_class.new(instance_integration, Service.inherit_from_id(instance_integration.id)).execute + it 'updates the data fields from the integration', :aggregate_failures do + described_class.new(subgroup_integration, batch).execute + + expect(integration.data_fields.attributes.except(*excluded_attributes)) + .to eq(subgroup_integration.data_fields.attributes.except(*excluded_attributes)) - expect(integration.reload.data_fields.attributes.except(*excluded_attributes)) - .to eq(instance_integration.data_fields.attributes.except(*excluded_attributes)) + expect(integration.data_fields.attributes.except(*excluded_attributes)) + .not_to eq(excluded_integration.data_fields.attributes.except(*excluded_attributes)) end end end diff --git a/spec/services/ci/append_build_trace_service_spec.rb b/spec/services/ci/append_build_trace_service_spec.rb new file mode 100644 index 00000000000..a0a7f594881 --- /dev/null +++ b/spec/services/ci/append_build_trace_service_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::AppendBuildTraceService do + let_it_be(:project) { create(:project) } + let_it_be(:pipeline) { create(:ci_pipeline, project: project) } + let_it_be(:build) { create(:ci_build, :running, pipeline: pipeline) } + + before do + stub_feature_flags(ci_enable_live_trace: true) + end + + context 'build trace append is successful' do + it 'returns a correct stream size and status code' do + stream_size = 192.kilobytes + body_data = 'x' * stream_size + content_range = "0-#{stream_size}" + + result = described_class + .new(build, content_range: content_range) + .execute(body_data) + + expect(result.status).to eq 202 + expect(result.stream_size).to eq stream_size + expect(build.trace_chunks.count).to eq 2 + end + end + + context 'when could not correctly append to a trace' do + it 'responds with content range violation and data stored' do + allow(build).to receive_message_chain(:trace, :append) { 16 } + + result = described_class + .new(build, content_range: '0-128') + .execute('x' * 128) + + expect(result.status).to eq 416 + expect(result.stream_size).to eq 16 + end + + it 'logs exception if build has live trace' do + build.trace.append('abcd', 0) + + expect(::Gitlab::ErrorTracking) + .to receive(:log_exception) + .with(anything, hash_including(chunk_index: 0, chunk_store: 'redis')) + + result = described_class + .new(build, content_range: '0-128') + .execute('x' * 128) + + expect(result.status).to eq 416 + expect(result.stream_size).to eq 4 + end + end +end diff --git a/spec/services/ci/build_report_result_service_spec.rb b/spec/services/ci/build_report_result_service_spec.rb index 134b662a72a..7c2702af086 100644 --- a/spec/services/ci/build_report_result_service_spec.rb +++ b/spec/services/ci/build_report_result_service_spec.rb @@ -26,18 +26,6 @@ RSpec.describe Ci::BuildReportResultService do expect(unique_test_cases_parsed).to eq(4) end - context 'when feature flag for tracking is disabled' do - before do - stub_feature_flags(track_unique_test_cases_parsed: false) - end - - it 'creates the report but does not track the event' do - expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event) - expect(build_report_result.tests_name).to eq("test") - expect(Ci::BuildReportResult.count).to eq(1) - end - end - context 'when data has already been persisted' do it 'raises an error and do not persist the same data twice' do expect { 2.times { described_class.new.execute(build) } }.to raise_error(ActiveRecord::RecordNotUnique) diff --git a/spec/services/ci/compare_test_reports_service_spec.rb b/spec/services/ci/compare_test_reports_service_spec.rb index 7d31db73b6a..377c801b008 100644 --- a/spec/services/ci/compare_test_reports_service_spec.rb +++ b/spec/services/ci/compare_test_reports_service_spec.rb @@ -7,15 +7,15 @@ RSpec.describe Ci::CompareTestReportsService do let(:project) { create(:project, :repository) } describe '#execute' do - subject { service.execute(base_pipeline, head_pipeline) } + subject(:comparison) { service.execute(base_pipeline, head_pipeline) } context 'when head pipeline has test reports' do let!(:base_pipeline) { nil } let!(:head_pipeline) { create(:ci_pipeline, :with_test_reports, project: project) } it 'returns status and data' do - expect(subject[:status]).to eq(:parsed) - expect(subject[:data]).to match_schema('entities/test_reports_comparer') + expect(comparison[:status]).to eq(:parsed) + expect(comparison[:data]).to match_schema('entities/test_reports_comparer') end end @@ -24,8 +24,8 @@ RSpec.describe Ci::CompareTestReportsService do let!(:head_pipeline) { create(:ci_pipeline, :with_test_reports, project: project) } it 'returns status and data' do - expect(subject[:status]).to eq(:parsed) - expect(subject[:data]).to match_schema('entities/test_reports_comparer') + expect(comparison[:status]).to eq(:parsed) + expect(comparison[:data]).to match_schema('entities/test_reports_comparer') end end @@ -39,9 +39,44 @@ RSpec.describe Ci::CompareTestReportsService do end it 'returns a parsed TestReports success status and failure on the individual suite' do - expect(subject[:status]).to eq(:parsed) - expect(subject.dig(:data, 'status')).to eq('success') - expect(subject.dig(:data, 'suites', 0, 'status') ).to eq('error') + expect(comparison[:status]).to eq(:parsed) + expect(comparison.dig(:data, 'status')).to eq('success') + expect(comparison.dig(:data, 'suites', 0, 'status') ).to eq('error') + end + end + + context 'test failure history' do + let!(:base_pipeline) { nil } + let!(:head_pipeline) { create(:ci_pipeline, :with_test_reports_with_three_failures, project: project) } + + let(:new_failures) do + comparison.dig(:data, 'suites', 0, 'new_failures') + end + + let(:recent_failures_per_test_case) do + new_failures.map { |f| f['recent_failures'] } + end + + # Create test case failure records based on the head pipeline build + before do + stub_const("Gitlab::Ci::Reports::TestSuiteComparer::DEFAULT_MAX_TESTS", 2) + stub_const("Gitlab::Ci::Reports::TestSuiteComparer::DEFAULT_MIN_TESTS", 1) + + build = head_pipeline.builds.last + build.update_column(:finished_at, 1.day.ago) # Just to be sure we are included in the report window + + # The JUnit fixture for the given build has 3 failures. + # This service will create 1 test case failure record for each. + Ci::TestCasesService.new.execute(build) + end + + it 'loads recent failures on limited test cases to avoid building up a huge DB query', :aggregate_failures do + expect(comparison[:data]).to match_schema('entities/test_reports_comparer') + expect(recent_failures_per_test_case).to eq([ + { 'count' => 1, 'base_branch' => 'master' }, + { 'count' => 1, 'base_branch' => 'master' } + ]) + expect(new_failures.count).to eq(2) end end end diff --git a/spec/services/ci/create_downstream_pipeline_service_spec.rb b/spec/services/ci/create_downstream_pipeline_service_spec.rb index 0cc380439a7..03cea4074bf 100644 --- a/spec/services/ci/create_downstream_pipeline_service_spec.rb +++ b/spec/services/ci/create_downstream_pipeline_service_spec.rb @@ -581,5 +581,40 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do ) end end + + context 'when downstream pipeline has workflow rule' do + before do + stub_ci_pipeline_yaml_file(config) + end + + let(:config) do + <<-EOY + workflow: + rules: + - if: $my_var + + regular-job: + script: 'echo Hello, World!' + EOY + end + + context 'when passing the required variable' do + before do + bridge.yaml_variables = [{ key: 'my_var', value: 'var', public: true }] + end + + it 'creates the pipeline' do + expect { service.execute(bridge) }.to change(downstream_project.ci_pipelines, :count).by(1) + + expect(bridge.reload).to be_success + end + end + + context 'when not passing the required variable' do + it 'does not create the pipeline' do + expect { service.execute(bridge) }.not_to change(downstream_project.ci_pipelines, :count) + end + end + end end end diff --git a/spec/services/ci/create_pipeline_service/cache_spec.rb b/spec/services/ci/create_pipeline_service/cache_spec.rb index 1438c2e4aa0..5f74c2f1cef 100644 --- a/spec/services/ci/create_pipeline_service/cache_spec.rb +++ b/spec/services/ci/create_pipeline_service/cache_spec.rb @@ -4,13 +4,13 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do context 'cache' do - let(:user) { create(:admin) } + let(:project) { create(:project, :custom_repo, files: files) } + let(:user) { project.owner } let(:ref) { 'refs/heads/master' } let(:source) { :push } let(:service) { described_class.new(project, user, { ref: ref }) } let(:pipeline) { service.execute(source) } let(:job) { pipeline.builds.find_by(name: 'job') } - let(:project) { create(:project, :custom_repo, files: files) } before do stub_ci_pipeline_yaml_file(config) 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 b5b3832ac00..9ccf289df7c 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 @@ -4,8 +4,8 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do describe 'creation errors and warnings' do - let_it_be(:user) { create(:admin) } - let_it_be(:project) { create(:project, :repository, creator: user) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { project.owner } let(:ref) { 'refs/heads/master' } let(:source) { :push } diff --git a/spec/services/ci/create_pipeline_service/custom_config_content_spec.rb b/spec/services/ci/create_pipeline_service/custom_config_content_spec.rb index 122870e0f3a..6320a16d646 100644 --- a/spec/services/ci/create_pipeline_service/custom_config_content_spec.rb +++ b/spec/services/ci/create_pipeline_service/custom_config_content_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { create(:admin) } + let_it_be(:user) { project.owner } let(:ref) { 'refs/heads/master' } let(:service) { described_class.new(project, user, { ref: ref }) } diff --git a/spec/services/ci/create_pipeline_service/dry_run_spec.rb b/spec/services/ci/create_pipeline_service/dry_run_spec.rb index 93378df80f0..60c56ed0f67 100644 --- a/spec/services/ci/create_pipeline_service/dry_run_spec.rb +++ b/spec/services/ci/create_pipeline_service/dry_run_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { create(:admin) } + let_it_be(:user) { project.owner } let(:ref) { 'refs/heads/master' } let(:service) { described_class.new(project, user, { ref: ref }) } diff --git a/spec/services/ci/create_pipeline_service/needs_spec.rb b/spec/services/ci/create_pipeline_service/needs_spec.rb index 915dc46d664..512091035a2 100644 --- a/spec/services/ci/create_pipeline_service/needs_spec.rb +++ b/spec/services/ci/create_pipeline_service/needs_spec.rb @@ -4,8 +4,8 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do context 'needs' do - let_it_be(:user) { create(:admin) } - let_it_be(:project) { create(:project, :repository, creator: user) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { project.owner } let(:ref) { 'refs/heads/master' } let(:source) { :push } @@ -14,6 +14,7 @@ RSpec.describe Ci::CreatePipelineService do before do stub_ci_pipeline_yaml_file(config) + project.add_developer(user) end context 'with a valid config' do diff --git a/spec/services/ci/create_pipeline_service/parameter_content_spec.rb b/spec/services/ci/create_pipeline_service/parameter_content_spec.rb index f656ad52ac8..90b8baa23a7 100644 --- a/spec/services/ci/create_pipeline_service/parameter_content_spec.rb +++ b/spec/services/ci/create_pipeline_service/parameter_content_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { create(:admin) } + let_it_be(:user) { project.owner } let(:service) { described_class.new(project, user, { ref: 'refs/heads/master' }) } let(:content) do <<~EOY diff --git a/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb b/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb index fb6cdf55be3..8df9b0c3e60 100644 --- a/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb +++ b/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb @@ -279,6 +279,40 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do end end end + + context 'when specifying multiple files' do + let(:config) do + <<~YAML + test: + script: rspec + deploy: + variables: + CROSS: downstream + stage: deploy + trigger: + include: + - project: my-namespace/my-project + file: + - 'path/to/child1.yml' + - 'path/to/child2.yml' + YAML + end + + it_behaves_like 'successful creation' do + let(:expected_bridge_options) do + { + 'trigger' => { + 'include' => [ + { + 'file' => ["path/to/child1.yml", "path/to/child2.yml"], + 'project' => 'my-namespace/my-project' + } + ] + } + } + end + end + end end end diff --git a/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb b/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb index 00a2dd74968..c84d9a53973 100644 --- a/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb +++ b/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb @@ -3,8 +3,8 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do describe '.pre/.post stages' do - let_it_be(:user) { create(:admin) } - let_it_be(:project) { create(:project, :repository, creator: user) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { project.owner } let(:source) { :push } let(:service) { described_class.new(project, user, { ref: ref }) } diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index 1a1fa6e8f5d..a0ff2fff0ef 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -2,10 +2,10 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do - let(:user) { create(:admin) } + let(:project) { create(:project, :repository) } + let(:user) { project.owner } let(:ref) { 'refs/heads/master' } let(:source) { :push } - let(:project) { create(:project, :repository) } let(:service) { described_class.new(project, user, { ref: ref }) } let(:pipeline) { service.execute(source) } let(:build_names) { pipeline.builds.pluck(:name) } diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index c28c3449485..f9015752644 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Ci::CreatePipelineService do include ProjectForksHelper let_it_be(:project, reload: true) { create(:project, :repository) } - let(:user) { create(:admin) } + let_it_be(:user, reload: true) { project.owner } let(:ref_name) { 'refs/heads/master' } before do @@ -41,7 +41,9 @@ RSpec.describe Ci::CreatePipelineService do save_on_errors: save_on_errors, trigger_request: trigger_request, merge_request: merge_request, - external_pull_request: external_pull_request) + external_pull_request: external_pull_request) do |pipeline| + yield(pipeline) if block_given? + end end # rubocop:enable Metrics/ParameterLists @@ -153,6 +155,11 @@ RSpec.describe Ci::CreatePipelineService do context 'when merge request target project is different from source project' do let!(:project) { fork_project(target_project, nil, repository: true) } let!(:target_project) { create(:project, :repository) } + let!(:user) { create(:user) } + + before do + project.add_developer(user) + end it 'updates head pipeline for merge request', :sidekiq_might_not_need_inline do merge_request = create(:merge_request, source_branch: 'feature', @@ -1440,6 +1447,11 @@ RSpec.describe Ci::CreatePipelineService do let(:ref_name) { 'refs/heads/feature' } let!(:project) { fork_project(target_project, nil, repository: true) } let!(:target_project) { create(:project, :repository) } + let!(:user) { create(:user) } + + before do + project.add_developer(user) + end it 'creates a legacy detached merge request pipeline in the forked project', :sidekiq_might_not_need_inline do expect(pipeline).to be_persisted @@ -1858,6 +1870,12 @@ RSpec.describe Ci::CreatePipelineService do - changes: - README.md allow_failure: true + + README: + script: "I use variables for changes!" + rules: + - changes: + - $CI_JOB_NAME* EOY end @@ -1867,10 +1885,10 @@ RSpec.describe Ci::CreatePipelineService do .to receive(:modified_paths).and_return(%w[README.md]) end - it 'creates two jobs' do + it 'creates five jobs' do expect(pipeline).to be_persisted expect(build_names) - .to contain_exactly('regular-job', 'rules-job', 'delayed-job', 'negligible-job') + .to contain_exactly('regular-job', 'rules-job', 'delayed-job', 'negligible-job', 'README') end it 'sets when: for all jobs' do @@ -2274,6 +2292,207 @@ RSpec.describe Ci::CreatePipelineService do end end end + + context 'with workflow rules with persisted variables' do + let(:config) do + <<-EOY + workflow: + rules: + - if: $CI_COMMIT_REF_NAME == "master" + + regular-job: + script: 'echo Hello, World!' + EOY + end + + context 'with matches' do + it 'creates a pipeline' do + expect(pipeline).to be_persisted + expect(build_names).to contain_exactly('regular-job') + end + end + + context 'with no matches' do + let(:ref_name) { 'refs/heads/feature' } + + it 'does not create a pipeline' do + expect(pipeline).not_to be_persisted + end + end + end + + context 'with workflow rules with pipeline variables' do + let(:pipeline) do + execute_service(variables_attributes: variables_attributes) + end + + let(:config) do + <<-EOY + workflow: + rules: + - if: $SOME_VARIABLE + + regular-job: + script: 'echo Hello, World!' + EOY + end + + context 'with matches' do + let(:variables_attributes) do + [{ key: 'SOME_VARIABLE', secret_value: 'SOME_VAR' }] + end + + it 'creates a pipeline' do + expect(pipeline).to be_persisted + expect(build_names).to contain_exactly('regular-job') + end + end + + context 'with no matches' do + let(:variables_attributes) { {} } + + it 'does not create a pipeline' do + expect(pipeline).not_to be_persisted + end + end + end + + context 'with workflow rules with trigger variables' do + let(:pipeline) do + execute_service do |pipeline| + pipeline.variables.build(variables) + end + end + + let(:config) do + <<-EOY + workflow: + rules: + - if: $SOME_VARIABLE + + regular-job: + script: 'echo Hello, World!' + EOY + end + + context 'with matches' do + let(:variables) do + [{ key: 'SOME_VARIABLE', secret_value: 'SOME_VAR' }] + end + + it 'creates a pipeline' do + expect(pipeline).to be_persisted + expect(build_names).to contain_exactly('regular-job') + end + + context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do + before do + stub_feature_flags(ci_seed_block_run_before_workflow_rules: false) + end + + it 'does not a pipeline' do + expect(pipeline).not_to be_persisted + end + end + + context 'when a job requires the same variable' do + let(:config) do + <<-EOY + workflow: + rules: + - if: $SOME_VARIABLE + + build: + stage: build + script: 'echo build' + rules: + - if: $SOME_VARIABLE + + test1: + stage: test + script: 'echo test1' + needs: [build] + + test2: + stage: test + script: 'echo test2' + EOY + end + + it 'creates a pipeline' do + expect(pipeline).to be_persisted + expect(build_names).to contain_exactly('build', 'test1', 'test2') + end + + context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do + before do + stub_feature_flags(ci_seed_block_run_before_workflow_rules: false) + end + + it 'does not a pipeline' do + expect(pipeline).not_to be_persisted + end + end + end + end + + context 'with no matches' do + let(:variables) { {} } + + it 'does not create a pipeline' do + expect(pipeline).not_to be_persisted + end + + context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do + before do + stub_feature_flags(ci_seed_block_run_before_workflow_rules: false) + end + + it 'does not create a pipeline' do + expect(pipeline).not_to be_persisted + end + end + + context 'when a job requires the same variable' do + let(:config) do + <<-EOY + workflow: + rules: + - if: $SOME_VARIABLE + + build: + stage: build + script: 'echo build' + rules: + - if: $SOME_VARIABLE + + test1: + stage: test + script: 'echo test1' + needs: [build] + + test2: + stage: test + script: 'echo test2' + EOY + end + + it 'does not create a pipeline' do + expect(pipeline).not_to be_persisted + end + + context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do + before do + stub_feature_flags(ci_seed_block_run_before_workflow_rules: false) + end + + it 'does not create a pipeline' do + expect(pipeline).not_to be_persisted + end + end + end + end + end end end diff --git a/spec/services/ci/daily_build_group_report_result_service_spec.rb b/spec/services/ci/daily_build_group_report_result_service_spec.rb index f196afb05e8..e54f10cc4f4 100644 --- a/spec/services/ci/daily_build_group_report_result_service_spec.rb +++ b/spec/services/ci/daily_build_group_report_result_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do let!(:rspec_job) { create(:ci_build, pipeline: pipeline, name: '3/3 rspec', coverage: 80) } let!(:karma_job) { create(:ci_build, pipeline: pipeline, name: '2/2 karma', coverage: 90) } let!(:extra_job) { create(:ci_build, pipeline: pipeline, name: 'extra', coverage: nil) } + let(:coverages) { Ci::DailyBuildGroupReportResult.all } it 'creates daily code coverage record for each job in the pipeline that has coverage value' do described_class.new.execute(pipeline) @@ -158,4 +159,30 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do expect { described_class.new.execute(new_pipeline) }.not_to raise_error end end + + context 'when pipeline ref_path is the project default branch' do + let(:default_branch) { 'master' } + + before do + allow(pipeline.project).to receive(:default_branch).and_return(default_branch) + end + + it 'sets default branch to true' do + described_class.new.execute(pipeline) + + coverages.each do |coverage| + expect(coverage.default_branch).to be_truthy + end + end + end + + context 'when pipeline ref_path is not the project default branch' do + it 'sets default branch to false' do + described_class.new.execute(pipeline) + + coverages.each do |coverage| + expect(coverage.default_branch).to be_falsey + end + end + 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 3d5329811ad..c8d426ee657 100644 --- a/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb +++ b/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb @@ -5,26 +5,85 @@ require 'spec_helper' RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared_state do include ExclusiveLeaseHelpers + let(:service) { described_class.new } + describe '.execute' do subject { service.execute } - let(:service) { described_class.new } - - let_it_be(:artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) } + let_it_be(:artifact, reload: true) do + create(:ci_job_artifact, expire_at: 1.day.ago) + end before(:all) do artifact.job.pipeline.unlocked! end context 'when artifact is expired' do - context 'when artifact is not locked' do + context 'with preloaded relationships' do before do - artifact.job.pipeline.unlocked! + job = create(:ci_build, pipeline: artifact.job.pipeline) + create(:ci_job_artifact, :archive, :expired, job: job) + + stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 1) end - it 'destroys job artifact' do + it 'performs the smallest number of queries for job_artifacts' do + log = ActiveRecord::QueryRecorder.new { subject } + + # SELECT expired ci_job_artifacts + # PRELOAD projects, routes, project_statistics + # BEGIN + # INSERT into ci_deleted_objects + # DELETE loaded ci_job_artifacts + # DELETE security_findings -- for EE + # COMMIT + expect(log.count).to be_within(1).of(8) + end + end + + context 'when artifact is not locked' do + it 'deletes job artifact record' do expect { subject }.to change { Ci::JobArtifact.count }.by(-1) end + + context 'when the artifact does not a file attached to it' do + it 'does not create deleted objects' do + expect(artifact.exists?).to be_falsy # sanity check + + expect { subject }.not_to change { Ci::DeletedObject.count } + end + end + + context 'when the artifact has a file attached to it' do + before do + artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') + artifact.save! + end + + it 'creates a deleted object' do + expect { subject }.to change { Ci::DeletedObject.count }.by(1) + end + + it 'resets project statistics' do + expect(ProjectStatistics).to receive(:increment_statistic).once + .with(artifact.project, :build_artifacts_size, -artifact.file.size) + .and_call_original + + subject + end + + it 'does not remove the files' do + expect { subject }.not_to change { artifact.file.exists? } + end + + it 'reports metrics for destroyed artifacts' do + counter = service.send(:destroyed_artifacts_counter) + + expect(counter).to receive(:increment).with({}, 1).and_call_original + + subject + end + end end context 'when artifact is locked' do @@ -61,14 +120,34 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared context 'when failed to destroy artifact' do before do stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 10) + end - allow_any_instance_of(Ci::JobArtifact) - .to receive(:destroy!) - .and_raise(ActiveRecord::RecordNotDestroyed) + context 'when the import fails' do + before do + expect(Ci::DeletedObject) + .to receive(:bulk_import) + .once + .and_raise(ActiveRecord::RecordNotDestroyed) + end + + it 'raises an exception and stop destroying' do + expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed) + .and not_change { Ci::JobArtifact.count }.from(1) + end end - it 'raises an exception and stop destroying' do - expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed) + context 'when the delete fails' do + before do + expect(Ci::JobArtifact) + .to receive(:id_in) + .once + .and_raise(ActiveRecord::RecordNotDestroyed) + end + + it 'raises an exception rolls back the insert' do + expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed) + .and not_change { Ci::DeletedObject.count }.from(0) + end end end @@ -85,7 +164,7 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared context 'when timeout happens' do before do stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_TIMEOUT', 1.second) - allow_any_instance_of(described_class).to receive(:destroy_batch) { true } + allow_any_instance_of(described_class).to receive(:destroy_artifacts_batch) { true } end it 'returns false and does not continue destroying' do @@ -176,4 +255,16 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared end end end + + describe '.destroy_job_artifacts_batch' do + it 'returns a falsy value without artifacts' do + expect(service.send(:destroy_job_artifacts_batch)).to be_falsy + end + end + + describe '.destroy_pipeline_artifacts_batch' do + it 'returns a falsy value without artifacts' do + expect(service.send(:destroy_pipeline_artifacts_batch)).to be_falsy + end + end end diff --git a/spec/services/ci/list_config_variables_service_spec.rb b/spec/services/ci/list_config_variables_service_spec.rb index 5cc0481768b..95c98c2b5ef 100644 --- a/spec/services/ci/list_config_variables_service_spec.rb +++ b/spec/services/ci/list_config_variables_service_spec.rb @@ -3,8 +3,9 @@ require 'spec_helper' RSpec.describe Ci::ListConfigVariablesService do - let_it_be(:project) { create(:project, :repository) } - let(:service) { described_class.new(project) } + let(:project) { create(:project, :repository) } + let(:user) { project.creator } + let(:service) { described_class.new(project, user) } let(:result) { YAML.dump(ci_config) } subject { service.execute(sha) } @@ -38,6 +39,40 @@ RSpec.describe Ci::ListConfigVariablesService do end end + context 'when config has includes' do + let(:sha) { 'master' } + let(:ci_config) do + { + include: [{ local: 'other_file.yml' }], + variables: { + KEY1: { value: 'val 1', description: 'description 1' } + }, + test: { + stage: 'test', + script: 'echo' + } + } + end + + before do + allow_next_instance_of(Repository) do |repository| + allow(repository).to receive(:blob_data_at).with(sha, 'other_file.yml') do + <<~HEREDOC + variables: + KEY2: + value: 'val 2' + description: 'description 2' + HEREDOC + end + end + end + + it 'returns variable list' do + expect(subject['KEY1']).to eq({ value: 'val 1', description: 'description 1' }) + expect(subject['KEY2']).to eq({ value: 'val 2', description: 'description 2' }) + end + end + context 'when sending an invalid sha' do let(:sha) { 'invalid-sha' } let(:ci_config) { nil } diff --git a/spec/services/ci/test_cases_service_spec.rb b/spec/services/ci/test_cases_service_spec.rb new file mode 100644 index 00000000000..b61d308640f --- /dev/null +++ b/spec/services/ci/test_cases_service_spec.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::TestCasesService, :aggregate_failures do + describe '#execute' do + subject(:execute_service) { described_class.new.execute(build) } + + context 'when build has test reports' do + let(:build) { create(:ci_build, :success, :test_reports) } # The test report has 2 test case failures + + it 'creates test case failures records' do + execute_service + + expect(Ci::TestCase.count).to eq(2) + expect(Ci::TestCaseFailure.count).to eq(2) + end + + context 'when feature flag for test failure history is disabled' do + before do + stub_feature_flags(test_failure_history: false) + end + + it 'does not persist data' do + execute_service + + expect(Ci::TestCase.count).to eq(0) + expect(Ci::TestCaseFailure.count).to eq(0) + end + end + + context 'when build is not for the default branch' do + before do + build.update_column(:ref, 'new-feature') + end + + it 'does not persist data' do + execute_service + + expect(Ci::TestCase.count).to eq(0) + expect(Ci::TestCaseFailure.count).to eq(0) + end + end + + context 'when test failure data have already been persisted with the same exact attributes' do + before do + execute_service + end + + it 'does not fail but does not persist new data' do + expect { described_class.new.execute(build) }.not_to raise_error + + expect(Ci::TestCase.count).to eq(2) + expect(Ci::TestCaseFailure.count).to eq(2) + end + end + + context 'when test failure data have duplicates within the same payload (happens when the JUnit report has duplicate test case names but have different failures)' do + let(:build) { create(:ci_build, :success, :test_reports_with_duplicate_failed_test_names) } # The test report has 2 test case failures but with the same test case keys + + it 'does not fail but does not persist duplicate data' do + expect { described_class.new.execute(build) }.not_to raise_error + + expect(Ci::TestCase.count).to eq(1) + expect(Ci::TestCaseFailure.count).to eq(1) + end + end + + context 'when number of failed test cases exceed the limit' do + before do + stub_const("#{described_class.name}::MAX_TRACKABLE_FAILURES", 1) + end + + it 'does not persist data' do + execute_service + + expect(Ci::TestCase.count).to eq(0) + expect(Ci::TestCaseFailure.count).to eq(0) + end + end + end + + context 'when build has no test reports' do + let(:build) { create(:ci_build, :running) } + + it 'does not persist data' do + execute_service + + expect(Ci::TestCase.count).to eq(0) + expect(Ci::TestCaseFailure.count).to eq(0) + end + end + end +end diff --git a/spec/services/clusters/applications/check_installation_progress_service_spec.rb b/spec/services/clusters/applications/check_installation_progress_service_spec.rb index 13f7cd62002..698804ff6af 100644 --- a/spec/services/clusters/applications/check_installation_progress_service_spec.rb +++ b/spec/services/clusters/applications/check_installation_progress_service_spec.rb @@ -161,10 +161,10 @@ RSpec.describe Clusters::Applications::CheckInstallationProgressService, '#execu expect(application.status_reason).to be_nil end - it 'tracks application install' do - expect(Gitlab::Tracking).to receive(:event).with('cluster:applications', "cluster_application_helm_installed") - + it 'tracks application install', :snowplow do service.execute + + expect_snowplow_event(category: 'cluster:applications', action: 'cluster_application_helm_installed') end end diff --git a/spec/services/clusters/applications/uninstall_service_spec.rb b/spec/services/clusters/applications/uninstall_service_spec.rb index 50d7e82c47e..bfe38ba670d 100644 --- a/spec/services/clusters/applications/uninstall_service_spec.rb +++ b/spec/services/clusters/applications/uninstall_service_spec.rb @@ -14,7 +14,7 @@ RSpec.describe Clusters::Applications::UninstallService, '#execute' do context 'when there are no errors' do before do - expect(helm_client).to receive(:uninstall).with(kind_of(Gitlab::Kubernetes::Helm::DeleteCommand)) + expect(helm_client).to receive(:uninstall).with(kind_of(Gitlab::Kubernetes::Helm::V3::DeleteCommand)) allow(worker_class).to receive(:perform_in).and_return(nil) end @@ -36,7 +36,7 @@ RSpec.describe Clusters::Applications::UninstallService, '#execute' do let(:error) { Kubeclient::HttpError.new(500, 'system failure', nil) } before do - expect(helm_client).to receive(:uninstall).with(kind_of(Gitlab::Kubernetes::Helm::DeleteCommand)).and_raise(error) + expect(helm_client).to receive(:uninstall).with(kind_of(Gitlab::Kubernetes::Helm::V3::DeleteCommand)).and_raise(error) end include_examples 'logs kubernetes errors' do @@ -58,7 +58,7 @@ RSpec.describe Clusters::Applications::UninstallService, '#execute' do let(:error) { StandardError.new('something bad happened') } before do - expect(helm_client).to receive(:uninstall).with(kind_of(Gitlab::Kubernetes::Helm::DeleteCommand)).and_raise(error) + expect(helm_client).to receive(:uninstall).with(kind_of(Gitlab::Kubernetes::Helm::V3::DeleteCommand)).and_raise(error) end include_examples 'logs kubernetes errors' do diff --git a/spec/services/clusters/aws/authorize_role_service_spec.rb b/spec/services/clusters/aws/authorize_role_service_spec.rb index 5b47cf0ecde..302bae6e3ff 100644 --- a/spec/services/clusters/aws/authorize_role_service_spec.rb +++ b/spec/services/clusters/aws/authorize_role_service_spec.rb @@ -11,14 +11,16 @@ RSpec.describe Clusters::Aws::AuthorizeRoleService do let(:credentials_service) { instance_double(Clusters::Aws::FetchCredentialsService, execute: credentials) } let(:role_arn) { 'arn:my-role' } + let(:region) { 'region' } let(:params) do params = ActionController::Parameters.new({ cluster: { - role_arn: role_arn + role_arn: role_arn, + region: region } }) - params.require(:cluster).permit(:role_arn) + params.require(:cluster).permit(:role_arn, :region) end before do diff --git a/spec/services/clusters/aws/fetch_credentials_service_spec.rb b/spec/services/clusters/aws/fetch_credentials_service_spec.rb index a0e63d96a5c..361a947f634 100644 --- a/spec/services/clusters/aws/fetch_credentials_service_spec.rb +++ b/spec/services/clusters/aws/fetch_credentials_service_spec.rb @@ -19,7 +19,7 @@ RSpec.describe Clusters::Aws::FetchCredentialsService do subject { described_class.new(provision_role, provider: provider).execute } context 'provision role is configured' do - let(:provision_role) { create(:aws_role, user: user) } + let(:provision_role) { create(:aws_role, user: user, region: 'custom-region') } before do stub_application_setting(eks_access_key_id: gitlab_access_key_id) @@ -53,10 +53,12 @@ RSpec.describe Clusters::Aws::FetchCredentialsService do context 'provider is not specifed' do let(:provider) { nil } - let(:region) { Clusters::Providers::Aws::DEFAULT_REGION } + let(:region) { provision_role.region } let(:session_name) { "gitlab-eks-autofill-user-#{user.id}" } let(:session_policy) { 'policy-document' } + subject { described_class.new(provision_role, provider: provider).execute } + before do allow(File).to receive(:read) .with(Rails.root.join('vendor', 'aws', 'iam', 'eks_cluster_read_only_policy.json')) @@ -64,6 +66,13 @@ RSpec.describe Clusters::Aws::FetchCredentialsService do end it { is_expected.to eq assumed_role_credentials } + + context 'region is not specifed' do + let(:region) { Clusters::Providers::Aws::DEFAULT_REGION } + let(:provision_role) { create(:aws_role, user: user, region: nil) } + + it { is_expected.to eq assumed_role_credentials } + end end end diff --git a/spec/services/clusters/kubernetes/create_or_update_namespace_service_spec.rb b/spec/services/clusters/kubernetes/create_or_update_namespace_service_spec.rb index 7e3f1fdb379..90956e7b4ea 100644 --- a/spec/services/clusters/kubernetes/create_or_update_namespace_service_spec.rb +++ b/spec/services/clusters/kubernetes/create_or_update_namespace_service_spec.rb @@ -28,6 +28,7 @@ RSpec.describe Clusters::Kubernetes::CreateOrUpdateNamespaceService, '#execute' stub_kubeclient_get_secret_error(api_url, 'gitlab-token') stub_kubeclient_create_secret(api_url) + stub_kubeclient_delete_role_binding(api_url, "gitlab-#{namespace}", namespace: namespace) stub_kubeclient_put_role_binding(api_url, "gitlab-#{namespace}", namespace: namespace) stub_kubeclient_get_namespace(api_url, namespace: namespace) stub_kubeclient_get_service_account_error(api_url, "#{namespace}-service-account", namespace: namespace) diff --git a/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb b/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb index 257e2e53733..a4f018aec0c 100644 --- a/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb +++ b/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb @@ -141,6 +141,7 @@ RSpec.describe Clusters::Kubernetes::CreateOrUpdateServiceAccountService do before do cluster.platform_kubernetes.rbac! + stub_kubeclient_delete_role_binding(api_url, role_binding_name, namespace: namespace) stub_kubeclient_put_role_binding(api_url, role_binding_name, namespace: namespace) stub_kubeclient_put_role(api_url, Clusters::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_NAME, namespace: namespace) stub_kubeclient_put_role_binding(api_url, Clusters::Kubernetes::GITLAB_KNATIVE_SERVING_ROLE_BINDING_NAME, namespace: namespace) @@ -160,60 +161,26 @@ RSpec.describe Clusters::Kubernetes::CreateOrUpdateServiceAccountService do it_behaves_like 'creates service account and token' - context 'kubernetes_cluster_namespace_role_admin FF is enabled' do - before do - stub_feature_flags(kubernetes_cluster_namespace_role_admin: true) - end - - it 'creates a namespaced role binding with admin access' do - subject - - expect(WebMock).to have_requested(:put, api_url + "/apis/rbac.authorization.k8s.io/v1/namespaces/#{namespace}/rolebindings/#{role_binding_name}").with( - body: hash_including( - metadata: { name: "gitlab-#{namespace}", namespace: "#{namespace}" }, - roleRef: { - apiGroup: 'rbac.authorization.k8s.io', - kind: 'ClusterRole', - name: 'admin' - }, - subjects: [ - { - kind: 'ServiceAccount', - name: service_account_name, - namespace: namespace - } - ] - ) - ) - end - end + it 'creates a namespaced role binding with admin access' do + subject - context 'kubernetes_cluster_namespace_role_admin FF is disabled' do - before do - stub_feature_flags(kubernetes_cluster_namespace_role_admin: false) - end - - it 'creates a namespaced role binding with edit access' do - subject - - expect(WebMock).to have_requested(:put, api_url + "/apis/rbac.authorization.k8s.io/v1/namespaces/#{namespace}/rolebindings/#{role_binding_name}").with( - body: hash_including( - metadata: { name: "gitlab-#{namespace}", namespace: "#{namespace}" }, - roleRef: { - apiGroup: 'rbac.authorization.k8s.io', - kind: 'ClusterRole', - name: 'edit' - }, - subjects: [ - { - kind: 'ServiceAccount', - name: service_account_name, - namespace: namespace - } - ] - ) + expect(WebMock).to have_requested(:put, api_url + "/apis/rbac.authorization.k8s.io/v1/namespaces/#{namespace}/rolebindings/#{role_binding_name}").with( + body: hash_including( + metadata: { name: "gitlab-#{namespace}", namespace: "#{namespace}" }, + roleRef: { + apiGroup: 'rbac.authorization.k8s.io', + kind: 'ClusterRole', + name: 'admin' + }, + subjects: [ + { + kind: 'ServiceAccount', + name: service_account_name, + namespace: namespace + } + ] ) - end + ) end it 'creates a role binding granting crossplane database permissions to the service account' do diff --git a/spec/services/clusters/update_service_spec.rb b/spec/services/clusters/update_service_spec.rb index e496ccd5c23..9aead97f41c 100644 --- a/spec/services/clusters/update_service_spec.rb +++ b/spec/services/clusters/update_service_spec.rb @@ -197,7 +197,7 @@ RSpec.describe Clusters::UpdateService do context 'manangement_project is outside of the namespace scope' do before do - management_project.update(group: create(:group)) + management_project.update!(group: create(:group)) end let(:params) do @@ -224,7 +224,7 @@ RSpec.describe Clusters::UpdateService do context 'manangement_project is outside of the namespace scope' do before do - management_project.update(group: create(:group)) + management_project.update!(group: create(:group)) end let(:params) do diff --git a/spec/services/container_expiration_policies/cleanup_service_spec.rb b/spec/services/container_expiration_policies/cleanup_service_spec.rb new file mode 100644 index 00000000000..2da35cfc3fb --- /dev/null +++ b/spec/services/container_expiration_policies/cleanup_service_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ContainerExpirationPolicies::CleanupService do + let_it_be(:repository, reload: true) { create(:container_repository) } + let_it_be(:project) { repository.project } + + let(:service) { described_class.new(repository) } + + describe '#execute' do + subject { service.execute } + + context 'with a successful cleanup tags service execution' do + let(:cleanup_tags_service_params) { project.container_expiration_policy.policy_params.merge('container_expiration_policy' => true) } + let(:cleanup_tags_service) { instance_double(Projects::ContainerRepository::CleanupTagsService) } + + it 'completely clean up the repository' do + expect(Projects::ContainerRepository::CleanupTagsService) + .to receive(:new).with(project, nil, cleanup_tags_service_params).and_return(cleanup_tags_service) + expect(cleanup_tags_service).to receive(:execute).with(repository).and_return(status: :success) + + response = subject + + aggregate_failures "checking the response and container repositories" do + expect(response.success?).to eq(true) + expect(response.payload).to include(cleanup_status: :finished, container_repository_id: repository.id) + expect(ContainerRepository.waiting_for_cleanup.count).to eq(0) + expect(repository.reload.cleanup_unscheduled?).to be_truthy + expect(repository.expiration_policy_started_at).to eq(nil) + end + end + end + + context 'without a successful cleanup tags service execution' do + it 'partially clean up the repository' do + expect(Projects::ContainerRepository::CleanupTagsService) + .to receive(:new).and_return(double(execute: { status: :error, message: 'timeout' })) + + response = subject + + aggregate_failures "checking the response and container repositories" do + expect(response.success?).to eq(true) + expect(response.payload).to include(cleanup_status: :unfinished, container_repository_id: repository.id) + expect(ContainerRepository.waiting_for_cleanup.count).to eq(1) + expect(repository.reload.cleanup_unfinished?).to be_truthy + expect(repository.expiration_policy_started_at).not_to eq(nil) + end + end + end + + context 'with no repository' do + let(:service) { described_class.new(nil) } + + it 'returns an error response' do + response = subject + + expect(response.success?).to eq(false) + end + end + end +end diff --git a/spec/services/container_expiration_policy_service_spec.rb b/spec/services/container_expiration_policy_service_spec.rb index dfce51d73ad..4294e6b3f06 100644 --- a/spec/services/container_expiration_policy_service_spec.rb +++ b/spec/services/container_expiration_policy_service_spec.rb @@ -27,20 +27,5 @@ RSpec.describe ContainerExpirationPolicyService do expect(container_expiration_policy.next_run_at).to be > Time.zone.now end - - context 'with an invalid container expiration policy' do - before do - allow(container_expiration_policy).to receive(:valid?).and_return(false) - end - - it 'disables it' do - expect(container_expiration_policy).not_to receive(:schedule_next_run!) - expect(CleanupContainerRepositoryWorker).not_to receive(:perform_async) - - expect { subject } - .to change { container_expiration_policy.reload.enabled }.from(true).to(false) - .and raise_error(ContainerExpirationPolicyService::InvalidPolicyError) - end - end end end diff --git a/spec/services/dependency_proxy/download_blob_service_spec.rb b/spec/services/dependency_proxy/download_blob_service_spec.rb new file mode 100644 index 00000000000..4b5c6b5bd6a --- /dev/null +++ b/spec/services/dependency_proxy/download_blob_service_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe DependencyProxy::DownloadBlobService do + include DependencyProxyHelpers + + let(:image) { 'alpine' } + let(:token) { Digest::SHA256.hexdigest('123') } + let(:blob_sha) { Digest::SHA256.hexdigest('ruby:2.7.0') } + + subject { described_class.new(image, blob_sha, token).execute } + + context 'remote request is successful' do + before do + stub_blob_download(image, blob_sha) + end + + it { expect(subject[:status]).to eq(:success) } + it { expect(subject[:file]).to be_a(Tempfile) } + it { expect(subject[:file].size).to eq(6) } + end + + context 'remote request is not found' do + before do + stub_blob_download(image, blob_sha, 404) + end + + it { expect(subject[:status]).to eq(:error) } + it { expect(subject[:http_status]).to eq(404) } + it { expect(subject[:message]).to eq('Non-success response code on downloading blob fragment') } + end + + context 'net timeout exception' do + before do + blob_url = DependencyProxy::Registry.blob_url(image, blob_sha) + + stub_full_request(blob_url).to_timeout + end + + it { expect(subject[:status]).to eq(:error) } + it { expect(subject[:http_status]).to eq(599) } + it { expect(subject[:message]).to eq('execution expired') } + end +end diff --git a/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb b/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb new file mode 100644 index 00000000000..4ba53d49d38 --- /dev/null +++ b/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe DependencyProxy::FindOrCreateBlobService do + include DependencyProxyHelpers + + let(:blob) { create(:dependency_proxy_blob) } + let(:group) { blob.group } + let(:image) { 'alpine' } + let(:tag) { '3.9' } + let(:token) { Digest::SHA256.hexdigest('123') } + let(:blob_sha) { '40bd001563085fc35165329ea1ff5c5ecbdbbeef' } + + subject { described_class.new(group, image, token, blob_sha).execute } + + before do + stub_registry_auth(image, token) + end + + context 'no cache' do + before do + stub_blob_download(image, blob_sha) + end + + it 'downloads blob from remote registry if there is no cached one' do + expect(subject[:status]).to eq(:success) + expect(subject[:blob]).to be_a(DependencyProxy::Blob) + expect(subject[:blob]).to be_persisted + end + end + + context 'cached blob' do + let(:blob_sha) { blob.file_name.sub('.gz', '') } + + it 'uses cached blob instead of downloading one' do + expect(subject[:status]).to eq(:success) + expect(subject[:blob]).to be_a(DependencyProxy::Blob) + expect(subject[:blob]).to eq(blob) + end + end + + context 'no such blob exists remotely' do + before do + stub_blob_download(image, blob_sha, 404) + end + + it 'returns error message and http status' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Failed to download the blob') + expect(subject[:http_status]).to eq(404) + end + end +end diff --git a/spec/services/dependency_proxy/pull_manifest_service_spec.rb b/spec/services/dependency_proxy/pull_manifest_service_spec.rb new file mode 100644 index 00000000000..030ed9c001b --- /dev/null +++ b/spec/services/dependency_proxy/pull_manifest_service_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe DependencyProxy::PullManifestService do + include DependencyProxyHelpers + + let(:image) { 'alpine' } + let(:tag) { '3.9' } + let(:token) { Digest::SHA256.hexdigest('123') } + let(:manifest) { { foo: 'bar' }.to_json } + + subject { described_class.new(image, tag, token).execute } + + context 'remote request is successful' do + before do + stub_manifest_download(image, tag) + end + + it { expect(subject[:status]).to eq(:success) } + it { expect(subject[:manifest]).to eq(manifest) } + end + + context 'remote request is not found' do + before do + stub_manifest_download(image, tag, 404, 'Not found') + end + + it { expect(subject[:status]).to eq(:error) } + it { expect(subject[:http_status]).to eq(404) } + it { expect(subject[:message]).to eq('Not found') } + end + + context 'net timeout exception' do + before do + manifest_link = DependencyProxy::Registry.manifest_url(image, tag) + + stub_full_request(manifest_link).to_timeout + end + + it { expect(subject[:status]).to eq(:error) } + it { expect(subject[:http_status]).to eq(599) } + it { expect(subject[:message]).to eq('execution expired') } + end +end diff --git a/spec/services/dependency_proxy/request_token_service_spec.rb b/spec/services/dependency_proxy/request_token_service_spec.rb new file mode 100644 index 00000000000..8b3ba783b8d --- /dev/null +++ b/spec/services/dependency_proxy/request_token_service_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe DependencyProxy::RequestTokenService do + include DependencyProxyHelpers + + let(:image) { 'alpine:3.9' } + let(:token) { Digest::SHA256.hexdigest('123') } + + subject { described_class.new(image).execute } + + context 'remote request is successful' do + before do + stub_registry_auth(image, token) + end + + it { expect(subject[:status]).to eq(:success) } + it { expect(subject[:token]).to eq(token) } + end + + context 'remote request is not found' do + before do + stub_registry_auth(image, token, 404) + end + + it { expect(subject[:status]).to eq(:error) } + it { expect(subject[:http_status]).to eq(404) } + it { expect(subject[:message]).to eq('Expected 200 response code for an access token') } + end + + context 'failed to parse response body' do + before do + stub_registry_auth(image, token, 200, 'dasd1321: wow') + end + + it { expect(subject[:status]).to eq(:error) } + it { expect(subject[:http_status]).to eq(500) } + it { expect(subject[:message]).to eq('Failed to parse a response body for an access token') } + end + + context 'net timeout exception' do + before do + auth_link = DependencyProxy::Registry.auth_url(image) + + stub_full_request(auth_link, method: :any).to_timeout + end + + it { expect(subject[:status]).to eq(:error) } + it { expect(subject[:http_status]).to eq(599) } + it { expect(subject[:message]).to eq('execution expired') } + end +end diff --git a/spec/services/deploy_keys/collect_keys_service_spec.rb b/spec/services/deploy_keys/collect_keys_service_spec.rb deleted file mode 100644 index 3442e5e456a..00000000000 --- a/spec/services/deploy_keys/collect_keys_service_spec.rb +++ /dev/null @@ -1,58 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe DeployKeys::CollectKeysService do - let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project, :private) } - - subject { DeployKeys::CollectKeysService.new(project, user) } - - before do - project&.add_developer(user) - end - - context 'when no project is passed in' do - let(:project) { nil } - - it 'returns an empty Array' do - expect(subject.execute).to be_empty - end - end - - context 'when no user is passed in' do - let(:user) { nil } - - it 'returns an empty Array' do - expect(subject.execute).to be_empty - end - end - - context 'when a project is passed in' do - let_it_be(:deploy_keys_project) { create(:deploy_keys_project, :write_access, project: project) } - let_it_be(:deploy_key) { deploy_keys_project.deploy_key } - - it 'only returns deploy keys with write access' do - create(:deploy_keys_project, project: project) - - expect(subject.execute).to contain_exactly(deploy_key) - end - - it 'returns deploy keys only for this project' do - other_project = create(:project) - create(:deploy_keys_project, :write_access, project: other_project) - - expect(subject.execute).to contain_exactly(deploy_key) - end - end - - context 'when the user cannot read the project' do - before do - project.members.delete_all - end - - it 'returns an empty Array' do - expect(subject.execute).to be_empty - end - end -end diff --git a/spec/services/design_management/copy_design_collection/copy_service_spec.rb b/spec/services/design_management/copy_design_collection/copy_service_spec.rb index e93e5f13fea..ddbed91815f 100644 --- a/spec/services/design_management/copy_design_collection/copy_service_spec.rb +++ b/spec/services/design_management/copy_design_collection/copy_service_spec.rb @@ -68,6 +68,31 @@ RSpec.describe DesignManagement::CopyDesignCollection::CopyService, :clean_gitla include_examples 'service error', message: 'Target design collection already has designs' end + context 'when target project already has designs' do + let!(:issue_x) { create(:issue, project: target_issue.project) } + let!(:existing) { create(:design, issue: issue_x, project: target_issue.project) } + + let(:new_designs) do + target_issue.reset + target_issue.designs.where.not(id: existing.id) + end + + it 'sets IIDs for new designs above existing ones' do + subject + + expect(new_designs).to all(have_attributes(iid: (be > existing.iid))) + end + + it 'does not allow for IID collisions' do + subject + create(:design, issue: issue_x, project: target_issue.project) + + design_iids = target_issue.project.designs.map(&:id) + + expect(design_iids).to match_array(design_iids.uniq) + end + end + include_examples 'service success' it 'creates a design repository for the target project' do @@ -162,9 +187,7 @@ RSpec.describe DesignManagement::CopyDesignCollection::CopyService, :clean_gitla it 'copies the Git repository data', :aggregate_failures do subject - commit_shas = target_repository.commits('master', limit: 99).map(&:id) - - expect(commit_shas).to include(*target_issue.design_versions.ordered.pluck(:sha)) + expect(commits_on_master(limit: 99)).to include(*target_issue.design_versions.ordered.pluck(:sha)) end it 'creates a master branch if none previously existed' do @@ -212,9 +235,7 @@ RSpec.describe DesignManagement::CopyDesignCollection::CopyService, :clean_gitla issue_2 = create(:issue, project: target_issue.project) create(:design, :with_file, issue: issue_2, project: target_issue.project) - expect { subject }.not_to change { - expect(target_repository.commits('master', limit: 10).size).to eq(1) - } + expect { subject }.not_to change { commits_on_master } end it 'sets the design collection copy state' do @@ -223,6 +244,10 @@ RSpec.describe DesignManagement::CopyDesignCollection::CopyService, :clean_gitla expect(target_issue.design_collection).to be_copy_error end end + + def commits_on_master(limit: 10) + target_repository.commits('master', limit: limit).map(&:id) + end end end end diff --git a/spec/services/design_management/generate_image_versions_service_spec.rb b/spec/services/design_management/generate_image_versions_service_spec.rb index 749030af97d..e06b6fbf116 100644 --- a/spec/services/design_management/generate_image_versions_service_spec.rb +++ b/spec/services/design_management/generate_image_versions_service_spec.rb @@ -44,7 +44,7 @@ RSpec.describe DesignManagement::GenerateImageVersionsService do end it 'logs if the raw image cannot be found' do - version.designs.first.update(filename: 'foo.png') + version.designs.first.update!(filename: 'foo.png') expect(Gitlab::AppLogger).to receive(:error).with("No design file found for Action: #{action.id}") diff --git a/spec/services/discussions/resolve_service_spec.rb b/spec/services/discussions/resolve_service_spec.rb index 5ff0d535b46..42c4ef52741 100644 --- a/spec/services/discussions/resolve_service_spec.rb +++ b/spec/services/discussions/resolve_service_spec.rb @@ -40,11 +40,11 @@ RSpec.describe Discussions::ResolveService do context 'with a project that requires all discussion to be resolved' do before do - project.update(only_allow_merge_if_all_discussions_are_resolved: true) + project.update!(only_allow_merge_if_all_discussions_are_resolved: true) end after do - project.update(only_allow_merge_if_all_discussions_are_resolved: false) + project.update!(only_allow_merge_if_all_discussions_are_resolved: false) end let_it_be(:other_discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion } diff --git a/spec/services/draft_notes/destroy_service_spec.rb b/spec/services/draft_notes/destroy_service_spec.rb index f725f08f3c7..1f246a56eb3 100644 --- a/spec/services/draft_notes/destroy_service_spec.rb +++ b/spec/services/draft_notes/destroy_service_spec.rb @@ -22,7 +22,7 @@ RSpec.describe DraftNotes::DestroyService do it 'destroys all draft notes for a user in a merge request' do create_list(:draft_note, 2, merge_request: merge_request, author: user) - expect { destroy }.to change { DraftNote.count }.by(-2) + expect { destroy }.to change { DraftNote.count }.by(-2) # rubocop:disable Rails/SaveBang expect(DraftNote.count).to eq(0) end @@ -45,7 +45,7 @@ RSpec.describe DraftNotes::DestroyService do allow_any_instance_of(DraftNote).to receive_message_chain(:diff_file, :unfolded?) { true } expect(merge_request).to receive_message_chain(:diffs, :clear_cache) - destroy + destroy # rubocop:disable Rails/SaveBang end end end diff --git a/spec/services/emails/confirm_service_spec.rb b/spec/services/emails/confirm_service_spec.rb index 935a673f548..d3a745bc744 100644 --- a/spec/services/emails/confirm_service_spec.rb +++ b/spec/services/emails/confirm_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Emails::ConfirmService do describe '#execute' do it 'enqueues a background job to send confirmation email again' do - email = user.emails.create(email: 'new@email.com') + email = user.emails.create!(email: 'new@email.com') expect { service.execute(email) }.to have_enqueued_job.on_queue('mailers') end diff --git a/spec/services/feature_flags/update_service_spec.rb b/spec/services/feature_flags/update_service_spec.rb index a982dd5166b..66a75a2c24e 100644 --- a/spec/services/feature_flags/update_service_spec.rb +++ b/spec/services/feature_flags/update_service_spec.rb @@ -100,6 +100,13 @@ RSpec.describe FeatureFlags::UpdateService do include('Updated active from <strong>"true"</strong> to <strong>"false"</strong>.') ) end + + it 'executes hooks' do + hook = create(:project_hook, :all_events_enabled, project: project) + expect(WebHookWorker).to receive(:perform_async).with(hook.id, an_instance_of(Hash), 'feature_flag_hooks') + + subject + end end context 'when scope active state is changed' do diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index 31afdba8192..e06f09d0463 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -95,7 +95,7 @@ RSpec.describe Groups::DestroyService do context 'projects in pending_delete' do before do project.pending_delete = true - project.save + project.save! end it_behaves_like 'group destruction', false diff --git a/spec/services/groups/import_export/import_service_spec.rb b/spec/services/groups/import_export/import_service_spec.rb index f284225e23a..f8cb55a9955 100644 --- a/spec/services/groups/import_export/import_service_spec.rb +++ b/spec/services/groups/import_export/import_service_spec.rb @@ -63,7 +63,7 @@ RSpec.describe Groups::ImportExport::ImportService do before do stub_feature_flags(group_import_ndjson: false) - ImportExportUpload.create(group: group, import_file: import_file) + ImportExportUpload.create!(group: group, import_file: import_file) allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger) allow(import_logger).to receive(:error) @@ -105,7 +105,7 @@ RSpec.describe Groups::ImportExport::ImportService do subject { service.execute } before do - ImportExportUpload.create(group: group, import_file: import_file) + ImportExportUpload.create!(group: group, import_file: import_file) allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger) allow(import_logger).to receive(:error) @@ -216,7 +216,7 @@ RSpec.describe Groups::ImportExport::ImportService do subject { service.execute } before do - ImportExportUpload.create(group: group, import_file: import_file) + ImportExportUpload.create!(group: group, import_file: import_file) allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger) allow(import_logger).to receive(:error) diff --git a/spec/services/issuable/common_system_notes_service_spec.rb b/spec/services/issuable/common_system_notes_service_spec.rb index fc01ee8f672..a988ab81754 100644 --- a/spec/services/issuable/common_system_notes_service_spec.rb +++ b/spec/services/issuable/common_system_notes_service_spec.rb @@ -36,28 +36,28 @@ RSpec.describe Issuable::CommonSystemNotesService do context 'adding Draft note' do let(:issuable) { create(:merge_request, title: "merge request") } - it_behaves_like 'system note creation', { title: "Draft: merge request" }, 'marked as a **Work In Progress**' + it_behaves_like 'system note creation', { title: "Draft: merge request" }, 'marked this merge request as **draft**' context 'and changing title' do before do issuable.update_attribute(:title, "Draft: changed title") end - it_behaves_like 'draft notes creation', 'marked' + it_behaves_like 'draft notes creation', 'draft' end end context 'removing Draft note' do let(:issuable) { create(:merge_request, title: "Draft: merge request") } - it_behaves_like 'system note creation', { title: "merge request" }, 'unmarked as a **Work In Progress**' + it_behaves_like 'system note creation', { title: "merge request" }, 'marked this merge request as **ready**' context 'and changing title' do before do issuable.update_attribute(:title, "changed title") end - it_behaves_like 'draft notes creation', 'unmarked' + it_behaves_like 'draft notes creation', 'ready' end end end diff --git a/spec/services/issues/import_csv_service_spec.rb b/spec/services/issues/import_csv_service_spec.rb index cc3e1d23a74..fa40b75190f 100644 --- a/spec/services/issues/import_csv_service_spec.rb +++ b/spec/services/issues/import_csv_service_spec.rb @@ -5,108 +5,15 @@ require 'spec_helper' RSpec.describe Issues::ImportCsvService do let(:project) { create(:project) } let(:user) { create(:user) } - - subject do + let(:service) do uploader = FileUploader.new(project) uploader.store!(file) - described_class.new(user, project, uploader).execute + described_class.new(user, project, uploader) end - describe '#execute' do - context 'invalid file' do - let(:file) { fixture_file_upload('spec/fixtures/banana_sample.gif') } - - it 'returns invalid file error' do - expect(Notify).to receive_message_chain(:import_issues_csv_email, :deliver_later) - - expect(subject[:success]).to eq(0) - expect(subject[:parse_error]).to eq(true) - end - end - - context 'with a file generated by Gitlab CSV export' do - let(:file) { fixture_file_upload('spec/fixtures/csv_gitlab_export.csv') } - - it 'imports the CSV without errors' do - expect(Notify).to receive_message_chain(:import_issues_csv_email, :deliver_later) - - expect(subject[:success]).to eq(4) - expect(subject[:error_lines]).to eq([]) - expect(subject[:parse_error]).to eq(false) - end - - it 'correctly sets the issue attributes' do - expect { subject }.to change { project.issues.count }.by 4 - - expect(project.issues.reload.last).to have_attributes( - title: 'Test Title', - description: 'Test Description' - ) - end - end - - context 'comma delimited file' do - let(:file) { fixture_file_upload('spec/fixtures/csv_comma.csv') } - - it 'imports CSV without errors' do - expect(Notify).to receive_message_chain(:import_issues_csv_email, :deliver_later) - - expect(subject[:success]).to eq(3) - expect(subject[:error_lines]).to eq([]) - expect(subject[:parse_error]).to eq(false) - end - - it 'correctly sets the issue attributes' do - expect { subject }.to change { project.issues.count }.by 3 - - expect(project.issues.reload.last).to have_attributes( - title: 'Title with quote"', - description: 'Description' - ) - end - end - - context 'tab delimited file with error row' do - let(:file) { fixture_file_upload('spec/fixtures/csv_tab.csv') } - - it 'imports CSV with some error rows' do - expect(Notify).to receive_message_chain(:import_issues_csv_email, :deliver_later) - - expect(subject[:success]).to eq(2) - expect(subject[:error_lines]).to eq([3]) - expect(subject[:parse_error]).to eq(false) - end - - it 'correctly sets the issue attributes' do - expect { subject }.to change { project.issues.count }.by 2 - - expect(project.issues.reload.last).to have_attributes( - title: 'Hello', - description: 'World' - ) - end - end - - context 'semicolon delimited file with CRLF' do - let(:file) { fixture_file_upload('spec/fixtures/csv_semicolon.csv') } - - it 'imports CSV with a blank row' do - expect(Notify).to receive_message_chain(:import_issues_csv_email, :deliver_later) - - expect(subject[:success]).to eq(3) - expect(subject[:error_lines]).to eq([4]) - expect(subject[:parse_error]).to eq(false) - end - - it 'correctly sets the issue attributes' do - expect { subject }.to change { project.issues.count }.by 3 - - expect(project.issues.reload.last).to have_attributes( - title: 'Hello', - description: 'World' - ) - end - end + include_examples 'issuable import csv service', 'issue' do + let(:issuables) { project.issues } + let(:email_method) { :import_issues_csv_email } end end diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index ae1454ce9bb..9b8d21bb8eb 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -321,21 +321,40 @@ RSpec.describe Issues::MoveService do before do authorized_project.add_developer(user) + authorized_project.add_developer(admin) authorized_project2.add_developer(user) + authorized_project2.add_developer(admin) end context 'multiple related issues' do - it 'moves all related issues and retains permissions' do - new_issue = move_service.execute(old_issue, new_project) + context 'when admin mode is enabled', :enable_admin_mode 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 - expect(new_issue.related_issues(admin)) - .to match_array([authorized_issue_b, authorized_issue_c, authorized_issue_d, unauthorized_issue]) + context 'when admin mode is disabled' do + it 'moves all related issues and retains permissions' do + new_issue = move_service.execute(old_issue, new_project) - expect(new_issue.related_issues(user)) - .to match_array([authorized_issue_b, authorized_issue_c, authorized_issue_d]) + expect(new_issue.related_issues(admin)) + .to match_array([authorized_issue_b, authorized_issue_c, authorized_issue_d]) - expect(authorized_issue_d.related_issues(user)) - .to match_array([new_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 end diff --git a/spec/services/issues/related_branches_service_spec.rb b/spec/services/issues/related_branches_service_spec.rb index 1780023803a..a8a1f95e800 100644 --- a/spec/services/issues/related_branches_service_spec.rb +++ b/spec/services/issues/related_branches_service_spec.rb @@ -74,8 +74,16 @@ RSpec.describe Issues::RelatedBranchesService do context 'the user has access to otherwise unreadable pipelines' do let(:user) { create(:admin) } - it 'returns info a developer could not see' do - expect(branch_info.pluck(:pipeline_status)).to include(an_instance_of(Gitlab::Ci::Status::Running)) + context 'when admin mode is enabled', :enable_admin_mode do + it 'returns info a developer could not see' do + expect(branch_info.pluck(:pipeline_status)).to include(an_instance_of(Gitlab::Ci::Status::Running)) + end + end + + context 'when admin mode is disabled' do + it 'does not return info a developer could not see' do + expect(branch_info.pluck(:pipeline_status)).not_to include(an_instance_of(Gitlab::Ci::Status::Running)) + end end end diff --git a/spec/services/issues/zoom_link_service_spec.rb b/spec/services/issues/zoom_link_service_spec.rb index b095cb24212..8e8adc516cf 100644 --- a/spec/services/issues/zoom_link_service_spec.rb +++ b/spec/services/issues/zoom_link_service_spec.rb @@ -46,10 +46,15 @@ RSpec.describe Issues::ZoomLinkService do expect(ZoomMeeting.canonical_meeting_url(issue)).to eq(zoom_link) end - it 'tracks the add event' do - expect(Gitlab::Tracking).to receive(:event) - .with('IncidentManagement::ZoomIntegration', 'add_zoom_meeting', label: 'Issue ID', value: issue.id) + it 'tracks the add event', :snowplow do result + + expect_snowplow_event( + category: 'IncidentManagement::ZoomIntegration', + action: 'add_zoom_meeting', + label: 'Issue ID', + value: issue.id + ) end it 'creates a zoom_link_added notification' do @@ -180,10 +185,15 @@ RSpec.describe Issues::ZoomLinkService do expect(ZoomMeeting.canonical_meeting_url(issue)).to eq(nil) end - it 'tracks the remove event' do - expect(Gitlab::Tracking).to receive(:event) - .with('IncidentManagement::ZoomIntegration', 'remove_zoom_meeting', label: 'Issue ID', value: issue.id) + it 'tracks the remove event', :snowplow do result + + expect_snowplow_event( + category: 'IncidentManagement::ZoomIntegration', + action: 'remove_zoom_meeting', + label: 'Issue ID', + value: issue.id + ) end end diff --git a/spec/services/jira_connect/sync_service_spec.rb b/spec/services/jira_connect/sync_service_spec.rb index e26ca30d0e1..83088bb2e79 100644 --- a/spec/services/jira_connect/sync_service_spec.rb +++ b/spec/services/jira_connect/sync_service_spec.rb @@ -23,7 +23,8 @@ RSpec.describe JiraConnect::SyncService do project: project, commits: commits, branches: [instance_of(Gitlab::Git::Branch)], - merge_requests: merge_requests + merge_requests: merge_requests, + update_sequence_id: anything ).and_return(return_value) end end diff --git a/spec/services/jira_connect_subscriptions/create_service_spec.rb b/spec/services/jira_connect_subscriptions/create_service_spec.rb index 77e758cf6fe..9750c671fa2 100644 --- a/spec/services/jira_connect_subscriptions/create_service_spec.rb +++ b/spec/services/jira_connect_subscriptions/create_service_spec.rb @@ -32,6 +32,36 @@ RSpec.describe JiraConnectSubscriptions::CreateService do it 'returns success' do expect(subject[:status]).to eq(:success) end + + context 'namespace has projects' do + let!(:project_1) { create(:project, group: group) } + let!(:project_2) { create(:project, group: group) } + + before do + stub_const("#{described_class}::MERGE_REQUEST_SYNC_BATCH_SIZE", 1) + end + + it 'starts workers to sync projects in batches with delay' do + allow(Atlassian::JiraConnect::Client).to receive(:generate_update_sequence_id).and_return(123) + + expect(JiraConnect::SyncProjectWorker).to receive(:bulk_perform_in).with(1.minute, [[project_1.id, 123]]) + expect(JiraConnect::SyncProjectWorker).to receive(:bulk_perform_in).with(2.minutes, [[project_2.id, 123]]) + + subject + end + + context 'when the jira_connect_full_namespace_sync feature flag is disabled' do + before do + stub_feature_flags(jira_connect_full_namespace_sync: false) + end + + specify do + expect(JiraConnect::SyncProjectWorker).not_to receive(:bulk_perform_in_with_contexts) + + subject + end + end + end end context 'when path is invalid' do diff --git a/spec/services/jira_import/cloud_users_mapper_service_spec.rb b/spec/services/jira_import/cloud_users_mapper_service_spec.rb index 591f80f3efc..6b06a982a80 100644 --- a/spec/services/jira_import/cloud_users_mapper_service_spec.rb +++ b/spec/services/jira_import/cloud_users_mapper_service_spec.rb @@ -5,15 +5,44 @@ require 'spec_helper' RSpec.describe JiraImport::CloudUsersMapperService do let(:start_at) { 7 } let(:url) { "/rest/api/2/users?maxResults=50&startAt=#{start_at}" } + + let_it_be(:user_1) { create(:user, username: 'randomuser', name: 'USER-name1', email: 'uji@example.com') } + let_it_be(:user_2) { create(:user, username: 'username-2') } + let_it_be(:user_5) { create(:user, username: 'username-5') } + let_it_be(:user_4) { create(:user, email: 'user-4@example.com') } + let_it_be(:user_6) { create(:user, email: 'user-6@example.com') } + let_it_be(:user_7) { create(:user, username: 'username-7') } + let_it_be(:user_8) do + create(:user).tap { |user| create(:email, user: user, email: 'user8_email@example.com') } + end + + let_it_be(:current_user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + let(:jira_users) do [ - { 'accountId' => 'abcd', 'displayName' => 'user1' }, - { 'accountId' => 'efg' }, - { 'accountId' => 'hij', 'displayName' => 'user3', 'emailAddress' => 'user3@example.com' } + { 'accountId' => 'abcd', 'displayName' => 'User-Name1' }, # matched by name + { 'accountId' => 'efg', 'displayName' => 'username-2' }, # matcher by username + { 'accountId' => 'hij' }, # no match + { 'accountId' => '123', 'displayName' => 'user-4', 'emailAddress' => 'user-4@example.com' }, # matched by email + { 'accountId' => '456', 'displayName' => 'username5foo', 'emailAddress' => 'user-5@example.com' }, # no match + { 'accountId' => '789', 'displayName' => 'user-6', 'emailAddress' => 'user-6@example.com' }, # matched by email, no project member + { 'accountId' => 'xyz', 'displayName' => 'username-7', 'emailAddress' => 'user-7@example.com' }, # matched by username, no project member + { 'accountId' => 'vhk', 'displayName' => 'user-8', 'emailAddress' => 'user8_email@example.com' }, # matched by secondary email + { 'accountId' => 'uji', 'displayName' => 'user-9', 'emailAddress' => 'uji@example.com' } # matched by email, same as user_1 ] end describe '#execute' do + before do + project.add_developer(current_user) + project.add_developer(user_1) + project.add_developer(user_2) + group.add_developer(user_4) + group.add_guest(user_8) + end + it_behaves_like 'mapping jira users' end end diff --git a/spec/services/jira_import/server_users_mapper_service_spec.rb b/spec/services/jira_import/server_users_mapper_service_spec.rb index 22cb0327cc5..71cb8aea0be 100644 --- a/spec/services/jira_import/server_users_mapper_service_spec.rb +++ b/spec/services/jira_import/server_users_mapper_service_spec.rb @@ -5,15 +5,44 @@ require 'spec_helper' RSpec.describe JiraImport::ServerUsersMapperService do let(:start_at) { 7 } let(:url) { "/rest/api/2/user/search?username=''&maxResults=50&startAt=#{start_at}" } + + let_it_be(:user_1) { create(:user, username: 'randomuser', name: 'USER-name1', email: 'uji@example.com') } + let_it_be(:user_2) { create(:user, username: 'username-2') } + let_it_be(:user_5) { create(:user, username: 'username-5') } + let_it_be(:user_4) { create(:user, email: 'user-4@example.com') } + let_it_be(:user_6) { create(:user, email: 'user-6@example.com') } + let_it_be(:user_7) { create(:user, username: 'username-7') } + let_it_be(:user_8) do + create(:user).tap { |user| create(:email, user: user, email: 'user8_email@example.com') } + end + + let_it_be(:current_user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + let(:jira_users) do [ - { 'key' => 'abcd', 'name' => 'user1' }, - { 'key' => 'efg' }, - { 'key' => 'hij', 'name' => 'user3', 'emailAddress' => 'user3@example.com' } + { 'key' => 'abcd', 'name' => 'User-Name1' }, # matched by name + { 'key' => 'efg', 'name' => 'username-2' }, # matcher by username + { 'key' => 'hij' }, # no match + { 'key' => '123', 'name' => 'user-4', 'emailAddress' => 'user-4@example.com' }, # matched by email + { 'key' => '456', 'name' => 'username5foo', 'emailAddress' => 'user-5@example.com' }, # no match + { 'key' => '789', 'name' => 'user-6', 'emailAddress' => 'user-6@example.com' }, # matched by email, no project member + { 'key' => 'xyz', 'name' => 'username-7', 'emailAddress' => 'user-7@example.com' }, # matched by username, no project member + { 'key' => 'vhk', 'name' => 'user-8', 'emailAddress' => 'user8_email@example.com' }, # matched by secondary email + { 'key' => 'uji', 'name' => 'user-9', 'emailAddress' => 'uji@example.com' } # matched by email, same as user_1 ] end describe '#execute' do + before do + project.add_developer(current_user) + project.add_developer(user_1) + project.add_developer(user_2) + group.add_developer(user_4) + group.add_guest(user_8) + end + it_behaves_like 'mapping jira users' end end diff --git a/spec/services/jira_import/users_importer_spec.rb b/spec/services/jira_import/users_importer_spec.rb index efb303dab9f..7112443502c 100644 --- a/spec/services/jira_import/users_importer_spec.rb +++ b/spec/services/jira_import/users_importer_spec.rb @@ -6,7 +6,8 @@ RSpec.describe JiraImport::UsersImporter do include JiraServiceHelper let_it_be(:user) { create(:user) } - let_it_be(:project, reload: true) { create(:project) } + let_it_be(:group) { create(:group) } + let_it_be(:project, reload: true) { create(:project, group: group) } let_it_be(:start_at) { 7 } let(:importer) { described_class.new(user, project, start_at) } @@ -18,19 +19,15 @@ RSpec.describe JiraImport::UsersImporter do [ { jira_account_id: 'acc1', - jira_display_name: 'user1', + jira_display_name: 'user-name1', jira_email: 'sample@jira.com', - gitlab_id: nil, - gitlab_username: nil, - gitlab_name: nil + gitlab_id: project_member.id }, { jira_account_id: 'acc2', - jira_display_name: 'user2', + jira_display_name: 'user-name2', jira_email: nil, - gitlab_id: nil, - gitlab_username: nil, - gitlab_name: nil + gitlab_id: group_member.id } ] end @@ -69,13 +66,22 @@ RSpec.describe JiraImport::UsersImporter do context 'when jira client returns an empty array' do let(:jira_users) { [] } - it 'retturns nil payload' do + it 'returns nil payload' do expect(subject.success?).to be_truthy expect(subject.payload).to be_empty end end context 'when jira client returns an results' do + let_it_be(:project_member) { create(:user, email: 'sample@jira.com') } + let_it_be(:group_member) { create(:user, name: 'user-name2') } + let_it_be(:other_user) { create(:user) } + + before do + project.add_developer(project_member) + group.add_developer(group_member) + end + it 'returns the mapped users' do expect(subject.success?).to be_truthy expect(subject.payload).to eq(mapped_users) @@ -90,8 +96,8 @@ RSpec.describe JiraImport::UsersImporter do let(:url) { "/rest/api/2/user/search?username=''&maxResults=50&startAt=#{start_at}" } let(:jira_users) do [ - { 'key' => 'acc1', 'name' => 'user1', 'emailAddress' => 'sample@jira.com' }, - { 'key' => 'acc2', 'name' => 'user2' } + { 'key' => 'acc1', 'name' => 'user-name1', 'emailAddress' => 'sample@jira.com' }, + { 'key' => 'acc2', 'name' => 'user-name2' } ] end @@ -110,8 +116,8 @@ RSpec.describe JiraImport::UsersImporter do let(:url) { "/rest/api/2/users?maxResults=50&startAt=#{start_at}" } let(:jira_users) do [ - { 'accountId' => 'acc1', 'displayName' => 'user1', 'emailAddress' => 'sample@jira.com' }, - { 'accountId' => 'acc2', 'displayName' => 'user2' } + { 'accountId' => 'acc1', 'displayName' => 'user-name1', 'emailAddress' => 'sample@jira.com' }, + { 'accountId' => 'acc2', 'displayName' => 'user-name2' } ] end diff --git a/spec/services/labels/promote_service_spec.rb b/spec/services/labels/promote_service_spec.rb index 7674ec36331..15d53857f33 100644 --- a/spec/services/labels/promote_service_spec.rb +++ b/spec/services/labels/promote_service_spec.rb @@ -63,139 +63,149 @@ RSpec.describe Labels::PromoteService do expect(service.execute(group_label)).to be_falsey end - it 'is truthy on success' do - expect(service.execute(project_label_1_1)).to be_truthy - end + shared_examples 'promoting a project label to a group label' do + it 'is truthy on success' do + expect(service.execute(project_label_1_1)).to be_truthy + end - it 'recreates the label as a group label' do - expect { service.execute(project_label_1_1) } - .to change(project_1.labels, :count).by(-1) - .and change(group_1.labels, :count).by(1) - expect(new_label).not_to be_nil - end + it 'removes all project labels with that title within the group' do + expect { service.execute(project_label_1_1) }.to change(project_2.labels, :count).by(-1).and \ + change(project_3.labels, :count).by(-1) + end - it 'copies title, description and color' do - service.execute(project_label_1_1) + it 'keeps users\' subscriptions' do + user2 = create(:user) + project_label_1_1.subscriptions.create!(user: user, subscribed: true) + project_label_2_1.subscriptions.create!(user: user, subscribed: true) + project_label_3_2.subscriptions.create!(user: user, subscribed: true) + project_label_2_1.subscriptions.create!(user: user2, subscribed: true) - expect(new_label.title).to eq(promoted_label_name) - expect(new_label.description).to eq(promoted_description) - expect(new_label.color).to eq(promoted_color) - end + expect { service.execute(project_label_1_1) }.to change { Subscription.count }.from(4).to(3) - it 'merges labels with the same name in group' do - expect { service.execute(project_label_1_1) }.to change(project_2.labels, :count).by(-1).and \ - change(project_3.labels, :count).by(-1) - end - - it 'keeps users\' subscriptions' do - user2 = create(:user) - project_label_1_1.subscriptions.create(user: user, subscribed: true) - project_label_2_1.subscriptions.create(user: user, subscribed: true) - project_label_3_2.subscriptions.create(user: user, subscribed: true) - project_label_2_1.subscriptions.create(user: user2, subscribed: true) + expect(new_label.subscribed?(user)).to be_truthy + expect(new_label.subscribed?(user2)).to be_truthy + end - expect { service.execute(project_label_1_1) }.to change { Subscription.count }.from(4).to(3) + it 'recreates priorities' do + service.execute(project_label_1_1) - expect(new_label.subscribed?(user)).to be_truthy - expect(new_label.subscribed?(user2)).to be_truthy - end + expect(new_label.priority(project_1)).to be_nil + expect(new_label.priority(project_2)).to eq(label_2_1_priority) + expect(new_label.priority(project_3)).to eq(label_3_1_priority) + end - it 'recreates priorities' do - service.execute(project_label_1_1) + it 'does not touch project out of promoted group' do + service.execute(project_label_1_1) + project_4_new_label = project_4.labels.find_by(title: promoted_label_name) - expect(new_label.priority(project_1)).to be_nil - expect(new_label.priority(project_2)).to eq(label_2_1_priority) - expect(new_label.priority(project_3)).to eq(label_3_1_priority) - end + expect(project_4_new_label).not_to be_nil + expect(project_4_new_label.id).to eq(project_label_4_1.id) + end - it 'does not touch project out of promoted group' do - service.execute(project_label_1_1) - project_4_new_label = project_4.labels.find_by(title: promoted_label_name) + it 'does not touch out of group priority' do + service.execute(project_label_1_1) - expect(project_4_new_label).not_to be_nil - expect(project_4_new_label.id).to eq(project_label_4_1.id) - end + expect(new_label.priority(project_4)).to be_nil + end - it 'does not touch out of group priority' do - service.execute(project_label_1_1) + it 'relinks issue with the promoted label' do + service.execute(project_label_1_1) + issue_label = issue_1_1.labels.find_by(title: promoted_label_name) - expect(new_label.priority(project_4)).to be_nil - end + expect(issue_label).not_to be_nil + expect(issue_label.id).to eq(new_label.id) + end - it 'relinks issue with the promoted label' do - service.execute(project_label_1_1) - issue_label = issue_1_1.labels.find_by(title: promoted_label_name) + it 'does not remove untouched labels from issue' do + expect { service.execute(project_label_1_1) }.not_to change(issue_1_1.labels, :count) + end - expect(issue_label).not_to be_nil - expect(issue_label.id).to eq(new_label.id) - end + it 'does not relink untouched label in issue' do + service.execute(project_label_1_1) + issue_label = issue_1_2.labels.find_by(title: untouched_label_name) - it 'does not remove untouched labels from issue' do - expect { service.execute(project_label_1_1) }.not_to change(issue_1_1.labels, :count) - end + expect(issue_label).not_to be_nil + expect(issue_label.id).to eq(project_label_1_2.id) + end - it 'does not relink untouched label in issue' do - service.execute(project_label_1_1) - issue_label = issue_1_2.labels.find_by(title: untouched_label_name) + it 'relinks issues with merged labels' do + service.execute(project_label_1_1) + issue_label = issue_2_1.labels.find_by(title: promoted_label_name) - expect(issue_label).not_to be_nil - expect(issue_label.id).to eq(project_label_1_2.id) - end + expect(issue_label).not_to be_nil + expect(issue_label.id).to eq(new_label.id) + end - it 'relinks issues with merged labels' do - service.execute(project_label_1_1) - issue_label = issue_2_1.labels.find_by(title: promoted_label_name) + it 'does not relink issues from other group' do + service.execute(project_label_1_1) + issue_label = issue_4_1.labels.find_by(title: promoted_label_name) - expect(issue_label).not_to be_nil - expect(issue_label.id).to eq(new_label.id) - end + expect(issue_label).not_to be_nil + expect(issue_label.id).to eq(project_label_4_1.id) + end - it 'does not relink issues from other group' do - service.execute(project_label_1_1) - issue_label = issue_4_1.labels.find_by(title: promoted_label_name) + it 'updates merge request' do + service.execute(project_label_1_1) + merge_label = merge_3_1.labels.find_by(title: promoted_label_name) - expect(issue_label).not_to be_nil - expect(issue_label.id).to eq(project_label_4_1.id) - end + expect(merge_label).not_to be_nil + expect(merge_label.id).to eq(new_label.id) + end - it 'updates merge request' do - service.execute(project_label_1_1) - merge_label = merge_3_1.labels.find_by(title: promoted_label_name) + it 'updates board lists' do + service.execute(project_label_1_1) + list = issue_board_2_1.lists.find_by(label: new_label) - expect(merge_label).not_to be_nil - expect(merge_label.id).to eq(new_label.id) - end + expect(list).not_to be_nil + end - it 'updates board lists' do - service.execute(project_label_1_1) - list = issue_board_2_1.lists.find_by(label: new_label) + # In case someone adds a new relation to Label.rb and forgets to relink it + # and the database doesn't have foreign key constraints + it 'relinks all relations' do + service.execute(project_label_1_1) - expect(list).not_to be_nil + Label.reflect_on_all_associations.each do |association| + expect(project_label_1_1.send(association.name).any?).to be_falsey + end + end end - # In case someone adds a new relation to Label.rb and forgets to relink it - # and the database doesn't have foreign key constraints - it 'relinks all relations' do - service.execute(project_label_1_1) + context 'if there is an existing identical group label' do + let!(:existing_group_label) { create(:group_label, group: group_1, title: project_label_1_1.title ) } + + it 'uses the existing group label' do + expect { service.execute(project_label_1_1) } + .to change(project_1.labels, :count).by(-1) + .and not_change(group_1.labels, :count) + expect(new_label).not_to be_nil + end - Label.reflect_on_all_associations.each do |association| - expect(project_label_1_1.send(association.name).any?).to be_falsey + it 'does not create a new group label clone' do + expect { service.execute(project_label_1_1) }.not_to change { GroupLabel.maximum(:id) } end + + it_behaves_like 'promoting a project label to a group label' end - context 'with invalid group label' do - before do - allow(service).to receive(:clone_label_to_group_label).and_wrap_original do |m, *args| - label = m.call(*args) - allow(label).to receive(:valid?).and_return(false) + context 'if there is no existing identical group label' do + let(:existing_group_label) { nil } - label - end + it 'recreates the label as a group label' do + expect { service.execute(project_label_1_1) } + .to change(project_1.labels, :count).by(-1) + .and change(group_1.labels, :count).by(1) + expect(new_label).not_to be_nil end - it 'raises an exception' do - expect { service.execute(project_label_1_1) }.to raise_error(ActiveRecord::RecordInvalid) + it 'copies title, description and color to cloned group label' do + service.execute(project_label_1_1) + + expect(new_label.title).to eq(promoted_label_name) + expect(new_label.description).to eq(promoted_description) + expect(new_label.color).to eq(promoted_color) end + + it_behaves_like 'promoting a project label to a group label' end end end diff --git a/spec/services/labels/transfer_service_spec.rb b/spec/services/labels/transfer_service_spec.rb index 2c0c82ed976..18fd401f383 100644 --- a/spec/services/labels/transfer_service_spec.rb +++ b/spec/services/labels/transfer_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Labels::TransferService do describe '#execute' do - let_it_be(:user) { create(:admin) } + let_it_be(:user) { create(:user) } let_it_be(:old_group_ancestor) { create(:group) } let_it_be(:old_group) { create(:group, parent: old_group_ancestor) } @@ -15,6 +15,11 @@ RSpec.describe Labels::TransferService do subject(:service) { described_class.new(user, old_group, project) } + before do + old_group_ancestor.add_developer(user) + new_group.add_developer(user) + end + it 'recreates missing group labels at project level and assigns them to the issuables' do old_group_label_1 = create(:group_label, group: old_group) old_group_label_2 = create(:group_label, group: old_group) diff --git a/spec/services/members/invite_service_spec.rb b/spec/services/members/invite_service_spec.rb new file mode 100644 index 00000000000..12a1a54696b --- /dev/null +++ b/spec/services/members/invite_service_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Members::InviteService do + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:project_user) { create(:user) } + + before do + project.add_maintainer(user) + end + + it 'adds an existing user to members' do + params = { email: project_user.email.to_s, access_level: Gitlab::Access::GUEST } + result = described_class.new(user, params).execute(project) + + expect(result[:status]).to eq(:success) + expect(project.users).to include project_user + end + + it 'creates a new user for an unknown email address' do + params = { email: 'email@example.org', access_level: Gitlab::Access::GUEST } + result = described_class.new(user, params).execute(project) + + expect(result[:status]).to eq(:success) + end + + it 'limits the number of emails to 100' do + emails = Array.new(101).map { |n| "email#{n}@example.com" } + params = { email: emails, access_level: Gitlab::Access::GUEST } + + result = described_class.new(user, params).execute(project) + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('Too many users specified (limit is 100)') + end + + it 'does not invite an invalid email' do + params = { email: project_user.id.to_s, access_level: Gitlab::Access::GUEST } + result = described_class.new(user, params).execute(project) + + expect(result[:status]).to eq(:error) + expect(result[:message][project_user.id.to_s]).to eq("Invite email is invalid") + expect(project.users).not_to include project_user + end + + it 'does not invite to an invalid access level' do + params = { email: project_user.email, access_level: -1 } + result = described_class.new(user, params).execute(project) + + expect(result[:status]).to eq(:error) + expect(result[:message][project_user.email]).to eq("Access level is not included in the list") + end + + it 'does not add a member with an existing invite' do + invited_member = create(:project_member, :invited, project: project) + + params = { email: invited_member.invite_email, + access_level: Gitlab::Access::GUEST } + result = described_class.new(user, params).execute(project) + + expect(result[:status]).to eq(:error) + expect(result[:message][invited_member.invite_email]).to eq("Member already invited to #{project.name}") + end +end diff --git a/spec/services/merge_requests/add_context_service_spec.rb b/spec/services/merge_requests/add_context_service_spec.rb index 58ed91218d1..27b46a9023c 100644 --- a/spec/services/merge_requests/add_context_service_spec.rb +++ b/spec/services/merge_requests/add_context_service_spec.rb @@ -12,10 +12,20 @@ RSpec.describe MergeRequests::AddContextService do subject(:service) { described_class.new(project, admin, merge_request: merge_request, commits: commits) } describe "#execute" do - it "adds context commit" do - service.execute + context "when admin mode is enabled", :enable_admin_mode do + it "adds context commit" do + service.execute - expect(merge_request.merge_request_context_commit_diff_files.length).to eq(2) + expect(merge_request.merge_request_context_commit_diff_files.length).to eq(2) + end + end + + context "when admin mode is disabled" do + it "doesn't add context commit" do + subject.execute + + expect(merge_request.merge_request_context_commit_diff_files.length).to eq(0) + end end context "when user doesn't have permission to update merge request" do diff --git a/spec/services/merge_requests/cleanup_refs_service_spec.rb b/spec/services/merge_requests/cleanup_refs_service_spec.rb index a051b3c9355..38c0e204e54 100644 --- a/spec/services/merge_requests/cleanup_refs_service_spec.rb +++ b/spec/services/merge_requests/cleanup_refs_service_spec.rb @@ -4,14 +4,15 @@ require 'spec_helper' RSpec.describe MergeRequests::CleanupRefsService do describe '.schedule' do - let(:merge_request) { build(:merge_request) } + let(:merge_request) { create(:merge_request) } - it 'schedules MergeRequestCleanupRefsWorker' do - expect(MergeRequestCleanupRefsWorker) - .to receive(:perform_in) - .with(described_class::TIME_THRESHOLD, merge_request.id) + it 'creates a merge request cleanup schedule' do + freeze_time do + described_class.schedule(merge_request) - described_class.schedule(merge_request) + expect(merge_request.reload.cleanup_schedule.scheduled_at) + .to eq(described_class::TIME_THRESHOLD.from_now) + end end end @@ -20,6 +21,8 @@ RSpec.describe MergeRequests::CleanupRefsService 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 + + merge_request.create_cleanup_schedule(scheduled_at: described_class::TIME_THRESHOLD.ago) end subject(:result) { described_class.new(merge_request).execute } @@ -32,6 +35,7 @@ RSpec.describe MergeRequests::CleanupRefsService do expect(result[:status]).to eq(:success) expect(kept_around?(old_ref_head)).to be_truthy expect(ref_head).to be_nil + expect(merge_request.cleanup_schedule.completed_at).to be_present end end @@ -43,6 +47,7 @@ RSpec.describe MergeRequests::CleanupRefsService do it 'does not fail' do expect(result[:status]).to eq(:success) + expect(merge_request.cleanup_schedule.completed_at).to be_present end end @@ -85,6 +90,44 @@ RSpec.describe MergeRequests::CleanupRefsService do it_behaves_like 'service that does not clean up merge request refs' end + + context 'when cleanup schedule fails to update' do + before do + allow(merge_request.cleanup_schedule).to receive(:update).and_return(false) + end + + it 'creates keep around ref and deletes merge request refs' do + old_ref_head = ref_head + + aggregate_failures do + expect(result[:status]).to eq(:error) + expect(kept_around?(old_ref_head)).to be_truthy + expect(ref_head).to be_nil + expect(merge_request.cleanup_schedule.completed_at).not_to be_present + end + end + end + + context 'when merge request is not scheduled to be cleaned up yet' do + before do + merge_request.cleanup_schedule.update!(scheduled_at: 1.day.from_now) + end + + it_behaves_like 'service that does not clean up merge request refs' + end + + context 'when repository no longer exists' do + before do + Repositories::DestroyService.new(merge_request.project.repository).execute + end + + it 'does not fail and still mark schedule as complete' do + aggregate_failures do + expect(result[:status]).to eq(:success) + expect(merge_request.cleanup_schedule.completed_at).to be_present + end + end + end end shared_examples_for 'service that does not clean up merge request refs' do @@ -92,6 +135,7 @@ RSpec.describe MergeRequests::CleanupRefsService do aggregate_failures do expect(result[:status]).to eq(:error) expect(ref_head).to be_present + expect(merge_request.cleanup_schedule.completed_at).not_to be_present end end end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index d603cbb16aa..3ccf02fcdfb 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -683,14 +683,14 @@ RSpec.describe MergeRequests::RefreshService do end end - context 'marking the merge request as work in progress' do + context 'marking the merge request as draft' do let(:refresh_service) { service.new(@project, @user) } before do allow(refresh_service).to receive(:execute_hooks) end - it 'marks the merge request as work in progress from fixup commits' do + it 'marks the merge request as draft from fixup commits' do fixup_merge_request = create(:merge_request, source_project: @project, source_branch: 'wip', @@ -705,11 +705,11 @@ RSpec.describe MergeRequests::RefreshService do expect(fixup_merge_request.work_in_progress?).to eq(true) expect(fixup_merge_request.notes.last.note).to match( - /marked as a \*\*Work In Progress\*\* from #{Commit.reference_pattern}/ + /marked this merge request as \*\*draft\*\* from #{Commit.reference_pattern}/ ) end - it 'references the commit that caused the Work in Progress status' do + it 'references the commit that caused the draft status' do wip_merge_request = create(:merge_request, source_project: @project, source_branch: 'wip', @@ -724,11 +724,11 @@ RSpec.describe MergeRequests::RefreshService do refresh_service.execute(oldrev, newrev, 'refs/heads/wip') expect(wip_merge_request.reload.notes.last.note).to eq( - "marked as a **Work In Progress** from #{wip_commit.id}" + "marked this merge request as **draft** from #{wip_commit.id}" ) end - it 'does not mark as WIP based on commits that do not belong to an MR' do + it 'does not mark as draft based on commits that do not belong to an MR' do allow(refresh_service).to receive(:find_new_commits) refresh_service.instance_variable_set("@commits", [ double( diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb index ffc2ebb344c..2bd83dc36a8 100644 --- a/spec/services/merge_requests/reopen_service_spec.rb +++ b/spec/services/merge_requests/reopen_service_spec.rb @@ -23,6 +23,8 @@ RSpec.describe MergeRequests::ReopenService do before do allow(service).to receive(:execute_hooks) + merge_request.create_cleanup_schedule(scheduled_at: Time.current) + merge_request.update_column(:merge_ref_sha, 'abc123') perform_enqueued_jobs do service.execute(merge_request) @@ -43,6 +45,14 @@ RSpec.describe MergeRequests::ReopenService do expect(email.subject).to include(merge_request.title) end + it 'destroys cleanup schedule record' do + expect(merge_request.reload.cleanup_schedule).to be_nil + end + + it 'clears the cached merge_ref_sha' do + expect(merge_request.reload.merge_ref_sha).to be_nil + end + context 'note creation' do it 'creates resource state event about merge_request reopen' do event = merge_request.resource_state_events.last diff --git a/spec/services/metrics/dashboard/panel_preview_service_spec.rb b/spec/services/metrics/dashboard/panel_preview_service_spec.rb index d58dee3e7a3..2877d22d1f3 100644 --- a/spec/services/metrics/dashboard/panel_preview_service_spec.rb +++ b/spec/services/metrics/dashboard/panel_preview_service_spec.rb @@ -11,6 +11,7 @@ RSpec.describe Metrics::Dashboard::PanelPreviewService do title: test panel YML end + let_it_be(:dashboard) do { panel_groups: [ diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 1e5536a2d0b..3118956951e 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -67,147 +67,164 @@ RSpec.describe Notes::CreateService do let(:current_user) { user } end end - end - context 'noteable highlight cache clearing' do - let(:project_with_repo) { create(:project, :repository) } - let(:merge_request) do - create(:merge_request, source_project: project_with_repo, - target_project: project_with_repo) - end + it 'tracks issue comment usage data', :clean_gitlab_redis_shared_state do + event = Gitlab::UsageDataCounters::IssueActivityUniqueCounter::ISSUE_COMMENT_ADDED + counter = Gitlab::UsageDataCounters::HLLRedisCounter - let(:position) do - Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", - new_path: "files/ruby/popen.rb", - old_line: nil, - new_line: 14, - diff_refs: merge_request.diff_refs) + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_comment_added_action).with(author: user).and_call_original + expect do + described_class.new(project, user, opts).execute + end.to change { counter.unique_events(event_names: event, start_date: 1.day.ago, end_date: 1.day.from_now) }.by(1) end - let(:new_opts) do - opts.merge(in_reply_to_discussion_id: nil, - type: 'DiffNote', - noteable_type: 'MergeRequest', - noteable_id: merge_request.id, - position: position.to_h) - end + context 'in a merge request' do + let_it_be(:project_with_repo) { create(:project, :repository) } + let_it_be(:merge_request) do + create(:merge_request, source_project: project_with_repo, + target_project: project_with_repo) + end - before do - allow_any_instance_of(Gitlab::Diff::Position) - .to receive(:unfolded_diff?) { true } - end + context 'issue comment usage data' do + let(:opts) do + { note: 'Awesome comment', noteable_type: 'MergeRequest', noteable_id: merge_request.id } + end - it 'clears noteable diff cache when it was unfolded for the note position' do - expect_any_instance_of(Gitlab::Diff::HighlightCache).to receive(:clear) + it 'does not track' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_comment_added_action) - described_class.new(project_with_repo, user, new_opts).execute - end + described_class.new(project, user, opts).execute + end + end - it 'does not clear cache when note is not the first of the discussion' do - prev_note = - create(:diff_note_on_merge_request, noteable: merge_request, - project: project_with_repo) - reply_opts = - opts.merge(in_reply_to_discussion_id: prev_note.discussion_id, - type: 'DiffNote', - noteable_type: 'MergeRequest', - noteable_id: merge_request.id, - position: position.to_h) + context 'noteable highlight cache clearing' do + let(:position) do + Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 14, + diff_refs: merge_request.diff_refs) + end - expect(merge_request).not_to receive(:diffs) + let(:new_opts) do + opts.merge(in_reply_to_discussion_id: nil, + type: 'DiffNote', + noteable_type: 'MergeRequest', + noteable_id: merge_request.id, + position: position.to_h) + end - described_class.new(project_with_repo, user, reply_opts).execute - end - end + before do + allow_any_instance_of(Gitlab::Diff::Position) + .to receive(:unfolded_diff?) { true } + end - context 'note diff file' do - let(:project_with_repo) { create(:project, :repository) } - let(:merge_request) do - create(:merge_request, - source_project: project_with_repo, - target_project: project_with_repo) - end + it 'clears noteable diff cache when it was unfolded for the note position' do + expect_any_instance_of(Gitlab::Diff::HighlightCache).to receive(:clear) - let(:line_number) { 14 } - let(:position) do - Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", - new_path: "files/ruby/popen.rb", - old_line: nil, - new_line: line_number, - diff_refs: merge_request.diff_refs) - end + described_class.new(project_with_repo, user, new_opts).execute + end - let(:previous_note) do - create(:diff_note_on_merge_request, noteable: merge_request, project: project_with_repo) - end + it 'does not clear cache when note is not the first of the discussion' do + prev_note = + create(:diff_note_on_merge_request, noteable: merge_request, + project: project_with_repo) + reply_opts = + opts.merge(in_reply_to_discussion_id: prev_note.discussion_id, + type: 'DiffNote', + noteable_type: 'MergeRequest', + noteable_id: merge_request.id, + position: position.to_h) - before do - project_with_repo.add_maintainer(user) - end + expect(merge_request).not_to receive(:diffs) - context 'when eligible to have a note diff file' do - let(:new_opts) do - opts.merge(in_reply_to_discussion_id: nil, - type: 'DiffNote', - noteable_type: 'MergeRequest', - noteable_id: merge_request.id, - position: position.to_h) + described_class.new(project_with_repo, user, reply_opts).execute + end end - it 'note is associated with a note diff file' do - MergeRequests::MergeToRefService.new(merge_request.project, merge_request.author).execute(merge_request) - - note = described_class.new(project_with_repo, user, new_opts).execute - - expect(note).to be_persisted - expect(note.note_diff_file).to be_present - expect(note.diff_note_positions).to be_present - end - end + context 'note diff file' do + let(:line_number) { 14 } + let(:position) do + Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: line_number, + diff_refs: merge_request.diff_refs) + end - context 'when DiffNote is a reply' do - let(:new_opts) do - opts.merge(in_reply_to_discussion_id: previous_note.discussion_id, - type: 'DiffNote', - noteable_type: 'MergeRequest', - noteable_id: merge_request.id, - position: position.to_h) - end + let(:previous_note) do + create(:diff_note_on_merge_request, noteable: merge_request, project: project_with_repo) + end - it 'note is not associated with a note diff file' do - expect(Discussions::CaptureDiffNotePositionService).not_to receive(:new) + before do + project_with_repo.add_maintainer(user) + end - note = described_class.new(project_with_repo, user, new_opts).execute + context 'when eligible to have a note diff file' do + let(:new_opts) do + opts.merge(in_reply_to_discussion_id: nil, + type: 'DiffNote', + noteable_type: 'MergeRequest', + noteable_id: merge_request.id, + position: position.to_h) + end - expect(note).to be_persisted - expect(note.note_diff_file).to be_nil - end + it 'note is associated with a note diff file' do + MergeRequests::MergeToRefService.new(merge_request.project, merge_request.author).execute(merge_request) - context 'when DiffNote from an image' do - let(:image_position) do - Gitlab::Diff::Position.new(old_path: "files/images/6049019_460s.jpg", - new_path: "files/images/6049019_460s.jpg", - width: 100, - height: 100, - x: 1, - y: 100, - diff_refs: merge_request.diff_refs, - position_type: 'image') - end + note = described_class.new(project_with_repo, user, new_opts).execute - let(:new_opts) do - opts.merge(in_reply_to_discussion_id: nil, - type: 'DiffNote', - noteable_type: 'MergeRequest', - noteable_id: merge_request.id, - position: image_position.to_h) + expect(note).to be_persisted + expect(note.note_diff_file).to be_present + expect(note.diff_note_positions).to be_present + end end - it 'note is not associated with a note diff file' do - note = described_class.new(project_with_repo, user, new_opts).execute - - expect(note).to be_persisted - expect(note.note_diff_file).to be_nil + context 'when DiffNote is a reply' do + let(:new_opts) do + opts.merge(in_reply_to_discussion_id: previous_note.discussion_id, + type: 'DiffNote', + noteable_type: 'MergeRequest', + noteable_id: merge_request.id, + position: position.to_h) + end + + it 'note is not associated with a note diff file' do + expect(Discussions::CaptureDiffNotePositionService).not_to receive(:new) + + note = described_class.new(project_with_repo, user, new_opts).execute + + expect(note).to be_persisted + expect(note.note_diff_file).to be_nil + end + + context 'when DiffNote from an image' do + let(:image_position) do + Gitlab::Diff::Position.new(old_path: "files/images/6049019_460s.jpg", + new_path: "files/images/6049019_460s.jpg", + width: 100, + height: 100, + x: 1, + y: 100, + diff_refs: merge_request.diff_refs, + position_type: 'image') + end + + let(:new_opts) do + opts.merge(in_reply_to_discussion_id: nil, + type: 'DiffNote', + noteable_type: 'MergeRequest', + noteable_id: merge_request.id, + position: image_position.to_h) + end + + it 'note is not associated with a note diff file' do + note = described_class.new(project_with_repo, user, new_opts).execute + + expect(note).to be_persisted + expect(note.note_diff_file).to be_nil + end + end end end end @@ -286,7 +303,7 @@ RSpec.describe Notes::CreateService do QuickAction.new( action_text: "/wip", before_action: -> { - issuable.reload.update(title: "title") + issuable.reload.update!(title: "title") }, expectation: ->(issuable, can_use_quick_action) { expect(issuable.work_in_progress?).to eq(can_use_quick_action) @@ -296,7 +313,7 @@ RSpec.describe Notes::CreateService do QuickAction.new( action_text: "/wip", before_action: -> { - issuable.reload.update(title: "WIP: title") + issuable.reload.update!(title: "WIP: title") }, expectation: ->(noteable, can_use_quick_action) { expect(noteable.work_in_progress?).not_to eq(can_use_quick_action) diff --git a/spec/services/notes/destroy_service_spec.rb b/spec/services/notes/destroy_service_spec.rb index d1076f77cec..0859c28cbe7 100644 --- a/spec/services/notes/destroy_service_spec.rb +++ b/spec/services/notes/destroy_service_spec.rb @@ -24,36 +24,55 @@ RSpec.describe Notes::DestroyService do .to change { user.todos_pending_count }.from(1).to(0) end - context 'noteable highlight cache clearing' do - let(:repo_project) { create(:project, :repository) } - let(:merge_request) do + it 'tracks issue comment removal usage data', :clean_gitlab_redis_shared_state do + note = create(:note, project: project, noteable: issue) + event = Gitlab::UsageDataCounters::IssueActivityUniqueCounter::ISSUE_COMMENT_REMOVED + counter = Gitlab::UsageDataCounters::HLLRedisCounter + + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_comment_removed_action).with(author: user).and_call_original + expect do + described_class.new(project, user).execute(note) + end.to change { counter.unique_events(event_names: event, start_date: 1.day.ago, end_date: 1.day.from_now) }.by(1) + end + + context 'in a merge request' do + let_it_be(:repo_project) { create(:project, :repository) } + let_it_be(:merge_request) do create(:merge_request, source_project: repo_project, - target_project: repo_project) + target_project: repo_project) end - let(:note) do + let_it_be(:note) do create(:diff_note_on_merge_request, project: repo_project, - noteable: merge_request) + noteable: merge_request) end - before do - allow(note.position).to receive(:unfolded_diff?) { true } - end - - it 'clears noteable diff cache when it was unfolded for the note position' do - expect(merge_request).to receive_message_chain(:diffs, :clear_cache) + it 'does not track issue comment removal usage data' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_comment_removed_action) described_class.new(repo_project, user).execute(note) end - it 'does not clear cache when note is not the first of the discussion' do - reply_note = create(:diff_note_on_merge_request, in_reply_to: note, - project: repo_project, - noteable: merge_request) + context 'noteable highlight cache clearing' do + before do + allow(note.position).to receive(:unfolded_diff?) { true } + end + + it 'clears noteable diff cache when it was unfolded for the note position' do + expect(merge_request).to receive_message_chain(:diffs, :clear_cache) + + described_class.new(repo_project, user).execute(note) + end + + it 'does not clear cache when note is not the first of the discussion' do + reply_note = create(:diff_note_on_merge_request, in_reply_to: note, + project: repo_project, + noteable: merge_request) - expect(merge_request).not_to receive(:diffs) + expect(merge_request).not_to receive(:diffs) - described_class.new(repo_project, user).execute(reply_note) + described_class.new(repo_project, user).execute(reply_note) + end end end end diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index 66efdf8abe7..e2f51c9af67 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -47,6 +47,22 @@ RSpec.describe Notes::UpdateService do end end + it 'does not track usage data when params is blank' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_comment_edited_action) + + update_note({}) + end + + it 'tracks usage data', :clean_gitlab_redis_shared_state do + event = Gitlab::UsageDataCounters::IssueActivityUniqueCounter::ISSUE_COMMENT_EDITED + counter = Gitlab::UsageDataCounters::HLLRedisCounter + + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_comment_edited_action).with(author: user).and_call_original + expect do + update_note(note: 'new text') + end.to change { counter.unique_events(event_names: event, start_date: 1.day.ago, end_date: 1.day.from_now) }.by(1) + end + context 'with system note' do before do note.update_column(:system, true) @@ -55,6 +71,12 @@ RSpec.describe Notes::UpdateService do it 'does not update the note' do expect { update_note(note: 'new text') }.not_to change { note.reload.note } end + + it 'does not track usage data' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_comment_edited_action) + + update_note(note: 'new text') + end end context 'suggestions' do @@ -220,6 +242,12 @@ RSpec.describe Notes::UpdateService do expect(note).not_to receive(:create_new_cross_references!) update_note({ note: "Updated with new reference: #{issue.to_reference}" }) end + + it 'does not track usage data' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_comment_edited_action) + + update_note(note: 'new text') + end end end end diff --git a/spec/services/notification_recipients/build_service_spec.rb b/spec/services/notification_recipients/build_service_spec.rb index 5c8add250c2..cc08f9fceff 100644 --- a/spec/services/notification_recipients/build_service_spec.rb +++ b/spec/services/notification_recipients/build_service_spec.rb @@ -44,7 +44,7 @@ RSpec.describe NotificationRecipients::BuildService do context 'when there are multiple subscribers' do def create_user subscriber = create(:user) - issue.subscriptions.create(user: subscriber, project: project, subscribed: true) + issue.subscriptions.create!(user: subscriber, project: project, subscribed: true) end include_examples 'no N+1 queries' @@ -96,7 +96,7 @@ RSpec.describe NotificationRecipients::BuildService do context 'when there are multiple subscribers' do def create_user subscriber = create(:user) - merge_request.subscriptions.create(user: subscriber, project: project, subscribed: true) + merge_request.subscriptions.create!(user: subscriber, project: project, subscribed: true) end include_examples 'no N+1 queries' diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index caa9961424e..f9a89c6281e 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -361,7 +361,7 @@ RSpec.describe NotificationService, :mailer do context 'where the project has disabled the feature' do before do - project.update(service_desk_enabled: false) + project.update!(service_desk_enabled: false) end it_should_not_email! @@ -453,7 +453,7 @@ RSpec.describe NotificationService, :mailer do context 'by note' do before do note.author = @u_lazy_participant - note.save + note.save! end it { expect { subject }.not_to have_enqueued_email(@u_lazy_participant.id, note.id, mail: "note_issue_email") } @@ -467,7 +467,7 @@ RSpec.describe NotificationService, :mailer 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 + note.project.save! @u_watcher.notification_settings_for(note.project).participating! @u_watcher.notification_settings_for(group).global! @@ -834,53 +834,47 @@ RSpec.describe NotificationService, :mailer do end end - context 'when notified of a new design diff note' do + context 'design diff note', :deliver_mails_inline do include DesignManagementTestHelpers let_it_be(:design) { create(:design, :with_file) } let_it_be(:project) { design.project } - let_it_be(:dev) { create(:user) } - let_it_be(:stranger) { create(:user) } + let_it_be(:member_and_mentioned) { create(:user, developer_projects: [project]) } + let_it_be(:member_and_author_of_second_note) { create(:user, developer_projects: [project]) } + let_it_be(:member_and_not_mentioned) { create(:user, developer_projects: [project]) } + let_it_be(:non_member_and_mentioned) { create(:user) } let_it_be(:note) do create(:diff_note_on_design, noteable: design, - note: "Hello #{dev.to_reference}, G'day #{stranger.to_reference}") + note: "Hello #{member_and_mentioned.to_reference}, G'day #{non_member_and_mentioned.to_reference}") + end + + let_it_be(:note_2) do + create(:diff_note_on_design, noteable: design, author: member_and_author_of_second_note) end - let(:mailer) { double(deliver_later: true) } context 'design management is enabled' do before do enable_design_management - project.add_developer(dev) - allow(Notify).to receive(:note_design_email) { mailer } - end - - it 'sends new note notifications' do - expect(subject).to receive(:send_new_note_notifications).with(note) - - subject.new_note(note) - end - - it 'sends a mail to the developer' do - expect(Notify) - .to receive(:note_design_email).with(dev.id, note.id, 'mentioned') - - subject.new_note(note) end - it 'does not notify non-developers' do - expect(Notify) - .not_to receive(:note_design_email).with(stranger.id, note.id) + it 'sends new note notifications', :aggregate_failures do + notification.new_note(note) - subject.new_note(note) + should_email(design.authors.first) + should_email(member_and_mentioned) + should_email(member_and_author_of_second_note) + should_not_email(member_and_not_mentioned) + should_not_email(non_member_and_mentioned) + should_not_email(note.author) end end context 'design management is disabled' do - it 'does not notify the user' do - expect(Notify).not_to receive(:note_design_email) + it 'does not notify anyone' do + notification.new_note(note) - subject.new_note(note) + should_not_email_anyone end end end @@ -1719,7 +1713,7 @@ RSpec.describe NotificationService, :mailer do before do merge_request.author = participant - merge_request.save + merge_request.save! notification.new_merge_request(merge_request, @u_disabled) end @@ -1962,7 +1956,7 @@ RSpec.describe NotificationService, :mailer do describe 'when merge_when_pipeline_succeeds is true' do before do - merge_request.update( + merge_request.update!( merge_when_pipeline_succeeds: true, merge_user: create(:user) ) @@ -2312,6 +2306,26 @@ RSpec.describe NotificationService, :mailer do end end + describe '#new_instance_access_request', :deliver_mails_inline do + let_it_be(:user) { create(:user, :blocked_pending_approval) } + let_it_be(:admins) { create_list(:admin, 12, :with_sign_ins) } + + subject { notification.new_instance_access_request(user) } + + before do + reset_delivered_emails! + stub_application_setting(require_admin_approval_after_user_signup: true) + end + + it 'sends notification only to a maximum of ten most recently active instance admins' do + ten_most_recently_active_instance_admins = User.admins.active.sort_by(&:current_sign_in_at).last(10) + + subject + + should_only_email(*ten_most_recently_active_instance_admins) + end + end + describe 'GroupMember', :deliver_mails_inline do let(:added_user) { create(:user) } @@ -2725,7 +2739,7 @@ RSpec.describe NotificationService, :mailer do before do group = create(:group) - project.update(group: group) + project.update!(group: group) create(:email, :confirmed, user: u_custom_notification_enabled, email: group_notification_email) create(:notification_setting, user: u_custom_notification_enabled, source: group, notification_email: group_notification_email) @@ -2761,7 +2775,7 @@ RSpec.describe NotificationService, :mailer do before do group = create(:group) - project.update(group: group) + project.update!(group: group) create(:email, :confirmed, user: u_member, email: group_notification_email) create(:notification_setting, user: u_member, source: group, notification_email: group_notification_email) end @@ -2821,7 +2835,7 @@ RSpec.describe NotificationService, :mailer do context 'when the creator has no read_build access' do before do pipeline = create_pipeline(u_member, :failed) - project.update(public_builds: false) + project.update!(public_builds: false) project.team.truncate notification.pipeline_finished(pipeline) end @@ -2855,7 +2869,7 @@ RSpec.describe NotificationService, :mailer do before do group = create(:group) - project.update(group: group) + project.update!(group: group) create(:email, :confirmed, user: u_member, email: group_notification_email) create(:notification_setting, user: u_member, source: group, notification_email: group_notification_email) end @@ -3089,37 +3103,51 @@ RSpec.describe NotificationService, :mailer do subject.new_issue(issue, member) end - it 'still delivers email to admins' do - member.update!(admin: true) + context 'with admin user' do + before do + member.update!(admin: true) + end - expect(Notify).to receive(:new_issue_email).at_least(:once).with(member.id, issue.id, nil).and_call_original + context 'when admin mode is enabled', :enable_admin_mode do + it 'still delivers email to admins' do + expect(Notify).to receive(:new_issue_email).at_least(:once).with(member.id, issue.id, nil).and_call_original - subject.new_issue(issue, member) + subject.new_issue(issue, member) + end + end + + context 'when admin mode is disabled' do + it 'does not send an email' do + expect(Notify).not_to receive(:new_issue_email) + + subject.new_issue(issue, member) + end + end end end end describe '#prometheus_alerts_fired' do - let!(:project) { create(:project) } - let!(:master) { create(:user) } - let!(:developer) { create(:user) } - let(:alert_attributes) { build(:alert_management_alert, project: project).attributes } + let_it_be(:project) { create(:project) } + let_it_be(:master) { create(:user) } + let_it_be(:developer) { create(:user) } + let_it_be(:alert) { create(:alert_management_alert, project: project) } before do project.add_maintainer(master) end it 'sends the email to owners and masters' do - expect(Notify).to receive(:prometheus_alert_fired_email).with(project.id, master.id, alert_attributes).and_call_original - expect(Notify).to receive(:prometheus_alert_fired_email).with(project.id, project.owner.id, alert_attributes).and_call_original - expect(Notify).not_to receive(:prometheus_alert_fired_email).with(project.id, developer.id, alert_attributes) + expect(Notify).to receive(:prometheus_alert_fired_email).with(project, master, alert).and_call_original + expect(Notify).to receive(:prometheus_alert_fired_email).with(project, project.owner, alert).and_call_original + expect(Notify).not_to receive(:prometheus_alert_fired_email).with(project, developer, alert) - subject.prometheus_alerts_fired(project, [alert_attributes]) + subject.prometheus_alerts_fired(project, [alert]) end it_behaves_like 'project emails are disabled' do let(:notification_target) { project } - let(:notification_trigger) { subject.prometheus_alerts_fired(project, [alert_attributes]) } + let(:notification_trigger) { subject.prometheus_alerts_fired(project, [alert]) } around do |example| perform_enqueued_jobs { example.run } @@ -3212,7 +3240,7 @@ RSpec.describe NotificationService, :mailer do # with different notification settings def build_group(project, visibility: :public) group = create_nested_group(visibility) - project.update(namespace_id: group.id) + project.update!(namespace_id: group.id) # Group member: global=disabled, group=watch @g_watcher ||= create_user_with_notification(:watch, 'group_watcher', project.group) @@ -3277,11 +3305,11 @@ RSpec.describe NotificationService, :mailer do end def add_user_subscriptions(issuable) - issuable.subscriptions.create(user: @unsubscribed_mentioned, project: project, subscribed: false) - issuable.subscriptions.create(user: @subscriber, project: project, subscribed: true) - issuable.subscriptions.create(user: @subscribed_participant, project: project, subscribed: true) - issuable.subscriptions.create(user: @unsubscriber, project: project, subscribed: false) + issuable.subscriptions.create!(user: @unsubscribed_mentioned, project: project, subscribed: false) + issuable.subscriptions.create!(user: @subscriber, project: project, subscribed: true) + issuable.subscriptions.create!(user: @subscribed_participant, project: project, subscribed: true) + issuable.subscriptions.create!(user: @unsubscriber, project: project, subscribed: false) # Make the watcher a subscriber to detect dupes - issuable.subscriptions.create(user: @watcher_and_subscriber, project: project, subscribed: true) + issuable.subscriptions.create!(user: @watcher_and_subscriber, project: project, subscribed: true) end end diff --git a/spec/services/packages/composer/version_parser_service_spec.rb b/spec/services/packages/composer/version_parser_service_spec.rb index 904c75ab0a1..1a2f653c042 100644 --- a/spec/services/packages/composer/version_parser_service_spec.rb +++ b/spec/services/packages/composer/version_parser_service_spec.rb @@ -19,9 +19,11 @@ RSpec.describe Packages::Composer::VersionParserService do nil | '1.7.x' | '1.7.x-dev' 'v1.0.0' | nil | '1.0.0' 'v1.0' | nil | '1.0' + 'v1.0.1+meta' | nil | '1.0.1+meta' '1.0' | nil | '1.0' '1.0.2' | nil | '1.0.2' '1.0.2-beta2' | nil | '1.0.2-beta2' + '1.0.1+meta' | nil | '1.0.1+meta' end with_them do diff --git a/spec/services/packages/conan/create_package_file_service_spec.rb b/spec/services/packages/conan/create_package_file_service_spec.rb index 0e9cbba5fc1..bd6a91c883a 100644 --- a/spec/services/packages/conan/create_package_file_service_spec.rb +++ b/spec/services/packages/conan/create_package_file_service_spec.rb @@ -100,7 +100,7 @@ RSpec.describe Packages::Conan::CreatePackageFileService do end let(:tmp_object) do - fog_connection.directories.new(key: 'packages').files.create( + fog_connection.directories.new(key: 'packages').files.create( # rubocop:disable Rails/SaveBang key: "tmp/uploads/#{file_name}", body: 'content' ) diff --git a/spec/services/packages/create_event_service_spec.rb b/spec/services/packages/create_event_service_spec.rb index 7e66b430a8c..55703e9127f 100644 --- a/spec/services/packages/create_event_service_spec.rb +++ b/spec/services/packages/create_event_service_spec.rb @@ -15,40 +15,92 @@ RSpec.describe Packages::CreateEventService do subject { described_class.new(nil, user, params).execute } describe '#execute' do - shared_examples 'package event creation' do |originator_type, expected_scope| - it 'creates the event' do - expect { subject }.to change { Packages::Event.count }.by(1) - - expect(subject.originator_type).to eq(originator_type) - expect(subject.originator).to eq(user&.id) - expect(subject.event_scope).to eq(expected_scope) - expect(subject.event_type).to eq(event_name) + shared_examples 'db package event creation' do |originator_type, expected_scope| + before do + allow(::Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event) + end + + context 'with feature flag disable' do + before do + stub_feature_flags(collect_package_events: false) + end + + it 'does not create an event object' do + expect { subject }.not_to change { Packages::Event.count } + end + end + + context 'with feature flag enabled' do + before do + stub_feature_flags(collect_package_events: true) + end + + it 'creates the event' do + expect { subject }.to change { Packages::Event.count }.by(1) + + expect(subject.originator_type).to eq(originator_type) + expect(subject.originator).to eq(user&.id) + expect(subject.event_scope).to eq(expected_scope) + expect(subject.event_type).to eq(event_name) + end + end + end + + shared_examples 'redis package event creation' do |originator_type, expected_scope| + context 'with feature flag disable' do + before do + stub_feature_flags(collect_package_events_redis: false) + end + + it 'does not track the event' do + expect(::Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event) + + subject + end + end + + it 'tracks the event' do + expect(::Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event).with(user.id, Packages::Event.allowed_event_name(expected_scope, event_name, originator_type)) + + subject end end context 'with a user' do let(:user) { create(:user) } - it_behaves_like 'package event creation', 'user', 'container' + it_behaves_like 'db package event creation', 'user', 'container' + it_behaves_like 'redis package event creation', 'user', 'container' end context 'with a deploy token' do let(:user) { create(:deploy_token) } - it_behaves_like 'package event creation', 'deploy_token', 'container' + it_behaves_like 'db package event creation', 'deploy_token', 'container' + it_behaves_like 'redis package event creation', 'deploy_token', 'container' end context 'with no user' do let(:user) { nil } - it_behaves_like 'package event creation', 'guest', 'container' + it_behaves_like 'db package event creation', 'guest', 'container' end context 'with a package as scope' do - let(:user) { nil } let(:scope) { create(:npm_package) } - it_behaves_like 'package event creation', 'guest', 'npm' + context 'as guest' do + let(:user) { nil } + + it_behaves_like 'db package event creation', 'guest', 'npm' + end + + context 'with user' do + let(:user) { create(:user) } + + it_behaves_like 'db package event creation', 'user', 'npm' + it_behaves_like 'redis package event creation', 'user', 'npm' + end end end end diff --git a/spec/services/packages/create_package_file_service_spec.rb b/spec/services/packages/create_package_file_service_spec.rb index 93dde54916a..12fd1039d30 100644 --- a/spec/services/packages/create_package_file_service_spec.rb +++ b/spec/services/packages/create_package_file_service_spec.rb @@ -2,7 +2,10 @@ require 'spec_helper' RSpec.describe Packages::CreatePackageFileService do - let(:package) { create(:maven_package) } + let_it_be(:package) { create(:maven_package) } + let_it_be(:user) { create(:user) } + + subject { described_class.new(package, params) } describe '#execute' do context 'with valid params' do @@ -14,7 +17,7 @@ RSpec.describe Packages::CreatePackageFileService do end it 'creates a new package file' do - package_file = described_class.new(package, params).execute + package_file = subject.execute expect(package_file).to be_valid expect(package_file.file_name).to eq('foo.jar') @@ -29,9 +32,17 @@ RSpec.describe Packages::CreatePackageFileService do end it 'raises an error' do - service = described_class.new(package, params) + expect { subject.execute }.to raise_error(ActiveRecord::RecordInvalid) + end + end + + context 'with a build' do + let_it_be(:pipeline) { create(:ci_pipeline, user: user) } + let(:build) { double('build', pipeline: pipeline) } + let(:params) { { file: Tempfile.new, file_name: 'foo.jar', build: build } } - expect { service.execute }.to raise_error(ActiveRecord::RecordInvalid) + it 'creates a build_info' do + expect { subject.execute }.to change { Packages::PackageFileBuildInfo.count }.by(1) end end end diff --git a/spec/services/packages/debian/extract_deb_metadata_service_spec.rb b/spec/services/packages/debian/extract_deb_metadata_service_spec.rb new file mode 100644 index 00000000000..33059adf8a2 --- /dev/null +++ b/spec/services/packages/debian/extract_deb_metadata_service_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Packages::Debian::ExtractDebMetadataService do + subject { described_class.new(file_path) } + + let(:file_name) { 'libsample0_1.2.3~alpha2_amd64.deb' } + let(:file_path) { "spec/fixtures/packages/debian/#{file_name}" } + + context 'with correct file' do + it 'return as expected' do + expected = { + 'Package': 'libsample0', + 'Source': 'sample', + 'Version': '1.2.3~alpha2', + 'Architecture': 'amd64', + 'Maintainer': 'John Doe <john.doe@example.com>', + 'Installed-Size': '7', + 'Section': 'libs', + 'Priority': 'optional', + 'Multi-Arch': 'same', + 'Homepage': 'https://gitlab.com/', + 'Description': "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph." + } + + expect(subject.execute).to eq expected + end + end + + context 'with incorrect file' do + let(:file_name) { 'README.md' } + + it 'raise error' do + expect {subject.execute}.to raise_error(described_class::CommandFailedError, /is not a Debian format archive/i) + end + end +end diff --git a/spec/services/packages/debian/parse_debian822_service_spec.rb b/spec/services/packages/debian/parse_debian822_service_spec.rb new file mode 100644 index 00000000000..b67daca89c4 --- /dev/null +++ b/spec/services/packages/debian/parse_debian822_service_spec.rb @@ -0,0 +1,148 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Packages::Debian::ParseDebian822Service do + subject { described_class.new(input) } + + context 'with dpkg-deb --field output' do + let(:input) do + <<~HEREDOC + Package: libsample0 + Source: sample + Version: 1.2.3~alpha2 + Architecture: amd64 + Maintainer: John Doe <john.doe@example.com> + Installed-Size: 9 + Section: libs + Priority: optional + Multi-Arch: same + Homepage: https://gitlab.com/ + Description: Some mostly empty lib + Used in GitLab tests. + . + Testing another paragraph. + HEREDOC + end + + it 'return as expected, preserving order' do + expected = { + 'Package: libsample0' => { + 'Package': 'libsample0', + 'Source': 'sample', + 'Version': '1.2.3~alpha2', + 'Architecture': 'amd64', + 'Maintainer': 'John Doe <john.doe@example.com>', + 'Installed-Size': '9', + 'Section': 'libs', + 'Priority': 'optional', + 'Multi-Arch': 'same', + 'Homepage': 'https://gitlab.com/', + 'Description': "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph." + } + } + + expect(subject.execute.to_s).to eq(expected.to_s) + end + end + + context 'with control file' do + let(:input) { fixture_file('packages/debian/sample/debian/control') } + + it 'return as expected, preserving order' do + expected = { + 'Source: sample' => { + 'Source': 'sample', + 'Priority': 'optional', + 'Maintainer': 'John Doe <john.doe@example.com>', + 'Build-Depends': 'debhelper-compat (= 13)', + 'Standards-Version': '4.5.0', + 'Section': 'libs', + 'Homepage': 'https://gitlab.com/', + # 'Vcs-Browser': 'https://salsa.debian.org/debian/sample-1.2.3', + # '#Vcs-Git': 'https://salsa.debian.org/debian/sample-1.2.3.git', + 'Rules-Requires-Root': 'no' + }, + 'Package: sample-dev' => { + 'Package': 'sample-dev', + 'Section': 'libdevel', + 'Architecture': 'any', + 'Multi-Arch': 'same', + 'Depends': 'libsample0 (= ${binary:Version}), ${misc:Depends}', + 'Description': "Some mostly empty developpement files\nUsed in GitLab tests.\n\nTesting another paragraph." + }, + 'Package: libsample0' => { + 'Package': 'libsample0', + 'Architecture': 'any', + 'Multi-Arch': 'same', + 'Depends': '${shlibs:Depends}, ${misc:Depends}', + 'Description': "Some mostly empty lib\nUsed in GitLab tests.\n\nTesting another paragraph." + }, + 'Package: sample-udeb' => { + 'Package': 'sample-udeb', + 'Package-Type': 'udeb', + 'Architecture': 'any', + 'Depends': 'installed-base', + 'Description': 'Some mostly empty udeb' + } + } + + expect(subject.execute.to_s).to eq(expected.to_s) + end + end + + context 'with empty input' do + let(:input) { '' } + + it 'return a empty hash' do + expect(subject.execute).to eq({}) + end + end + + context 'with unexpected continuation line' do + let(:input) { ' continuation' } + + it 'raise error' do + expect {subject.execute}.to raise_error(described_class::InvalidDebian822Error, 'Parse error. Unexpected continuation line') + end + end + + context 'with duplicate field' do + let(:input) do + <<~HEREDOC + Package: libsample0 + Source: sample + Source: sample + HEREDOC + end + + it 'raise error' do + expect {subject.execute}.to raise_error(described_class::InvalidDebian822Error, "Duplicate field 'Source' in section 'Package: libsample0'") + end + end + + context 'with incorrect input' do + let(:input) do + <<~HEREDOC + Hello + HEREDOC + end + + it 'raise error' do + expect {subject.execute}.to raise_error(described_class::InvalidDebian822Error, 'Parse error on line Hello') + end + end + + context 'with duplicate section' do + let(:input) do + <<~HEREDOC + Package: libsample0 + + Package: libsample0 + HEREDOC + end + + it 'raise error' do + expect {subject.execute}.to raise_error(described_class::InvalidDebian822Error, "Duplicate section 'Package: libsample0'") + end + end +end diff --git a/spec/services/packages/generic/create_package_file_service_spec.rb b/spec/services/packages/generic/create_package_file_service_spec.rb index 0ae109ef996..907483e3d7f 100644 --- a/spec/services/packages/generic/create_package_file_service_spec.rb +++ b/spec/services/packages/generic/create_package_file_service_spec.rb @@ -5,6 +5,8 @@ require 'spec_helper' RSpec.describe Packages::Generic::CreatePackageFileService do let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } + let_it_be(:pipeline) { create(:ci_pipeline, user: user) } + let(:build) { double('build', pipeline: pipeline) } describe '#execute' do let(:sha256) { '440e5e148a25331bbd7991575f7d54933c0ebf6cc735a18ee5066ac1381bb590' } @@ -16,7 +18,8 @@ RSpec.describe Packages::Generic::CreatePackageFileService do package_name: 'mypackage', package_version: '0.0.1', file: file, - file_name: 'myfile.tar.gz.1' + file_name: 'myfile.tar.gz.1', + build: build } end @@ -41,6 +44,7 @@ RSpec.describe Packages::Generic::CreatePackageFileService do service = described_class.new(project, user, params) expect { service.execute }.to change { package.package_files.count }.by(1) + .and change { Packages::PackageFileBuildInfo.count }.by(1) package_file = package.package_files.last aggregate_failures do diff --git a/spec/services/packages/generic/find_or_create_package_service_spec.rb b/spec/services/packages/generic/find_or_create_package_service_spec.rb index 5a9b8b03279..a045cb36418 100644 --- a/spec/services/packages/generic/find_or_create_package_service_spec.rb +++ b/spec/services/packages/generic/find_or_create_package_service_spec.rb @@ -27,7 +27,7 @@ RSpec.describe Packages::Generic::FindOrCreatePackageService do expect(package.creator).to eq(user) expect(package.name).to eq('mypackage') expect(package.version).to eq('0.0.1') - expect(package.build_info).to be_nil + expect(package.original_build_info).to be_nil end end @@ -42,7 +42,7 @@ RSpec.describe Packages::Generic::FindOrCreatePackageService do expect(package.creator).to eq(user) expect(package.name).to eq('mypackage') expect(package.version).to eq('0.0.1') - expect(package.build_info.pipeline).to eq(ci_build.pipeline) + expect(package.original_build_info.pipeline).to eq(ci_build.pipeline) end end end @@ -60,7 +60,7 @@ RSpec.describe Packages::Generic::FindOrCreatePackageService do expect(found_package).to eq(package) end.not_to change { project.packages.generic.count } - expect(package.reload.build_info).to be_nil + expect(package.reload.original_build_info).to be_nil end end @@ -68,7 +68,7 @@ RSpec.describe Packages::Generic::FindOrCreatePackageService do let(:pipeline) { create(:ci_pipeline, project: project) } before do - package.create_build_info!(pipeline: pipeline) + package.build_infos.create!(pipeline: pipeline) end it 'finds the package and does not change package build info even if build is provided' do @@ -80,7 +80,7 @@ RSpec.describe Packages::Generic::FindOrCreatePackageService do expect(found_package).to eq(package) end.not_to change { project.packages.generic.count } - expect(package.reload.build_info.pipeline).to eq(pipeline) + expect(package.reload.original_build_info.pipeline).to eq(pipeline) 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 7ec368aa00f..11bf00c1399 100644 --- a/spec/services/packages/maven/create_package_service_spec.rb +++ b/spec/services/packages/maven/create_package_service_spec.rb @@ -33,8 +33,6 @@ RSpec.describe Packages::Maven::CreatePackageService do expect(package.maven_metadatum.app_version).to eq(version) end - it_behaves_like 'assigns build to package' - it_behaves_like 'assigns the package creator' end diff --git a/spec/services/packages/maven/find_or_create_package_service_spec.rb b/spec/services/packages/maven/find_or_create_package_service_spec.rb index 4406e4037e2..2eaad7db445 100644 --- a/spec/services/packages/maven/find_or_create_package_service_spec.rb +++ b/spec/services/packages/maven/find_or_create_package_service_spec.rb @@ -10,11 +10,12 @@ RSpec.describe Packages::Maven::FindOrCreatePackageService do let(:version) { '1.0.0' } let(:file_name) { 'test.jar' } let(:param_path) { "#{path}/#{version}" } + let(:params) { { path: param_path, file_name: file_name } } describe '#execute' do using RSpec::Parameterized::TableSyntax - subject { described_class.new(project, user, { path: param_path, file_name: file_name }).execute } + subject { described_class.new(project, user, params).execute } RSpec.shared_examples 'reuse existing package' do it { expect { subject}.not_to change { Packages::Package.count } } @@ -23,7 +24,7 @@ RSpec.describe Packages::Maven::FindOrCreatePackageService do end RSpec.shared_examples 'create package' do - it { expect { subject}.to change { Packages::Package.count }.by(1) } + it { expect { subject }.to change { Packages::Package.count }.by(1) } it 'sets the proper name and version' do pkg = subject @@ -31,6 +32,8 @@ RSpec.describe Packages::Maven::FindOrCreatePackageService do expect(pkg.name).to eq(path) expect(pkg.version).to eq(version) end + + it_behaves_like 'assigns build to package' end context 'path with version' do @@ -77,5 +80,15 @@ RSpec.describe Packages::Maven::FindOrCreatePackageService do end end end + + context 'with a build' do + let_it_be(:pipeline) { create(:ci_pipeline, user: user) } + let(:build) { double('build', pipeline: pipeline) } + let(:params) { { path: param_path, file_name: file_name, build: build } } + + it 'creates a build_info' do + expect { subject }.to change { Packages::BuildInfo.count }.by(1) + end + end end end diff --git a/spec/services/packages/npm/create_package_service_spec.rb b/spec/services/packages/npm/create_package_service_spec.rb index c8431c640da..6db3777cde8 100644 --- a/spec/services/packages/npm/create_package_service_spec.rb +++ b/spec/services/packages/npm/create_package_service_spec.rb @@ -48,7 +48,16 @@ RSpec.describe Packages::Npm::CreatePackageService do context 'scoped package' do it_behaves_like 'valid package' - it_behaves_like 'assigns build to package' + context 'with build info' do + let(:job) { create(:ci_build, user: user) } + let(:params) { super().merge(build: job) } + + it_behaves_like 'assigns build to package' + + it 'creates a package file build info' do + expect { subject }.to change { Packages::PackageFileBuildInfo.count }.by(1) + end + end end context 'invalid package name' do diff --git a/spec/services/pages/destroy_deployments_service_spec.rb b/spec/services/pages/destroy_deployments_service_spec.rb new file mode 100644 index 00000000000..0f8e8b6573e --- /dev/null +++ b/spec/services/pages/destroy_deployments_service_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Pages::DestroyDeploymentsService do + let(:project) { create(:project) } + let!(:old_deployments) { create_list(:pages_deployment, 2, project: project) } + let!(:last_deployment) { create(:pages_deployment, project: project) } + let!(:newer_deployment) { create(:pages_deployment, project: project) } + let!(:deployment_from_another_project) { create(:pages_deployment) } + + it 'destroys all deployments of the project' do + expect do + described_class.new(project).execute + end.to change { PagesDeployment.count }.by(-4) + + expect(deployment_from_another_project.reload).to be + end + + it 'destroy only deployments older than last deployment if it is provided' do + expect do + described_class.new(project, last_deployment.id).execute + end.to change { PagesDeployment.count }.by(-2) + + expect(last_deployment.reload).to be + expect(newer_deployment.reload).to be + expect(deployment_from_another_project.reload).to be + end +end diff --git a/spec/services/personal_access_tokens/create_service_spec.rb b/spec/services/personal_access_tokens/create_service_spec.rb index 475ade95948..842bebd13a1 100644 --- a/spec/services/personal_access_tokens/create_service_spec.rb +++ b/spec/services/personal_access_tokens/create_service_spec.rb @@ -3,21 +3,59 @@ require 'spec_helper' RSpec.describe PersonalAccessTokens::CreateService do + shared_examples_for 'a successfully created token' do + it 'creates personal access token record' do + expect(subject.success?).to be true + expect(token.name).to eq(params[:name]) + expect(token.impersonation).to eq(params[:impersonation]) + expect(token.scopes).to eq(params[:scopes]) + expect(token.expires_at).to eq(params[:expires_at]) + expect(token.user).to eq(user) + end + + it 'logs the event' do + expect(Gitlab::AppLogger).to receive(:info).with(/PAT CREATION: created_by: '#{current_user.username}', created_for: '#{user.username}', token_id: '\d+'/) + + subject + end + end + + shared_examples_for 'an unsuccessfully created token' do + it { expect(subject.success?).to be false } + it { expect(subject.message).to eq('Not permitted to create') } + it { expect(token).to be_nil } + end + describe '#execute' do - context 'with valid params' do - it 'creates personal access token record' do - user = create(:user) - params = { name: 'Test token', impersonation: true, scopes: [:api], expires_at: Date.today + 1.month } - - response = described_class.new(user, params).execute - personal_access_token = response.payload[:personal_access_token] - - expect(response.success?).to be true - expect(personal_access_token.name).to eq(params[:name]) - expect(personal_access_token.impersonation).to eq(params[:impersonation]) - expect(personal_access_token.scopes).to eq(params[:scopes]) - expect(personal_access_token.expires_at).to eq(params[:expires_at]) - expect(personal_access_token.user).to eq(user) + subject { service.execute } + + let(:current_user) { create(:user) } + let(:user) { create(:user) } + let(:params) { { name: 'Test token', impersonation: false, scopes: [:api], expires_at: Date.today + 1.month } } + let(:service) { described_class.new(current_user: current_user, target_user: user, params: params) } + let(:token) { subject.payload[:personal_access_token] } + + context 'when current_user is an administrator' do + let(:current_user) { create(:admin) } + + context 'when admin mode is enabled', :enable_admin_mode do + it_behaves_like 'a successfully created token' + end + + context 'when admin mode is disabled' do + it_behaves_like 'an unsuccessfully created token' + end + end + + context 'when current_user is not an administrator' do + context 'target_user is not the same as current_user' do + it_behaves_like 'an unsuccessfully created token' + end + + context 'target_user is same as current_user' do + let(:current_user) { user } + + it_behaves_like 'a successfully created token' end end end diff --git a/spec/services/personal_access_tokens/revoke_service_spec.rb b/spec/services/personal_access_tokens/revoke_service_spec.rb index 5afa43cef76..a25484e218e 100644 --- a/spec/services/personal_access_tokens/revoke_service_spec.rb +++ b/spec/services/personal_access_tokens/revoke_service_spec.rb @@ -6,6 +6,11 @@ RSpec.describe PersonalAccessTokens::RevokeService do shared_examples_for 'a successfully revoked token' do it { expect(subject.success?).to be true } it { expect(service.token.revoked?).to be true } + it 'logs the event' do + expect(Gitlab::AppLogger).to receive(:info).with(/PAT REVOCATION: revoked_by: '#{current_user.username}', revoked_for: '#{token.user.username}', token_id: '\d+'/) + + subject + end end shared_examples_for 'an unsuccessfully revoked token' do @@ -19,10 +24,19 @@ RSpec.describe PersonalAccessTokens::RevokeService do let(:service) { described_class.new(current_user, token: token) } context 'when current_user is an administrator' do - let_it_be(:current_user) { create(:admin) } - let_it_be(:token) { create(:personal_access_token) } + context 'when admin mode is enabled', :enable_admin_mode do + let_it_be(:current_user) { create(:admin) } + let_it_be(:token) { create(:personal_access_token) } + + it_behaves_like 'a successfully revoked token' + end + + context 'when admin mode is disabled' do + let_it_be(:current_user) { create(:admin) } + let_it_be(:token) { create(:personal_access_token) } - it_behaves_like 'a successfully revoked token' + it_behaves_like 'an unsuccessfully revoked token' + end end context 'when current_user is not an administrator' do diff --git a/spec/services/post_receive_service_spec.rb b/spec/services/post_receive_service_spec.rb index c726e1851a7..7c4b7f51cc3 100644 --- a/spec/services/post_receive_service_spec.rb +++ b/spec/services/post_receive_service_spec.rb @@ -232,6 +232,49 @@ RSpec.describe PostReceiveService do end end + context "broadcast message has a target_path" do + let!(:older_scoped_message) do + create(:broadcast_message, message: "Old top secret", target_path: "/company/sekrit-project") + end + + let!(:latest_scoped_message) do + create(:broadcast_message, message: "Top secret", target_path: "/company/sekrit-project") + end + + let!(:unscoped_message) do + create(:broadcast_message, message: "Hi") + end + + context "no project path matches" do + it "does not output the scoped broadcast messages" do + expect(subject).not_to include(build_alert_message(older_scoped_message.message)) + expect(subject).not_to include(build_alert_message(latest_scoped_message.message)) + end + + it "does output another message that doesn't have a target_path" do + expect(subject).to include(build_alert_message(unscoped_message.message)) + end + end + + context "project path matches" do + before do + allow(project).to receive(:full_path).and_return("/company/sekrit-project") + end + + it "does output the latest scoped broadcast message" do + expect(subject).to include(build_alert_message(latest_scoped_message.message)) + end + + it "does not output the older scoped broadcast message" do + expect(subject).not_to include(build_alert_message(older_scoped_message.message)) + end + + it "does not output another message that doesn't have a target_path" do + expect(subject).not_to include(build_alert_message(unscoped_message.message)) + end + end + end + context 'with a redirected data' do it 'returns redirected message on the response' do project_moved = Gitlab::Checks::ProjectMoved.new(project.repository, user, 'http', 'foo/baz') diff --git a/spec/services/projects/alerting/notify_service_spec.rb b/spec/services/projects/alerting/notify_service_spec.rb index 809b12910a1..4674f614cf1 100644 --- a/spec/services/projects/alerting/notify_service_spec.rb +++ b/spec/services/projects/alerting/notify_service_spec.rb @@ -34,13 +34,11 @@ RSpec.describe Projects::Alerting::NotifyService do let(:payload) { ActionController::Parameters.new(payload_raw).permit! } - subject { service.execute(token) } - - context 'with activated Alerts Service' do - let_it_be_with_reload(:alerts_service) { create(:alerts_service, project: project) } + subject { service.execute(token, nil) } + shared_examples 'notifcations are handled correctly' do context 'with valid token' do - let(:token) { alerts_service.token } + let(:token) { integration.token } 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 } @@ -131,6 +129,12 @@ RSpec.describe Projects::Alerting::NotifyService do it { expect { subject }.to change { issue.reload.state }.from('opened').to('closed') } it { expect { subject }.to change(ResourceStateEvent, :count).by(1) } end + + context 'with issue enabled' do + let(:issue_enabled) { true } + + it_behaves_like 'does not process incident issues' + end end end @@ -197,7 +201,7 @@ RSpec.describe Projects::Alerting::NotifyService do it 'creates a system note corresponding to alert creation' do expect { subject }.to change(Note, :count).by(1) - expect(Note.last.note).to include('Generic Alert Endpoint') + expect(Note.last.note).to include(source) end end end @@ -247,10 +251,20 @@ RSpec.describe Projects::Alerting::NotifyService do it_behaves_like 'does not process incident issues due to error', http_status: :unauthorized it_behaves_like 'does not an create alert management alert' end + end + + context 'with an HTTP Integration' do + let_it_be_with_reload(:integration) { create(:alert_management_http_integration, project: project) } + + subject { service.execute(token, integration) } + + it_behaves_like 'notifcations are handled correctly' do + let(:source) { integration.name } + end - context 'with deactivated Alerts Service' do + context 'with deactivated HTTP Integration' do before do - alerts_service.update!(active: false) + integration.update!(active: false) end it_behaves_like 'does not process incident issues due to error', http_status: :forbidden diff --git a/spec/services/projects/autocomplete_service_spec.rb b/spec/services/projects/autocomplete_service_spec.rb index aff1aa41091..ef7741c2d0f 100644 --- a/spec/services/projects/autocomplete_service_spec.rb +++ b/spec/services/projects/autocomplete_service_spec.rb @@ -79,14 +79,28 @@ RSpec.describe Projects::AutocompleteService do expect(issues.count).to eq 3 end - it 'lists all project issues for admin' do - autocomplete = described_class.new(project, admin) - issues = autocomplete.issues.map(&:iid) + context 'when admin mode is enabled', :enable_admin_mode do + it 'lists all project issues for admin', :enable_admin_mode do + autocomplete = described_class.new(project, admin) + issues = autocomplete.issues.map(&:iid) + + expect(issues).to include issue.iid + expect(issues).to include security_issue_1.iid + expect(issues).to include security_issue_2.iid + expect(issues.count).to eq 3 + end + end - expect(issues).to include issue.iid - expect(issues).to include security_issue_1.iid - expect(issues).to include security_issue_2.iid - expect(issues.count).to eq 3 + context 'when admin mode is disabled' do + it 'does not list project confidential issues for admin' do + autocomplete = described_class.new(project, admin) + issues = autocomplete.issues.map(&:iid) + + expect(issues).to include issue.iid + expect(issues).not_to include security_issue_1.iid + expect(issues).not_to include security_issue_2.iid + expect(issues.count).to eq 1 + end end end end diff --git a/spec/services/projects/cleanup_service_spec.rb b/spec/services/projects/cleanup_service_spec.rb index 7c28b729e84..6fd29813d98 100644 --- a/spec/services/projects/cleanup_service_spec.rb +++ b/spec/services/projects/cleanup_service_spec.rb @@ -3,14 +3,84 @@ require 'spec_helper' RSpec.describe Projects::CleanupService do - let(:project) { create(:project, :repository, bfg_object_map: fixture_file_upload('spec/fixtures/bfg_object_map.txt')) } - let(:object_map) { project.bfg_object_map } + subject(:service) { described_class.new(project) } - let(:cleaner) { service.__send__(:repository_cleaner) } + describe '.enqueue' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } - subject(:service) { described_class.new(project) } + let(:object_map_file) { fixture_file_upload('spec/fixtures/bfg_object_map.txt') } + + subject(:enqueue) { described_class.enqueue(project, user, object_map_file) } + + it 'makes the repository read-only' do + expect { enqueue } + .to change(project, :repository_read_only?) + .from(false) + .to(true) + end + + it 'sets the bfg_object_map of the project' do + enqueue + + expect(project.bfg_object_map.read).to eq(object_map_file.read) + end + + it 'enqueues a RepositoryCleanupWorker' do + enqueue + + expect(RepositoryCleanupWorker.jobs.count).to eq(1) + end + + it 'returns success' do + expect(enqueue[:status]).to eq(:success) + end + + it 'returns an error if making the repository read-only fails' do + project.set_repository_read_only! + + expect(enqueue[:status]).to eq(:error) + end + + it 'returns an error if updating the project fails' do + expect_next_instance_of(Projects::UpdateService) do |service| + expect(service).to receive(:execute).and_return(status: :error) + end + + expect(enqueue[:status]).to eq(:error) + expect(project.reload.repository_read_only?).to be_falsy + end + end + + describe '.cleanup_after' do + let(:project) { create(:project, :repository, bfg_object_map: fixture_file_upload('spec/fixtures/bfg_object_map.txt')) } + + subject(:cleanup_after) { described_class.cleanup_after(project) } + + before do + project.set_repository_read_only! + end + + it 'sets the repository read-write' do + expect { cleanup_after }.to change(project, :repository_read_only?).from(true).to(false) + end + + it 'removes the BFG object map' do + cleanup_after + + expect(project.bfg_object_map).not_to be_exist + end + end describe '#execute' do + let(:project) { create(:project, :repository, bfg_object_map: fixture_file_upload('spec/fixtures/bfg_object_map.txt')) } + let(:object_map) { project.bfg_object_map } + let(:cleaner) { service.__send__(:repository_cleaner) } + + before do + project.set_repository_read_only! + end + it 'runs the apply_bfg_object_map_stream gitaly RPC' do expect(cleaner).to receive(:apply_bfg_object_map_stream).with(kind_of(IO)) @@ -19,7 +89,7 @@ RSpec.describe Projects::CleanupService do it 'runs garbage collection on the repository' do expect_next_instance_of(GitGarbageCollectWorker) do |worker| - expect(worker).to receive(:perform).with(project.id, :gc, "project_cleanup:gc:#{project.id}") + expect(worker).to receive(:perform).with(project.id, :prune, "project_cleanup:gc:#{project.id}") end service.execute @@ -37,6 +107,13 @@ RSpec.describe Projects::CleanupService do expect(object_map.exists?).to be_falsy end + it 'makes the repository read-write again' do + expect { service.execute } + .to change(project, :repository_read_only?) + .from(true) + .to(false) + end + context 'with a tainted merge request diff' do let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } let(:diff) { merge_request.merge_request_diff } 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 c3ae26b1f05..a012ec29be5 100644 --- a/spec/services/projects/container_repository/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/delete_tags_service_spec.rb @@ -27,13 +27,17 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do end end - RSpec.shared_examples 'logging an error response' do |message: 'could not delete tags'| + RSpec.shared_examples 'logging an error response' do |message: 'could not delete tags', extra_log: {}| it 'logs an error message' do - expect(service).to receive(:log_error).with( - service_class: 'Projects::ContainerRepository::DeleteTagsService', - message: message, - container_repository_id: repository.id - ) + log_data = { + service_class: 'Projects::ContainerRepository::DeleteTagsService', + message: message, + container_repository_id: repository.id + } + + log_data.merge!(extra_log) if extra_log.any? + + expect(service).to receive(:log_error).with(log_data) subject end @@ -115,7 +119,7 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do it { is_expected.to include(status: :error, message: 'timeout while deleting tags') } - it_behaves_like 'logging an error response', message: 'timeout while deleting tags' + it_behaves_like 'logging an error response', message: 'timeout while deleting tags', extra_log: { deleted_tags_count: 0 } end end end 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 3bbcec8775e..988971171fc 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 @@ -67,7 +67,7 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService do stub_delete_reference_requests('A' => 200) end - it { is_expected.to include(status: :error, message: 'timeout while deleting tags') } + it { is_expected.to eq(status: :error, message: 'timeout while deleting tags', deleted: ['A']) } it 'tracks the exception' do expect(::Gitlab::ErrorTracking) diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index d959cc87901..6c0e6654622 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -72,14 +72,25 @@ RSpec.describe Projects::CreateService, '#execute' do end context "admin creates project with other user's namespace_id" do - it 'sets the correct permissions' do - admin = create(:admin) - project = create_project(admin, opts) + context 'when admin mode is enabled', :enable_admin_mode do + it 'sets the correct permissions' do + admin = create(:admin) + project = create_project(admin, opts) - expect(project).to be_persisted - expect(project.owner).to eq(user) - expect(project.team.maintainers).to contain_exactly(user) - expect(project.namespace).to eq(user.namespace) + expect(project).to be_persisted + expect(project.owner).to eq(user) + expect(project.team.maintainers).to contain_exactly(user) + expect(project.namespace).to eq(user.namespace) + end + end + + context 'when admin mode is disabled' do + it 'is not allowed' do + admin = create(:admin) + project = create_project(admin, opts) + + expect(project).not_to be_persisted + end end end @@ -336,7 +347,15 @@ RSpec.describe Projects::CreateService, '#execute' do ) end - it 'allows a restricted visibility level for admins' do + it 'does not allow a restricted visibility level for admins when admin mode is disabled' do + admin = create(:admin) + project = create_project(admin, opts) + + expect(project.errors.any?).to be(true) + expect(project.saved?).to be_falsey + end + + it 'allows a restricted visibility level for admins when admin mode is enabled', :enable_admin_mode do admin = create(:admin) project = create_project(admin, opts) 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 cfe8e863223..1b829df6e6a 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -241,18 +241,6 @@ RSpec.describe Projects::LfsPointers::LfsDownloadService do 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 diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index d3eb84a3137..d2c6c0eb971 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -27,7 +27,7 @@ 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) } + let!(:artifacts_archive) { create(:ci_job_artifact, :correct_checksum, file: file, job: build) } before do create(:ci_job_artifact, file_type: :metadata, file_format: :gzip, file: metadata, job: build) @@ -66,9 +66,45 @@ RSpec.describe Projects::UpdatePagesService do expect(deployment.size).to eq(file.size) expect(deployment.file).to be + expect(deployment.file_count).to eq(3) + expect(deployment.file_sha256).to eq(artifacts_archive.file_sha256) expect(project.pages_metadatum.reload.pages_deployment_id).to eq(deployment.id) end + it 'does not fail if pages_metadata is absent' do + project.pages_metadatum.destroy! + project.reload + + expect do + expect(execute).to eq(:success) + end.to change { project.pages_deployments.count }.by(1) + + expect(project.pages_metadatum.reload.pages_deployment).to eq(project.pages_deployments.last) + end + + context 'when there is an old pages deployment' do + let!(:old_deployment_from_another_project) { create(:pages_deployment) } + let!(:old_deployment) { create(:pages_deployment, project: project) } + + it 'schedules a destruction of older deployments' do + expect(DestroyPagesDeploymentsWorker).to( + receive(:perform_in).with(described_class::OLD_DEPLOYMENTS_DESTRUCTION_DELAY, + project.id, + instance_of(Integer)) + ) + + execute + end + + it 'removes older deployments', :sidekiq_inline do + expect do + execute + end.not_to change { PagesDeployment.count } # it creates one and deletes one + + expect(PagesDeployment.find_by_id(old_deployment.id)).to be_nil + end + end + it 'does not create deployment when zip_pages_deployments feature flag is disabled' do stub_feature_flags(zip_pages_deployments: false) @@ -188,6 +224,25 @@ RSpec.describe Projects::UpdatePagesService do end end + # this situation should never happen in real life because all new archives have sha256 + # and we only use new archives + # this test is here just to clarify that this behavior is intentional + context 'when artifacts archive does not have sha256' do + let!(:artifacts_archive) { create(:ci_job_artifact, file: file, job: build) } + + before do + create(:ci_job_artifact, file_type: :metadata, file_format: :gzip, file: metadata, job: build) + + build.reload + end + + it 'fails with exception raised' do + expect do + execute + end.to raise_error("Validation failed: File sha256 can't be blank") + end + end + it 'fails to remove project pages when no pages is deployed' do expect(PagesWorker).not_to receive(:perform_in) expect(project.pages_deployed?).to be_falsey @@ -210,7 +265,7 @@ RSpec.describe Projects::UpdatePagesService do file = fixture_file_upload('spec/fixtures/pages.zip') metafile = fixture_file_upload('spec/fixtures/pages.zip.meta') - create(:ci_job_artifact, :archive, file: file, job: build) + create(:ci_job_artifact, :archive, :correct_checksum, file: file, job: build) create(:ci_job_artifact, :metadata, file: metafile, job: build) allow(build).to receive(:artifacts_metadata_entry) diff --git a/spec/services/projects/update_repository_storage_service_spec.rb b/spec/services/projects/update_repository_storage_service_spec.rb index 0fcd14f3bc9..123f604e7a4 100644 --- a/spec/services/projects/update_repository_storage_service_spec.rb +++ b/spec/services/projects/update_repository_storage_service_spec.rb @@ -168,6 +168,24 @@ RSpec.describe Projects::UpdateRepositoryStorageService do end end + context 'project with no repositories' do + let(:project) { create(:project) } + let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: 'test_second_storage') } + + it 'updates the database' do + allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original + allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid) + + result = subject.execute + project.reload + + expect(result).to be_success + expect(project).not_to be_repository_read_only + expect(project.repository_storage).to eq('test_second_storage') + expect(project.project_repository.shard_name).to eq('test_second_storage') + end + end + context 'with wiki repository' do include_examples 'moves repository to another storage', 'wiki' do let(:project) { create(:project, :repository, wiki_enabled: true) } diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 989426fde8b..760cd85bf71 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -127,11 +127,22 @@ RSpec.describe Projects::UpdateService do end context 'when updated by an admin' do - it 'updates the project to public' do - result = update_project(project, admin, visibility_level: Gitlab::VisibilityLevel::PUBLIC) + context 'when admin mode is enabled', :enable_admin_mode do + it 'updates the project to public' do + result = update_project(project, admin, visibility_level: Gitlab::VisibilityLevel::PUBLIC) - expect(result).to eq({ status: :success }) - expect(project).to be_public + expect(result).to eq({ status: :success }) + expect(project).to be_public + end + end + + context 'when admin mode is disabled' do + it 'does not update the project to public' do + result = update_project(project, admin, visibility_level: Gitlab::VisibilityLevel::PUBLIC) + + expect(result).to eq({ status: :error, message: 'New visibility level not allowed!' }) + expect(project).to be_private + end end end end @@ -144,7 +155,7 @@ RSpec.describe Projects::UpdateService do project.update!(namespace: group, visibility_level: group.visibility_level) end - it 'does not update project visibility level' do + it 'does not update project visibility level even if admin', :enable_admin_mode do result = update_project(project, admin, visibility_level: Gitlab::VisibilityLevel::PUBLIC) expect(result).to eq({ status: :error, message: 'Visibility level public is not allowed in a internal group.' }) @@ -181,6 +192,7 @@ RSpec.describe Projects::UpdateService do describe 'when updating project that has forks' do let(:project) { create(:project, :internal) } + let(:user) { project.owner } let(:forked_project) { fork_project(project) } context 'and unlink forks feature flag is off' do @@ -194,7 +206,7 @@ RSpec.describe Projects::UpdateService do expect(project).to be_internal expect(forked_project).to be_internal - expect(update_project(project, admin, opts)).to eq({ status: :success }) + expect(update_project(project, user, opts)).to eq({ status: :success }) expect(project).to be_private expect(forked_project.reload).to be_private @@ -206,7 +218,7 @@ RSpec.describe Projects::UpdateService do expect(project).to be_internal expect(forked_project).to be_internal - expect(update_project(project, admin, opts)).to eq({ status: :success }) + expect(update_project(project, user, opts)).to eq({ status: :success }) expect(project).to be_public expect(forked_project.reload).to be_internal @@ -220,7 +232,7 @@ RSpec.describe Projects::UpdateService do expect(project).to be_internal expect(forked_project).to be_internal - expect(update_project(project, admin, opts)).to eq({ status: :success }) + expect(update_project(project, user, opts)).to eq({ status: :success }) expect(project).to be_private expect(forked_project.reload).to be_internal @@ -576,15 +588,21 @@ RSpec.describe Projects::UpdateService do context 'authenticated as admin' do let(:user) { create(:admin) } - it 'schedules the transfer of the repository to the new storage and locks the project' do - update_project(project, admin, opts) + context 'when admin mode is enabled', :enable_admin_mode do + it 'schedules the transfer of the repository to the new storage and locks the project' do + update_project(project, admin, opts) - expect(project).to be_repository_read_only - expect(project.repository_storage_moves.last).to have_attributes( - state: ::ProjectRepositoryStorageMove.state_machines[:state].states[:scheduled].value, - source_storage_name: 'default', - destination_storage_name: 'test_second_storage' - ) + expect(project).to be_repository_read_only + expect(project.repository_storage_moves.last).to have_attributes( + state: ::ProjectRepositoryStorageMove.state_machines[:state].states[:scheduled].value, + source_storage_name: 'default', + destination_storage_name: 'test_second_storage' + ) + end + end + + context 'when admin mode is disabled' do + it_behaves_like 'the transfer was not scheduled' end context 'the repository is read-only' do diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 6f3814095f9..1f521ed4a93 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -312,8 +312,8 @@ RSpec.describe QuickActions::InterpretService do end end - shared_examples 'wip command' do - it 'returns wip_event: "wip" if content contains /wip' do + shared_examples 'draft command' do + it 'returns wip_event: "wip" if content contains /draft' do _, updates, _ = service.execute(content, issuable) expect(updates).to eq(wip_event: 'wip') @@ -322,12 +322,12 @@ RSpec.describe QuickActions::InterpretService do it 'returns the wip message' do _, _, message = service.execute(content, issuable) - expect(message).to eq("Marked this #{issuable.to_ability_name.humanize(capitalize: false)} as Work In Progress.") + expect(message).to eq("Marked this #{issuable.to_ability_name.humanize(capitalize: false)} as a draft.") end end - shared_examples 'unwip command' do - it 'returns wip_event: "unwip" if content contains /wip' do + shared_examples 'undraft command' do + it 'returns wip_event: "unwip" if content contains /draft' do issuable.update!(title: issuable.wip_title) _, updates, _ = service.execute(content, issuable) @@ -338,7 +338,7 @@ RSpec.describe QuickActions::InterpretService do issuable.update!(title: issuable.wip_title) _, _, message = service.execute(content, issuable) - expect(message).to eq("Unmarked this #{issuable.to_ability_name.humanize(capitalize: false)} as Work In Progress.") + expect(message).to eq("Unmarked this #{issuable.to_ability_name.humanize(capitalize: false)} as a draft.") end end @@ -1026,16 +1026,26 @@ RSpec.describe QuickActions::InterpretService do let(:issuable) { issue } end - it_behaves_like 'wip command' do + it_behaves_like 'draft command' do let(:content) { '/wip' } let(:issuable) { merge_request } end - it_behaves_like 'unwip command' do + it_behaves_like 'undraft command' do let(:content) { '/wip' } let(:issuable) { merge_request } end + it_behaves_like 'draft command' do + let(:content) { '/draft' } + let(:issuable) { merge_request } + end + + it_behaves_like 'undraft command' do + let(:content) { '/draft' } + let(:issuable) { merge_request } + end + it_behaves_like 'empty command' do let(:content) { '/remove_due_date' } let(:issuable) { merge_request } @@ -1896,13 +1906,13 @@ RSpec.describe QuickActions::InterpretService do end end - describe 'wip command' do - let(:content) { '/wip' } + describe 'draft command' do + let(:content) { '/draft' } it 'includes the new status' do _, explanations = service.explain(content, merge_request) - expect(explanations).to eq(['Marks this merge request as Work In Progress.']) + expect(explanations).to eq(['Marks this merge request as a draft.']) end end diff --git a/spec/services/releases/create_service_spec.rb b/spec/services/releases/create_service_spec.rb index 90648340b66..b9294182883 100644 --- a/spec/services/releases/create_service_spec.rb +++ b/spec/services/releases/create_service_spec.rb @@ -22,6 +22,12 @@ RSpec.describe Releases::CreateService do it 'creates a new release' do expected_job_count = MailScheduler::NotificationServiceWorker.jobs.size + 1 + expect_next_instance_of(Release) do |release| + expect(release) + .to receive(:execute_hooks) + .with('create') + end + result = service.execute expect(project.releases.count).to eq(1) diff --git a/spec/services/releases/update_service_spec.rb b/spec/services/releases/update_service_spec.rb index 00544b820cb..932a7fab5ec 100644 --- a/spec/services/releases/update_service_spec.rb +++ b/spec/services/releases/update_service_spec.rb @@ -32,6 +32,12 @@ RSpec.describe Releases::UpdateService do expect(result[:release].description).to eq(new_description) end + it 'executes hooks' do + expect(service.release).to receive(:execute_hooks).with('update') + + service.execute + end + context 'when the tag does not exists' do let(:tag_name) { 'foobar' } diff --git a/spec/services/reset_project_cache_service_spec.rb b/spec/services/reset_project_cache_service_spec.rb index 3e79270da8d..165b38ee338 100644 --- a/spec/services/reset_project_cache_service_spec.rb +++ b/spec/services/reset_project_cache_service_spec.rb @@ -20,7 +20,7 @@ RSpec.describe ResetProjectCacheService do context 'when project cache_index is a numeric value' do before do - project.update(jobs_cache_index: 1) + project.update!(jobs_cache_index: 1) end it 'increments project cache index' do diff --git a/spec/services/resource_access_tokens/create_service_spec.rb b/spec/services/resource_access_tokens/create_service_spec.rb index d8b12cda632..5cfa1ae93e6 100644 --- a/spec/services/resource_access_tokens/create_service_spec.rb +++ b/spec/services/resource_access_tokens/create_service_spec.rb @@ -11,16 +11,15 @@ RSpec.describe ResourceAccessTokens::CreateService do describe '#execute' do # Created shared_examples as it will easy to include specs for group bots in https://gitlab.com/gitlab-org/gitlab/-/issues/214046 - shared_examples 'fails when user does not have the permission to create a Resource Bot' do - before_all do - resource.add_developer(user) - end + shared_examples 'token creation fails' do + let(:resource) { create(:project)} - it 'returns error' do - response = subject + it 'does not add the project bot as a member' do + expect { subject }.not_to change { resource.members.count } + end - expect(response.error?).to be true - expect(response.message).to eq("User does not have permission to create #{resource_type} Access Token") + it 'immediately destroys the bot user if one was created', :sidekiq_inline do + expect { subject }.not_to change { User.bots.count } end end @@ -47,8 +46,18 @@ RSpec.describe ResourceAccessTokens::CreateService do end context 'when created by an admin' do - it_behaves_like 'creates a user that has their email confirmed' do - let(:user) { create(:admin) } + let(:user) { create(:admin) } + + context 'when admin mode is enabled', :enable_admin_mode do + it_behaves_like 'creates a user that has their email confirmed' + end + + context 'when admin mode is disabled' do + it 'returns error' do + response = subject + + expect(response.error?).to be true + end end end @@ -154,24 +163,36 @@ RSpec.describe ResourceAccessTokens::CreateService do context 'when invalid scope is passed' do let_it_be(:params) { { scopes: [:invalid_scope] } } - it 'returns error' do + it_behaves_like 'token creation fails' + + it 'returns the scope error message' do response = subject expect(response.error?).to be true + expect(response.errors).to include("Scopes can only contain available scopes") end end end - end - context 'when access provisioning fails' do - before do - allow(resource).to receive(:add_user).and_return(nil) - end + context "when access provisioning fails" do + let_it_be(:bot_user) { create(:user, :project_bot) } + let(:unpersisted_member) { build(:project_member, source: resource, user: bot_user) } - it 'returns error' do - response = subject + before do + allow_next_instance_of(ResourceAccessTokens::CreateService) do |service| + allow(service).to receive(:create_user).and_return(bot_user) + allow(service).to receive(:create_membership).and_return(unpersisted_member) + end + end - expect(response.error?).to be true + it_behaves_like 'token creation fails' + + it 'returns the provisioning error message' do + response = subject + + expect(response.error?).to be true + expect(response.errors).to include("Could not provision maintainer access to project access token") + end end end end @@ -180,7 +201,16 @@ RSpec.describe ResourceAccessTokens::CreateService do let_it_be(:resource_type) { 'project' } let_it_be(:resource) { project } - it_behaves_like 'fails when user does not have the permission to create a Resource Bot' + context 'when user does not have permission to create a resource bot' do + it_behaves_like 'token creation fails' + + it 'returns the permission error message' do + response = subject + + expect(response.error?).to be true + expect(response.errors).to include("User does not have permission to create #{resource_type} Access Token") + end + end context 'user with valid permission' do before_all do diff --git a/spec/services/resource_events/change_milestone_service_spec.rb b/spec/services/resource_events/change_milestone_service_spec.rb index 3a9dadbd40e..a2131c5c1b0 100644 --- a/spec/services/resource_events/change_milestone_service_spec.rb +++ b/spec/services/resource_events/change_milestone_service_spec.rb @@ -11,7 +11,7 @@ RSpec.describe ResourceEvents::ChangeMilestoneService do [:issue, :merge_request].each do |issuable| it_behaves_like 'timebox(milestone or iteration) resource events creator', ResourceMilestoneEvent do - let_it_be(:resource) { create(issuable) } + let_it_be(:resource) { create(issuable) } # rubocop:disable Rails/SaveBang end end end diff --git a/spec/services/search/snippet_service_spec.rb b/spec/services/search/snippet_service_spec.rb index ceaf3d055bf..d204f626635 100644 --- a/spec/services/search/snippet_service_spec.rb +++ b/spec/services/search/snippet_service_spec.rb @@ -49,12 +49,24 @@ RSpec.describe Search::SnippetService do expect(results.objects('snippet_titles')).to match_array [public_snippet, internal_snippet, private_snippet, project_public_snippet, project_internal_snippet] end - it 'returns all snippets when user is admin' do - admin = create(:admin) - search = described_class.new(admin, search: 'bar') - results = search.execute + context 'when admin mode is enabled', :enable_admin_mode do + it 'returns all snippets when user is admin' do + admin = create(:admin) + search = described_class.new(admin, search: 'bar') + results = search.execute + + expect(results.objects('snippet_titles')).to match_array [public_snippet, internal_snippet, private_snippet, project_public_snippet, project_internal_snippet, project_private_snippet] + end + end + + context 'when admin mode is disabled' do + it 'returns only public & internal snippets when user is admin' do + admin = create(:admin) + search = described_class.new(admin, search: 'bar') + results = search.execute - expect(results.objects('snippet_titles')).to match_array [public_snippet, internal_snippet, private_snippet, project_public_snippet, project_internal_snippet, project_private_snippet] + expect(results.objects('snippet_titles')).to match_array [public_snippet, internal_snippet, project_public_snippet, project_internal_snippet] + end end end end diff --git a/spec/services/search_service_spec.rb b/spec/services/search_service_spec.rb index fc613a6224a..40fb257b23e 100644 --- a/spec/services/search_service_spec.rb +++ b/spec/services/search_service_spec.rb @@ -18,9 +18,10 @@ RSpec.describe SearchService do let(:group_project) { create(:project, group: accessible_group, name: 'group_project') } let(:public_project) { create(:project, :public, name: 'public_project') } + let(:page) { 1 } let(:per_page) { described_class::DEFAULT_PER_PAGE } - subject(:search_service) { described_class.new(user, search: search, scope: scope, page: 1, per_page: per_page) } + subject(:search_service) { described_class.new(user, search: search, scope: scope, page: page, per_page: per_page) } before do accessible_project.add_maintainer(user) @@ -242,10 +243,10 @@ RSpec.describe SearchService do end describe '#search_objects' do - context 'handling per_page param' do - let(:search) { '' } - let(:scope) { nil } + let(:search) { '' } + let(:scope) { nil } + describe 'per_page: parameter' do context 'when nil' do let(:per_page) { nil } @@ -312,6 +313,34 @@ RSpec.describe SearchService do end end + describe 'page: parameter' do + context 'when < 1' do + let(:page) { 0 } + + it "defaults to 1" do + expect_any_instance_of(Gitlab::SearchResults) + .to receive(:objects) + .with(anything, hash_including(page: 1)) + .and_call_original + + subject.search_objects + end + end + + context 'when nil' do + let(:page) { nil } + + it "defaults to 1" do + expect_any_instance_of(Gitlab::SearchResults) + .to receive(:objects) + .with(anything, hash_including(page: 1)) + .and_call_original + + subject.search_objects + end + end + end + context 'with accessible project_id' do it 'returns objects in the project' do search_objects = described_class.new( diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index b7fb5a98d06..96807fd629f 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -147,9 +147,11 @@ RSpec.describe Snippets::CreateService do end context 'when the commit action fails' do + let(:error) { SnippetRepository::CommitError.new('foobar') } + before do allow_next_instance_of(SnippetRepository) do |instance| - allow(instance).to receive(:multi_files_action).and_raise(SnippetRepository::CommitError.new('foobar')) + allow(instance).to receive(:multi_files_action).and_raise(error) end end @@ -172,7 +174,7 @@ RSpec.describe Snippets::CreateService do end it 'logs the error' do - expect(Gitlab::AppLogger).to receive(:error).with('foobar') + expect(Gitlab::ErrorTracking).to receive(:log_exception).with(error, service: 'Snippets::CreateService') subject end diff --git a/spec/services/snippets/update_service_spec.rb b/spec/services/snippets/update_service_spec.rb index 406ece30bd7..a2341dc71b2 100644 --- a/spec/services/snippets/update_service_spec.rb +++ b/spec/services/snippets/update_service_spec.rb @@ -277,14 +277,14 @@ RSpec.describe Snippets::UpdateService do end context 'when an error is raised' do - let(:error_message) { 'foobar' } + let(:error) { SnippetRepository::CommitError.new('foobar') } before do - allow(snippet.snippet_repository).to receive(:multi_files_action).and_raise(SnippetRepository::CommitError, error_message) + allow(snippet.snippet_repository).to receive(:multi_files_action).and_raise(error) end it 'logs the error' do - expect(Gitlab::AppLogger).to receive(:error).with(error_message) + expect(Gitlab::ErrorTracking).to receive(:log_exception).with(error, service: 'Snippets::UpdateService') subject end diff --git a/spec/services/system_hooks_service_spec.rb b/spec/services/system_hooks_service_spec.rb index bdc40a92e91..b25837ca260 100644 --- a/spec/services/system_hooks_service_spec.rb +++ b/spec/services/system_hooks_service_spec.rb @@ -85,7 +85,7 @@ RSpec.describe SystemHooksService do end it 'handles nil datetime columns' do - user.update(created_at: nil, updated_at: nil) + user.update!(created_at: nil, updated_at: nil) data = event_data(user, :destroy) expect(data[:created_at]).to be(nil) diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 42e48b9ad81..a4ae7e42958 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -378,13 +378,13 @@ RSpec.describe SystemNoteService do noteable_types.each do |type| context "when noteable is a #{type}" do it "blocks cross reference when #{type.underscore}_events is false" do - jira_tracker.update("#{type}_events" => false) + jira_tracker.update!("#{type}_events" => false) expect(cross_reference(type)).to eq(s_('JiraService|Events for %{noteable_model_name} are disabled.') % { noteable_model_name: type.pluralize.humanize.downcase }) end it "creates cross reference when #{type.underscore}_events is true" do - jira_tracker.update("#{type}_events" => true) + jira_tracker.update!("#{type}_events" => true) expect(cross_reference(type)).to eq(success_message) end @@ -566,25 +566,25 @@ RSpec.describe SystemNoteService do end end - describe '.handle_merge_request_wip' do + describe '.handle_merge_request_draft' do it 'calls MergeRequestsService' do expect_next_instance_of(::SystemNotes::MergeRequestsService) do |service| - expect(service).to receive(:handle_merge_request_wip) + expect(service).to receive(:handle_merge_request_draft) end - described_class.handle_merge_request_wip(noteable, project, author) + described_class.handle_merge_request_draft(noteable, project, author) end end - describe '.add_merge_request_wip_from_commit' do + describe '.add_merge_request_draft_from_commit' do it 'calls MergeRequestsService' do commit = double expect_next_instance_of(::SystemNotes::MergeRequestsService) do |service| - expect(service).to receive(:add_merge_request_wip_from_commit).with(commit) + expect(service).to receive(:add_merge_request_draft_from_commit).with(commit) end - described_class.add_merge_request_wip_from_commit(noteable, project, author, commit) + described_class.add_merge_request_draft_from_commit(noteable, project, author, commit) end end diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index e78b00fb67a..b70c5e899fc 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -373,7 +373,7 @@ RSpec.describe ::SystemNotes::IssuablesService do before do # Mention issue (noteable) from commit0 system_note = service.cross_reference(commit0) - system_note.update(note: system_note.note.capitalize) + system_note.update!(note: system_note.note.capitalize) end it 'is truthy when already mentioned' do @@ -407,7 +407,7 @@ RSpec.describe ::SystemNotes::IssuablesService do before do # Mention commit1 from commit0 system_note = service.cross_reference(commit1) - system_note.update(note: system_note.note.capitalize) + system_note.update!(note: system_note.note.capitalize) end it 'is truthy when already mentioned' do @@ -436,7 +436,7 @@ RSpec.describe ::SystemNotes::IssuablesService do context 'legacy capitalized cross reference' do before do system_note = service.cross_reference(commit0) - system_note.update(note: system_note.note.capitalize) + system_note.update!(note: system_note.note.capitalize) end it 'is true when a fork mentions an external issue' do @@ -582,7 +582,7 @@ RSpec.describe ::SystemNotes::IssuablesService do it 'creates the note text correctly' do [:issue, :merge_request].each do |type| - issuable = create(type) + issuable = create(type) # rubocop:disable Rails/SaveBang service = described_class.new(noteable: issuable, author: author) expect(service.discussion_lock.note) diff --git a/spec/services/system_notes/merge_requests_service_spec.rb b/spec/services/system_notes/merge_requests_service_spec.rb index 067e1cef64d..50d16231e8f 100644 --- a/spec/services/system_notes/merge_requests_service_spec.rb +++ b/spec/services/system_notes/merge_requests_service_spec.rb @@ -51,44 +51,44 @@ RSpec.describe ::SystemNotes::MergeRequestsService do end end - describe '.handle_merge_request_wip' do + describe '.handle_merge_request_draft' do context 'adding draft note' do let(:noteable) { create(:merge_request, source_project: project, title: 'Draft: Lorem ipsum') } - subject { service.handle_merge_request_wip } + subject { service.handle_merge_request_draft } it_behaves_like 'a system note' do let(:action) { 'title' } end it 'sets the note text' do - expect(subject.note).to eq 'marked as a **Work In Progress**' + expect(subject.note).to eq 'marked this merge request as **draft**' end end - context 'removing wip note' do - subject { service.handle_merge_request_wip } + context 'removing draft note' do + subject { service.handle_merge_request_draft } it_behaves_like 'a system note' do let(:action) { 'title' } end it 'sets the note text' do - expect(subject.note).to eq 'unmarked as a **Work In Progress**' + expect(subject.note).to eq 'marked this merge request as **ready**' end end end - describe '.add_merge_request_wip_from_commit' do - subject { service.add_merge_request_wip_from_commit(noteable.diff_head_commit) } + describe '.add_merge_request_draft_from_commit' do + subject { service.add_merge_request_draft_from_commit(noteable.diff_head_commit) } it_behaves_like 'a system note' do let(:action) { 'title' } end - it "posts the 'marked as a Work In Progress from commit' system note" do + it "posts the 'marked this merge request as draft from commit' system note" do expect(subject.note).to match( - /marked as a \*\*Work In Progress\*\* from #{Commit.reference_pattern}/ + /marked this merge request as \*\*draft\*\* from #{Commit.reference_pattern}/ ) end end diff --git a/spec/services/test_hooks/project_service_spec.rb b/spec/services/test_hooks/project_service_spec.rb index e4cc3a2d652..7470bdff527 100644 --- a/spec/services/test_hooks/project_service_spec.rb +++ b/spec/services/test_hooks/project_service_spec.rb @@ -186,5 +186,23 @@ RSpec.describe TestHooks::ProjectService do expect(service.execute).to include(success_result) end end + + context 'releases_events' do + let(:trigger) { 'releases_events' } + let(:trigger_key) { :release_hooks } + + it 'returns error message if not enough data' do + expect(hook).not_to receive(:execute) + expect(service.execute).to include({ status: :error, message: 'Ensure the project has releases.' }) + end + + it 'executes hook' do + allow(project).to receive(:releases).and_return([Release.new]) + allow_any_instance_of(Release).to receive(:to_hook_data).and_return(sample_data) + + expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result) + expect(service.execute).to include(success_result) + end + end end end diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 60903f8f367..90325c564bc 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -145,12 +145,12 @@ RSpec.describe TodoService do end it 'creates correct todos for each valid user based on the type of mention' do - issue.update(description: directly_addressed_and_mentioned) + issue.update!(description: directly_addressed_and_mentioned) service.new_issue(issue, author) should_create_todo(user: member, target: issue, action: Todo::DIRECTLY_ADDRESSED) - should_create_todo(user: admin, target: issue, action: Todo::MENTIONED) + should_not_create_todo(user: admin, target: issue, action: Todo::MENTIONED) should_create_todo(user: guest, target: issue, action: Todo::MENTIONED) end @@ -160,7 +160,7 @@ RSpec.describe TodoService do should_create_todo(user: assignee, target: confidential_issue, author: john_doe, action: Todo::ASSIGNED) should_create_todo(user: author, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) should_create_todo(user: member, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) - should_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) + should_not_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) should_not_create_todo(user: guest, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) should_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) end @@ -171,7 +171,7 @@ RSpec.describe TodoService do should_create_todo(user: assignee, target: addressed_confident_issue, author: john_doe, action: Todo::ASSIGNED) should_create_todo(user: author, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED) should_create_todo(user: member, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED) - should_create_todo(user: admin, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED) + should_not_create_todo(user: admin, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED) should_not_create_todo(user: guest, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED) should_create_todo(user: john_doe, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED) end @@ -222,13 +222,13 @@ RSpec.describe TodoService do end it 'creates a todo for each valid user not included in skip_users based on the type of mention' do - issue.update(description: directly_addressed_and_mentioned) + issue.update!(description: directly_addressed_and_mentioned) service.update_issue(issue, author, skip_users) should_create_todo(user: member, target: issue, action: Todo::DIRECTLY_ADDRESSED) should_create_todo(user: guest, target: issue, action: Todo::MENTIONED) - should_create_todo(user: admin, target: issue, action: Todo::MENTIONED) + should_not_create_todo(user: admin, target: issue, action: Todo::MENTIONED) should_not_create_todo(user: skipped, target: issue) end @@ -273,7 +273,7 @@ RSpec.describe TodoService do should_create_todo(user: author, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) should_create_todo(user: assignee, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) should_create_todo(user: member, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) - should_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) + should_not_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) should_not_create_todo(user: guest, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) should_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) end @@ -284,14 +284,14 @@ RSpec.describe TodoService do should_create_todo(user: author, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED) should_create_todo(user: assignee, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED) should_create_todo(user: member, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED) - should_create_todo(user: admin, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED) + should_not_create_todo(user: admin, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED) should_not_create_todo(user: guest, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED) should_create_todo(user: john_doe, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED) end context 'issues with a task list' do it 'does not create todo when tasks are marked as completed' do - issue.update(description: "- [x] Task 1\n- [X] Task 2 #{mentions}") + issue.update!(description: "- [x] Task 1\n- [X] Task 2 #{mentions}") service.update_issue(issue, author) @@ -304,7 +304,7 @@ RSpec.describe TodoService do end it 'does not create directly addressed todo when tasks are marked as completed' do - addressed_issue.update(description: "#{directly_addressed}\n- [x] Task 1\n- [x] Task 2\n") + addressed_issue.update!(description: "#{directly_addressed}\n- [x] Task 1\n- [x] Task 2\n") service.update_issue(addressed_issue, author) @@ -317,7 +317,7 @@ RSpec.describe TodoService do end it 'does not raise an error when description not change' do - issue.update(title: 'Sample') + issue.update!(title: 'Sample') expect { service.update_issue(issue, author) }.not_to raise_error end @@ -427,12 +427,12 @@ RSpec.describe TodoService do end it 'creates a todo for each valid user based on the type of mention' do - note.update(note: directly_addressed_and_mentioned) + note.update!(note: directly_addressed_and_mentioned) service.new_note(note, john_doe) should_create_todo(user: member, target: issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: note) - should_create_todo(user: admin, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) + should_not_create_todo(user: admin, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) should_create_todo(user: guest, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) end @@ -452,7 +452,7 @@ RSpec.describe TodoService do should_create_todo(user: author, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) should_create_todo(user: assignee, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) should_create_todo(user: member, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) - should_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) + should_not_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) should_not_create_todo(user: guest, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) should_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) end @@ -463,7 +463,7 @@ RSpec.describe TodoService do should_create_todo(user: author, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue) should_create_todo(user: assignee, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue) should_create_todo(user: member, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue) - should_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue) + should_not_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue) should_not_create_todo(user: guest, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue) should_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue) end @@ -694,12 +694,12 @@ RSpec.describe TodoService do end it 'creates a todo for each valid user based on the type of mention' do - mr_assigned.update(description: directly_addressed_and_mentioned) + mr_assigned.update!(description: directly_addressed_and_mentioned) service.new_merge_request(mr_assigned, author) should_create_todo(user: member, target: mr_assigned, action: Todo::DIRECTLY_ADDRESSED) - should_create_todo(user: admin, target: mr_assigned, action: Todo::MENTIONED) + should_not_create_todo(user: admin, target: mr_assigned, action: Todo::MENTIONED) end it 'creates a directly addressed todo for each valid addressed user' do @@ -726,12 +726,12 @@ RSpec.describe TodoService do end it 'creates a todo for each valid user not included in skip_users based on the type of mention' do - mr_assigned.update(description: directly_addressed_and_mentioned) + mr_assigned.update!(description: directly_addressed_and_mentioned) service.update_merge_request(mr_assigned, author, skip_users) should_create_todo(user: member, target: mr_assigned, action: Todo::DIRECTLY_ADDRESSED) - should_create_todo(user: admin, target: mr_assigned, action: Todo::MENTIONED) + should_not_create_todo(user: admin, target: mr_assigned, action: Todo::MENTIONED) should_not_create_todo(user: skipped, target: mr_assigned) end @@ -772,7 +772,7 @@ RSpec.describe TodoService do context 'with a task list' do it 'does not create todo when tasks are marked as completed' do - mr_assigned.update(description: "- [x] Task 1\n- [X] Task 2 #{mentions}") + mr_assigned.update!(description: "- [x] Task 1\n- [X] Task 2 #{mentions}") service.update_merge_request(mr_assigned, author) @@ -786,7 +786,7 @@ RSpec.describe TodoService do end it 'does not create directly addressed todo when tasks are marked as completed' do - addressed_mr_assigned.update(description: "#{directly_addressed}\n- [x] Task 1\n- [X] Task 2") + addressed_mr_assigned.update!(description: "#{directly_addressed}\n- [x] Task 1\n- [X] Task 2") service.update_merge_request(addressed_mr_assigned, author) @@ -800,7 +800,7 @@ RSpec.describe TodoService do end it 'does not raise an error when description not change' do - mr_assigned.update(title: 'Sample') + mr_assigned.update!(title: 'Sample') expect { service.update_merge_request(mr_assigned, author) }.not_to raise_error end @@ -883,7 +883,7 @@ RSpec.describe TodoService do end it 'creates a pending todo for each merge_participant' do - mr_unassigned.update(merge_when_pipeline_succeeds: true, merge_user: admin) + mr_unassigned.update!(merge_when_pipeline_succeeds: true, merge_user: admin) service.merge_request_became_unmergeable(mr_unassigned) merge_participants.each do |participant| @@ -991,13 +991,13 @@ RSpec.describe TodoService do end it 'creates a todo for each valid user not included in skip_users based on the type of mention' do - note.update(note: directly_addressed_and_mentioned) + note.update!(note: directly_addressed_and_mentioned) service.update_note(note, author, skip_users) should_create_todo(user: member, target: noteable, action: Todo::DIRECTLY_ADDRESSED) should_create_todo(user: guest, target: noteable, action: Todo::MENTIONED) - should_create_todo(user: admin, target: noteable, action: Todo::MENTIONED) + should_not_create_todo(user: admin, target: noteable, action: Todo::MENTIONED) should_not_create_todo(user: skipped, target: noteable) end diff --git a/spec/services/todos/destroy/confidential_issue_service_spec.rb b/spec/services/todos/destroy/confidential_issue_service_spec.rb index ddce45e7968..e3dcc2bae95 100644 --- a/spec/services/todos/destroy/confidential_issue_service_spec.rb +++ b/spec/services/todos/destroy/confidential_issue_service_spec.rb @@ -37,7 +37,7 @@ RSpec.describe Todos::Destroy::ConfidentialIssueService do context 'when provided issue is not confidential' do it 'does not remove any todos' do - issue_1.update(confidential: false) + issue_1.update!(confidential: false) expect { subject }.not_to change { Todo.count } end diff --git a/spec/services/two_factor/destroy_service_spec.rb b/spec/services/two_factor/destroy_service_spec.rb index 3df4d1593c6..30c189520fd 100644 --- a/spec/services/two_factor/destroy_service_spec.rb +++ b/spec/services/two_factor/destroy_service_spec.rb @@ -85,7 +85,7 @@ RSpec.describe TwoFactor::DestroyService do it_behaves_like 'disables two-factor authentication' end - context 'admin disables the two-factor authentication of another user' do + context 'admin disables the two-factor authentication of another user', :enable_admin_mode do let(:current_user) { create(:admin) } let(:user) { create(:user, :two_factor) } diff --git a/spec/services/users/approve_service_spec.rb b/spec/services/users/approve_service_spec.rb index 50f2b6b0827..55b2c83f4a8 100644 --- a/spec/services/users/approve_service_spec.rb +++ b/spec/services/users/approve_service_spec.rb @@ -19,85 +19,101 @@ RSpec.describe Users::ApproveService do end end - context 'when user is not in pending approval state' do - let(:user) { create(:user, state: 'active') } - + context 'when the executor user is an admin not in admin mode' do it 'returns error result' do expect(subject[:status]).to eq(:error) - expect(subject[:message]) - .to match(/The user you are trying to approve is not pending an approval/) + expect(subject[:message]).to match(/You are not allowed to approve a user/) end end - context 'when user cannot be activated' do - let(:user) do - build(:user, state: 'blocked_pending_approval', email: 'invalid email') - end + context 'when the executor user is an admin in admin mode', :enable_admin_mode do + context 'when user is not in pending approval state' do + let(:user) { create(:user, state: 'active') } - it 'returns error result' do - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to match(/Email is invalid/) + it 'returns error result' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]) + .to match(/The user you are trying to approve is not pending an approval/) + end end - it 'does not change the state of the user' do - expect { subject }.not_to change { user.state } + context 'when user cannot be activated' do + let(:user) do + build(:user, state: 'blocked_pending_approval', email: 'invalid email') + end + + it 'returns error result' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to match(/Email is invalid/) + end + + it 'does not change the state of the user' do + expect { subject }.not_to change { user.state } + end end end end context 'success' do - it 'activates the user' do - expect(subject[:status]).to eq(:success) - expect(user.reload).to be_active - end + context 'when the executor user is an admin in admin mode', :enable_admin_mode do + it 'activates the user' do + expect(subject[:status]).to eq(:success) + expect(user.reload).to be_active + end - context 'email confirmation status' do - context 'user is unconfirmed' do - let(:user) { create(:user, :blocked_pending_approval, :unconfirmed) } + it 'emails the user on approval' do + expect(DeviseMailer).to receive(:user_admin_approval).with(user).and_call_original + expect { subject }.to have_enqueued_mail(DeviseMailer, :user_admin_approval) + end - it 'sends confirmation instructions' do - expect { subject } - .to have_enqueued_mail(DeviseMailer, :confirmation_instructions) + context 'email confirmation status' do + context 'user is unconfirmed' do + let(:user) { create(:user, :blocked_pending_approval, :unconfirmed) } + + it 'sends confirmation instructions' do + expect { subject } + .to have_enqueued_mail(DeviseMailer, :confirmation_instructions) + end end - end - context 'user is confirmed' do - it 'does not send a confirmation email' do - expect { subject } - .not_to have_enqueued_mail(DeviseMailer, :confirmation_instructions) + context 'user is confirmed' do + it 'does not send a confirmation email' do + expect { subject } + .not_to have_enqueued_mail(DeviseMailer, :confirmation_instructions) + end end end - end - context 'pending invitiations' do - let!(:project_member_invite) { create(:project_member, :invited, invite_email: user.email) } - let!(:group_member_invite) { create(:group_member, :invited, invite_email: user.email) } + context 'pending invitations' do + let!(:project_member_invite) { create(:project_member, :invited, invite_email: user.email) } + let!(:group_member_invite) { create(:group_member, :invited, invite_email: user.email) } - context 'user is unconfirmed' do - let(:user) { create(:user, :blocked_pending_approval, :unconfirmed) } + context 'user is unconfirmed' do + let(:user) { create(:user, :blocked_pending_approval, :unconfirmed) } - it 'does not accept pending invites of the user' do - expect(subject[:status]).to eq(:success) + it 'does not accept pending invites of the user' do + expect(subject[:status]).to eq(:success) - group_member_invite.reload - project_member_invite.reload + group_member_invite.reload + project_member_invite.reload - expect(group_member_invite).to be_invite - expect(project_member_invite).to be_invite + expect(group_member_invite).to be_invite + expect(project_member_invite).to be_invite + end end - end - context 'user is confirmed' do - it 'accepts pending invites of the user' do - expect(subject[:status]).to eq(:success) + context 'user is confirmed' do + it 'accepts pending invites of the user' do + expect(subject[:status]).to eq(:success) - group_member_invite.reload - project_member_invite.reload + group_member_invite.reload + project_member_invite.reload - expect(group_member_invite).not_to be_invite - expect(project_member_invite).not_to be_invite - expect(group_member_invite.user).to eq(user) - expect(project_member_invite.user).to eq(user) + expect(group_member_invite).not_to be_invite + expect(project_member_invite).not_to be_invite + expect(group_member_invite.user).to eq(user) + expect(project_member_invite.user).to eq(user) + end end end end diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index 6de685dd89a..76b84e3b4ab 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -3,14 +3,14 @@ require 'spec_helper' RSpec.describe Users::DestroyService do - describe "Deletes a user and all their personal projects" do - let!(:user) { create(:user) } - let!(:admin) { create(:admin) } - let!(:namespace) { user.namespace } - let!(:project) { create(:project, namespace: namespace) } - let(:service) { described_class.new(admin) } - let(:gitlab_shell) { Gitlab::Shell.new } - + let!(:user) { create(:user) } + let!(:admin) { create(:admin) } + let!(:namespace) { user.namespace } + let!(:project) { create(:project, namespace: namespace) } + let(:service) { described_class.new(admin) } + let(:gitlab_shell) { Gitlab::Shell.new } + + describe "Deletes a user and all their personal projects", :enable_admin_mode do context 'no options are given' do it 'deletes the user' do user_data = service.execute(user) @@ -108,7 +108,7 @@ RSpec.describe Users::DestroyService do context 'projects in pending_delete' do before do project.pending_delete = true - project.save + project.save! end it 'destroys a project in pending_delete' do @@ -215,35 +215,6 @@ RSpec.describe Users::DestroyService do end end - context "deletion permission checks" do - it 'does not delete the user when user is not an admin' do - other_user = create(:user) - - expect { described_class.new(other_user).execute(user) }.to raise_error(Gitlab::Access::AccessDeniedError) - expect(User.exists?(user.id)).to be(true) - end - - it 'allows admins to delete anyone' do - described_class.new(admin).execute(user) - - expect(User.exists?(user.id)).to be(false) - end - - it 'allows users to delete their own account' do - described_class.new(user).execute(user) - - expect(User.exists?(user.id)).to be(false) - end - - it 'allows user to be deleted if skip_authorization: true' do - other_user = create(:user) - - described_class.new(user).execute(other_user, skip_authorization: true) - - expect(User.exists?(other_user.id)).to be(false) - end - end - context "migrating associated records" do let!(:issue) { create(:issue, author: user) } @@ -310,7 +281,7 @@ RSpec.describe Users::DestroyService do it 'of group_members' do group_member = create(:group_member) - group_member.group.group_members.create(user: user, access_level: 40) + group_member.group.group_members.create!(user: user, access_level: 40) expect_any_instance_of(GroupMember).to receive(:run_callbacks).with(:find).once expect_any_instance_of(GroupMember).to receive(:run_callbacks).with(:initialize).once @@ -320,4 +291,43 @@ RSpec.describe Users::DestroyService do end end end + + describe "Deletion permission checks" do + it 'does not delete the user when user is not an admin' do + other_user = create(:user) + + expect { described_class.new(other_user).execute(user) }.to raise_error(Gitlab::Access::AccessDeniedError) + expect(User.exists?(user.id)).to be(true) + end + + context 'when admin mode is enabled', :enable_admin_mode do + it 'allows admins to delete anyone' do + described_class.new(admin).execute(user) + + expect(User.exists?(user.id)).to be(false) + end + end + + context 'when admin mode is disabled' do + it 'disallows admins to delete anyone' do + expect { described_class.new(admin).execute(user) }.to raise_error(Gitlab::Access::AccessDeniedError) + + expect(User.exists?(user.id)).to be(true) + end + end + + it 'allows users to delete their own account' do + described_class.new(user).execute(user) + + expect(User.exists?(user.id)).to be(false) + end + + it 'allows user to be deleted if skip_authorization: true' do + other_user = create(:user) + + described_class.new(user).execute(other_user, skip_authorization: true) + + expect(User.exists?(other_user.id)).to be(false) + end + end end diff --git a/spec/services/users/repair_ldap_blocked_service_spec.rb b/spec/services/users/repair_ldap_blocked_service_spec.rb index b33dcb92f45..54540d68af2 100644 --- a/spec/services/users/repair_ldap_blocked_service_spec.rb +++ b/spec/services/users/repair_ldap_blocked_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe Users::RepairLdapBlockedService do describe '#execute' do it 'changes to normal block after destroying last ldap identity' do - identity.destroy + identity.destroy! service.execute expect(user.reload).not_to be_ldap_blocked diff --git a/spec/services/users/set_status_service_spec.rb b/spec/services/users/set_status_service_spec.rb index 54489adceb0..69cd647eaeb 100644 --- a/spec/services/users/set_status_service_spec.rb +++ b/spec/services/users/set_status_service_spec.rb @@ -9,13 +9,14 @@ RSpec.describe Users::SetStatusService do describe '#execute' do context 'when params are set' do - let(:params) { { emoji: 'taurus', message: 'a random status' } } + let(:params) { { emoji: 'taurus', message: 'a random status', availability: 'busy' } } it 'creates a status' do service.execute expect(current_user.status.emoji).to eq('taurus') expect(current_user.status.message).to eq('a random status') + expect(current_user.status.availability).to eq('busy') end it 'updates a status if it already existed' do @@ -25,13 +26,33 @@ RSpec.describe Users::SetStatusService do expect(current_user.status.message).to eq('a random status') end + it 'returns true' do + create(:user_status, user: current_user) + expect(service.execute).to be(true) + end + + context 'when the given availability value is not valid' do + let(:params) { { availability: 'not a valid value' } } + + it 'does not update the status' do + user_status = create(:user_status, user: current_user) + + expect { service.execute }.not_to change { user_status.reload } + end + + it 'returns false' do + create(:user_status, user: current_user) + expect(service.execute).to be(false) + end + end + context 'for another user' do let(:target_user) { create(:user) } let(:params) do { emoji: 'taurus', message: 'a random status', user: target_user } end - context 'the current user is admin' do + context 'the current user is admin', :enable_admin_mode do let(:current_user) { create(:admin) } it 'changes the status when the current user is allowed to do that' do diff --git a/spec/services/verify_pages_domain_service_spec.rb b/spec/services/verify_pages_domain_service_spec.rb index 29ad85a16ce..ae079229891 100644 --- a/spec/services/verify_pages_domain_service_spec.rb +++ b/spec/services/verify_pages_domain_service_spec.rb @@ -189,7 +189,7 @@ RSpec.describe VerifyPagesDomainService do let(:domain) { build(:pages_domain, :expired, :with_missing_chain) } before do - domain.save(validate: false) + domain.save!(validate: false) end it 'can be disabled' do diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index b7b81d33c3e..a607a6734b0 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -10,6 +10,7 @@ RSpec.describe WebHookService do let(:headers) do { 'Content-Type' => 'application/json', + 'User-Agent' => "GitLab/#{Gitlab::VERSION}", 'X-Gitlab-Event' => 'Push Hook' } end |